Code review comment for lp:~gary/bzr/bug835035

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

On 8/28/2011 2:29 PM, Gary Poster wrote:
> Thanks, John.
>
> The reason I didn't do that is because of this comment (of yours,
> it seems, from bzr annotate) that you can see in the diff:
>
> # This repository will call fallback.unlock() when we transition
> to # the unlocked state, so we make sure to increment the lock
> count
>
> That doesn't happen in the case of bug 835035, but apparently it
> does in some common case or other that I don't understand (I don't
> even know what a "fallback" is in this context).
>
> I could simply try making a change like you describe (using a
> "try/finally" or a "with" within add_fallback_repository) and then
> run the entire test suite and see if anything breaks. If it
> doesn't break, I guess I could remove the comment, or supply
> another that you suggested. Does that sound reasonable, or would
> you suggest something different?

If you have a stacked branch, the one it is stacked *on* is the fallback.

If you look at the code, immediately after checking if it works as a
fallback, it adds it as a fallback.

The original code is:

if self.is_locked():
    # We will call fallback.unlock() when we transition to the unlocked
    # state, so always add a lock here. If a caller passes us a locked
    # repository, they are responsible for unlocking it later.
    repository.lock_read()
self._check_fallback_repository(repository)
self._fallback_repositories.append(repository)

so maybe:

dif_lock = False

if self.is_locked():
    # We will call fallback.unlock() when we transition to the unlocked
    # state, so always add a lock here. If a caller passes us a locked
    # repository, they are responsible for unlocking it later.
    repository.lock_read()
    did_lock = True
try:
    self._check_fallback_repository(repository)
except errors.IncompatibleRepositories:
    if did_lock:
      repository.unlock()
    raise
self._fallback_repositories.append(repository)

However, looking at the code, I think yours is more straigtforward to
understand, and there doesn't seem to be any data that is being cached
that matters.

  merge: approve

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

iEYEARECAAYFAk5aSdYACgkQJdeBCYSNAANtQgCbBTUmTj3T23c8OtXrpd/0obn7
GJgAoNcBe4GdzPu5rAuYs4O9v8rjjlle
=sBIE
-----END PGP SIGNATURE-----