Patchwork [3,of,3] backout: add a message after backout that need manual commit

login
register
mail settings
Submitter Pierre-Yves David
Date Jan. 9, 2014, 1:40 a.m.
Message ID <01daa33a1bd975621e9e.1389231617@marginatus.fb.com>
Download mbox | patch
Permalink /patch/3281/
State Superseded
Commit 6545770bd37991b4ff0400479455a6e3ffa5976b
Headers show

Comments

Pierre-Yves David - Jan. 9, 2014, 1:40 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1389230606 28800
#      Wed Jan 08 17:23:26 2014 -0800
# Node ID 01daa33a1bd975621e9e54ef359f306746c69e23
# Parent  693ba13bf57b3654617cc77e485df7f68235227b
backout: add a message after backout that need manual commit

In some case Backout silently succeeded to back out but left all the change
uncommitted. This may be confusing for user so this changeset  add a note
reminding to commit. Other backout case already actively informs the user about
created commit.
Augie Fackler - Jan. 16, 2014, 2:48 p.m.
On Wed, Jan 08, 2014 at 05:40:17PM -0800, pierre-yves.david@ens-lyon.org wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1389230606 28800
> #      Wed Jan 08 17:23:26 2014 -0800
> # Node ID 01daa33a1bd975621e9e54ef359f306746c69e23
> # Parent  693ba13bf57b3654617cc77e485df7f68235227b
> backout: add a message after backout that need manual commit

I think the message makes sense, but I think I'd rather it just said
"changeset %s backed out - do not forget to commit" rather than
including the potentially-confusing "in-place" statement. Thoughts?

I've queued patches 1 and 2.

