Re: [PATCH V3] uncommit: abort if an explicitly given file cannot be uncommitted
On Mon, 01 Apr 2019 18:59:10 -0400, Yuya Nishihara wrote: On Sun, 31 Mar 2019 22:46:25 -0400, Matt Harbison wrote: # HG changeset patch # User Matt Harbison # 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 doesn't match any modified files? If I do "hg ", I don't care if is empty or not. I expect it'll be basically the same as "cd && hg .". 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH V3] uncommit: abort if an explicitly given file cannot be uncommitted
On Sun, 31 Mar 2019 22:46:25 -0400, Matt Harbison wrote: > # HG changeset patch > # User Matt Harbison > # 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 doesn't match any modified files? If I do "hg ", I don't care if is empty or not. I expect it'll be basically the same as "cd && hg .". > +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) ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
[PATCH V3] uncommit: abort if an explicitly given file cannot be uncommitted
# HG changeset patch # User Matt Harbison # 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 +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 + 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 + ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel