[PATCH 1 of 1] Handle prepush logic for named branches
Sune Foldager
cryo at cyanite.org
Thu May 21 02:50:16 CDT 2009
Matt Mackall wrote:
> Can we break this into a few pieces?
> - server branch support + long description
> - client branch support
> - client branch use + fix broken tests
> - new tests
>
Yes. It isn't easy to separate the server and client code (since the
branchmap function in localrepo is the client code, but is also needed
by the server code), so I have them in the same patch.
> Hmm. Can we name this branchheads instead for better parallelism with
> heads? Or does that too strongly suggest only heads of one branch?
>
Yes, I am afraid so. The name 'branchheads' is used meaning "heads of
branch x" already.
> Looks like there's nothing currently branches from containing nasty
> things like spaces or newlines. Hmm.
>
Right. In the version of the patch I just posted, I URL-encode the
branch names (using urllib.quote) to avoid this.
> Is this nested function using local vars? If not, can we at least bump
> it back to the top of the function scope? It's way too indented.
>
Done.
> Also, I don't like the name. Underbars aside, putting a negation in a
> boolean function is not great. This could simply be newheads().
>
Done (well, I called it checkbranch).
> These lines are getting too long.
>
Fixed.
> I think we could probably arrange for there to be only one call to the
> heads function? Doesn't look like heads/remote_heads/update gets used
> after this point.
>
I don't understand this part. What call to heads are you refering to?
>
>> +invalidating branch cache (tip differs)
>>
>
> Why are these appearing? Simply because we're checking the branches now?
>
Yes... the message is issued when the branch cache is invalidated, which
is at a different point now than before the patch. Unfortunately, a lot
of tests tend to use --debug and will pick up on such things, even
though they are of course unrelated to the issue being tested.
> We've already got test-push warn for this. Please extend that test,
> with an eye to test run-time. I suspect there's a way to do this test
> without building multiple pairs of repos from scratch.
>
I am not sure I understand this either. Do you want us to simply merge
our new tests into the existing file or should we try to re-use existing
repositories from that test?
--
Sune Foldager
More information about the Mercurial-devel
mailing list