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

[Martin von Zweigbergk]

On Tue Dec 02 2014 at 7:01:06 PM Matt Harbison <matt_harbison at yahoo.com>
wrote:

> 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.
>

That changeset says "return 1 if we failed to handle any explicit files". I
suspect calling files() was just the natural way to limit to "explicit
files". But as it seems like there are very few other cases that can cause
bad() to be called, it might be better to report all errors. I'll try to
remember to ask mpm that in patch-form some day.


> 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.
>

The conflict between this patch and my prospective patch is small, so I
don't want to hold your series up. If I do end up sending a patch for that,
it will just be one more line to remove.


> >     +            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.


That makes sense. Thanks for a thoughtful response. I have disliked the the
matcher's bad() method for no clear reason for a while, and without having
any real idea for improvements. Thanks for for articulating one reason to
dislike it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20141203/2e45188d/attachment.html>