Patchwork D7570: match: resolve filesets against the passed `cwd`, not the current one

login
register
mail settings
Submitter phabricator
Date Dec. 7, 2019, 3:16 a.m.
Message ID <differential-rev-PHID-DREV-zra3xmlg6hs3a5y4hnud-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43618/
State Superseded
Headers show

Comments

phabricator - Dec. 7, 2019, 3:16 a.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This allows filesets to be resolved relative to `repo.root`, the same as other
  patterns are since f02d3c0eed18 <https://phab.mercurial-scm.org/rHGf02d3c0eed18be9a7a0790405e5de284d0727713>.  The current example in contrib/ wasn't working
  from the tests directory because of this.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

AFFECTED FILES
  mercurial/context.py
  mercurial/fileset.py
  mercurial/match.py
  mercurial/subrepo.py
  tests/test-fix.t

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 7, 2019, 3:44 a.m.
mharbison72 added a comment.
mharbison72 added a subscriber: martinvonz.


  CC: @martinvonz

INLINE COMMENTS

> test-fix.t:1336-1342
>    $ hg fix -r '.::'
>    $ hg cat -r . ../quux
>    quux
>    $ hg cat -r tip ../quux
> -  quux
> +  fs: $TESTTMP/subprocesscwd
>    $ cat ../quux
>    quux

I found a couple of things surprising here:

1. `.::` doesn't include fixing wdir()
2. `.` isn't updated unless `-w` is specified
3. `-w` doesn't alter `wdir()` content unless there are dirty files (makes sense), but //will// update `.` if starting with a clean `wdir()`

The first issue seems related to https://www.mercurial-scm.org/wiki/RevsetVirtualRevisionPlan

I can't think of any reason you'd want to fix `.` and not `wdir()` other than "I'm afraid the fixer will eat my uncommitted data".  Is there a reason not to do it by default, and instead require --no-working-dir to get the current behavior?  If there are uncommitted changes, it seems that the user would be caught with needing to commit on top of the obsolete commit, and then evolve/rebase and deal with the merge conflicts.

I ended up spending a bunch of time troubleshooting why the content of `-r .` didn't seem to be updated here, without realizing the automatic evolve was conditional.  Part of this was not being able to easily peek into the finished test to see the state of the repo.  I guess it makes sense to keep the diff from `wdir()` unchanged, and therefore keep the original parent.  But I think this confusion is another reason to default to `-w`.

Alternately if `.` is in the set of revisions being fixed and `wdir()` is dirty, maybe it can abort and hint to add `-w` or `--force`?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers
Cc: martinvonz, mercurial-devel
Yuya Nishihara - Dec. 7, 2019, 4:19 a.m.
>              if listsubrepos:
>                  for subpath in ctx.substate:
> -                    sm = ctx.sub(subpath).matchfileset(pat, badfn=badfn)
> +                    sm = ctx.sub(subpath).matchfileset(
> +                        pat, badfn=badfn, cwd=cwd
> +                    )

Might have to adjust cwd since it may be relative to the parent's repo.root.

>  class matchctx(object):
> -    def __init__(self, basectx, ctx, badfn=None):
> +    def __init__(self, basectx, ctx, badfn=None, cwd=None):
>          self._basectx = basectx
>          self.ctx = ctx
>          self._badfn = badfn
>          self._match = None
>          self._status = None
> +        self.cwd = cwd
>  
>      def narrowed(self, match):
>          """Create matchctx for a sub-tree narrowed by the given matcher"""

Perhaps self.cwd would have to be copied to the matchctx objects created
by itself.

One option to detect this kind of bugs is to remove the default parameters.
phabricator - Dec. 7, 2019, 4:56 a.m.
yuja added a comment.


  >   if listsubrepos:
  >       for subpath in ctx.substate:
  >
  > - sm = ctx.sub(subpath).matchfileset(pat, badfn=badfn)
  >
  > +                    sm = ctx.sub(subpath).matchfileset(
  > +                        pat, badfn=badfn, cwd=cwd
  > +                    )
  
  Might have to adjust cwd since it may be relative to the parent's repo.root.
  
  > class matchctx(object):
  >
  > - def __init__(self, basectx, ctx, badfn=None):
  >
  > +    def __init__(self, basectx, ctx, badfn=None, cwd=None):
  >
  >   self._basectx = basectx
  >   self.ctx = ctx
  >   self._badfn = badfn
  >   self._match = None
  >   self._status = None
  >
  > +        self.cwd = cwd
  >
  >   def narrowed(self, match):
  >       """Create matchctx for a sub-tree narrowed by the given matcher"""
  
  Perhaps self.cwd would have to be copied to the matchctx objects created
  by itself.
  
  One option to detect this kind of bugs is to remove the default parameters.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers
