[Master] revisionspec in_history calls fetch, which requires the branch to be writable

The root problem is that we should not be implicitly fetching when we just want to look up the tip of the other branch.

The reason we fetch is because we may not have the revision id of the remote tip.

But we could:

a) Only fetch if we don't have it, which would cause us to fail some of the time, but not quite as often.

b) Overhaul the revision code to be more flexible about getting branches from the spec, rather than elsewhere. The big downside of this is that it effects every command that uses them. Since now we may need to be getting data from multiple locations.

For example, that status command is trying to determine the local disk changes relative to a random other revision id (which may not be in the local repository.) Now I think that "Tree.changes_from(tree_in_other_repo)" works, but I couldn't guarantee it. (Plus, if it does work, I'm not sure it has any idea that it should be grabbing revisions from the local repository when possible.)

This may be a duplicate of bug 144421.

See also bug 274172

bug 129045 has a patch for this, I'm going to mark it as a dupe simply because LP makes it hard to mark this one as a dupe of that. The patch is at http://launchpadlibrarian.net/8595399/bzr_missing_catch_unlockabletransport.diff

On 24.06.2010 21:26, Robert Collins wrote:
> bug 129045 has a patch for this, I'm going to mark it as a dupe simply
> because LP makes it hard to mark this one as a dupe of that. The patch
> is at
> http://launchpadlibrarian.net/8595399/bzr_missing_catch_unlockabletransport.diff

Doesn't work for the bug 497676 from which I came here. That bug is
about the log builtin, while the patch you refer to only addresses the
missing plugin. At least as far as I can tell. In other words, that
patch is a workaround for one special case, but not a solution to the
underlying problem. Judging from the exception name, I'm not even sure
that bug 129045 really is the same problem at heart.

Bug 244278, bug 497676, bug 503031 and the original report here all deal
with branch: revspecs in combination with various read-only commands:
status, log and vimdiff. Addressing this at the command level seems
ill-advised to me. Instead I'd look at RevisionSpec_branch for a place
to fix.

There the idea seems to be that we simply fetch the revisions from the
remote branch into the local repository for the sake of diff. We could
probably fall back to not fetching the remote revisions if the
transaction is read-only. I'll attach a branch with an idea for a fix.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin von Gagern wrote:
> On 24.06.2010 21:26, Robert Collins wrote:
>> bug 129045 has a patch for this, I'm going to mark it as a dupe simply
>> because LP makes it hard to mark this one as a dupe of that. The patch
>> is at
>> http://launchpadlibrarian.net/8595399/bzr_missing_catch_unlockabletransport.diff
>
> Doesn't work for the bug 497676 from which I came here. That bug is
> about the log builtin, while the patch you refer to only addresses the
> missing plugin. At least as far as I can tell. In other words, that
> patch is a workaround for one special case, but not a solution to the
> underlying problem. Judging from the exception name, I'm not even sure
> that bug 129045 really is the same problem at heart.
>
> Bug 244278, bug 497676, bug 503031 and the original report here all deal
> with branch: revspecs in combination with various read-only commands:
> status, log and vimdiff. Addressing this at the command level seems
> ill-advised to me. Instead I'd look at RevisionSpec_branch for a place
> to fix.
>
> There the idea seems to be that we simply fetch the revisions from the
> remote branch into the local repository for the sake of diff. We could
> probably fall back to not fetching the remote revisions if the
> transaction is read-only. I'll attach a branch with an idea for a fix.
>
> ** Branch linked: lp:~gagern/bzr/bug149270_RevisionSpec_branch_readonly
>

The problem is that if we don't fetch the revisions, we may not already
have them, but expect them to be present in the local repo.

For comparison "bzr XXX -r -1:path" will do the same thing as "bzr XXX
- -r branch:path", but will fail if those revs aren't present locally (for
most commands XXX).

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwjyfEACgkQJdeBCYSNAAMsiQCgqreFgNFrUoUHHzT/4fJcqsnh
2I4AmQGNpHtirT3BaoxUob5M96olu37G
=IvtI
-----END PGP SIGNATURE-----

I think the particular source of readonliness is different, but the
underlying issue is the same as you say: we copy content locally just
to diff it, and we can [perhaps] avoid that.

We could convert this into 'upgrade readlocks to writelocks' which
would fix the majority of cases (but leave running missing in a
readonly filesystem (e.g. a different users homedir) not working.

I agree completely about not solving it at the command layer: solving
this per-command is likely to be painful and incomplete ;)

-Rob

On 24.06.2010 23:11, John A Meinel wrote:
> For comparison "bzr XXX -r -1:path" will do the same thing as "bzr XXX
> -r branch:path", but will fail if those revs aren't present locally (for
> most commands XXX).

OK, so this is along the lines of your comment #2 b) about "Overhaul the
revision code to be more flexible about getting branches from the spec,
rather than elsewhere", right?

For XXX = log it seems to work. For those commands that use write locks,
it will work as well, as in that cases old behaviour is maintained. This
leads to the difference between diff and vimdiff reported in bug 503031,
I assume.

So for now, the fix from my branch will allow a few cases that used to
fail before, and will probably cause others to fail in different places
than they used to. On the whole, I'd consider it a step in the right
direction, in particular as it doesn't break any existing selftests. I
would deem commands that disregard the branch from the spec to be a
separate problem worth a separate bug report, or maybe even one report
for each broken command.

For your reference: it seems that for stat the issue was addressed by bug 144421.