Patchwork [3,of,9] update: add error message for dirty non-linear update with no rev

login
register
mail settings
Submitter Siddharth Agarwal
Date Sept. 24, 2013, 4:51 a.m.
Message ID <3c9d933940fabae7052a.1379998260@dev1091.prn1.facebook.com>
Download mbox | patch
Permalink /patch/2615/
State Accepted
Commit ab3e42225dbc16b20086313100d6e7b7ba01525b
Headers show

Comments

Siddharth Agarwal - Sept. 24, 2013, 4:51 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1379992050 25200
#      Mon Sep 23 20:07:30 2013 -0700
# Node ID 3c9d933940fabae7052a1bdad0eb9b47e61fb455
# Parent  eddb6f0b067c90910adb968dfb7eb798961e0ab2
update: add error message for dirty non-linear update with no rev

Previously, the error message for a dirty non-linear update was the same (and
relatively unhelpful) whether or not a rev was specified. This patch and an
upcoming one will introduce separate, more helpful hints.
Martin Geisler - Sept. 24, 2013, 7:09 a.m.
Siddharth Agarwal <sid0@fb.com> writes:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1379992050 25200
> #      Mon Sep 23 20:07:30 2013 -0700
> # Node ID 3c9d933940fabae7052a1bdad0eb9b47e61fb455
> # Parent  eddb6f0b067c90910adb968dfb7eb798961e0ab2
> update: add error message for dirty non-linear update with no rev
>
> Previously, the error message for a dirty non-linear update was the
> same (and relatively unhelpful) whether or not a rev was specified.
> This patch and an upcoming one will introduce separate, more helpful
> hints.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -656,19 +656,21 @@
>      -c  -C  dirty  rev  |  linear   same  cross
>       n   n    n     n   |    ok     (1)     x
>       n   n    n     y   |    ok     ok     ok
> -     n   n    y     *   |   merge   (2)    (2)
> +     n   n    y     n   |   merge   (2)    (2)
> +     n   n    y     y   |   merge   (3)    (3)
>       n   y    *     *   |    ---  discard  ---
> -     y   n    y     *   |    ---    (3)    ---
> +     y   n    y     *   |    ---    (4)    ---
>       y   n    n     *   |    ---    ok     ---
> -     y   y    *     *   |    ---    (4)    ---
> +     y   y    *     *   |    ---    (5)    ---
>  
>      x = can't happen
>      * = don't-care
>      1 = abort: not a linear update (merge or update --check to force update)
>      2 = abort: crosses branches (use 'hg merge' to merge or
>                   use 'hg update -C' to discard changes)
> -    3 = abort: uncommitted local changes
> -    4 = incompatible options (checked in commands.py)
> +    3 = abort: uncommitted changes (commit or update --clean to discard changes)

I know you didn't introduce this behaviour originally and that you're
just making the existing message more clear. But is this not bad advice?

I almost *never* want to use 'hg update --clean'. It's a bad option
since it throws away data. Suggesting that people use 'hg revert' would
be better IMHO since that gives them backup files by default.

Further: if I say 'hg update X', and Mercurial aborts, then I must go
through the 'hg update "ancestor(., X)"; hg update X' dance. What I
really need in that situation is for Mercurial to help me move (merge)
them into the target revision. It is extremely rare that I want to throw
away my changes with 'hg update --clean'.
Matt Mackall - Sept. 24, 2013, 6:09 p.m.
On Tue, 2013-09-24 at 09:09 +0200, Martin Geisler wrote:
> Siddharth Agarwal <sid0@fb.com> writes:
> 
> > # HG changeset patch
> > # User Siddharth Agarwal <sid0@fb.com>
> > # Date 1379992050 25200
> > #      Mon Sep 23 20:07:30 2013 -0700
> > # Node ID 3c9d933940fabae7052a1bdad0eb9b47e61fb455
> > # Parent  eddb6f0b067c90910adb968dfb7eb798961e0ab2
> > update: add error message for dirty non-linear update with no rev
> >
> > Previously, the error message for a dirty non-linear update was the
> > same (and relatively unhelpful) whether or not a rev was specified.
> > This patch and an upcoming one will introduce separate, more helpful
> > hints.
> >
> > diff --git a/mercurial/merge.py b/mercurial/merge.py
> > --- a/mercurial/merge.py
> > +++ b/mercurial/merge.py
> > @@ -656,19 +656,21 @@
> >      -c  -C  dirty  rev  |  linear   same  cross
> >       n   n    n     n   |    ok     (1)     x
> >       n   n    n     y   |    ok     ok     ok
> > -     n   n    y     *   |   merge   (2)    (2)
> > +     n   n    y     n   |   merge   (2)    (2)
> > +     n   n    y     y   |   merge   (3)    (3)
> >       n   y    *     *   |    ---  discard  ---
> > -     y   n    y     *   |    ---    (3)    ---
> > +     y   n    y     *   |    ---    (4)    ---
> >       y   n    n     *   |    ---    ok     ---
> > -     y   y    *     *   |    ---    (4)    ---
> > +     y   y    *     *   |    ---    (5)    ---
> >  
> >      x = can't happen
> >      * = don't-care
> >      1 = abort: not a linear update (merge or update --check to force update)
> >      2 = abort: crosses branches (use 'hg merge' to merge or
> >                   use 'hg update -C' to discard changes)
> > -    3 = abort: uncommitted local changes
> > -    4 = incompatible options (checked in commands.py)
> > +    3 = abort: uncommitted changes (commit or update --clean to discard changes)
> 
> I know you didn't introduce this behaviour originally and that you're
> just making the existing message more clear. But is this not bad advice?

