Patchwork addremove: print relative paths when called with -I/-X

login
register
mail settings
Submitter Martin von Zweigbergk
Date Dec. 2, 2014, 5:56 a.m.
Message ID <0ec94796e8c2d5c14297.1417499785@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6944/
State Accepted
Headers show

Comments

Martin von Zweigbergk - Dec. 2, 2014, 5:56 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1417499312 28800
#      Mon Dec 01 21:48:32 2014 -0800
# Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
# Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
addremove: print relative paths when called with -I/-X

For "hg addremove 'glob:*.py'", we print any paths added or removed as
relative to the current directory, but when "hg addremove -I
'glob:*.py'" is used, we use the absolute path (relative from the repo
root). It seems like they should be the same, so change it so we use
relative paths in both cases. Continue to use absolute paths when no
patterns are given.
Martin von Zweigbergk - Dec. 2, 2014, 7:02 a.m.
mpm, I see you have a lot of patches in flight. If you agree with this
patch, you may want to pick it before the other Matt's patches, and instead
make the poor guy send a V3.

Matt, I didn't look past the first 6 or so patches. Did the anyfiles()
method end up being used anywhere else, or would this patch remove the need
for it completely?

On Mon Dec 01 2014 at 9:56:26 PM Martin von Zweigbergk <
martinvonz@google.com> wrote:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1417499312 28800
> #      Mon Dec 01 21:48:32 2014 -0800
> # Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
> # Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
> addremove: print relative paths when called with -I/-X
>
> For "hg addremove 'glob:*.py'", we print any paths added or removed as
> relative to the current directory, but when "hg addremove -I
> 'glob:*.py'" is used, we use the absolute path (relative from the repo
> root). It seems like they should be the same, so change it so we use
> relative paths in both cases. Continue to use absolute paths when no
> patterns are given.
>
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -732,9 +732,9 @@
>          if repo.ui.verbose or not m.exact(abs):
>              rel = m.rel(abs)
>              if abs in unknownset:
> -                status = _('adding %s\n') % ((pats and rel) or abs)
> +                status = _('adding %s\n') % ((m.anypats() and rel) or abs)
>              else:
> -                status = _('removing %s\n') % ((pats and rel) or abs)
> +                status = _('removing %s\n') % ((m.anypats() and rel) or
> abs)
>              repo.ui.status(status)
>
>      renames = _findrenames(repo, m, added + unknown, removed + deleted,
> diff --git a/tests/test-addremove.t b/tests/test-addremove.t
> --- a/tests/test-addremove.t
> +++ b/tests/test-addremove.t
> @@ -24,6 +24,21 @@
>    adding foo
>    $ cd ..
>
> +  $ hg init subdir
> +  $ cd subdir
> +  $ mkdir dir
> +  $ cd dir
> +  $ touch a.py
> +  $ hg addremove 'glob:*.py'
> +  adding a.py
> +  $ hg forget a.py
> +  $ hg addremove -I 'glob:*.py'
> +  adding a.py
> +  $ hg forget a.py
> +  $ hg addremove
> +  adding dir/a.py
> +  $ cd ..
> +
>    $ hg init sim
>    $ cd sim
>    $ echo a > a
>
Matt Harbison - Dec. 2, 2014, 3:23 p.m.
Martin von Zweigbergk <martinvonz <at> google.com> writes:

> 
> 
> mpm, I see you have a lot of patches in flight. If you agree with this
patch, you may want to pick it before the other Matt's patches, and instead
make the poor guy send a V3.
> 
> Matt, I didn't look past the first 6 or so patches. Did the anyfiles()
method end up being used anywhere else, or would this patch remove the need
for it completely?

I'll check when I get home, but I doubt it.  The other 8 or so patches that
I didn't send yet deal with making the largefiles methods print the path
correctly.  I was puzzled too why the discrepancy with how the path was
printed with pats vs -I/-X, but superseded v1 because anypats() caused an
existing test to change, and I figured I'd have a better chance of getting
this through if there weren't subtle side effect changes like that.

A grep for '\(pats and' shows that annotate, locate and status use a similar
pattern, so maybe you want to fix those too?  I didn't dig deeper to see if
the pattern occurs with other names.  No clue why other commands that allow
patterns (like add and forget) don't also do this too- they just print
match.rel().

FYI, the other patches I'm waiting to send change match.rel() to internally
call path.join(), and then I added a match.abs() to do similar.  I didn't
need it, but got to thinking later, maybe we want a match.show(f) or
something similar to encapsulate this rel vs abs logic, so as not to clutter
up the caller's code?  OTOH, maybe this pattern isn't used enough to justify it.

I do agree with the reasoning behind this patch though, so I don't mind
sending a v3 if I have to.

--Matt
Augie Fackler - Dec. 2, 2014, 3:52 p.m.
On Mon, Dec 01, 2014 at 09:56:25PM -0800, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1417499312 28800
> #      Mon Dec 01 21:48:32 2014 -0800
> # Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
> # Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
> addremove: print relative paths when called with -I/-X

LGTM, queued with (bc) flag

>
> For "hg addremove 'glob:*.py'", we print any paths added or removed as
> relative to the current directory, but when "hg addremove -I
> 'glob:*.py'" is used, we use the absolute path (relative from the repo
> root). It seems like they should be the same, so change it so we use
> relative paths in both cases. Continue to use absolute paths when no
> patterns are given.
>
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -732,9 +732,9 @@
>          if repo.ui.verbose or not m.exact(abs):
>              rel = m.rel(abs)
>              if abs in unknownset:
> -                status = _('adding %s\n') % ((pats and rel) or abs)
> +                status = _('adding %s\n') % ((m.anypats() and rel) or abs)
>              else:
> -                status = _('removing %s\n') % ((pats and rel) or abs)
> +                status = _('removing %s\n') % ((m.anypats() and rel) or abs)
>              repo.ui.status(status)
>
>      renames = _findrenames(repo, m, added + unknown, removed + deleted,
> diff --git a/tests/test-addremove.t b/tests/test-addremove.t
> --- a/tests/test-addremove.t
> +++ b/tests/test-addremove.t
> @@ -24,6 +24,21 @@
>    adding foo
>    $ cd ..
>
> +  $ hg init subdir
> +  $ cd subdir
> +  $ mkdir dir
> +  $ cd dir
> +  $ touch a.py
> +  $ hg addremove 'glob:*.py'
> +  adding a.py
> +  $ hg forget a.py
> +  $ hg addremove -I 'glob:*.py'
> +  adding a.py
> +  $ hg forget a.py
> +  $ hg addremove
> +  adding dir/a.py
> +  $ cd ..
> +
>    $ hg init sim
>    $ cd sim
>    $ echo a > a
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Dec. 2, 2014, 4:21 p.m.
On Tue Dec 02 2014 at 7:30:25 AM Matt Harbison <mharbison@attotech.com>
wrote:

> Martin von Zweigbergk <martinvonz <at> google.com> writes:
>
> >
> >
> > mpm, I see you have a lot of patches in flight. If you agree with this
> patch, you may want to pick it before the other Matt's patches, and instead
> make the poor guy send a V3.
> >
> > Matt, I didn't look past the first 6 or so patches. Did the anyfiles()
> method end up being used anywhere else, or would this patch remove the need
> for it completely?
>
> I'll check when I get home, but I doubt it.  The other 8 or so patches that
> I didn't send yet deal with making the largefiles methods print the path
> correctly.  I was puzzled too why the discrepancy with how the path was
> printed with pats vs -I/-X, but superseded v1 because anypats() caused an
> existing test to change, and I figured I'd have a better chance of getting
> this through if there weren't subtle side effect changes like that.
>
> A grep for '\(pats and' shows that annotate, locate and status use a
> similar
> pattern, so maybe you want to fix those too?
>

Thanks for checking. Yes, locate even has a test for the absolute path,
although it's not very clear if that was the intent. Maybe there's a reason
that the two forms should behave differently. I suppose one could say that
-I/-X should have as little effect as possible on the command and just
restrict the set of files acted on. I'll wait for comments from mpm or
someone else who remembers how to view -I/-X before I send more patches.
Martin von Zweigbergk - Dec. 2, 2014, 11:41 p.m.
My patch is now in 'default'. I take that to mean that I should go ahead
and change the other cases (locate and status) so they also use relative
paths whether '<pattern>' or '-I <pattern>' is used. I'll start working on
those soon.

On Tue Dec 02 2014 at 8:21:23 AM Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Tue Dec 02 2014 at 7:30:25 AM Matt Harbison <mharbison@attotech.com>
> wrote:
>
>> Martin von Zweigbergk <martinvonz <at> google.com> writes:
>>
>> >
>> >
>> > mpm, I see you have a lot of patches in flight. If you agree with this
>> patch, you may want to pick it before the other Matt's patches, and
>> instead
>> make the poor guy send a V3.
>> >
>> > Matt, I didn't look past the first 6 or so patches. Did the anyfiles()
>> method end up being used anywhere else, or would this patch remove the
>> need
>> for it completely?
>>
>> I'll check when I get home, but I doubt it.  The other 8 or so patches
>> that
>> I didn't send yet deal with making the largefiles methods print the path
>> correctly.  I was puzzled too why the discrepancy with how the path was
>> printed with pats vs -I/-X, but superseded v1 because anypats() caused an
>> existing test to change, and I figured I'd have a better chance of getting
>> this through if there weren't subtle side effect changes like that.
>>
>> A grep for '\(pats and' shows that annotate, locate and status use a
>> similar
>> pattern, so maybe you want to fix those too?
>>
>
> Thanks for checking. Yes, locate even has a test for the absolute path,
> although it's not very clear if that was the intent. Maybe there's a reason
> that the two forms should behave differently. I suppose one could say that
> -I/-X should have as little effect as possible on the command and just
> restrict the set of files acted on. I'll wait for comments from mpm or
> someone else who remembers how to view -I/-X before I send more patches.
>
>
Matt Harbison - Dec. 4, 2014, 5:10 a.m.
On Tue, 02 Dec 2014 07:02:29 +0000, Martin von Zweigbergk wrote:

> mpm, I see you have a lot of patches in flight. If you agree with this
> patch, you may want to pick it before the other Matt's patches, and instead
> make the poor guy send a V3.
> 
> Matt, I didn't look past the first 6 or so patches. Did the anyfiles()
> method end up being used anywhere else, or would this patch remove the need
> for it completely?
> 
> On Mon Dec 01 2014 at 9:56:26 PM Martin von Zweigbergk <
> martinvonz@google.com> wrote:
> 
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1417499312 28800
>> #      Mon Dec 01 21:48:32 2014 -0800
>> # Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
>> # Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
>> addremove: print relative paths when called with -I/-X
>>

So I rebased my V2 series on top of this, and it changed the tests on me.
Specifically the test I tried was basically:

	$ hg init repo
	$ mkdir repo/dir
	<create various files>
	$ cd repo/dir
	$ hg addremove ..

This prints out absolute files instead of relative, unlike before.  I put
some prints in match, and what happens (in my real test) is '..' gets
normalized to:

	[('relpath', '.hglf')]"

match._anypats() doesn't consider 'relpath' to be a pattern, so it returns
None, and match.anypats() therefore returns False.  Maybe it should be OR'd
with the fact that the pattern parameter is not empty?  I didn't run the
tests to see if that breaks anything.  (I'm not sure why it didn't also
list the directory I was in as a relpath too.)

We should probably figure this out and get it squared away before applying
this change to status, annotate et al, and before I resubmit my series with
bad tests.

--Matt
Martin von Zweigbergk - Dec. 4, 2014, 5:35 a.m.
On Wed Dec 03 2014 at 9:11:19 PM Matt Harbison <matt_harbison@yahoo.com>
wrote:

> On Tue, 02 Dec 2014 07:02:29 +0000, Martin von Zweigbergk wrote:
>
> > mpm, I see you have a lot of patches in flight. If you agree with this
> > patch, you may want to pick it before the other Matt's patches, and
> instead
> > make the poor guy send a V3.
> >
> > Matt, I didn't look past the first 6 or so patches. Did the anyfiles()
> > method end up being used anywhere else, or would this patch remove the
> need
> > for it completely?
> >
> > On Mon Dec 01 2014 at 9:56:26 PM Martin von Zweigbergk <
> > martinvonz@google.com> wrote:
> >
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> # Date 1417499312 28800
> >> #      Mon Dec 01 21:48:32 2014 -0800
> >> # Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
> >> # Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
> >> addremove: print relative paths when called with -I/-X
> >>
>
> So I rebased my V2 series on top of this, and it changed the tests on me.
> Specifically the test I tried was basically:
>
>         $ hg init repo
>         $ mkdir repo/dir
>         <create various files>
>         $ cd repo/dir
>         $ hg addremove ..
>
> This prints out absolute files instead of relative, unlike before.  I put
> some prints in match, and what happens (in my real test) is '..' gets
> normalized to:
>
>         [('relpath', '.hglf')]"
>
> match._anypats() doesn't consider 'relpath' to be a pattern, so it returns
> None, and match.anypats() therefore returns False.  Maybe it should be OR'd
> with the fact that the pattern parameter is not empty?  I didn't run the
> tests to see if that breaks anything.  (I'm not sure why it didn't also
> list the directory I was in as a relpath too.)
>

Without having spent much thought to it, does 'not m.always()' work better?
I ask because that's what it seemed like status and locate wanted when I
checked. It's just mostly the same for addremove since exact() was already
discounted.


>
> We should probably figure this out and get it squared away before applying
> this change to status, annotate et al, and before I resubmit my series with
> bad tests.
>
> --Matt
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Matt Harbison - Dec. 4, 2014, 6:27 a.m.
On Thu, 04 Dec 2014 00:35:11 -0500, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> On Wed Dec 03 2014 at 9:11:19 PM Matt Harbison <matt_harbison@yahoo.com>
> wrote:
>
>> On Tue, 02 Dec 2014 07:02:29 +0000, Martin von Zweigbergk wrote:
>>
>> > mpm, I see you have a lot of patches in flight. If you agree with this
>> > patch, you may want to pick it before the other Matt's patches, and
>> instead
>> > make the poor guy send a V3.
>> >
>> > Matt, I didn't look past the first 6 or so patches. Did the anyfiles()
>> > method end up being used anywhere else, or would this patch remove the
>> need
>> > for it completely?
>> >
>> > On Mon Dec 01 2014 at 9:56:26 PM Martin von Zweigbergk <
>> > martinvonz@google.com> wrote:
>> >
>> >> # HG changeset patch
>> >> # User Martin von Zweigbergk <martinvonz@google.com>
>> >> # Date 1417499312 28800
>> >> #      Mon Dec 01 21:48:32 2014 -0800
>> >> # Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
>> >> # Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
>> >> addremove: print relative paths when called with -I/-X
>> >>
>>
>> So I rebased my V2 series on top of this, and it changed the tests on  
>> me.
>> Specifically the test I tried was basically:
>>
>>         $ hg init repo
>>         $ mkdir repo/dir
>>         <create various files>
>>         $ cd repo/dir
>>         $ hg addremove ..
>>
>> This prints out absolute files instead of relative, unlike before.  I  
>> put
>> some prints in match, and what happens (in my real test) is '..' gets
>> normalized to:
>>
>>         [('relpath', '.hglf')]"
>>
>> match._anypats() doesn't consider 'relpath' to be a pattern, so it  
>> returns
>> None, and match.anypats() therefore returns False.  Maybe it should be  
>> OR'd
>> with the fact that the pattern parameter is not empty?  I didn't run the
>> tests to see if that breaks anything.  (I'm not sure why it didn't also
>> list the directory I was in as a relpath too.)
>>
>
> Without having spent much thought to it, does 'not m.always()' work  
> better?
> I ask because that's what it seemed like status and locate wanted when I
> checked. It's just mostly the same for addremove since exact() was  
> already
> discounted.

I'm not sure.  My concern is that largefiles creates matchers with the  
_always field set to False.  See composenormalfilematcher() in  
largefiles/overrides.py.  I haven't sent the second half of the series  
yet, but that matcher will be passed to this code, and it basically  
eliminates the conditional when it is hardcoded to False.  It makes sense  
that the largefiles matcher is hardcoded, since it is excluding some  
files.  So this idea seems close, but a step in the wrong direction.

It seems surprising to me that a list called patterns can be given to the  
matcher, and then the matcher reports that no patterns were given.  Maybe  
it is just an unfortunate similarity in the names, but it seems like that  
needs to be fixed (or better documented what the differences are).

--Matt
Martin von Zweigbergk - Dec. 4, 2014, 5:15 p.m.
On Wed Dec 03 2014 at 10:27:39 PM Matt Harbison <mharbison72@gmail.com>
wrote:

> On Thu, 04 Dec 2014 00:35:11 -0500, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > On Wed Dec 03 2014 at 9:11:19 PM Matt Harbison <matt_harbison@yahoo.com>
> > wrote:
> >
> >> On Tue, 02 Dec 2014 07:02:29 +0000, Martin von Zweigbergk wrote:
> >>
> >> > mpm, I see you have a lot of patches in flight. If you agree with this
> >> > patch, you may want to pick it before the other Matt's patches, and
> >> instead
> >> > make the poor guy send a V3.
> >> >
> >> > Matt, I didn't look past the first 6 or so patches. Did the anyfiles()
> >> > method end up being used anywhere else, or would this patch remove the
> >> need
> >> > for it completely?
> >> >
> >> > On Mon Dec 01 2014 at 9:56:26 PM Martin von Zweigbergk <
> >> > martinvonz@google.com> wrote:
> >> >
> >> >> # HG changeset patch
> >> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> >> # Date 1417499312 28800
> >> >> #      Mon Dec 01 21:48:32 2014 -0800
> >> >> # Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
> >> >> # Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
> >> >> addremove: print relative paths when called with -I/-X
> >> >>
> >>
> >> So I rebased my V2 series on top of this, and it changed the tests on
> >> me.
> >> Specifically the test I tried was basically:
> >>
> >>         $ hg init repo
> >>         $ mkdir repo/dir
> >>         <create various files>
> >>         $ cd repo/dir
> >>         $ hg addremove ..
> >>
> >> This prints out absolute files instead of relative, unlike before.  I
> >> put
> >> some prints in match, and what happens (in my real test) is '..' gets
> >> normalized to:
> >>
> >>         [('relpath', '.hglf')]"
> >>
> >> match._anypats() doesn't consider 'relpath' to be a pattern, so it
> >> returns
> >> None, and match.anypats() therefore returns False.  Maybe it should be
> >> OR'd
> >> with the fact that the pattern parameter is not empty?


I think anypats() is currently used for checking whether optimization are
possible, so we probably don't want to change that method.


>   I didn't run the
> >> tests to see if that breaks anything.  (I'm not sure why it didn't also
> >> list the directory I was in as a relpath too.)
> >>
> >
> > Without having spent much thought to it, does 'not m.always()' work
> > better?
> > I ask because that's what it seemed like status and locate wanted when I
> > checked. It's just mostly the same for addremove since exact() was
> > already
> > discounted.
>
> I'm not sure.  My concern is that largefiles creates matchers with the
> _always field set to False.  See composenormalfilematcher() in
> largefiles/overrides.py.  I haven't sent the second half of the series
> yet, but that matcher will be passed to this code, and it basically
> eliminates the conditional when it is hardcoded to False.  It makes sense
> that the largefiles matcher is hardcoded, since it is excluding some
> files.  So this idea seems close, but a step in the wrong direction.
>
> It seems surprising to me that a list called patterns can be given to the
> matcher, and then the matcher reports that no patterns were given.  Maybe
> it is just an unfortunate similarity in the names, but it seems like that
> needs to be fixed (or better documented what the differences are).
>
> --Matt
>

Does it seem fair to say that we want to use relative paths in the UI in a
few cases (addremove, status, locate) if the user has not restricted the
paths in any way? If it is, it seems like we would need a flag on the
matcher that is disconnected from what the matcher actually matches. That
might be your _anyfiles, but modified to be "bool(patterns or include or
exclude)" and maybe renamed to _userelativepathsinui (but shortened). *sigh*

