missing, in_history do a fetch during a logically readonly operation

Bug #129045 reported by Tim Hatch on 2007-07-29
2
Affects Status Importance Assigned to Milestone
Bazaar
Medium
Unassigned

Bug Description

The easiest way to trigger this is make a lightweight checkout of something on http: w/o the smart server, then run "bzr missing <my own url>". bzr missing will die horribly, ironically trying to set my own parent to myself. I'm more of the opinion that it should just realize it can't set the parent and move on (perhaps giving a little warning, even though it's a side effect that fails).

I mentioned this on irc:

23:24 < thatch> I've discovered an oddity with lightweight checkouts and 'bzr missing'
23:25 < thatch> bzr.dev/bzrlib/builtins.py:3093 tries to lock write, which causes it to blow up if the checkout's
                transport is non-writable (i.e. dumb http)
23:30 < thatch> this appear to work ok on a heavyweight checkout, but I'm not sure 'missing' should fail so badly
                if it's not able to set the parent. Would a one-line warning get the point across as well?
...
01:52 < lifeless> thatch: if it fails to set the parent, and its because its readonly, it should swallow it I
                  think
01:52 < lifeless> because being readonly is not something thats usually fixable

I'll attach a little patch in a second that catches UnlockableTransport but I'm not sure if there are more exceptions that ought to be swallowed too.

Tim Hatch (timhatch) wrote :
Martin Pool (mbp) on 2007-07-30
Changed in bzr:
importance: Undecided → Medium
status: New → Confirmed
Martin Pool (mbp) wrote :

Thanks for the patch Tim.

I'm not convinced that missing should be settitng the parent implicitly: it's a logically readonly command and so should probably not have side effects. So I'm inclined to just remove that section. We would also need to document the removal in NEWS, and add some tests for missing between two readonly branches.

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

Martin Pool wrote:
> Thanks for the patch Tim.
>
> I'm not convinced that missing should be settitng the parent implicitly:
> it's a logically readonly command and so should probably not have side
> effects. So I'm inclined to just remove that section. We would also
> need to document the removal in NEWS, and add some tests for missing
> between two readonly branches.

I don't think I really agree with that. For example, bzr send is
logically readonly, but actually
- - does a fetch of the remote last_revision, so that it can determine a
  common ancestry locally
- - sets the submit branch
- - sets the public branch

If logically readonly commands can't do write at all, then generating a
bundle becomes more expensive, and we never get to set the submit branch
or submit branch.

Aaron

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

iD8DBQFGrh0d0F+nu1YWqI0RAhR2AJ0e5o1zidxADVYAz+SUI5+EpAHCkQCaAtgs
WAF9VCMfVBVn7dGPew93zSg=
=Lb2x
-----END PGP SIGNATURE-----

I'd suggest that this needs a new test in tests/blackbox/test_missing.py that checks that both the source and destination branches can be readonly, and that missing does not set the parent.

At the moment missing depends on the current directory. It would make the test simpler if you add a -d/--directory option to missing similar to other commands like cmd_tag. Then make two braches and use get_readonly_transport('a').base to get a url that will deny write access to them. Then try run_bzr missing between the two. Finally check that the parent was not set using branch.get_parent.

On 7/30/07, Aaron Bentley <email address hidden> wrote:
> I don't think I really agree with that. For example, bzr send is
> logically readonly, but actually
> - - does a fetch of the remote last_revision, so that it can determine a
> common ancestry locally
> - - sets the submit branch
> - - sets the public branch
>
> If logically readonly commands can't do write at all, then generating a
> bundle becomes more expensive, and we never get to set the submit branch
> or submit branch.

There's a couple of things here. Implicitly fetching because of a
readonly operation makes later operations faster, but also causes bugs
with readonly branches. We also tend to get locks knotted up because
a readonly operation needs to get a write lock to fetch, and you can't
upgrade locks that way.

Actually with lockdirs it might be ok to allow upgrading because the
readlock is a no-op and can't cause contention. There could be a
problem if we have things cached because of the read lock which are
invalidated by write operations. But I don't think we always
guarantee repeatable reads within a read lock.

I think it's ok that bzr send sets them because it's basically
remembering the defaults for send. Why is this different? I guess
because parent affects pull and merge, and it's not clear that missing
affects just the same thing: you can use it to compare to the place
you're going to push to. Maybe the heart of it is

 * missing should instead be 'pull --preview' or 'pull --dry-run' or
something similar
 * 'parent' is maybe not such a good name compared to 'default' or
'from_default' or 'pull_default' (I'm not sure they're great names
either)

On consideration it's ok with me to keep the same behaviour of trying
to set it but skipping if it can't be. I think when we do set it we
should say so, and if we fail we should say so too.

--
Martin

Martin Pool (mbp) on 2010-02-15
summary: - 'bzr missing' will traceback when branch is read-only
+ missing, in_history do a fetch during a logically readonly operation
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers