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

Vincent Ladeuil (vila) wrote :

>>>>> John Arbash Meinel <email address hidden> writes:

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

That's what the test does.

    > It also allows you to add an assert after which checks that the
    > tree has been updated to the new revision.

I could add that.

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

I agree with that but I don't think the stable branch is the place to
add more fixes (and I expect to encounter such cases when adding tests).

    > (handling "+" vs "%2B", or "~" vs "%7E", etc.) As is, you added
    > "normalize_path" but have nothing that tests that code path.

Indeed. While I switch away from get_master_branch() I looked a bit
which parts where still needed though. Since the url comes a config
file, it's received as unicode for once and then it should be
url-encoded and finally we should some fuzziness in the path matching, so
normalize_path() + transport.relpath() was the most expedient way to get
there without writing specific code (which would have make the proposal
even harder to review too).

    > You do test the trailing-slash because of your
    > "relative_path()==''".

Yup, but not only.

    > Put another way, you could remove "normalize" and the test would still
    > pass.

No, try it, you'll get an UnicodeError.

    > Hence we have incomplete test coverage.

Indeed, but it's a little less incomplete, *this* bug now has a test to
cover it ;)

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

Far more intrusive so ruled out for a stable branch.

    > Which also hints at an incomplete test.)

Agreed too.

    > I personally am ok with landing what you have,


    > except I do feel it is incomplete.

Which is why I said, let's do it better on trunk.

    > It fixes the bug, and if we were under a crunch to get 2.3.4 out
    > the door for SRU,

Well, look at the 2.3 history:

- 2.3.1 released 2011-03-10
- 2.3.2 never released due to tag deletes not propagating,
- 2.3.3 blocked on this bug

So while you may not consider this a 'crunch' that's still 4 months
without a stable release for 2.3 for Ubuntu Natty.

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

I definitely agree with this sentiment.

« Back to merge proposal