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

On Aug 28, 2011, at 9:59 AM, John Arbash Meinel wrote:

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

Um. Duh. I should have figured that one out from context. Thanks for the explanation.

> 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

Cool, thank you. Should I just leave the XXX comment in the test, as is? Or maybe just leave the comment but remove the "XXX"?