What do you think?
Martin von Zweigbergk - Dec. 4, 2014, 5:40 p.m.
On Thu Dec 04 2014 at 9:15:30 AM Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Wed Dec 03 2014 at 10:27:39 PM Matt Harbison <mharbison72@gmail.com>
> wrote:
>
>> On Thu, 04 Dec 2014 00:35:11 -0500, Martin von Zweigbergk
>> <martinvonz@google.com> wrote:
>>
>> > On Wed Dec 03 2014 at 9:11:19 PM Matt Harbison <matt_harbison@yahoo.com
>> >
>> > wrote:
>> >
>> >> On Tue, 02 Dec 2014 07:02:29 +0000, Martin von Zweigbergk wrote:
>> >>
>> >> > mpm, I see you have a lot of patches in flight. If you agree with
>> this
>> >> > patch, you may want to pick it before the other Matt's patches, and
>> >> instead
>> >> > make the poor guy send a V3.
>> >> >
>> >> > Matt, I didn't look past the first 6 or so patches. Did the
>> anyfiles()
>> >> > method end up being used anywhere else, or would this patch remove
>> the
>> >> need
>> >> > for it completely?
>> >> >
>> >> > On Mon Dec 01 2014 at 9:56:26 PM Martin von Zweigbergk <
>> >> > martinvonz@google.com> wrote:
>> >> >
>> >> >> # HG changeset patch
>> >> >> # User Martin von Zweigbergk <martinvonz@google.com>
>> >> >> # Date 1417499312 28800
>> >> >> #      Mon Dec 01 21:48:32 2014 -0800
>> >> >> # Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
>> >> >> # Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
>> >> >> addremove: print relative paths when called with -I/-X
>> >> >>
>> >>
>> >> So I rebased my V2 series on top of this, and it changed the tests on
>> >> me.
>> >> Specifically the test I tried was basically:
>> >>
>> >>         $ hg init repo
>> >>         $ mkdir repo/dir
>> >>         <create various files>
>> >>         $ cd repo/dir
>> >>         $ hg addremove ..
>> >>
>> >> This prints out absolute files instead of relative, unlike before.  I
>> >> put
>> >> some prints in match, and what happens (in my real test) is '..' gets
>> >> normalized to:
>> >>
>> >>         [('relpath', '.hglf')]"
>> >>
>> >> match._anypats() doesn't consider 'relpath' to be a pattern, so it
>> >> returns
>> >> None, and match.anypats() therefore returns False.  Maybe it should be
>> >> OR'd
>> >> with the fact that the pattern parameter is not empty?
>
>
> I think anypats() is currently used for checking whether optimization are
> possible, so we probably don't want to change that method.
>
>
>>   I didn't run the
>> >> tests to see if that breaks anything.  (I'm not sure why it didn't also
>> >> list the directory I was in as a relpath too.)
>> >>
>> >
>> > Without having spent much thought to it, does 'not m.always()' work
>> > better?
>> > I ask because that's what it seemed like status and locate wanted when I
>> > checked. It's just mostly the same for addremove since exact() was
>> > already
>> > discounted.
>>
>> I'm not sure.  My concern is that largefiles creates matchers with the
>> _always field set to False.  See composenormalfilematcher() in
>> largefiles/overrides.py.  I haven't sent the second half of the series
>> yet, but that matcher will be passed to this code, and it basically
>> eliminates the conditional when it is hardcoded to False.  It makes sense
>> that the largefiles matcher is hardcoded, since it is excluding some
>> files.  So this idea seems close, but a step in the wrong direction.
>>
>> It seems surprising to me that a list called patterns can be given to the
>> matcher, and then the matcher reports that no patterns were given.  Maybe
>> it is just an unfortunate similarity in the names, but it seems like that
>> needs to be fixed (or better documented what the differences are).
>>
>> --Matt
>>
>
> Does it seem fair to say that we want to use relative paths in the UI in a
> few cases (addremove, status, locate) if the user has not restricted the
> paths in any way? If it is, it seems like we would need a flag on the
> matcher that is disconnected from what the matcher actually matches. That
> might be your _anyfiles, but modified to be "bool(patterns or include or
> exclude)" and maybe renamed to _userelativepathsinui (but shortened). *sigh*
>

