Patchwork [V3] uncommit: abort if an explicitly given file cannot be uncommitted

login
register
mail settings
Submitter Matt Harbison
Date April 1, 2019, 2:46 a.m.
Message ID <9802203e693d83f40925.1554086785@Envy>
Download mbox | patch
Permalink /patch/39432/
State Superseded
Headers show

Comments

Matt Harbison - April 1, 2019, 2:46 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1553910795 14400
#      Fri Mar 29 21:53:15 2019 -0400
# Node ID 9802203e693d83f4092512be6d3e397926b36f8e
# Parent  eec20025ada33889233e553c5825aac36b708f6c
uncommit: abort if an explicitly given file cannot be uncommitted

I've gotten burned several times by this in the last few days.  The former tests
look simple enough, but if a good file and a bad file are given, the bad files
are silently ignored.  Some commands like `forget` will warn about bogus files,
but that would likely get lost in the noise of an interactive uncommit.  The
commit command aborts if a bad file is given, so this seems more consistent for
commands that alter the repository.
Yuya Nishihara - April 1, 2019, 10:59 p.m.
On Sun, 31 Mar 2019 22:46:25 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1553910795 14400
> #      Fri Mar 29 21:53:15 2019 -0400
> # Node ID 9802203e693d83f4092512be6d3e397926b36f8e
> # Parent  eec20025ada33889233e553c5825aac36b708f6c
> uncommit: abort if an explicitly given file cannot be uncommitted
> 
> I've gotten burned several times by this in the last few days.  The former tests
> look simple enough, but if a good file and a bad file are given, the bad files
> are silently ignored.  Some commands like `forget` will warn about bogus files,
> but that would likely get lost in the noise of an interactive uncommit.  The
> commit command aborts if a bad file is given, so this seems more consistent for
> commands that alter the repository.
> 
> diff --git a/hgext/uncommit.py b/hgext/uncommit.py
> --- a/hgext/uncommit.py
> +++ b/hgext/uncommit.py
> @@ -133,8 +133,36 @@ def uncommit(ui, repo, *pats, **opts):
>          if len(old.parents()) > 1:
>              raise error.Abort(_("cannot uncommit merge changeset"))
>  
> +        match = scmutil.match(old, pats, opts)
> +
> +        # Check all explicitly given files; abort if there's a problem.
> +        if match.files():
> +            s = old.status(old.p1(), match, listclean=True)
> +            eligible = set(s.added) | set(s.modified) | set(s.removed)
> +
> +            for f in match.files():
> +                if f not in eligible:
> +                    # Naming a parent directory of an eligible file is OK, even
> +                    # if not everything tracked in that directory can be
> +                    # uncommitted.
> +                    for e in eligible:
> +                        if e.startswith(f + '/'):
> +                            break

Perhaps, the eligible set can be extended to include util.dirs(eligible)
to get around possible quadratic computation.

That said, do we really want to error out if <dir> doesn't match any modified
files? If I do "hg <whatever> <dir>", I don't care if <dir> is empty or not.
I expect it'll be basically the same as "cd <dir> && hg <whatever> .".

> +                    else:
> +                        if f in s.clean:
> +                            hint = _(b"file was not changed in working "
> +                                     b"directory parent")
> +                        elif repo.wvfs.exists(f):
> +                            hint = _(b"file was untracked in working directory "
> +                                     b"parent")
> +                        else:
> +                            hint = _(b"file does not exist")
> +
> +                        raise error.Abort(_(b'cannot uncommit "%s"')
> +                                          % scmutil.getuipathfn(repo)(f),
> +                                          hint=hint)
Matt Harbison - April 2, 2019, 2:53 a.m.
On Mon, 01 Apr 2019 18:59:10 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 31 Mar 2019 22:46:25 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1553910795 14400
>> #      Fri Mar 29 21:53:15 2019 -0400
>> # Node ID 9802203e693d83f4092512be6d3e397926b36f8e
>> # Parent  eec20025ada33889233e553c5825aac36b708f6c
>> uncommit: abort if an explicitly given file cannot be uncommitted
>>
>> I've gotten burned several times by this in the last few days.  The  
>> former tests
>> look simple enough, but if a good file and a bad file are given, the  
>> bad files
>> are silently ignored.  Some commands like `forget` will warn about  
>> bogus files,
>> but that would likely get lost in the noise of an interactive  
>> uncommit.  The
>> commit command aborts if a bad file is given, so this seems more  
>> consistent for
>> commands that alter the repository.
>>
>> diff --git a/hgext/uncommit.py b/hgext/uncommit.py
>> --- a/hgext/uncommit.py
>> +++ b/hgext/uncommit.py
>> @@ -133,8 +133,36 @@ def uncommit(ui, repo, *pats, **opts):
>>          if len(old.parents()) > 1:
>>              raise error.Abort(_("cannot uncommit merge changeset"))
>>
>> +        match = scmutil.match(old, pats, opts)
>> +
>> +        # Check all explicitly given files; abort if there's a problem.
>> +        if match.files():
>> +            s = old.status(old.p1(), match, listclean=True)
>> +            eligible = set(s.added) | set(s.modified) | set(s.removed)
>> +
>> +            for f in match.files():
>> +                if f not in eligible:
>> +                    # Naming a parent directory of an eligible file is  
>> OK, even
>> +                    # if not everything tracked in that directory can  
>> be
>> +                    # uncommitted.
>> +                    for e in eligible:
>> +                        if e.startswith(f + '/'):
>> +                            break
>
> Perhaps, the eligible set can be extended to include util.dirs(eligible)
> to get around possible quadratic computation.

