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

Matt Harbison matt_harbison at yahoo.com
Tue Dec 2 23:58:04 CST 2014


Martin von Zweigbergk wrote:
>
>
> On Tue Dec 02 2014 at 7:01:06 PM Matt Harbison <matt_harbison at yahoo.com
> <mailto: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>
>      > <mailto:matt_harbison at yahoo.__com
>     <mailto:matt_harbison at yahoo.com>>> wrote:
>      >
>      >     # HG changeset patch
>      >     # User Matt Harbison <matt_harbison at yahoo.com
>     <mailto: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".

Agreed.  The "explicit files" bit didn't register when I read it.

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

I'm ambivalent about it.  It's useful for hooking into something and 
changing behavior.  Being new to python, it's intriguing because method 
replacement like that is not something I'm used to- but that takes me 
longer to figure out what is going on, especially since I can't just ask 
the IDE to tell me who all is calling it.

I wonder if an annotation on subrepo methods that holds the original 
bad() reference at the start of the call and restores it on return is 
better?  That would handle most of the recursive cases and is out of the 
way of core code.  (Though largefiles tends to override a lot too, and 
that is complicated enough without worrying if some function it called 
altered its finely crafted matcher object.)

I probably won't attempt the annotation until I see a couple of cases 
where these modifications are causing trouble, but I thought the restore 
in the patch stuck out too.

--Matt


More information about the Mercurial-devel mailing list