But in that case it seems better to just keep the knowledge outside of
matcher. Does the need to pass it around in the matcher arise from subrepo
use cases? I guess I didn't get that part from your patches.
Matt Harbison - Dec. 4, 2014, 5:47 p.m.
On Thu, Dec 4, 2014 at 12:15 PM, Martin von Zweigbergk <
martinvonz@google.com> wrote:

>
>
> On Wed Dec 03 2014 at 10:27:39 PM Matt Harbison <mharbison72@gmail.com>
> wrote:
>
>> On Thu, 04 Dec 2014 00:35:11 -0500, Martin von Zweigbergk
>> <martinvonz@google.com> wrote:
>>
>> > On Wed Dec 03 2014 at 9:11:19 PM Matt Harbison <matt_harbison@yahoo.com
>> >
>> > wrote:
>> >
>> >> On Tue, 02 Dec 2014 07:02:29 +0000, Martin von Zweigbergk wrote:
>> >>
>> >> > mpm, I see you have a lot of patches in flight. If you agree with
>> this
>> >> > patch, you may want to pick it before the other Matt's patches, and
>> >> instead
>> >> > make the poor guy send a V3.
>> >> >
>> >> > Matt, I didn't look past the first 6 or so patches. Did the
>> anyfiles()
>> >> > method end up being used anywhere else, or would this patch remove
>> the
>> >> need
>> >> > for it completely?
>> >> >
>> >> > On Mon Dec 01 2014 at 9:56:26 PM Martin von Zweigbergk <
>> >> > martinvonz@google.com> wrote:
>> >> >
>> >> >> # HG changeset patch
>> >> >> # User Martin von Zweigbergk <martinvonz@google.com>
>> >> >> # Date 1417499312 28800
>> >> >> #      Mon Dec 01 21:48:32 2014 -0800
>> >> >> # Node ID 0ec94796e8c2d5c142976edb49c911a92341b109
>> >> >> # Parent  19ebd2f88fc77282efe724b30de8750f79771e8f
>> >> >> addremove: print relative paths when called with -I/-X
>> >> >>
>> >>
>> >> So I rebased my V2 series on top of this, and it changed the tests on
>> >> me.
>> >> Specifically the test I tried was basically:
>> >>
>> >>         $ hg init repo
>> >>         $ mkdir repo/dir
>> >>         <create various files>
>> >>         $ cd repo/dir
>> >>         $ hg addremove ..
>> >>
>> >> This prints out absolute files instead of relative, unlike before.  I
>> >> put
>> >> some prints in match, and what happens (in my real test) is '..' gets
>> >> normalized to:
>> >>
>> >>         [('relpath', '.hglf')]"
>> >>
>> >> match._anypats() doesn't consider 'relpath' to be a pattern, so it
>> >> returns
>> >> None, and match.anypats() therefore returns False.  Maybe it should be
>> >> OR'd
>> >> with the fact that the pattern parameter is not empty?
>
>
> I think anypats() is currently used for checking whether optimization are
> possible, so we probably don't want to change that method.
>
>
>>   I didn't run the
>> >> tests to see if that breaks anything.  (I'm not sure why it didn't also
>> >> list the directory I was in as a relpath too.)
>> >>
>> >
>> > Without having spent much thought to it, does 'not m.always()' work
>> > better?
>> > I ask because that's what it seemed like status and locate wanted when I
>> > checked. It's just mostly the same for addremove since exact() was
>> > already
>> > discounted.
>>
>> I'm not sure.  My concern is that largefiles creates matchers with the
>> _always field set to False.  See composenormalfilematcher() in
>> largefiles/overrides.py.  I haven't sent the second half of the series
>> yet, but that matcher will be passed to this code, and it basically
>> eliminates the conditional when it is hardcoded to False.  It makes sense
>> that the largefiles matcher is hardcoded, since it is excluding some
>> files.  So this idea seems close, but a step in the wrong direction.
>>
>> It seems surprising to me that a list called patterns can be given to the
>> matcher, and then the matcher reports that no patterns were given.  Maybe
>> it is just an unfortunate similarity in the names, but it seems like that
>> needs to be fixed (or better documented what the differences are).
>>
>> --Matt
>>
>
> Does it seem fair to say that we want to use relative paths in the UI in a
> few cases (addremove, status, locate) if the user has not restricted the
> paths in any way? If it is, it seems like we would need a flag on the
> matcher that is disconnected from what the matcher actually matches. That
> might be your _anyfiles, but modified to be "bool(patterns or include or
> exclude)" and maybe renamed to _userelativepathsinui (but shortened). *sigh*
>
> What do you think?
>

