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

login
register
mail settings
Submitter Matt Harbison
Date Nov. 30, 2014, 5:52 a.m.
Message ID <9559f266ac03e53d20b2.1417326752@Envy>
Download mbox | patch
Permalink /patch/6899/
State Superseded
Commit 83bbedc16b3f71e34fb67007c437a54baf4faeac
Headers show

Comments

Matt Harbison - Nov. 30, 2014, 5:52 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1417030056 18000
#      Wed Nov 26 14:27:36 2014 -0500
# Node ID 9559f266ac03e53d20b2a2035bf5e417f37b79d3
# Parent  37af995c0feea059692cc4ed423febb7c722a808
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.
Martin von Zweigbergk - Dec. 2, 2014, 11:24 p.m.
On Sat Nov 29 2014 at 9:53:34 PM Matt Harbison <matt_harbison@yahoo.com>
wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1417030056 18000
> #      Wed Nov 26 14:27:36 2014 -0500
> # Node ID 9559f266ac03e53d20b2a2035bf5e417f37b79d3
> # Parent  37af995c0feea059692cc4ed423febb7c722a808
> 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().


> +            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?
Matt Harbison - Dec. 3, 2014, 3:01 a.m.
Martin von Zweigbergk wrote:
>
>
> On Sat Nov 29 2014 at 9:53:34 PM Matt Harbison <matt_harbison@yahoo.com
> <mailto:matt_harbison@yahoo.com>> wrote:
>
>     # HG changeset patch
>     # User Matt Harbison <matt_harbison@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
Martin von Zweigbergk - Dec. 3, 2014, 4:59 a.m.
On Tue Dec 02 2014 at 7:01:06 PM Matt Harbison <matt_harbison@yahoo.com>
wrote:

> Martin von Zweigbergk wrote:
> >
> >
> > On Sat Nov 29 2014 at 9:53:34 PM Matt Harbison <matt_harbison@yahoo.com
> > <mailto:matt_harbison@yahoo.com>> wrote:
> >
> >     # HG changeset patch
> >     # User Matt Harbison <matt_harbison@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.
Matt Harbison - Dec. 3, 2014, 5:58 a.m.
Martin von Zweigbergk wrote:
>
>
> On Tue Dec 02 2014 at 7:01:06 PM Matt Harbison <matt_harbison@yahoo.com
> <mailto:matt_harbison@yahoo.com>> wrote:
>
>     Martin von Zweigbergk wrote:
>      >
>      >
>      > On Sat Nov 29 2014 at 9:53:34 PM Matt Harbison
>     <matt_harbison@yahoo.com <mailto:matt_harbison@yahoo.com>
>      > <mailto:matt_harbison@yahoo.__com
>     <mailto:matt_harbison@yahoo.com>>> wrote:
>      >
>      >     # HG changeset patch
>      >     # User Matt Harbison <matt_harbison@yahoo.com
>     <mailto:matt_harbison@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

Patch

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():
+            origbad(f, msg)
+        rejected.append(f)
 
+    m.bad = badfn
     added, unknown, deleted, removed, forgotten = _interestingfiles(repo, m)
+    m.bad = origbad
 
     unknownset = set(unknown + forgotten)
     toprint = unknownset.copy()
diff --git a/tests/test-addremove.t b/tests/test-addremove.t
--- a/tests/test-addremove.t
+++ b/tests/test-addremove.t
@@ -22,6 +22,16 @@ 
   $ hg forget foo
   $ hg -v addremove
   adding foo
+  $ hg forget foo
+#if windows
+  $ hg -v addremove nonexistant
+  nonexistant: The system cannot find the file specified
+  [1]
+#else
+  $ hg -v addremove nonexistant
+  nonexistant: No such file or directory
+  [1]
+#endif
   $ cd ..
 
   $ hg init sim