Patchwork D6038: push: added clear warning message when pushing a closed branch(issue6080)

login
register
mail settings
Submitter phabricator
Date Feb. 28, 2019, 7:15 p.m.
Message ID <differential-rev-PHID-DREV-kc65zqjbpsu2qmevbsjs-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/38977/
State New
Headers show

Comments

phabricator - Feb. 28, 2019, 7:15 p.m.
taapas1128 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

AFFECTED FILES
  mercurial/discovery.py
  tests/test-push-warn.t
  tests/test-treediscovery-legacy.t
  tests/test-treediscovery.t

CHANGE DETAILS




To: taapas1128, #hg-reviewers
Cc: mercurial-devel
phabricator - March 2, 2019, 6:01 a.m.
mharbison72 added inline comments.

INLINE COMMENTS

> test-push-warn.t:235
>    searching for changes
> -  abort: push creates new remote branches: c!
> +  abort: push creates new remote branches:c(closed)!
>    (use 'hg push --new-branch' to create new remote branches)

Minor nit, but it looks like the existing space before the branch name was lost.  And it would probably be a little more readable if there was a space between the branch name and `(closed)`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - March 2, 2019, 7:15 a.m.
taapas1128 added a comment.


  @mharbison72 done.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - March 6, 2019, 1:16 p.m.
pulkit added inline comments.

INLINE COMMENTS