Isn't it the opposite- i.e. we want to use relative paths if the user
*does* restrict the paths?  I usually think of this as whatever I tab
complete out will be echoed back to me exactly.  It seems like relative
paths are fairly ubiquitous, and the absolute paths are limited to a
handful of commands when not path restricted.  (I think there are some
debug commands that use abs files too, but that probably isn't as big of a
concern for BC).

As far as an independent flag, I don't see any other way to do it.  Not
sure what to call it- hopefully something pretty short because some of the
calling lines already wrap.

--Matt
Matt Harbison - Dec. 4, 2014, 5:59 p.m.
On Thu, Dec 4, 2014 at 12:40 PM, Martin von Zweigbergk <
martinvonz@google.com> wrote:

>
>
> [snip]
>>
>>
>>>   I didn't run the
>>> >> tests to see if that breaks anything.  (I'm not sure why it didn't
>>> also
>>> >> list the directory I was in as a relpath too.)
>>> >>
>>> >
>>> > Without having spent much thought to it, does 'not m.always()' work
>>> > better?
>>> > I ask because that's what it seemed like status and locate wanted when
>>> I
>>> > checked. It's just mostly the same for addremove since exact() was
>>> > already
>>> > discounted.
>>>
>>> I'm not sure.  My concern is that largefiles creates matchers with the
>>> _always field set to False.  See composenormalfilematcher() in
>>> largefiles/overrides.py.  I haven't sent the second half of the series
>>> yet, but that matcher will be passed to this code, and it basically
>>> eliminates the conditional when it is hardcoded to False.  It makes sense
>>> that the largefiles matcher is hardcoded, since it is excluding some
>>> files.  So this idea seems close, but a step in the wrong direction.
>>>
>>> It seems surprising to me that a list called patterns can be given to the
>>> matcher, and then the matcher reports that no patterns were given.  Maybe
>>> it is just an unfortunate similarity in the names, but it seems like that
>>> needs to be fixed (or better documented what the differences are).
>>>
>>> --Matt
>>>
>>
>> Does it seem fair to say that we want to use relative paths in the UI in
>> a few cases (addremove, status, locate) if the user has not restricted the
>> paths in any way? If it is, it seems like we would need a flag on the
>> matcher that is disconnected from what the matcher actually matches. That
>> might be your _anyfiles, but modified to be "bool(patterns or include or
>> exclude)" and maybe renamed to _userelativepathsinui (but shortened). *sigh*
>>
>
> But in that case it seems better to just keep the knowledge outside of
> matcher. Does the need to pass it around in the matcher arise from subrepo
> use cases? I guess I didn't get that part from your patches.
>

