[PATCH 4 of 9 V2] addremove: warn when addremove fails to operate on a named path

[Matt Harbison]

Martin von Zweigbergk wrote:
>
>
> On Sat Nov 29 2014 at 9:53:34 PM Matt Harbison <matt_harbison at yahoo.com
> <mailto:matt_harbison at yahoo.com>> wrote:
>
>     # HG changeset patch
>     # User Matt Harbison <matt_harbison at yahoo.com>
>     # Date 1417030056 18000
>     #      Wed Nov 26 14:27:36 2014 -0500
>     # Node ID 9559f266ac03e53d20b2a2035bf5e4__17f37b79d3
>     # Parent  37af995c0feea059692cc4ed423feb__b7c722a808
>     addremove: warn when addremove fails to operate on a named path
>
>     It looks like a bad path is the only mode of failure for addremove.
>     This
>     warning is probably useful for the standalone command, but more
>     important for
>     'commit -A'.  That command doesn't currently abort if the addremove
>     fails, but
>     it will be made to do so prior to adding subrepo support, since not
>     all subrepos
>     will support addremove.  We could just abort here, but it looks like
>     addremove
>     has always silently ignored bad paths, except for the exit code.
>
>     diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>     --- a/mercurial/scmutil.py
>     +++ b/mercurial/scmutil.py
>     @@ -721,9 +721,15 @@
>               similarity = float(opts.get('similarity') or 0)
>
>           rejected = []
>     -    m.bad = lambda x, y: rejected.append(x)
>     +    origbad = m.bad
>     +    def badfn(f, msg):
>     +        if f in m.files():
>
>
> What is this check for? The only case I can think of is so 'hg addremove
> dir/' will not report 'dir: Permission denied'. It's unclear whether we
> want to prevent that. I saw that it's the done in the same way at the
> end of addremove().

Exactly- I was just carrying forward with the existing functionality, 
because its unrelated to subrepo support, and I wasn't sure either if 
that warning should be prevented.  I could have done the check at the 
bottom, but then it only prints the first bad file before returning.  It 
also seems like reusing the base bad() function is good for common 
output.  I added the check because I think it should only yell when the 
return is not 0.  mpm added the logic at the bottom in 94a8396c9305, but 
there isn't any detail on why the files() check.

Do you want to fix it before I rebase, or as a followup?  I agree, it 
seems useful to know if there is an access problem.  Running the 
*addremove* and *largefiles* tests (with my complete series) without the 
check didn't yield any diffs.

>     +            origbad(f, msg)
>     +        rejected.append(f)
>
>     +    m.bad = badfn
>           added, unknown, deleted, removed, forgotten =
>     _interestingfiles(repo, m)
>     +    m.bad = origbad
>
>
> What happens if we don't reset it?
>

Apparently nothing, because when I ran the tests mentioned above without 
it, nothing popped (although the test coverage certainly isn't perfect). 
  I was trying to play nice and not change the matcher when it returned 
to the caller, so that we don't have a chain of bad() functions when 
returning from deep in a subrepo.  It gets hard to reason about the 
state of things when the called function does surprising things like 
that.  I think I fixed an issue in largefiles awhile ago when these 
functions are setup and not restored, and I like the safety factor.

--Matt