> discovery.py:351
> +        raise error.Abort(_("push creates new remote branches:"
> +                            " %s (closed)!")% branchnames,
>                           hint=_("use 'hg push --new-branch' to create"

This is adding ` (closed)` unconditionally even if the branch we are pushing is open.

> test-push-warn.t:256
>    searching for changes
> -  abort: push creates new remote branches: c, d!
> +  abort: push creates new remote branches: c, d (closed)!
>    (use 'hg push --new-branch' to create new remote branches)

Here none of the branch `c` and `d` are closed, but it still appends `(closed)`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: pulkit, mharbison72, mercurial-devel
phabricator - March 11, 2019, 4:28 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> discovery.py:358
> +            errmsg = (_("push creates new remote branches: %s (closed)!")
> +                        % branchnames)
> +        else:

I had assumed from the bug report that the request was to annotate each closed branch as closed, so I interpreted the output incorrectly.  It might be too much output to put `(closed)` after each closed branch in the list if it is long.  But maybe `"(%d closed)" % len(closedbranches)` will make it less ambiguous?  See what others think.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: pulkit, mharbison72, mercurial-devel
phabricator - March 11, 2019, 4:38 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> mharbison72 wrote in discovery.py:358
> I had assumed from the bug report that the request was to annotate each closed branch as closed, so I interpreted the output incorrectly.  It might be too much output to put `(closed)` after each closed branch in the list if it is long.  But maybe `"(%d closed)" % len(closedbranches)` will make it less ambiguous?  See what others think.

maybe we can just add (contains closed branches) at the end of the message.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: pulkit, mharbison72, mercurial-devel
phabricator - March 15, 2019, 1:37 p.m.
pulkit added inline comments.

INLINE COMMENTS

> discovery.py:351
> +        if isclosed == True:
> +            closedbranches.append((tag))
> +

This is calculating all the closed branches in a repo. We need to find the closed branches which are in the newbranches list.

> mharbison72 wrote in discovery.py:358
> I had assumed from the bug report that the request was to annotate each closed branch as closed, so I interpreted the output incorrectly.  It might be too much output to put `(closed)` after each closed branch in the list if it is long.  But maybe `"(%d closed)" % len(closedbranches)` will make it less ambiguous?  See what others think.

> But maybe "(%d closed)" % len(closedbranches) will make it less ambiguous

This is a nice idea. Let's do it this way. What do you think?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: pulkit, mharbison72, mercurial-devel
phabricator - March 15, 2019, 1:42 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> pulkit wrote in discovery.py:358
> > But maybe "(%d closed)" % len(closedbranches) will make it less ambiguous
> 
> This is a nice idea. Let's do it this way. What do you think?

okay i will update the patch .

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: pulkit, mharbison72, mercurial-devel
phabricator - March 15, 2019, 2:03 p.m.
taapas1128 marked 3 inline comments as done.
taapas1128 added a comment.


  @pulkit done.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: pulkit, mharbison72, mercurial-devel
phabricator - March 16, 2019, 11:10 p.m.
pulkit added a comment.


  Nice work, I have left some inline comments. Can you also add a test where we pushing multiple branches and not every branch is a closed branch?

INLINE COMMENTS

> discovery.py:348
> +    # Makes a list of closed branches
> +    closedbranches = []
> +    for tag, heads, tip, isclosed in repo.branchmap().iterbranches():

How about having this as a set so that we don't need to convert it later.

> discovery.py:350
> +    for tag, heads, tip, isclosed in repo.branchmap().iterbranches():
> +        if isclosed == True:
> +            closedbranches.append((tag))

`if isclosed` will work.

> discovery.py:352
> +            closedbranches.append((tag))
> +    closedbranches = list(set(closedbranches) & set(newbranches))
>      # 1. Check for new branches on the remote.

No need to convert `closedbranches` back to list.

> discovery.py:356
>          branchnames = ', '.join(sorted(newbranches))
> -        raise error.Abort(_("push creates new remote branches: %s!")
> -                           % branchnames,
> -                         hint=_("use 'hg push --new-branch' to create"
> -                                " new remote branches"))
> +        if len(closedbranches) > 0:
> +            errmsg = (_("push creates new remote branches: %s (%d closed)!")

`if closedbranches` will work here.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: pulkit, mharbison72, mercurial-devel
phabricator - March 17, 2019, 1:22 p.m.
taapas1128 added a comment.


  @pulkit done.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers
Cc: pulkit, mharbison72, mercurial-devel
phabricator - March 19, 2019, 11:34 a.m.
pulkit added a comment.


  @taapas1128 when you get time, can you see whether reading the branchmap can slow down things or not. It other words, can you test some performance numbers with it?
  
  A good way to do that is to enable `contrib/perf.py` as perf extension and use `hg perfdiscovery` command.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6038

To: taapas1128, #hg-reviewers, pulkit
Cc: pulkit, mharbison72, mercurial-devel

Patch

diff --git a/tests/test-treediscovery.t b/tests/test-treediscovery.t
--- a/tests/test-treediscovery.t
+++ b/tests/test-treediscovery.t
@@ -269,7 +269,7 @@ 
   $ hg push $remote
   pushing to http://localhost:$HGPORT/
   searching for changes
-  abort: push creates new remote branches: both, name1!
+  abort: push creates new remote branches:both, name1(now closed)!
   (use 'hg push --new-branch' to create new remote branches)
   [255]
   $ hg push $remote --new-branch
diff --git a/tests/test-treediscovery-legacy.t b/tests/test-treediscovery-legacy.t
--- a/tests/test-treediscovery-legacy.t
+++ b/tests/test-treediscovery-legacy.t
@@ -285,7 +285,7 @@ 
   $ hg push $remote
   pushing to http://localhost:$HGPORT/
   searching for changes
-  abort: push creates new remote branches: both, name1!
+  abort: push creates new remote branches:both, name1(now closed)!
   (use 'hg push --new-branch' to create new remote branches)
   [255]
   $ hg push $remote --new-branch
diff --git a/tests/test-push-warn.t b/tests/test-push-warn.t
--- a/tests/test-push-warn.t
+++ b/tests/test-push-warn.t
@@ -232,14 +232,14 @@ 
   $ hg push ../f
   pushing to ../f
   searching for changes
-  abort: push creates new remote branches: c!
+  abort: push creates new remote branches:c(now closed)!
   (use 'hg push --new-branch' to create new remote branches)
   [255]
 
   $ hg push -r 4 -r 5 ../f
   pushing to ../f
   searching for changes
-  abort: push creates new remote branches: c!
+  abort: push creates new remote branches:c(now closed)!
   (use 'hg push --new-branch' to create new remote branches)
   [255]
 
@@ -253,14 +253,14 @@ 
   $ hg push ../f
   pushing to ../f
   searching for changes
-  abort: push creates new remote branches: c, d!
+  abort: push creates new remote branches:c, d(now closed)!
   (use 'hg push --new-branch' to create new remote branches)
   [255]
 
   $ hg push -r 4 -r 6 ../f
   pushing to ../f
   searching for changes
-  abort: push creates new remote branches: c, d!
+  abort: push creates new remote branches:c, d(now closed)!
   (use 'hg push --new-branch' to create new remote branches)
   [255]
 
@@ -353,7 +353,7 @@ 
   $ hg push -r 12 -r 13 ../f
   pushing to ../f
   searching for changes
-  abort: push creates new remote branches: e!
+  abort: push creates new remote branches:e(now closed)!
   (use 'hg push --new-branch' to create new remote branches)
   [255]
 
@@ -477,7 +477,7 @@ 
   $ hg -R k push -r a j
   pushing to j
   searching for changes
-  abort: push creates new remote branches: b!
+  abort: push creates new remote branches:b(now closed)!
   (use 'hg push --new-branch' to create new remote branches)
   [255]
 
diff --git a/mercurial/discovery.py b/mercurial/discovery.py
--- a/mercurial/discovery.py
+++ b/mercurial/discovery.py
@@ -347,8 +347,8 @@ 
     # 1. Check for new branches on the remote.
     if newbranches and not newbranch:  # new branch requires --new-branch
         branchnames = ', '.join(sorted(newbranches))
-        raise error.Abort(_("push creates new remote branches: %s!")
-                           % branchnames,
+        raise error.Abort(_("push creates new remote branches:"
+                            "%s(now closed)!")% branchnames,
                          hint=_("use 'hg push --new-branch' to create"
                                 " new remote branches"))