I'm not sure I understand.  I replaced the patterns list with a matcher,
because it seems like a matcher is more functional for subrepos.  E.g. the
pattern list isn't narrowed if I kept passing it along to subrepos, so it
is both (kind of) redundant with the matcher and wrong inside a subrepo.
The only thing it is good for at that point is to decide abs or rel.

It might be nice to move the ((pats and rel) or abs) logic into a matcher
function to hide that logic, and then however we store that info doesn't
have to be public on the matcher.  I just don't know if the pattern is used
enough to justify that move, and nobody chimed in about why some methods
use abs vs rel and others don't.  Passing along an external 'what print
style should is use' parameter seems ugly to me.

--Matt
Martin von Zweigbergk - Dec. 4, 2014, 9:27 p.m.
On Thu Dec 04 2014 at 9:59:20 AM Matt Harbison <mharbison72@gmail.com>
wrote:

>
>
> On Thu, Dec 4, 2014 at 12:40 PM, Martin von Zweigbergk <
> martinvonz@google.com> wrote:
>
>>
>>
>> [snip]
>>
>>
>>>
>>>
>>>>   I didn't run the
>>>> >> tests to see if that breaks anything.  (I'm not sure why it didn't
>>>> also
>>>> >> list the directory I was in as a relpath too.)
>>>> >>
>>>> >
>>>> > Without having spent much thought to it, does 'not m.always()' work
>>>> > better?
>>>> > I ask because that's what it seemed like status and locate wanted
>>>> when I
>>>> > checked. It's just mostly the same for addremove since exact() was
>>>> > already
>>>> > discounted.
>>>>
>>>> I'm not sure.  My concern is that largefiles creates matchers with the
>>>> _always field set to False.  See composenormalfilematcher() in
>>>> largefiles/overrides.py.  I haven't sent the second half of the series
>>>> yet, but that matcher will be passed to this code, and it basically
>>>> eliminates the conditional when it is hardcoded to False.  It makes
>>>> sense
>>>> that the largefiles matcher is hardcoded, since it is excluding some
>>>> files.  So this idea seems close, but a step in the wrong direction.
>>>>
>>>> It seems surprising to me that a list called patterns can be given to
>>>> the
>>>> matcher, and then the matcher reports that no patterns were given.
>>>> Maybe
>>>> it is just an unfortunate similarity in the names, but it seems like
>>>> that
>>>> needs to be fixed (or better documented what the differences are).
>>>>
>>>> --Matt
>>>>
>>>
>>> Does it seem fair to say that we want to use relative paths in the UI in
>>> a few cases (addremove, status, locate) if the user has not restricted the
>>> paths in any way? If it is, it seems like we would need a flag on the
>>> matcher that is disconnected from what the matcher actually matches. That
>>> might be your _anyfiles, but modified to be "bool(patterns or include
>>> or exclude)" and maybe renamed to _userelativepathsinui (but shortened).
>>> *sigh*
>>>
>> But in that case it seems better to just keep the knowledge outside of
>> matcher. Does the need to pass it around in the matcher arise from subrepo
>> use cases? I guess I didn't get that part from your patches.
>>
>
> I'm not sure I understand.  I replaced the patterns list with a matcher,
> because it seems like a matcher is more functional for subrepos.  E.g. the
> pattern list isn't narrowed if I kept passing it along to subrepos, so it
> is both (kind of) redundant with the matcher and wrong inside a subrepo.
> The only thing it is good for at that point is to decide abs or rel.
>

