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

# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1553910795 14400
#      Fri Mar 29 21:53:15 2019 -0400
# Node ID e0dfeb204949f70a34517d019b5f45037b943db7
# 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.

On Sat, 30 Mar 2019 15:02:51 -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 e0dfeb204949f70a34517d019b5f45037b943db7
> # 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,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)

Doesn't it break "hg uncommit <dir>"? match.files() isn't a list of matched
files.

On Sun, 31 Mar 2019 18:24:25 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sat, 30 Mar 2019 15:02:51 -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 e0dfeb204949f70a34517d019b5f45037b943db7
>> # Parent  eec20025ada33889233e553c5825aac36b708f6c
>> uncommit: abort if an explicitly given file cannot be uncommitted
>>
>> 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)
>
> Doesn't it break "hg uncommit <dir>"? match.files() isn't a list of  
> matched
> files.

Yeah, you're right.  `.` seems to work, but not a subdirectory.  The  
`badfn` and `explicitdir` callbacks on the matcher seem to only be invoked  
when walking wdir.  Any ideas?