[PATCH 2 of 3] improve pre-push logic to handle named branches better (issue736)

[Dirkjan Ochtman]

Some more nits...

On 21/05/2009 09:41, Sune Foldager wrote:
> # HG changeset patch
> # User Sune Foldager<cryo at cyanite.org>
> # Date 1242889821 -7200
> # Node ID eb08c59fab17c704a91bf1e6d0cecbc77132da4c
> # Parent  aa08c8294220bf4f5b8702c9353c6c13a64d73c2
> improve pre-push logic to handle named branches better (issue736)
>
> Each named branch is considered separately, and the push is allowed if
> no new branch heads are created for any named branch to be pushed.
>
> Due to some tests's use of --debug, their output will change after this
> addition. This has been fixed as well.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1432,42 +1432,83 @@
>           else:
>               bases, heads = update, self.changelog.heads()
>
> +        def checkbranch(lheads, rheads, updatelh):
> +            warn = 0
> +
> +            if not revs and len(lheads)>  len(rheads):

Weird spacing? Looks like that may have been in there before, but this 
would be a good time to fix it.

> +                warn = 1
> +            else:
> +                newheads = set(sum([self.changelog.heads(x, lheads) \
> +                                    for x in updatelh], []))
> +                newheads&= set(lheads)

That does't make sense to me... Why sum([blah], [])? Something like this 
seems much easier to read.

heads = [self.changelog.heads(x, lheads) for x in updatelh]
newheads = set(sum(heads, [])) & set(lheads)


> +
> +                if not newheads:
> +                    return True
> +
> +                for r in rheads:
> +                    if r in self.changelog.nodemap:
> +                        desc = self.changelog.heads(r, heads)
> +                        l = [h for h in heads if h in desc]
> +                        if not l:
> +                            newheads.add(r)
> +                    else:
> +                        newheads.add(r)
> +                if len(newheads)>  len(rheads):
> +                    warn = 1
> +
> +            if warn:
> +                if not rheads: # new branch requires --force
> +                    self.ui.warn(_("abort: push creates new remote branch '%s'!\n" % \
> +                                   self[updatelh[0]].branch()))
> +                else:
> +                    self.ui.warn(_("abort: push creates new remote heads!\n"))
> +
> +                self.ui.status(_("(did you forget to merge?"
> +                             " use push -f to force)\n"))

You are aligning the second-line quote with the first-line quote here, 
right? My mailer might be showing it wrong.

> +                return False
> +            return True
> +

Does a nested function really make sense here? Can we extract it? You'd 
have ui, changelog, and revs as extra args, I guess, so it may not be a 
good trade-off. It should probably get a good docstring either way.

>           if not bases:
>               self.ui.status(_("no changes found\n"))
>               return None, 1
>           elif not force:
> -            # check if we're creating new remote heads
> -            # to be a remote head after push, node must be either
> +            # Check for each named branch if we're creating new remote heads.
> +            # To be a remote head after push, node must be either:
>               # - unknown locally
>               # - a local outgoing head descended from update
>               # - a remote head that's known locally and not
>               #   ancestral to an outgoing head
> +            #
> +            # New named branches cannot be created without --force.
>
> -            warn = 0
> +            if remote_heads != [nullid]:
> +                if remote.capable('branchmap'):
> +                    local_hds = {}

Please don't make new names with underscores in them.

> +                    if not revs:
> +                        local_hds = self.branchmap()
> +                    else:
> +                        for n in heads:
> +                            branch = self[n].branch()
> +                            if local_hds.has_key(branch):

Please don't use has_key().

> +                                local_hds[branch].append(n)
> +                            else:
> +                                local_hds[branch] = [n]
>
> -            if remote_heads == [nullid]:
> -                warn = 0
> -            elif not revs and len(heads)>  len(remote_heads):
> -                warn = 1
> -            else:
> -                newheads = list(heads)
> -                for r in remote_heads:
> -                    if r in self.changelog.nodemap:
> -                        desc = self.changelog.heads(r, heads)
> -                        l = [h for h in heads if h in desc]
> -                        if not l:
> -                            newheads.append(r)
> -                    else:
> -                        newheads.append(r)
> -                if len(newheads)>  len(remote_heads):
> -                    warn = 1
> +                    remote_hds = remote.branchmap()
>
> -            if warn:
> -                self.ui.warn(_("abort: push creates new remote heads!\n"))
> -                self.ui.status(_("(did you forget to merge?"
> -                                 " use push -f to force)\n"))
> -                return None, 0
> -            elif inc:
> +                    for lh in local_hds:
> +                        rheads = remote_hds[lh] if remote_hds.has_key(lh) else []

More has_key().

> +                        lheads = local_hds[lh]
> +                        updatelh = [upd for upd in update if self[upd].branch() == lh]
> +                        if not updatelh:
> +                            continue
> +                        if not checkbranch(lheads, rheads, updatelh):
> +                            return None, 0
> +                else:
> +                    if not checkbranch(heads, remote_heads, update):
> +                        return None, 0
> +
> +            if inc:
>                   self.ui.warn(_("note: unsynced remote changes!\n"))
>
>
> diff --git a/tests/test-acl.out b/tests/test-acl.out
> --- a/tests/test-acl.out
> +++ b/tests/test-acl.out
> @@ -42,6 +42,7 @@
>   pushing to ../b
>   searching for changes
>   common changesets up to 6675d58eff77
> +invalidating branch cache (tip differs)
>   3 changesets found
>   list of changesets:
>   ef1ea85a6374b77d6da9dcda9541f498f2d17df7
> @@ -74,6 +75,7 @@
>   pushing to ../b
>   searching for changes
>   common changesets up to 6675d58eff77
> +invalidating branch cache (tip differs)
>   3 changesets found
>   list of changesets:
>   ef1ea85a6374b77d6da9dcda9541f498f2d17df7
> @@ -111,6 +113,7 @@
>   pushing to ../b
>   searching for changes
>   common changesets up to 6675d58eff77
> +invalidating branch cache (tip differs)
>   3 changesets found
>   list of changesets:
>   ef1ea85a6374b77d6da9dcda9541f498f2d17df7
> @@ -408,6 +411,7 @@
>   pushing to ../b
>   searching for changes
>   common changesets up to 6675d58eff77
> +invalidating branch cache (tip differs)
>   3 changesets found
>   list of changesets:
>   ef1ea85a6374b77d6da9dcda9541f498f2d17df7
> diff --git a/tests/test-hgweb-commands.out b/tests/test-hgweb-commands.out
> --- a/tests/test-hgweb-commands.out
> +++ b/tests/test-hgweb-commands.out
> @@ -848,7 +848,7 @@
>   % capabilities
>   200 Script output follows
>
> -lookup changegroupsubset unbundle=HG10GZ,HG10BZ,HG10UN% heads
> +lookup changegroupsubset branchmap unbundle=HG10GZ,HG10BZ,HG10UN% heads
>   200 Script output follows
>
>   1d22e65f027e5a0609357e7d8e7508cd2ba5d2fe
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>