I'm not sure what the right thing to do is. Do you think it makes sense to
add simply add "_pathrestricted = bool(patterns or include or exclude)"
(feel free to pick another name) and the other things you added in your
patches?


> It might be nice to move the ((pats and rel) or abs) logic into a matcher
> function to hide that logic, and then however we store that info doesn't
> have to be public on the matcher.  I just don't know if the pattern is used
> enough to justify that move, and nobody chimed in about why some methods
> use abs vs rel and others don't.  Passing along an external 'what print
> style should is use' parameter seems ugly to me.
>

Maybe pathrestricted() needs to be checked except where we print the path.
If that's the case, you can even skip adding the method and just have a
'def uipath(f): return self._pathrestricted and self.rel(f) or f'.
Matt Harbison - Dec. 5, 2014, 3:48 a.m.
On Thu, 04 Dec 2014 16:27:30 -0500, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> On Thu Dec 04 2014 at 9:59:20 AM Matt Harbison <mharbison72@gmail.com>
> wrote:
>
>>
>>
>> On Thu, Dec 4, 2014 at 12:40 PM, Martin von Zweigbergk <
>> martinvonz@google.com> wrote:
>>
>>>
>>>
>>> [snip]
>>>
>>>
>>>>
>>>>
>>>>>   I didn't run the
>>>>> >> tests to see if that breaks anything.  (I'm not sure why it didn't
>>>>> also
>>>>> >> list the directory I was in as a relpath too.)
>>>>> >>
>>>>> >
>>>>> > Without having spent much thought to it, does 'not m.always()' work
>>>>> > better?
>>>>> > I ask because that's what it seemed like status and locate wanted
>>>>> when I
>>>>> > checked. It's just mostly the same for addremove since exact() was
>>>>> > already
>>>>> > discounted.
>>>>>
>>>>> I'm not sure.  My concern is that largefiles creates matchers with  
>>>>> the
>>>>> _always field set to False.  See composenormalfilematcher() in
>>>>> largefiles/overrides.py.  I haven't sent the second half of the  
>>>>> series
>>>>> yet, but that matcher will be passed to this code, and it basically
>>>>> eliminates the conditional when it is hardcoded to False.  It makes
>>>>> sense
>>>>> that the largefiles matcher is hardcoded, since it is excluding some
>>>>> files.  So this idea seems close, but a step in the wrong direction.
>>>>>
>>>>> It seems surprising to me that a list called patterns can be given to
>>>>> the
>>>>> matcher, and then the matcher reports that no patterns were given.
>>>>> Maybe
>>>>> it is just an unfortunate similarity in the names, but it seems like
>>>>> that
>>>>> needs to be fixed (or better documented what the differences are).
>>>>>
>>>>> --Matt
>>>>>
>>>>
>>>> Does it seem fair to say that we want to use relative paths in the UI  
>>>> in
>>>> a few cases (addremove, status, locate) if the user has not  
>>>> restricted the
>>>> paths in any way? If it is, it seems like we would need a flag on the
>>>> matcher that is disconnected from what the matcher actually matches.  
>>>> That
>>>> might be your _anyfiles, but modified to be "bool(patterns or include
>>>> or exclude)" and maybe renamed to _userelativepathsinui (but  
>>>> shortened).
>>>> *sigh*
>>>>
>>> But in that case it seems better to just keep the knowledge outside of
>>> matcher. Does the need to pass it around in the matcher arise from  
>>> subrepo
>>> use cases? I guess I didn't get that part from your patches.
>>>
>>
>> I'm not sure I understand.  I replaced the patterns list with a matcher,
>> because it seems like a matcher is more functional for subrepos.  E.g.  
>> the
>> pattern list isn't narrowed if I kept passing it along to subrepos, so  
>> it
>> is both (kind of) redundant with the matcher and wrong inside a subrepo.
>> The only thing it is good for at that point is to decide abs or rel.
>>
>
> I'm not sure what the right thing to do is. Do you think it makes sense  
> to
> add simply add "_pathrestricted = bool(patterns or include or exclude)"
> (feel free to pick another name) and the other things you added in your
> patches?

