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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rHG Mercurial
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
CC: @martinvonz
| tests/test-fix.t | ||
|---|---|---|
| 1338–1344 | I found a couple of things surprising here:
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? | |
Comment Actions
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.
| tests/test-fix.t | ||
|---|---|---|
| 1338–1344 |
Yes, that's how revsets work (as you noted)
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.
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). | |
Comment Actions
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.
Comment Actions
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!
Comment Actions
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).
Comment Actions
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.
| tests/test-fix.t | ||
|---|---|---|
| 1338–1344 | Martin and I talked about this, and I think it would make sense to:
| |
Comment Actions
> 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.
Comment Actions
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?
Comment Actions
@yuja, maybe I've addressed both comments with the two new patches I added to the stack?
Comment Actions
This latest diff reverted the cwd is None changes. IIUC, the TODO shouldn't be needed anymore.
Comment Actions
Oops, I apparently removed only my TODO in match.py, not in subrepo.py. Done now. Thanks.
Comment Actions
It looks like the two if cwd is None lines are still reverted to if not cwd by comparing diff 3 to 5.
Comment Actions
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().
Comment Actions
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?
Comment Actions
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.
I found a couple of things surprising here:
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?