>
> In some case Backout silently succeeded to back out but left all the change
> uncommitted. This may be confusing for user so this changeset  add a note
> reminding to commit. Other backout case already actively informs the user about
> created commit.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -463,15 +463,21 @@ def backout(ui, repo, node=None, rev=Non
>          bheads = repo.branchheads(branch)
>          rctx = scmutil.revsingle(repo, hex(parent))
>          if not opts.get('merge') and op1 != node:
>              try:
>                  ui.setconfig('ui', 'forcemerge', opts.get('tool', ''))
> -                stats = mergemod.update(repo, parent, True, True, False, node, False)
> +                stats = mergemod.update(repo, parent, True, True, False,
> +                                        node, False)
>                  repo.setparents(op1, op2)
>                  hg._showstats(repo, stats)
>                  if stats[3]:
> -                    repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
> +                    repo.ui.status(_("use 'hg resolve' to retry unresolved "
> +                                     "file merges\n"))
> +                else:
> +                    msg = _("changeset %s backed out in place, "
> +                            "do not forget to commit.\n")
> +                    ui.status(msg % short(node))
>                  return stats[3] > 0
>              finally:
>                  ui.setconfig('ui', 'forcemerge', '')
>          else:
>              hg.clean(repo, node, show_stats=False)
> diff --git a/tests/test-backout.t b/tests/test-backout.t
> --- a/tests/test-backout.t
> +++ b/tests/test-backout.t
> @@ -187,10 +187,11 @@ backout should not back out subsequent c
>    update: (current)
>
>  without --merge
>    $ hg backout -d '3 0' 1 --tool=true
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  changeset 22bca4c721e5 backed out in place, do not forget to commit.
>    $ hg locate b
>    b
>    $ hg update -C tip
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    $ hg locate b
> @@ -322,10 +323,11 @@ named branches
>    adding file2
>
>  without --merge
>    $ hg backout -r 1 --tool=true
>    0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  changeset bf1602f437f3 backed out in place, do not forget to commit.
>    $ hg branch
>    branch2
>    $ hg status -A
>    R file1
>    C default
> diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
> --- a/tests/test-subrepo.t
> +++ b/tests/test-subrepo.t
> @@ -467,10 +467,11 @@ update
>  backout calls revert internally with minimal opts, which should not raise
>  KeyError
>
>    $ hg backout ".^"
>    0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  changeset c373c8102e68 backed out in place, do not forget to commit.
>
>    $ hg up -C # discard changes
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>
>  pull
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Jan. 16, 2014, 2:48 p.m.
On Wed, Jan 08, 2014 at 05:40:17PM -0800, pierre-yves.david@ens-lyon.org wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1389230606 28800
> #      Wed Jan 08 17:23:26 2014 -0800
> # Node ID 01daa33a1bd975621e9e54ef359f306746c69e23
> # Parent  693ba13bf57b3654617cc77e485df7f68235227b
> backout: add a message after backout that need manual commit

(Actually, on reflection I'm going to just queue patch 1, since I
wonder if maybe the lack of messaging would be confusing with patch 2
and not patch 3.)

>
> In some case Backout silently succeeded to back out but left all the change
> uncommitted. This may be confusing for user so this changeset  add a note
> reminding to commit. Other backout case already actively informs the user about
> created commit.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -463,15 +463,21 @@ def backout(ui, repo, node=None, rev=Non
>          bheads = repo.branchheads(branch)
>          rctx = scmutil.revsingle(repo, hex(parent))
>          if not opts.get('merge') and op1 != node:
>              try:
>                  ui.setconfig('ui', 'forcemerge', opts.get('tool', ''))
> -                stats = mergemod.update(repo, parent, True, True, False, node, False)
> +                stats = mergemod.update(repo, parent, True, True, False,
> +                                        node, False)
>                  repo.setparents(op1, op2)
>                  hg._showstats(repo, stats)
>                  if stats[3]:
> -                    repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
> +                    repo.ui.status(_("use 'hg resolve' to retry unresolved "
> +                                     "file merges\n"))
> +                else:
> +                    msg = _("changeset %s backed out in place, "
> +                            "do not forget to commit.\n")
> +                    ui.status(msg % short(node))
>                  return stats[3] > 0
>              finally:
>                  ui.setconfig('ui', 'forcemerge', '')
>          else:
>              hg.clean(repo, node, show_stats=False)
> diff --git a/tests/test-backout.t b/tests/test-backout.t
> --- a/tests/test-backout.t
> +++ b/tests/test-backout.t
> @@ -187,10 +187,11 @@ backout should not back out subsequent c
>    update: (current)
>
>  without --merge
>    $ hg backout -d '3 0' 1 --tool=true
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  changeset 22bca4c721e5 backed out in place, do not forget to commit.
>    $ hg locate b
>    b
>    $ hg update -C tip
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    $ hg locate b
> @@ -322,10 +323,11 @@ named branches
>    adding file2
>
>  without --merge
>    $ hg backout -r 1 --tool=true
>    0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  changeset bf1602f437f3 backed out in place, do not forget to commit.
>    $ hg branch
>    branch2
>    $ hg status -A
>    R file1
>    C default
> diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
> --- a/tests/test-subrepo.t
> +++ b/tests/test-subrepo.t
> @@ -467,10 +467,11 @@ update
>  backout calls revert internally with minimal opts, which should not raise
>  KeyError
>
>    $ hg backout ".^"
>    0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  changeset c373c8102e68 backed out in place, do not forget to commit.
>
>    $ hg up -C # discard changes
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>
>  pull
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Gregory Szorc - Jan. 17, 2014, 4:27 p.m.
On 1/16/14, 6:48 AM, Augie Fackler wrote:
> On Wed, Jan 08, 2014 at 05:40:17PM -0800, pierre-yves.david@ens-lyon.org wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1389230606 28800
>> #      Wed Jan 08 17:23:26 2014 -0800
>> # Node ID 01daa33a1bd975621e9e54ef359f306746c69e23
>> # Parent  693ba13bf57b3654617cc77e485df7f68235227b
>> backout: add a message after backout that need manual commit
> I think the message makes sense, but I think I'd rather it just said
> "changeset %s backed out - do not forget to commit" rather than
> including the potentially-confusing "in-place" statement. Thoughts?
>
> I've queued patches 1 and 2.
>
Should backout write an "unfinished state" marker (similar to how 
rebase, graft, etc work)? While I haven't written a patch yet, I'm 
pretty sure the "annotated backouts" feature described at [1] will 
require a persistent state marker to handle the manual commit case. I 
don't mean to bloat scope - just raising a future concern.

[1] http://www.selenic.com/pipermail/mercurial/2013-November/046246.html
Pierre-Yves David - Jan. 17, 2014, 9:30 p.m.
On 01/17/2014 08:27 AM, Gregory Szorc wrote:
> On 1/16/14, 6:48 AM, Augie Fackler wrote:
>> On Wed, Jan 08, 2014 at 05:40:17PM -0800, 
>> pierre-yves.david@ens-lyon.org wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1389230606 28800
>>> #      Wed Jan 08 17:23:26 2014 -0800
>>> # Node ID 01daa33a1bd975621e9e54ef359f306746c69e23
>>> # Parent  693ba13bf57b3654617cc77e485df7f68235227b
>>> backout: add a message after backout that need manual commit
>> I think the message makes sense, but I think I'd rather it just said
>> "changeset %s backed out - do not forget to commit" rather than
>> including the potentially-confusing "in-place" statement. Thoughts?
>>
>> I've queued patches 1 and 2.
>>
> Should backout write an "unfinished state" marker (similar to how 
> rebase, graft, etc work)?

Don't think so, neither merge nor shelve needs it.

> While I haven't written a patch yet, I'm pretty sure the "annotated 
> backouts" feature described at [1] will require a persistent state 
> marker to handle the manual commit case. I don't mean to bloat scope - 
> just raising a future concern.
>
> [1] http://www.selenic.com/pipermail/mercurial/2013-November/046246.html

Yeah out of scope but interresting feature.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -463,15 +463,21 @@  def backout(ui, repo, node=None, rev=Non
         bheads = repo.branchheads(branch)
         rctx = scmutil.revsingle(repo, hex(parent))
         if not opts.get('merge') and op1 != node:
             try:
                 ui.setconfig('ui', 'forcemerge', opts.get('tool', ''))
-                stats = mergemod.update(repo, parent, True, True, False, node, False)
+                stats = mergemod.update(repo, parent, True, True, False,
+                                        node, False)
                 repo.setparents(op1, op2)
                 hg._showstats(repo, stats)
                 if stats[3]:
-                    repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
+                    repo.ui.status(_("use 'hg resolve' to retry unresolved "
+                                     "file merges\n"))
+                else:
+                    msg = _("changeset %s backed out in place, "
+                            "do not forget to commit.\n")
+                    ui.status(msg % short(node))
                 return stats[3] > 0
             finally:
                 ui.setconfig('ui', 'forcemerge', '')
         else:
             hg.clean(repo, node, show_stats=False)
diff --git a/tests/test-backout.t b/tests/test-backout.t
--- a/tests/test-backout.t
+++ b/tests/test-backout.t
@@ -187,10 +187,11 @@  backout should not back out subsequent c
   update: (current)
 
 without --merge
   $ hg backout -d '3 0' 1 --tool=true
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  changeset 22bca4c721e5 backed out in place, do not forget to commit.
   $ hg locate b
   b
   $ hg update -C tip
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg locate b
@@ -322,10 +323,11 @@  named branches
   adding file2
 
 without --merge
   $ hg backout -r 1 --tool=true
   0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  changeset bf1602f437f3 backed out in place, do not forget to commit.
   $ hg branch
   branch2
   $ hg status -A
   R file1
   C default
diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
--- a/tests/test-subrepo.t
+++ b/tests/test-subrepo.t
@@ -467,10 +467,11 @@  update
 backout calls revert internally with minimal opts, which should not raise
 KeyError
 
   $ hg backout ".^"
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  changeset c373c8102e68 backed out in place, do not forget to commit.
 
   $ hg up -C # discard changes
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
 pull