> I almost *never* want to use 'hg update --clean'. It's a bad option
> since it throws away data. Suggesting that people use 'hg revert' would
> be better IMHO since that gives them backup files by default.

I think this is fine, as it's explicit that it loses changes. I think a
lot of people are confused by revert and/or annoyed by orig files so
it's not clear that suggesting revert would result in a net increase in
happiness.

> Further: if I say 'hg update X', and Mercurial aborts, then I must go
> through the 'hg update "ancestor(., X)"; hg update X' dance. What I
> really need in that situation is for Mercurial to help me move (merge)
> them into the target revision. It is extremely rare that I want to throw
> away my changes with 'hg update --clean'.

Yep. The answer to this might be shelve in the near future. There have
been a couple discussions about a --keep-my-changes-across-branch option
for update, but they haven't gone anywhere. Can't be on by default
because hopping branches by default is a known cause of user confusion.
Sean Farley - Sept. 24, 2013, 8:02 p.m.
martin@geisler.net writes:

> Siddharth Agarwal <sid0@fb.com> writes:
>
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com>
>> # Date 1379992050 25200
>> #      Mon Sep 23 20:07:30 2013 -0700
>> # Node ID 3c9d933940fabae7052a1bdad0eb9b47e61fb455
>> # Parent  eddb6f0b067c90910adb968dfb7eb798961e0ab2
>> update: add error message for dirty non-linear update with no rev
>>
>> Previously, the error message for a dirty non-linear update was the
>> same (and relatively unhelpful) whether or not a rev was specified.
>> This patch and an upcoming one will introduce separate, more helpful
>> hints.
>>
>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>> --- a/mercurial/merge.py
>> +++ b/mercurial/merge.py
>> @@ -656,19 +656,21 @@
>>      -c  -C  dirty  rev  |  linear   same  cross
>>       n   n    n     n   |    ok     (1)     x
>>       n   n    n     y   |    ok     ok     ok
>> -     n   n    y     *   |   merge   (2)    (2)
>> +     n   n    y     n   |   merge   (2)    (2)
>> +     n   n    y     y   |   merge   (3)    (3)
>>       n   y    *     *   |    ---  discard  ---
>> -     y   n    y     *   |    ---    (3)    ---
>> +     y   n    y     *   |    ---    (4)    ---
>>       y   n    n     *   |    ---    ok     ---
>> -     y   y    *     *   |    ---    (4)    ---
>> +     y   y    *     *   |    ---    (5)    ---
>>  
>>      x = can't happen
>>      * = don't-care
>>      1 = abort: not a linear update (merge or update --check to force update)
>>      2 = abort: crosses branches (use 'hg merge' to merge or
>>                   use 'hg update -C' to discard changes)
>> -    3 = abort: uncommitted local changes
>> -    4 = incompatible options (checked in commands.py)
>> +    3 = abort: uncommitted changes (commit or update --clean to discard changes)
>
> I know you didn't introduce this behaviour originally and that you're
> just making the existing message more clear. But is this not bad advice?
>
> I almost *never* want to use 'hg update --clean'. It's a bad option
> since it throws away data. Suggesting that people use 'hg revert' would
> be better IMHO since that gives them backup files by default.
>
> Further: if I say 'hg update X', and Mercurial aborts, then I must go
> through the 'hg update "ancestor(., X)"; hg update X' dance. What I
> really need in that situation is for Mercurial to help me move (merge)
> them into the target revision. It is extremely rare that I want to throw
> away my changes with 'hg update --clean'.

To follow-up on Martin's response, using 'hg update' also has the
potential of messing with a bookmark (e.g. moving or deactivating). I
almost always use 'hg revert' solely for this reason.
Martin Geisler - Oct. 3, 2013, 12:21 p.m.
Matt Mackall <mpm@selenic.com> writes:

Hi Matt,

Thanks for the thoughtful answers.

> On Tue, 2013-09-24 at 09:09 +0200, Martin Geisler wrote:
>
>> I almost *never* want to use 'hg update --clean'. It's a bad option
>> since it throws away data. Suggesting that people use 'hg revert'
>> would be better IMHO since that gives them backup files by default.
>
> I think this is fine, as it's explicit that it loses changes. I think
> a lot of people are confused by revert and/or annoyed by orig files so
> it's not clear that suggesting revert would result in a net increase
> in happiness.

Yeah, it's not clear what the wanted action is in all cases and people
are bound to become confused.