Oh, I forgot about that, thanks.

> That said, do we really want to error out if <dir> doesn't match any  
> modified
> files? If I do "hg <whatever> <dir>", I don't care if <dir> is empty or  
> not.
> I expect it'll be basically the same as "cd <dir> && hg <whatever> .".

I think if it's a command that's making changes to the repo history and we  
can't do exactly what the user requested, we do want to abort.  That's how  
commit seems to work.  Warnings for things that just modify dirstate seem  
fine, as those mistakes can easily be fixed up without cluttering the  
hidden view.  But really the problem is not recognizing that it is  
currently partially ignoring the request.

I'll submit a V4 with some extra tests (including your example IIUC), and  
the only difference with and without this code is the stderr message and  
the exit code (1 vs 255).  So we are failing in some of those cases  
already, just not consistently when given multiple files.

The slight edge case here is if you have a commit of 'dir/foo', and then a  
child commit with 'dir/bar' and 'dir/baz'.  On the child, `hg uncommit  
dir` will only uncommit bar and baz.  But that seems OK for a user that  
understands what uncommit does.  It's also what `cd dir && hg uncommit .`  
currently does.  This change just tries to catch typos.  FTR, I hit this  
being lazy and not wanting to type out full file names.  So I did:

     cd deep/nested/dir && hg uncommit file1 file2 ../otherdir/file3

The last path was missing another '../', and there was absolutely no  
indication from the command that it didn't work.  I only noticed later.

The other thing to keep in mind here is it isn't just modified files.   
Removed files can be uncommitted, so you can name things not in the  
filesystem, unlike a lot of commands.

Patch

diff --git a/hgext/uncommit.py b/hgext/uncommit.py
--- a/hgext/uncommit.py
+++ b/hgext/uncommit.py
@@ -133,8 +133,36 @@  def uncommit(ui, repo, *pats, **opts):
         if len(old.parents()) > 1:
             raise error.Abort(_("cannot uncommit merge changeset"))
 
+        match = scmutil.match(old, pats, opts)
+
+        # Check all explicitly given files; abort if there's a problem.
+        if match.files():
+            s = old.status(old.p1(), match, listclean=True)
+            eligible = set(s.added) | set(s.modified) | set(s.removed)
+
+            for f in match.files():
+                if f not in eligible:
+                    # Naming a parent directory of an eligible file is OK, even
+                    # if not everything tracked in that directory can be
+                    # uncommitted.
+                    for e in eligible:
+                        if e.startswith(f + '/'):
+                            break
+                    else:
+                        if f in s.clean:
+                            hint = _(b"file was not changed in working "
+                                     b"directory parent")
+                        elif repo.wvfs.exists(f):
+                            hint = _(b"file was untracked in working directory "
+                                     b"parent")
+                        else:
+                            hint = _(b"file does not exist")
+
+                        raise error.Abort(_(b'cannot uncommit "%s"')
+                                          % scmutil.getuipathfn(repo)(f),
+                                          hint=hint)
+
         with repo.transaction('uncommit'):
-            match = scmutil.match(old, pats, opts)
             keepcommit = pats
             if not keepcommit:
                 if opts.get('keep') is not None:
diff --git a/tests/test-uncommit.t b/tests/test-uncommit.t
--- a/tests/test-uncommit.t
+++ b/tests/test-uncommit.t
@@ -102,14 +102,16 @@  Recommit
   $ hg heads -T '{rev}:{node} {desc}'
   5:0c07a3ccda771b25f1cb1edbd02e683723344ef1 new change abcde (no-eol)
 
-Uncommit of non-existent and unchanged files has no effect
+Uncommit of non-existent and unchanged files aborts
   $ hg uncommit nothinghere
-  nothing to uncommit
-  [1]
+  abort: cannot uncommit "nothinghere"
+  (file does not exist)
+  [255]
   $ hg status
   $ hg uncommit file-abc
-  nothing to uncommit
-  [1]
+  abort: cannot uncommit "file-abc"
+  (file was not changed in working directory parent)
+  [255]
   $ hg status
 
 Try partial uncommit, also moves bookmark
@@ -513,3 +515,23 @@  Copy a->b1 and a->b2, then rename b1->c 
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     add a
   
+Removes can be uncommitted
+
+  $ hg ci -m 'modified b'
+  $ hg rm b
+  $ hg ci -m 'remove b'
+  $ hg uncommit b
+  note: keeping empty commit
+  $ hg status
+  R b
+
+Uncommitting a directory won't run afoul of the checks that an explicit file
+can be uncommitted.
+
+  $ mkdir dir
+  $ echo 1 > dir/file.txt
+  $ hg ci -Aqm 'add file in directory'
+  $ hg uncommit dir
+  $ hg status
+  A dir/file.txt
+