( )⚙ D7570 match: resolve filesets against the passed `cwd`, not the current one

This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by mharbison72 on Dec 6 2019, 10:15 PM.
Tags
None
Subscribers

Details

Summary

This allows filesets to be resolved relative to repo.root, the same as other
patterns are since f02d3c0eed18. The current example in contrib/ wasn't working
from the tests directory because of this.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

mharbison72 created this revision.Dec 6 2019, 10:15 PM
tests/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?

yuja added a subscriber: yuja.Dec 6 2019, 11:56 PM
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.

martinvonz added inline comments.
tests/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).

martinvonz updated this revision to Diff 18596.Dec 11 2019, 1:36 AM
In 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.

In 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!

durin42 accepted this revision.Dec 11 2019, 12:37 PM
durin42 added a subscriber: durin42.

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

This revision is now accepted and ready to land.Dec 11 2019, 12:37 PM

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.

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?

hooper added inline comments.Dec 12 2019, 4:29 PM
tests/test-fix.t
1336–1342

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".
yuja added a comment.Dec 12 2019, 7:20 PM
> 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.

In 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?

In 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?

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

martinvonz updated this revision to Diff 18707.Dec 13 2019, 5:13 PM
In 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.

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

yuja added a comment.Dec 14 2019, 4:28 AM
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().

In 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?

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.

martinvonz updated this revision to Diff 18895.Dec 20 2019, 9:52 AM
martinvonz updated this revision to Diff 19072.Jan 8 2020, 11:54 AM