>> Further: if I say 'hg update X', and Mercurial aborts, then I must go
>> through the 'hg update "ancestor(., X)"; hg update X' dance. What I
>> really need in that situation is for Mercurial to help me move
>> (merge) them into the target revision. It is extremely rare that I
>> want to throw away my changes with 'hg update --clean'.
>
> Yep. The answer to this might be shelve in the near future.

I'm very happy to see that the extension has been added!

> There have been a couple discussions about a
> --keep-my-changes-across-branch option for update, but they haven't
> gone anywhere. Can't be on by default because hopping branches by
> default is a known cause of user confusion.

Okay, that's fair enough. With the shelve tool I think the need for a
flag has mostly disappeared.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -656,19 +656,21 @@ 
     -c  -C  dirty  rev  |  linear   same  cross
      n   n    n     n   |    ok     (1)     x
      n   n    n     y   |    ok     ok     ok
-     n   n    y     *   |   merge   (2)    (2)
+     n   n    y     n   |   merge   (2)    (2)
+     n   n    y     y   |   merge   (3)    (3)
      n   y    *     *   |    ---  discard  ---
-     y   n    y     *   |    ---    (3)    ---
+     y   n    y     *   |    ---    (4)    ---
      y   n    n     *   |    ---    ok     ---
-     y   y    *     *   |    ---    (4)    ---
+     y   y    *     *   |    ---    (5)    ---
 
     x = can't happen
     * = don't-care
     1 = abort: not a linear update (merge or update --check to force update)
     2 = abort: crosses branches (use 'hg merge' to merge or
                  use 'hg update -C' to discard changes)
-    3 = abort: uncommitted local changes
-    4 = incompatible options (checked in commands.py)
+    3 = abort: uncommitted changes (commit or update --clean to discard changes)
+    4 = abort: uncommitted local changes
+    5 = incompatible options (checked in commands.py)
 
     Return the same tuple as applyupdates().
     """
@@ -726,10 +728,14 @@ 
                     # note: the <node> variable contains a random identifier
                     if repo[node].node() in foreground:
                         pa = p1  # allow updating to successors
-                    elif dirty:
+                    elif dirty and onode is None:
                         msg = _("crosses branches (merge branches or use"
                                 " --clean to discard changes)")
                         raise util.Abort(msg)
+                    elif dirty:
+                        msg = _("uncommitted changes")
+                        hint = _("commit or update --clean to discard changes")
+                        raise util.Abort(msg, hint=hint)
                     else:  # node is none
                         msg = _("not a linear update")
                         hint = _("merge or update --check to force update")
diff --git a/tests/test-merge5.t b/tests/test-merge5.t
--- a/tests/test-merge5.t
+++ b/tests/test-merge5.t
@@ -33,7 +33,8 @@ 
 Should abort:
 
   $ hg update -y 1
-  abort: crosses branches (merge branches or use --clean to discard changes)
+  abort: uncommitted changes
+  (commit or update --clean to discard changes)
   [255]
   $ mv c a
 
diff --git a/tests/test-subrepo-svn.t b/tests/test-subrepo-svn.t
--- a/tests/test-subrepo-svn.t
+++ b/tests/test-subrepo-svn.t
@@ -477,7 +477,8 @@ 
 This is surprising, but is also correct based on the current code:
   $ echo "updating should (maybe) fail" > obstruct/other
   $ hg co tip
-  abort: crosses branches (merge branches or use --clean to discard changes)
+  abort: uncommitted changes
+  (commit or update --clean to discard changes)
   [255]
 
 Point to a Subversion branch which has since been deleted and recreated
diff --git a/tests/test-update-branches.t b/tests/test-update-branches.t
--- a/tests/test-update-branches.t
+++ b/tests/test-update-branches.t
@@ -123,22 +123,26 @@ 
   M sub/suba
 
   $ revtest 'none dirty same'   dirty 2 3
-  abort: crosses branches (merge branches or use --clean to discard changes)
+  abort: uncommitted changes
+  (commit or update --clean to discard changes)
   parent=2
   M foo
 
   $ revtest 'none dirtysub same'   dirtysub 2 3
-  abort: crosses branches (merge branches or use --clean to discard changes)
+  abort: uncommitted changes
+  (commit or update --clean to discard changes)
   parent=2
   M sub/suba
 
   $ revtest 'none dirty cross'  dirty 3 4
-  abort: crosses branches (merge branches or use --clean to discard changes)
+  abort: uncommitted changes
+  (commit or update --clean to discard changes)
   parent=3
   M foo
 
   $ revtest 'none dirtysub cross'  dirtysub 3 4
-  abort: crosses branches (merge branches or use --clean to discard changes)
+  abort: uncommitted changes
+  (commit or update --clean to discard changes)
   parent=3
   M sub/suba
 
@@ -223,5 +227,6 @@ 
   $ hg up --quiet 0
   $ hg up --quiet 2
   $ hg up 5
-  abort: crosses branches (merge branches or use --clean to discard changes)
+  abort: uncommitted changes
+  (commit or update --clean to discard changes)
   [255]