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

Matt Harbison mharbison72 at gmail.com
Mon Apr 1 22:53:07 EDT 2019


On Mon, 01 Apr 2019 18:59:10 -0400, Yuya Nishihara <yuya at tcha.org> wrote:

> On Sun, 31 Mar 2019 22:46:25 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison at 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.


More information about the Mercurial-devel mailing list