Cc: yuja, martinvonz, mercurial-devel
phabricator - Dec. 11, 2019, 5:24 a.m.
martinvonz added inline comments.
martinvonz added a subscriber: hooper.

INLINE COMMENTS

> mharbison72 wrote in test-fix.t:1336-1342
> I found a couple of things surprising here:
> 
> 1. `.::` doesn't include fixing wdir()
> 2. `.` isn't updated unless `-w` is specified
> 3. `-w` doesn't alter `wdir()` content unless there are dirty files (makes sense), but //will// update `.` if starting with a clean `wdir()`
> 
> The first issue seems related to https://www.mercurial-scm.org/wiki/RevsetVirtualRevisionPlan
> 
> I can't think of any reason you'd want to fix `.` and not `wdir()` other than "I'm afraid the fixer will eat my uncommitted data".  Is there a reason not to do it by default, and instead require --no-working-dir to get the current behavior?  If there are uncommitted changes, it seems that the user would be caught with needing to commit on top of the obsolete commit, and then evolve/rebase and deal with the merge conflicts.
> 
> I ended up spending a bunch of time troubleshooting why the content of `-r .` didn't seem to be updated here, without realizing the automatic evolve was conditional.  Part of this was not being able to easily peek into the finished test to see the state of the repo.  I guess it makes sense to keep the diff from `wdir()` unchanged, and therefore keep the original parent.  But I think this confusion is another reason to default to `-w`.
> 
> Alternately if `.` is in the set of revisions being fixed and `wdir()` is dirty, maybe it can abort and hint to add `-w` or `--force`?

> `.::` doesn't include fixing wdir()

Yes, that's how revsets work (as you noted)

> `.` isn't updated unless `-w` is specified

Yes, because otherwise the working directory would appear to undo the formatting changes. I don't remember why we don't just always format the working directory when we format the parent of the working directory. I'm sure @hooper remembers the reasoning better.

> `-w` doesn't alter `wdir()` content unless there are dirty files (makes sense), but will update `.` if starting with a clean `wdir()`

Do you mean with *just* `-w`? That shouldn't change any commits, so it seems very weird if it checks out a different commit. If you meant something like `-w -r .`, then that *would* format the working copy too, but it may not seem like it if the working copy was clean before and after the command (but it was formatted to match the parent commit).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers
Cc: hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 11, 2019, 6:46 a.m.
martinvonz added a comment.


  In D7570#111295 <https://phab.mercurial-scm.org/D7570#111295>, @yuja wrote:
  
  >>   if listsubrepos:
  >>       for subpath in ctx.substate:
  >>
  >> - sm = ctx.sub(subpath).matchfileset(pat, badfn=badfn)
  >>
  >> +                    sm = ctx.sub(subpath).matchfileset(
  >> +                        pat, badfn=badfn, cwd=cwd
  >> +                    )
  >
  > Might have to adjust cwd since it may be relative to the parent's repo.root.
  
  Do you think this patch is making it worse or can we leave it as is for now and let someone who cares about subrepos fix it?
  
  >> class matchctx(object):
  >>
  >> - def __init__(self, basectx, ctx, badfn=None):
  >>
  >> +    def __init__(self, basectx, ctx, badfn=None, cwd=None):
  >>
  >>   self._basectx = basectx
  >>   self.ctx = ctx
  >>   self._badfn = badfn
  >>   self._match = None
  >>   self._status = None
  >>
  >> +        self.cwd = cwd
  >>
  >>   def narrowed(self, match):
  >>       """Create matchctx for a sub-tree narrowed by the given matcher"""
  >
  > Perhaps self.cwd would have to be copied to the matchctx objects created
  > by itself.
  > One option to detect this kind of bugs is to remove the default parameters.
  
  Good idea, I've updated Matt's patch with that suggestion.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers
