Code review comment for lp:~vila/bzr/786980-url-aliases

John A Meinel (jameinel) wrote :

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

On 7/12/2011 6:49 PM, Vincent Ladeuil wrote:
> Review: Needs Information
>> # bond_location" typo
>> "fron" typo
>
> Fixed.
>> the release note doesn't describe the bug very well, it should mention that it fixes the readonlyerror
>
> Bah, thanks. Fixed.
>
>> Suggestions to make the test more foolproof, add instances other than trailing slashes in URL such as including + or ~ characters.
>
> I limit myself to the reproducing test while targetting 2.3, for trunk I think we should even test that total garbage entries in the config file is handled cleanly. Can I get this proposal accepted and address this issue in a followup ?
>
>> Add changes to the branch before the update.
>
> That seems unrelated to the issue, what do you have in mind ?
>

You run "checkout.update()" which did trigger the existing bug. However,
we've had issues with things like this in the past, so I'm recommending
being extra cautious here.

I was actually surprised that "checkout.update()" with nothing to-do
didn't short-cut and skip the fetch. So having 1 more commit on the
master branch so that update must update the local branch/checkout would
be good. It also allows you to add an assert after which checks that the
tree has been updated to the new revision.

I think your code fix is fine. I think history has shown that this is an
area we've had problems with in the past, so adding a few more edge-case
tests is prudent. (handling "+" vs "%2B", or "~" vs "%7E", etc.)
As is, you added "normalize_path" but have nothing that tests that code
path. You do test the trailing-slash because of your "relative_path()==''".

Put another way, you could remove "normalize" and the test would still
pass. Hence we have incomplete test coverage.

(And the other bit is that we could have made your test pass by just
changing 'checkout.update()' to not do a fetch when the last-revision
doesn't change. Which also hints at an incomplete test.)

I personally am ok with landing what you have, except I do feel it is
incomplete. It fixes the bug, and if we were under a crunch to get 2.3.4
out the door for SRU, we could land it. And under the "it is better than
what we have now" we can land it. I'm being extra cautious because we've
had regressions here before.

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

iEYEARECAAYFAk4dR40ACgkQJdeBCYSNAAMnTQCgpKzyrzSliMhTMBhMmolppqPU
B5MAoJDb2cmKHkra8mf9lP2MxmBbP21T
=R7Fv
-----END PGP SIGNATURE-----

« Back to merge proposal