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

Bug #149270 reported by Martin Pool on 2007-10-05
58
This bug affects 6 people
Affects Status Importance Assigned to Milestone
Bazaar
Medium
Unassigned

Bug Description

Ken Basye <email address hidden>
to bazaar

This error report asked me to mail itself to this address, so I did.
    Best,
         Ken B.

C:\kbasye\work\my_repo2>bzr status -r branch:sftp://kbasye1@128.220.111.113/srv/
bzr/coe_repo/
Connected (version 2.0, client OpenSSH_4.5)
SSH kbasye1@128.220.111.113 password:
Authentication (password) successful!
Secsh channel 1 opened.
Opened sftp connection (server version 3)
bzr: ERROR: bzrlib.errors.ReadOnlyError: A write attempt was made in a read only
 transaction on LockableFiles(lock, file:///C:/kbasye/work/my_repo2/.bzr/reposit
ory/)

Traceback (most recent call last):
  File "c:\python251\lib\site-packages\bzrlib\commands.py", line 817, in run_bzr
_catch_errors
    return run_bzr(argv)
  File "c:\python251\lib\site-packages\bzrlib\commands.py", line 779, in run_bzr

    ret = run(*run_argv)
  File "c:\python251\lib\site-packages\bzrlib\commands.py", line 477, in run_arg
v_aliases
    return self.run(**all_cmd_args)
  File "c:\python251\lib\site-packages\bzrlib\commands.py", line 789, in ignore_
pipe
    result = func(*args, **kwargs)
  File "c:\python251\lib\site-packages\bzrlib\builtins.py", line 187, in run
    to_file=self.outf, short=short, versioned=versioned)
  File "c:\python251\lib\site-packages\bzrlib\status.py", line 130, in show_tree
_status
    rev_id = revision[0].in_history(wt.branch).rev_id
  File "c:\python251\lib\site-packages\bzrlib\revisionspec.py", line 225, in in_
history
    return self._match_on_and_check(branch, revs)
  File "c:\python251\lib\site-packages\bzrlib\revisionspec.py", line 207, in _ma
tch_on_and_check
    info = self._match_on(branch, revs)
  File "c:\python251\lib\site-packages\bzrlib\revisionspec.py", line 683, in _ma
tch_on
    branch.fetch(other_branch, revision_b)
  File "c:\python251\lib\site-packages\bzrlib\decorators.py", line 163, in write
_locked
    self.lock_write()
  File "c:\python251\lib\site-packages\bzrlib\branch.py", line 1329, in lock_wri
te
    repo_token = self.repository.lock_write()
  File "c:\python251\lib\site-packages\bzrlib\repository.py", line 307, in lock_
write
    result = self.control_files.lock_write(token=token)
  File "c:\python251\lib\site-packages\bzrlib\lockable_files.py", line 253, in l
ock_write
    raise errors.ReadOnlyError(self)
ReadOnlyError: A write attempt was made in a read only transaction on LockableFi
les(lock, file:///C:/kbasye/work/my_repo2/.bzr/repository/)

bzr 0.90.0 on python 2.5.1.final.0 (win32)
arguments: ['c:\\python251\\Scripts\\bzr', 'status', '-r', 'branch:sftp://kbasye
1@128.220.111.113/srv/bzr/coe_repo/']

** please send this report to <email address hidden>

C:\kbasye\work\my_repo2>

Related branches

Martin Pool (mbp) wrote :

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

Changed in bzr:
importance: Undecided → Medium
status: New → Confirmed
John A Meinel (jameinel) wrote :

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.)

Matthew Paul Thomas (mpt) wrote :

This may be a duplicate of bug 144421.

Nathan Adams (nadams) wrote :

See also bug 274172

Robert Collins (lifeless) 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

summary: - revisionspec in_history calls fetch, which requires the branch to be
- writable
+ Master: revisionspec in_history calls fetch, which requires the branch
+ to be writable
summary: - Master: revisionspec in_history calls fetch, which requires the branch
+ [Master] revisionspec in_history calls fetch, which requires the branch
to be writable

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.

Martin von Gagern (gagern) wrote :

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

Vincent Ladeuil (vila) on 2010-09-17
Changed in bzr:
milestone: none → 2.2.0
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers