[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