Cc: hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 11, 2019, 2:48 p.m.
mharbison72 added a comment.


  In D7570#111783 <https://phab.mercurial-scm.org/D7570#111783>, @martinvonz wrote:
  
  > In D7570#111295 <https://phab.mercurial-scm.org/D7570#111295>, @yuja wrote:
  >
  >>>   if listsubrepos:
  >>>       for subpath in ctx.substate:
  >>>
  >>> - sm = ctx.sub(subpath).matchfileset(pat, badfn=badfn)
  >>>
  >>> +                    sm = ctx.sub(subpath).matchfileset(
  >>> +                        pat, badfn=badfn, cwd=cwd
  >>> +                    )
  >>
  >> Might have to adjust cwd since it may be relative to the parent's repo.root.
  
  Is this case any different from `hg -R path/to/repo ...`?
  
  > Do you think this patch is making it worse or can we leave it as is for now and let someone who cares about subrepos fix it?
  
  I'll look and see how easy it is to drop the subrepo stuff for now.  It isn't operative until somebody adds a `-S` or support for breaking paths on a subrepo boundary to the command anyway.
  
  The real trap here is catching all of the subrepo references in the parent.  Say you have a repo that maps like this:
  
    2 -> S1
    1 -> S1
  
  If you `hg fix -r 2` and it edits S1, that will cause parent rev 1 to be dangling.  I guess we could modify `.hgsubstate` in 1 too without running fixers there.  But what if 1 is already public?  The preflight checks will be extensive.  That said, I'd like to get this sort of thing working because the UX with editing a subrepo is *really* bad, especially if you're editing anything other than the subrepo tip.
  
  >>> class matchctx(object):
  >>>
  >>> - def __init__(self, basectx, ctx, badfn=None):
  >>>
  >>> +    def __init__(self, basectx, ctx, badfn=None, cwd=None):
  >>>
  >>>   self._basectx = basectx
  >>>   self.ctx = ctx
  >>>   self._badfn = badfn
  >>>   self._match = None
  >>>   self._status = None
  >>>
  >>> +        self.cwd = cwd
  >>>
  >>>   def narrowed(self, match):
  >>>       """Create matchctx for a sub-tree narrowed by the given matcher"""
  >>
  >> Perhaps self.cwd would have to be copied to the matchctx objects created
  >> by itself.
  >> One option to detect this kind of bugs is to remove the default parameters.
  >
  > Good idea, I've updated Matt's patch with that suggestion.
  
  Thanks!

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers
Cc: hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 11, 2019, 5:37 p.m.
This revision is now accepted and ready to land.
durin42 added a comment.
durin42 accepted this revision.


  I'm happy with this, but didn't spend time figuring out if all concerns have been addressed (I'm mostly doing a fast triage path).

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 11, 2019, 5:50 p.m.
martinvonz added a comment.


  In D7570#111892 <https://phab.mercurial-scm.org/D7570#111892>, @durin42 wrote:
  
  > I'm happy with this, but didn't spend time figuring out if all concerns have been addressed (I'm mostly doing a fast triage path).
  
  I *think* they're addressed, but we there's no rush to get this in so let's give Yuya a chance to comment, because I'm not sure if the issue with subrepos is an existing or new issue.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 12, 2019, 4:37 p.m.
martinvonz added a comment.


  In D7570#111903 <https://phab.mercurial-scm.org/D7570#111903>, @martinvonz wrote:
  
  > In D7570#111892 <https://phab.mercurial-scm.org/D7570#111892>, @durin42 wrote:
  >
  >> I'm happy with this, but didn't spend time figuring out if all concerns have been addressed (I'm mostly doing a fast triage path).
  >
  > I *think* they're addressed, but we there's no rush to get this in so let's give Yuya a chance to comment, because I'm not sure if the issue with subrepos is an existing or new issue.
  
  @yuja, what do you think?

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 12, 2019, 9:29 p.m.
hooper added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-fix.t:1336-1342
> > `.::` doesn't include fixing wdir()
> 
> Yes, that's how revsets work (as you noted)
> 
> > `.` isn't updated unless `-w` is specified
> 
> Yes, because otherwise the working directory would appear to undo the formatting changes. I don't remember why we don't just always format the working directory when we format the parent of the working directory. I'm sure @hooper remembers the reasoning better.
> 
> > `-w` doesn't alter `wdir()` content unless there are dirty files (makes sense), but will update `.` if starting with a clean `wdir()`
> 
> Do you mean with *just* `-w`? That shouldn't change any commits, so it seems very weird if it checks out a different commit. If you meant something like `-w -r .`, then that *would* format the working copy too, but it may not seem like it if the working copy was clean before and after the command (but it was formatted to match the parent commit).

Martin and I talked about this, and I think it would make sense to:

1. Update/fix the wdir when the parent is fixed without -w, unless the wdir has uncommitted changes. We want to avoid triggering any merges, or clobbering uncommitted changes unless asked to. "Eating uncommitted data" is a very easy mistake to make when writing a config for hg fix, or when the formatter has a bug, etc.
2. Add a --source/-s flag to hg fix, where "-s foo" means "-r (foo)::" or "-r (foo):: -w" depending on whether "." is contained in "(foo)::". Similar to rebase.
3. Maybe remove the -r flag since it's a bit of a footgun.
4. Alternatively, keep the -r flag and make merges smart enough that "hg evolve" can apply formatters when that makes sense as a cleanup for "foo~-1" after "hg fix -r foo".

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
Yuya Nishihara - Dec. 12, 2019, 11:53 p.m.
>   > In D7570#111892 <https://phab.mercurial-scm.org/D7570#111892>, @durin42 wrote:
>   >
>   >> I'm happy with this, but didn't spend time figuring out if all concerns have been addressed (I'm mostly doing a fast triage path).
>   >
>   > I *think* they're addressed, but we there's no rush to get this in so let's give Yuya a chance to comment, because I'm not sure if the issue with subrepos is an existing or new issue.
>   
>   @yuja, what do you think?

I'm okay with this, but I would add "# TODO:" comment to subrepo handling
so future readers can see the problem. And one more nit: since `cwd=b''` is
a valid path, we have to test `cwd is None` explicitly.
phabricator - Dec. 13, 2019, 12:20 a.m.
yuja added a comment.


  >   > In D7570#111892 <https://phab.mercurial-scm.org/D7570#111892>, @durin42 wrote:
  >   >
  >   >> I'm happy with this, but didn't spend time figuring out if all concerns have been addressed (I'm mostly doing a fast triage path).
  >   >
  >   > I *think* they're addressed, but we there's no rush to get this in so let's give Yuya a chance to comment, because I'm not sure if the issue with subrepos is an existing or new issue.
  >   @yuja, what do you think?
  
  I'm okay with this, but I would add "# TODO:" comment to subrepo handling
  so future readers can see the problem. And one more nit: since `cwd=b''` is
  a valid path, we have to test `cwd is None` explicitly.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 13, 2019, 2:13 a.m.
mharbison72 added a comment.


  In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
  
  >>   @yuja, what do you think?
  >
  > I'm okay with this, but I would add "# TODO:" comment to subrepo handling
  > so future readers can see the problem. And one more nit: since `cwd=b''` is
  > a valid path, we have to test `cwd is None` explicitly.
  
  Good catch.  I don't see the meaning of b'' in this context defined anywhere.  But since `subinclude:` uses it to define the matcher, I'm assuming that's all of the adjustment we need?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 13, 2019, 7:52 p.m.
martinvonz added a comment.


  In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
  
  >>   > In D7570#111892 <https://phab.mercurial-scm.org/D7570#111892>, @durin42 wrote:
  >>   >
  >>   >> I'm happy with this, but didn't spend time figuring out if all concerns have been addressed (I'm mostly doing a fast triage path).
  >>   >
  >>   > I *think* they're addressed, but we there's no rush to get this in so let's give Yuya a chance to comment, because I'm not sure if the issue with subrepos is an existing or new issue.
  >>   @yuja, what do you think?
  >
  > I'm okay with this, but I would add "# TODO:" comment to subrepo handling
  > so future readers can see the problem. And one more nit: since `cwd=b''` is
  > a valid path, we have to test `cwd is None` explicitly.
  
  @yuja, maybe I've addressed both comments with the two new patches I added to the stack?

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 13, 2019, 8:39 p.m.
mharbison72 added a comment.


  In D7570#112436 <https://phab.mercurial-scm.org/D7570#112436>, @martinvonz wrote:
  
  > In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
  >
  >> I'm okay with this, but I would add "# TODO:" comment to subrepo handling
  >> so future readers can see the problem. And one more nit: since `cwd=b''` is
  >> a valid path, we have to test `cwd is None` explicitly.
  >
  > @yuja, maybe I've addressed both comments with the two new patches I added to the stack?
  
  This latest diff reverted the `cwd is None` changes.  IIUC, the TODO shouldn't be needed anymore.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 13, 2019, 10:14 p.m.
martinvonz added a comment.


  In D7570#112516 <https://phab.mercurial-scm.org/D7570#112516>, @mharbison72 wrote:
  
  > In D7570#112436 <https://phab.mercurial-scm.org/D7570#112436>, @martinvonz wrote:
  >
  >> In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
  >>
  >>> I'm okay with this, but I would add "# TODO:" comment to subrepo handling
  >>> so future readers can see the problem. And one more nit: since `cwd=b''` is
  >>> a valid path, we have to test `cwd is None` explicitly.
  >>
  >> @yuja, maybe I've addressed both comments with the two new patches I added to the stack?
  >
  > This latest diff reverted the `cwd is None` changes.  IIUC, the TODO shouldn't be needed anymore.
  
  Oops, I apparently removed only my TODO in match.py, not in subrepo.py. Done now. Thanks.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 14, 2019, 2:35 a.m.
mharbison72 added a comment.


  In D7570#112518 <https://phab.mercurial-scm.org/D7570#112518>, @martinvonz wrote:
  
  > In D7570#112516 <https://phab.mercurial-scm.org/D7570#112516>, @mharbison72 wrote:
  >
  >> In D7570#112436 <https://phab.mercurial-scm.org/D7570#112436>, @martinvonz wrote:
  >>
  >>> In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
  >>>
  >>>> I'm okay with this, but I would add "# TODO:" comment to subrepo handling
  >>>> so future readers can see the problem. And one more nit: since `cwd=b''` is
  >>>> a valid path, we have to test `cwd is None` explicitly.
  >>>
  >>> @yuja, maybe I've addressed both comments with the two new patches I added to the stack?
  >>
  >> This latest diff reverted the `cwd is None` changes.  IIUC, the TODO shouldn't be needed anymore.
  >
  > Oops, I apparently removed only my TODO in match.py, not in subrepo.py. Done now. Thanks.
  
  It looks like the two `if cwd is None` lines are still reverted to `if not cwd` by comparing diff 3 to 5.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
Yuya Nishihara - Dec. 14, 2019, 9:26 a.m.
>   In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
>   Good catch.  I don't see the meaning of b'' in this context defined anywhere.  But since `subinclude:` uses it to define the matcher, I'm assuming that's all of the adjustment we need?

It's basically the value meaning `cwd == repo.root`. See dirstate.getcwd().
phabricator - Dec. 14, 2019, 9:28 a.m.
yuja added a comment.


  >   In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
  >   Good catch.  I don't see the meaning of b'' in this context defined anywhere.  But since `subinclude:` uses it to define the matcher, I'm assuming that's all of the adjustment we need?
  
  It's basically the value meaning `cwd == repo.root`. See dirstate.getcwd().

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 16, 2019, 11:21 p.m.
martinvonz added a comment.


  In D7570#112576 <https://phab.mercurial-scm.org/D7570#112576>, @mharbison72 wrote:
  
  > In D7570#112518 <https://phab.mercurial-scm.org/D7570#112518>, @martinvonz wrote:
  >
  >> In D7570#112516 <https://phab.mercurial-scm.org/D7570#112516>, @mharbison72 wrote:
  >>
  >>> In D7570#112436 <https://phab.mercurial-scm.org/D7570#112436>, @martinvonz wrote:
  >>>
  >>>> In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
  >>>>
  >>>>> I'm okay with this, but I would add "# TODO:" comment to subrepo handling
  >>>>> so future readers can see the problem. And one more nit: since `cwd=b''` is
  >>>>> a valid path, we have to test `cwd is None` explicitly.
  >>>>
  >>>> @yuja, maybe I've addressed both comments with the two new patches I added to the stack?
  >>>
  >>> This latest diff reverted the `cwd is None` changes.  IIUC, the TODO shouldn't be needed anymore.
  >>
  >> Oops, I apparently removed only my TODO in match.py, not in subrepo.py. Done now. Thanks.
  >
  > It looks like the two `if cwd is None` lines are still reverted to `if not cwd` by comparing diff 3 to 5.
  
  Oh, because you fixed that and then I overwrote your changes? Because I don't remember changing it. Anyway, should be fine now that `cwd` is an absolute path, right?

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel
phabricator - Dec. 17, 2019, 2:06 a.m.
mharbison72 added a comment.


  In D7570#112809 <https://phab.mercurial-scm.org/D7570#112809>, @martinvonz wrote:
  
  > In D7570#112576 <https://phab.mercurial-scm.org/D7570#112576>, @mharbison72 wrote:
  >
  >> It looks like the two `if cwd is None` lines are still reverted to `if not cwd` by comparing diff 3 to 5.
  >
  > Oh, because you fixed that and then I overwrote your changes? Because I don't remember changing it.
  
  Yeah, that's probably all it was.
  
  > Anyway, should be fine now that `cwd` is an absolute path, right?
  
  Yeah, it looks like the way you handled it would still allow `cwd=''` input.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7570/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7570

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, hooper, yuja, martinvonz, mercurial-devel

Patch

diff --git a/tests/test-fix.t b/tests/test-fix.t
--- a/tests/test-fix.t
+++ b/tests/test-fix.t
@@ -1333,24 +1333,20 @@ 
 Apparently fixing p1() and its descendants doesn't include wdir() unless
 explicitly stated.
 
-BROKEN: fileset matches aren't relative to repo.root for commits
-
   $ hg fix -r '.::'
   $ hg cat -r . ../quux
   quux
   $ hg cat -r tip ../quux
-  quux
+  fs: $TESTTMP/subprocesscwd
   $ cat ../quux
   quux
 
 Clean files are not fixed unless explicitly named
   $ echo 'dirty' > ../quux
 
-BROKEN: fileset matches aren't relative to repo.root for wdir
-
   $ hg fix --working-dir
   $ cat ../quux
-  dirty
+  fs: $TESTTMP/subprocesscwd
 
   $ cd ../..
 
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -355,7 +355,7 @@ 
         """return file flags"""
         return b''
 
-    def matchfileset(self, expr, badfn=None):
+    def matchfileset(self, expr, badfn=None, cwd=None):
         """Resolve the fileset expression for this repo"""
         return matchmod.never(badfn=badfn)
 
@@ -894,20 +894,20 @@ 
         return cmdutil.files(ui, ctx, m, uipathfn, fm, fmt, subrepos)
 
     @annotatesubrepoerror
-    def matchfileset(self, expr, badfn=None):
+    def matchfileset(self, expr, badfn=None, cwd=None):
         if self._ctx.rev() is None:
             ctx = self._repo[None]
         else:
             rev = self._state[1]
             ctx = self._repo[rev]
 
-        matchers = [ctx.matchfileset(expr, badfn=badfn)]
+        matchers = [ctx.matchfileset(expr, badfn=badfn, cwd=cwd)]
 
         for subpath in ctx.substate:
             sub = ctx.sub(subpath)
 
             try:
-                sm = sub.matchfileset(expr, badfn=badfn)
+                sm = sub.matchfileset(expr, badfn=badfn, cwd=cwd)
                 pm = matchmod.prefixdirmatcher(subpath, sm, badfn=badfn)
                 matchers.append(pm)
             except error.LookupError:
diff --git a/mercurial/match.py b/mercurial/match.py
--- a/mercurial/match.py
+++ b/mercurial/match.py
@@ -57,7 +57,7 @@ 
         return m.match
 
 
-def _expandsets(kindpats, ctx=None, listsubrepos=False, badfn=None):
+def _expandsets(kindpats, ctx=None, listsubrepos=False, badfn=None, cwd=None):
     '''Returns the kindpats list with the 'set' patterns expanded to matchers'''
     matchers = []
     other = []
@@ -68,11 +68,13 @@ 
                 raise error.ProgrammingError(
                     b"fileset expression with no context"
                 )
-            matchers.append(ctx.matchfileset(pat, badfn=badfn))
+            matchers.append(ctx.matchfileset(pat, badfn=badfn, cwd=cwd))
 
             if listsubrepos:
                 for subpath in ctx.substate:
-                    sm = ctx.sub(subpath).matchfileset(pat, badfn=badfn)
+                    sm = ctx.sub(subpath).matchfileset(
+                        pat, badfn=badfn, cwd=cwd
+                    )
                     pm = prefixdirmatcher(subpath, sm, badfn=badfn)
                     matchers.append(pm)
 
@@ -117,11 +119,17 @@ 
 
 
 def _buildkindpatsmatcher(
-    matchercls, root, kindpats, ctx=None, listsubrepos=False, badfn=None
+    matchercls,
+    root,
+    kindpats,
+    ctx=None,
+    listsubrepos=False,
+    badfn=None,
+    cwd=None,
 ):
     matchers = []
     fms, kindpats = _expandsets(
-        kindpats, ctx=ctx, listsubrepos=listsubrepos, badfn=badfn
+        kindpats, ctx=ctx, listsubrepos=listsubrepos, badfn=badfn, cwd=cwd,
     )
     if kindpats:
         m = matchercls(root, kindpats, badfn=badfn)
@@ -260,6 +268,7 @@ 
                 ctx=ctx,
                 listsubrepos=listsubrepos,
                 badfn=badfn,
+                cwd=cwd,
             )
     else:
         # It's a little strange that no patterns means to match everything.
@@ -275,6 +284,7 @@ 
             ctx=ctx,
             listsubrepos=listsubrepos,
             badfn=None,
+            cwd=cwd,
         )
         m = intersectmatchers(m, im)
     if exclude:
@@ -286,6 +296,7 @@ 
             ctx=ctx,
             listsubrepos=listsubrepos,
             badfn=None,
+            cwd=cwd,
         )
         m = differencematcher(m, em)
     return m
diff --git a/mercurial/fileset.py b/mercurial/fileset.py
--- a/mercurial/fileset.py
+++ b/mercurial/fileset.py
@@ -520,12 +520,13 @@ 
 
 
 class matchctx(object):
-    def __init__(self, basectx, ctx, badfn=None):
+    def __init__(self, basectx, ctx, badfn=None, cwd=None):
         self._basectx = basectx
         self.ctx = ctx
         self._badfn = badfn
         self._match = None
         self._status = None
+        self.cwd = cwd
 
     def narrowed(self, match):
         """Create matchctx for a sub-tree narrowed by the given matcher"""
@@ -560,7 +561,7 @@ 
         return self._status
 
     def matcher(self, patterns):
-        return self.ctx.match(patterns, badfn=self._badfn)
+        return self.ctx.match(patterns, badfn=self._badfn, cwd=self.cwd)
 
     def predicate(self, predfn, predrepr=None, cache=False):
         """Create a matcher to select files by predfn(filename)"""
@@ -617,12 +618,12 @@ 
         return matchmod.never(badfn=self._badfn)
 
 
-def match(ctx, expr, badfn=None):
+def match(ctx, expr, badfn=None, cwd=None):
     """Create a matcher for a single fileset expression"""
     tree = filesetlang.parse(expr)
     tree = filesetlang.analyze(tree)
     tree = filesetlang.optimize(tree)
-    mctx = matchctx(ctx.p1(), ctx, badfn=badfn)
+    mctx = matchctx(ctx.p1(), ctx, badfn=badfn, cwd=cwd)
     return getmatch(mctx, tree)
 
 
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -200,8 +200,8 @@ 
     def mutable(self):
         return self.phase() > phases.public
 
-    def matchfileset(self, expr, badfn=None):
-        return fileset.match(self, expr, badfn=badfn)
+    def matchfileset(self, expr, badfn=None, cwd=None):
+        return fileset.match(self, expr, badfn=badfn, cwd=cwd)
 
     def obsolete(self):
         """True if the changeset is obsolete"""
@@ -328,11 +328,14 @@ 
         default=b'glob',
         listsubrepos=False,
         badfn=None,
+        cwd=None,
     ):
         r = self._repo
+        if not cwd:
+            cwd = r.getcwd()
         return matchmod.match(
             r.root,
-            r.getcwd(),
+            cwd,
             pats,
             include,
             exclude,
@@ -1694,15 +1697,18 @@ 
         default=b'glob',
         listsubrepos=False,
         badfn=None,
+        cwd=None,
     ):
         r = self._repo
+        if not cwd:
+            cwd = r.getcwd()
 
         # Only a case insensitive filesystem needs magic to translate user input
         # to actual case in the filesystem.
         icasefs = not util.fscasesensitive(r.root)
         return matchmod.match(
             r.root,
-            r.getcwd(),
+            cwd,
             pats,
             include,
             exclude,