Patchwork uncommit: abort if an explicitly given file is uncommittable

login
register
mail settings
Submitter Matt Harbison
Date March 30, 2019, 4:25 a.m.
Message ID <25c902e210625bf71459.1553919948@Envy>
Download mbox | patch
Permalink /patch/39417/
State New
Headers show

Comments

Matt Harbison - March 30, 2019, 4:25 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 25c902e210625bf71459da835ef4e5558fbab7dc
# Parent  eec20025ada33889233e553c5825aac36b708f6c
uncommit: abort if an explicitly given file is uncommittable

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.
Josef 'Jeff' Sipek - March 30, 2019, 9 a.m.
On Sat, Mar 30, 2019 at 00:25:48 -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 25c902e210625bf71459da835ef4e5558fbab7dc
> # Parent  eec20025ada33889233e553c5825aac36b708f6c
> uncommit: abort if an explicitly given file is uncommittable

Shouldn't this technically say "... is non-uncommittable" or something like
that?  :)

Jeff.

> 
> 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,28 @@ 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:
> +                    if f in s.clean:
> +                        hint = _(
> +                            b"file was not changed in working directory parent")
> +                    elif repo.wvfs.exists(f):
> +                        hint = _(
> +                            b"file was untracked in working directory 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,12 @@ 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
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Matt Harbison - March 30, 2019, 6:56 p.m.
On Sat, 30 Mar 2019 05:00:00 -0400, Josef 'Jeff' Sipek  
<jeffpc@josefsipek.net> wrote:

> On Sat, Mar 30, 2019 at 00:25:48 -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 25c902e210625bf71459da835ef4e5558fbab7dc
>> # Parent  eec20025ada33889233e553c5825aac36b708f6c
>> uncommit: abort if an explicitly given file is uncommittable
>
> Shouldn't this technically say "... is non-uncommittable" or something  
> like
> that?  :)
>
> Jeff.

Oops.  It took me a minute, even after you pointed it out.  Fun with  
double negatives...

Patch

diff --git a/hgext/uncommit.py b/hgext/uncommit.py
--- a/hgext/uncommit.py
+++ b/hgext/uncommit.py
@@ -133,8 +133,28 @@  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:
+                    if f in s.clean:
+                        hint = _(
+                            b"file was not changed in working directory parent")
+                    elif repo.wvfs.exists(f):
+                        hint = _(
+                            b"file was untracked in working directory 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,12 @@  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