Yes, that's probably the simplest (and externally cleanest) thing to do.   
You were right about not modifying anypats()- commit calls this to make  
sure that -I/-X isn't used on commit, but does allow committing specific  
files, and that would have broke.


>> It might be nice to move the ((pats and rel) or abs) logic into a  
>> matcher
>> function to hide that logic, and then however we store that info doesn't
>> have to be public on the matcher.  I just don't know if the pattern is  
>> used
>> enough to justify that move, and nobody chimed in about why some methods
>> use abs vs rel and others don't.  Passing along an external 'what print
>> style should is use' parameter seems ugly to me.
>>
>
> Maybe pathrestricted() needs to be checked except where we print the  
> path.
> If that's the case, you can even skip adding the method and just have a
> 'def uipath(f): return self._pathrestricted and self.rel(f) or f'.

I agree with the latter, and like that name.  This worked with the entire  
series, so now I'm running the full test suite on Windows, and hopefully  
will submit it tonight.  (It takes almost 2 hours to run the full thing.)   
If accepted, I'll rebase, submit v3 of the series and carry on.

Thanks for brainstorming out loud on this.

--Matt

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -732,9 +732,9 @@ 
         if repo.ui.verbose or not m.exact(abs):
             rel = m.rel(abs)
             if abs in unknownset:
-                status = _('adding %s\n') % ((pats and rel) or abs)
+                status = _('adding %s\n') % ((m.anypats() and rel) or abs)
             else:
-                status = _('removing %s\n') % ((pats and rel) or abs)
+                status = _('removing %s\n') % ((m.anypats() and rel) or abs)
             repo.ui.status(status)
 
     renames = _findrenames(repo, m, added + unknown, removed + deleted,
diff --git a/tests/test-addremove.t b/tests/test-addremove.t
--- a/tests/test-addremove.t
+++ b/tests/test-addremove.t
@@ -24,6 +24,21 @@ 
   adding foo
   $ cd ..
 
+  $ hg init subdir
+  $ cd subdir
+  $ mkdir dir
+  $ cd dir
+  $ touch a.py
+  $ hg addremove 'glob:*.py'
+  adding a.py
+  $ hg forget a.py
+  $ hg addremove -I 'glob:*.py'
+  adding a.py
+  $ hg forget a.py
+  $ hg addremove
+  adding dir/a.py
+  $ cd ..
+
   $ hg init sim
   $ cd sim
   $ echo a > a