Code review comment for lp:~spiv/bzr/lockcontention-pushing-tag-733350-2.3

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

On 3/18/2011 7:46 AM, Andrew Bennetts wrote:
> Martin Pool wrote:
>> Review: Approve
>> PEP8: docstrings ought to have just one sentence on the first line.
>> (I guess you accidentally reflowed them.)
>
> Changed (although the pydoctor output treats a first paragraph
> identically to a first line, so the practical benefit seems low).
>
>> I'd like a docstring mention for the new ivar.
>
> Done (although pydoctor doesn't appear to notice that underscored ivar
> names are private, probably because epydoc doesn't have a public/private
> distinction here either? Anyway documenting things is good, and our
> conventions are pretty clear and well-recognised even though we have
> lots of exceptions like _format).
>
>> You may want to wait for a second review in case someone knows of
>> problem consequences.
>
> Good idea. I'll wait. I hope there aren't any!
>
> Thanks for the review!
>

Check if I understand correctly.

The issue is that we pulled tags from the master, which updates are
local copy, which we then try to push back up to the master. We have
code in 'pull' that already sees 'if source == master:
dont_update_master' for the fetch case.

However, at the time we get to merging tags, we have forgotten that the
source was the master, and thus we shouldn't be trying to update it.

What I don't like about your fix, is that we will upload all the tags
back to the master, even though we just downloaded them *from* the master.

I do like caching the master branch.

As for changing _basic_push, I think we can just do it. If you really
want compatibility, you could do:

try:
  self._basic_push(..., new_flag=XXX)
except (TypeError, AttributeError):
  self._basic_push(...)

As for public/private, it has always been a little unclear what is
private (not meant to be directly called by people using Branch) vs
Private (not meant to be implemented by a subclass).

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

iEYEARECAAYFAk2DKZ4ACgkQJdeBCYSNAAMVyQCgsConJMIA7xCnPOjw/cwcmIM1
E/wAoJ/YCX9towwhx5yyYHUnfR3+21nv
=SU/r
-----END PGP SIGNATURE-----