This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7570

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

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

Download Raw Diff

Details

Reviewers

durin42

Group Reviewers

hg-reviewers

Commits

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

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

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

mharbison72 created this revision.Dec 6 2019, 10:15 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptDec 6 2019, 10:15 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

CC: @martinvonz

tests/test-fix.t
1336–1342	I found a couple of things surprising here: `.::` doesn't include fixing wdir() `.` isn't updated unless `-w` is specified `-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`?

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 a subscriber: hooper.Dec 11 2019, 12:24 AM

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#111783, @martinvonz wrote:
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!

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

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

In D7570#111903, @martinvonz wrote:

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

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: 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. 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. Maybe remove the -r flag since it's a bit of a footgun. 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".

mharbison72 mentioned this in D7617: fix: make example fix hgrc work in subdirs.Dec 12 2019, 5:54 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.

mharbison72 updated this revision to Diff 18664.Dec 12 2019, 8:59 PM

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?

martinvonz edited parent revisions, added: D7650: match: normalize `cwd` early; removed: D7569: tests: show that fileset patterns don't work with `fix` when not in repo root.Dec 13 2019, 2:50 PM

martinvonz updated this revision to Diff 18696.

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#112436, @martinvonz wrote:

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#112516, @mharbison72 wrote:

In D7570#112436, @martinvonz wrote:

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#112518, @martinvonz wrote:

In D7570#112516, @mharbison72 wrote:

In D7570#112436, @martinvonz wrote:

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.

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#112576, @mharbison72 wrote:

In D7570#112518, @martinvonz wrote:

In D7570#112516, @mharbison72 wrote:

In D7570#112436, @martinvonz wrote:

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?

In D7570#112809, @martinvonz wrote:

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

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

mharbison72 added a commit: rHG28ea223a65c5: match: resolve filesets against the passed `cwd`, not the current one.Jan 8 2020, 12:20 PM

Closed by commit rHG28ea223a65c5: match: resolve filesets against the passed `cwd`, not the current one (authored by mharbison72). · Explain Why

This revision was automatically updated to reflect the committed changes.

mharbison72 added a commit: rHGe685fac56693: match: resolve filesets against the passed `cwd`, not the current one.Jan 8 2020, 1:10 PM

Revision Contents
Changeset List

			Path	Packages
M			hgext/highlight/__init__.py (2 lines)
M			mercurial/context.py (14 lines)
M			mercurial/debugcommands.py (2 lines)
M			mercurial/fileset.py (15 lines)
M			mercurial/match.py (15 lines)
M			mercurial/subrepo.py (8 lines)
M			tests/test-fix.t (8 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	18508		Dec 6 2019, 10:15 PM	★	★
Diff 2	18596		Dec 11 2019, 1:36 AM	★	★
Diff 3	18664		Dec 12 2019, 8:59 PM	★	★
Diff 4	18696		Dec 13 2019, 2:50 PM	★	★
Diff 5	18707		Dec 13 2019, 5:13 PM	★	★
Diff 6	18895		Dec 20 2019, 9:52 AM	★	★
Diff 7	19072		Jan 8 2020, 11:54 AM	★	★
Diff 8	19073	rHG28ea223a65c57f17bed1ea42280ac744de4e70e6	Dec 6 2019, 8:40 PM	★	★

Commit	Parents	Author	Summary	Date
108743215979	bb8338451e5a	Matt Harbison		Dec 6 2019, 8:40 PM

Status	Author	Revision
Closed	mharbison72	D7570 match: resolve filesets against the passed `cwd`, not the current one
Closed	martinvonz	D7650 match: normalize `cwd` early
Closed	martinvonz	D7649 match: make sure `root` argument is always an absolute path (API)
Closed	mharbison72	D7569 tests: show that fileset patterns don't work with `fix` when not in repo root

Diff 18664

hgext/highlight/init.py



	def pygmentize(web, field, fctx, tmpl):			def pygmentize(web, field, fctx, tmpl):
	style = web.config(b'web', b'pygments_style', b'colorful')			style = web.config(b'web', b'pygments_style', b'colorful')
	expr = web.config(b'web', b'highlightfiles', b"size('<5M')")			expr = web.config(b'web', b'highlightfiles', b"size('<5M')")
	filenameonly = web.configbool(b'web', b'highlightonlymatchfilename', False)			filenameonly = web.configbool(b'web', b'highlightonlymatchfilename', False)

	ctx = fctx.changectx()			ctx = fctx.changectx()
	m = ctx.matchfileset(expr)			m = ctx.matchfileset(fctx.repo().root, expr)
	if m(fctx.path()):			if m(fctx.path()):
	highlight.pygmentize(			highlight.pygmentize(
	field, fctx, style, tmpl, guessfilenameonly=filenameonly			field, fctx, style, tmpl, guessfilenameonly=filenameonly
	)			)


	def filerevision_highlight(orig, web, fctx):			def filerevision_highlight(orig, web, fctx):
	mt = web.res.headers[b'Content-Type']			mt = web.res.headers[b'Content-Type']

mercurial/context.py

	return self._repo			return self._repo

	def phasestr(self):			def phasestr(self):
	return phases.phasenames[self.phase()]			return phases.phasenames[self.phase()]

	def mutable(self):			def mutable(self):
	return self.phase() > phases.public			return self.phase() > phases.public

	def matchfileset(self, expr, badfn=None):			def matchfileset(self, cwd, expr, badfn=None):
	return fileset.match(self, expr, badfn=badfn)			return fileset.match(self, cwd, expr, badfn=badfn)

	def obsolete(self):			def obsolete(self):
	"""True if the changeset is obsolete"""			"""True if the changeset is obsolete"""
	return self.rev() in obsmod.getrevs(self._repo, b'obsolete')			return self.rev() in obsmod.getrevs(self._repo, b'obsolete')

	def extinct(self):			def extinct(self):
	"""True if the changeset is extinct"""			"""True if the changeset is extinct"""
	return self.rev() in obsmod.getrevs(self._repo, b'extinct')			return self.rev() in obsmod.getrevs(self._repo, b'extinct')
	def match(			def match(
	self,			self,
	pats=None,			pats=None,
	include=None,			include=None,
	exclude=None,			exclude=None,
	default=b'glob',			default=b'glob',
	listsubrepos=False,			listsubrepos=False,
	badfn=None,			badfn=None,
				cwd=None,
	):			):
	r = self._repo			r = self._repo
				if cwd is None:
				cwd = r.getcwd()
	return matchmod.match(			return matchmod.match(
	r.root,			r.root,
	r.getcwd(),			cwd,
	pats,			pats,
	include,			include,
	exclude,			exclude,
	default,			default,
	auditor=r.nofsauditor,			auditor=r.nofsauditor,
	ctx=self,			ctx=self,
	listsubrepos=listsubrepos,			listsubrepos=listsubrepos,
	badfn=badfn,			badfn=badfn,
	def match(			def match(
	self,			self,
	pats=None,			pats=None,
	include=None,			include=None,
	exclude=None,			exclude=None,
	default=b'glob',			default=b'glob',
	listsubrepos=False,			listsubrepos=False,
	badfn=None,			badfn=None,
				cwd=None,
	):			):
	r = self._repo			r = self._repo
				if cwd is None:
				cwd = r.getcwd()

	# Only a case insensitive filesystem needs magic to translate user input			# Only a case insensitive filesystem needs magic to translate user input
	# to actual case in the filesystem.			# to actual case in the filesystem.
	icasefs = not util.fscasesensitive(r.root)			icasefs = not util.fscasesensitive(r.root)
	return matchmod.match(			return matchmod.match(
	r.root,			r.root,
	r.getcwd(),			cwd,
	pats,			pats,
	include,			include,
	exclude,			exclude,
	default,			default,
	auditor=r.auditor,			auditor=r.auditor,
	ctx=self,			ctx=self,
	listsubrepos=listsubrepos,			listsubrepos=listsubrepos,
	badfn=badfn,			badfn=badfn,

mercurial/debugcommands.py

	ignored=True,			ignored=True,
	)			)
	)			)
	files.update(wctx.substate)			files.update(wctx.substate)
	else:			else:
	files.update(ctx.files())			files.update(ctx.files())
	files.update(ctx.substate)			files.update(ctx.substate)

	m = ctx.matchfileset(expr)			m = ctx.matchfileset(repo.getcwd(), expr)
	if opts[b'show_matcher'] or (opts[b'show_matcher'] is None and ui.verbose):			if opts[b'show_matcher'] or (opts[b'show_matcher'] is None and ui.verbose):
	ui.writenoi18n(b'* matcher:\n', stringutil.prettyrepr(m), b'\n')			ui.writenoi18n(b'* matcher:\n', stringutil.prettyrepr(m), b'\n')
	for f in sorted(files):			for f in sorted(files):
	if not m(f):			if not m(f):
	continue			continue
	ui.write(b"%s\n" % f)			ui.write(b"%s\n" % f)

mercurial/fileset.py

	b'minus': minusmatch,			b'minus': minusmatch,
	b'list': listmatch,			b'list': listmatch,
	b'not': notmatch,			b'not': notmatch,
	b'func': func,			b'func': func,
	}			}


	class matchctx(object):			class matchctx(object):
	def __init__(self, basectx, ctx, badfn=None):			def __init__(self, basectx, ctx, cwd, badfn=None):
	self._basectx = basectx			self._basectx = basectx
	self.ctx = ctx			self.ctx = ctx
	self._badfn = badfn			self._badfn = badfn
	self._match = None			self._match = None
	self._status = None			self._status = None
				self.cwd = cwd

	def narrowed(self, match):			def narrowed(self, match):
	"""Create matchctx for a sub-tree narrowed by the given matcher"""			"""Create matchctx for a sub-tree narrowed by the given matcher"""
	mctx = matchctx(self._basectx, self.ctx, self._badfn)			mctx = matchctx(self._basectx, self.ctx, self.cwd, self._badfn)
	mctx._match = match			mctx._match = match
	# leave wider status which we don't have to care			# leave wider status which we don't have to care
	mctx._status = self._status			mctx._status = self._status
	return mctx			return mctx

	def switch(self, basectx, ctx):			def switch(self, basectx, ctx):
	mctx = matchctx(basectx, ctx, self._badfn)			mctx = matchctx(basectx, ctx, self.cwd, self._badfn)
	mctx._match = self._match			mctx._match = self._match
	return mctx			return mctx

	def withstatus(self, keys):			def withstatus(self, keys):
	"""Create matchctx which has precomputed status specified by the keys"""			"""Create matchctx which has precomputed status specified by the keys"""
	mctx = matchctx(self._basectx, self.ctx, self._badfn)			mctx = matchctx(self._basectx, self.ctx, self.cwd, self._badfn)
	mctx._match = self._match			mctx._match = self._match
	mctx._buildstatus(keys)			mctx._buildstatus(keys)
	return mctx			return mctx

	def _buildstatus(self, keys):			def _buildstatus(self, keys):
	self._status = self._basectx.status(			self._status = self._basectx.status(
	self.ctx,			self.ctx,
	self._match,			self._match,
	listignored=b'ignored' in keys,			listignored=b'ignored' in keys,
	listclean=b'clean' in keys,			listclean=b'clean' in keys,
	listunknown=b'unknown' in keys,			listunknown=b'unknown' in keys,
	)			)

	def status(self):			def status(self):
	return self._status			return self._status

	def matcher(self, patterns):			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):			def predicate(self, predfn, predrepr=None, cache=False):
	"""Create a matcher to select files by predfn(filename)"""			"""Create a matcher to select files by predfn(filename)"""
	if cache:			if cache:
	predfn = util.cachefunc(predfn)			predfn = util.cachefunc(predfn)
	return matchmod.predicatematcher(			return matchmod.predicatematcher(
	predfn, predrepr=predrepr, badfn=self._badfn			predfn, predrepr=predrepr, badfn=self._badfn
	)			)

	return self.predicate(fctxpredfn, predrepr=predrepr, cache=cache)			return self.predicate(fctxpredfn, predrepr=predrepr, cache=cache)

	def never(self):			def never(self):
	"""Create a matcher to select nothing"""			"""Create a matcher to select nothing"""
	return matchmod.never(badfn=self._badfn)			return matchmod.never(badfn=self._badfn)


	def match(ctx, expr, badfn=None):			def match(ctx, cwd, expr, badfn=None):
	"""Create a matcher for a single fileset expression"""			"""Create a matcher for a single fileset expression"""
	tree = filesetlang.parse(expr)			tree = filesetlang.parse(expr)
	tree = filesetlang.analyze(tree)			tree = filesetlang.analyze(tree)
	tree = filesetlang.optimize(tree)			tree = filesetlang.optimize(tree)
	mctx = matchctx(ctx.p1(), ctx, badfn=badfn)			mctx = matchctx(ctx.p1(), ctx, cwd, badfn=badfn)
	return getmatch(mctx, tree)			return getmatch(mctx, tree)


	def loadpredicate(ui, extname, registrarobj):			def loadpredicate(ui, extname, registrarobj):
	"""Load fileset predicates from specified registrarobj			"""Load fileset predicates from specified registrarobj
	"""			"""
	for name, func in pycompat.iteritems(registrarobj._table):			for name, func in pycompat.iteritems(registrarobj._table):
	symbols[name] = func			symbols[name] = func


	# tell hggettext to extract docstrings from these functions:			# tell hggettext to extract docstrings from these functions:
	i18nfunctions = symbols.values()			i18nfunctions = symbols.values()

mercurial/match.py

	m = util.re.compile(regex)			m = util.re.compile(regex)
	try:			try:
	# slightly faster, provided by facebook's re2 bindings			# slightly faster, provided by facebook's re2 bindings
	return m.test_match			return m.test_match
	except AttributeError:			except AttributeError:
	return m.match			return m.match


	def _expandsets(kindpats, ctx=None, listsubrepos=False, badfn=None):			def _expandsets(cwd, kindpats, ctx=None, listsubrepos=False, badfn=None):
	'''Returns the kindpats list with the 'set' patterns expanded to matchers'''			'''Returns the kindpats list with the 'set' patterns expanded to matchers'''
	matchers = []			matchers = []
	other = []			other = []

	for kind, pat, source in kindpats:			for kind, pat, source in kindpats:
	if kind == b'set':			if kind == b'set':
	if ctx is None:			if ctx is None:
	raise error.ProgrammingError(			raise error.ProgrammingError(
	b"fileset expression with no context"			b"fileset expression with no context"
	)			)
	matchers.append(ctx.matchfileset(pat, badfn=badfn))			matchers.append(ctx.matchfileset(cwd, pat, badfn=badfn))

	if listsubrepos:			if listsubrepos:
	for subpath in ctx.substate:			for subpath in ctx.substate:
	sm = ctx.sub(subpath).matchfileset(pat, badfn=badfn)			# TODO: cwd may be relative to the parent's repo.root, and
				# need to be adjusted.
				sm = ctx.sub(subpath).matchfileset(cwd, pat, badfn=badfn)
	pm = prefixdirmatcher(subpath, sm, badfn=badfn)			pm = prefixdirmatcher(subpath, sm, badfn=badfn)
	matchers.append(pm)			matchers.append(pm)

	continue			continue
	other.append((kind, pat, source))			other.append((kind, pat, source))
	return matchers, other			return matchers, other


	"""			"""
	for kind, pat, source in kindpats:			for kind, pat, source in kindpats:
	if pat != b'' or kind not in [b'relpath', b'glob']:			if pat != b'' or kind not in [b'relpath', b'glob']:
	return False			return False
	return True			return True


	def _buildkindpatsmatcher(			def _buildkindpatsmatcher(
	matchercls, root, kindpats, ctx=None, listsubrepos=False, badfn=None			matchercls, root, cwd, kindpats, ctx=None, listsubrepos=False, badfn=None,
	):			):
	matchers = []			matchers = []
	fms, kindpats = _expandsets(			fms, kindpats = _expandsets(
	kindpats, ctx=ctx, listsubrepos=listsubrepos, badfn=badfn			cwd, kindpats, ctx=ctx, listsubrepos=listsubrepos, badfn=badfn,
	)			)
	if kindpats:			if kindpats:
	m = matchercls(root, kindpats, badfn=badfn)			m = matchercls(root, kindpats, badfn=badfn)
	matchers.append(m)			matchers.append(m)
	if fms:			if fms:
	matchers.extend(fms)			matchers.extend(fms)
	if not matchers:			if not matchers:
	return nevermatcher(badfn=badfn)			return nevermatcher(badfn=badfn)
	if patterns:			if patterns:
	kindpats = normalize(patterns, default, root, cwd, auditor, warn)			kindpats = normalize(patterns, default, root, cwd, auditor, warn)
	if _kindpatsalwaysmatch(kindpats):			if _kindpatsalwaysmatch(kindpats):
	m = alwaysmatcher(badfn)			m = alwaysmatcher(badfn)
	else:			else:
	m = _buildkindpatsmatcher(			m = _buildkindpatsmatcher(
	patternmatcher,			patternmatcher,
	root,			root,
				cwd,
	kindpats,			kindpats,
	ctx=ctx,			ctx=ctx,
	listsubrepos=listsubrepos,			listsubrepos=listsubrepos,
	badfn=badfn,			badfn=badfn,
	)			)
	else:			else:
	# It's a little strange that no patterns means to match everything.			# It's a little strange that no patterns means to match everything.
	# Consider changing this to match nothing (probably using nevermatcher).			# Consider changing this to match nothing (probably using nevermatcher).
	m = alwaysmatcher(badfn)			m = alwaysmatcher(badfn)

	if include:			if include:
	kindpats = normalize(include, b'glob', root, cwd, auditor, warn)			kindpats = normalize(include, b'glob', root, cwd, auditor, warn)
	im = _buildkindpatsmatcher(			im = _buildkindpatsmatcher(
	includematcher,			includematcher,
	root,			root,
				cwd,
	kindpats,			kindpats,
	ctx=ctx,			ctx=ctx,
	listsubrepos=listsubrepos,			listsubrepos=listsubrepos,
	badfn=None,			badfn=None,
	)			)
	m = intersectmatchers(m, im)			m = intersectmatchers(m, im)
	if exclude:			if exclude:
	kindpats = normalize(exclude, b'glob', root, cwd, auditor, warn)			kindpats = normalize(exclude, b'glob', root, cwd, auditor, warn)
	em = _buildkindpatsmatcher(			em = _buildkindpatsmatcher(
	includematcher,			includematcher,
	root,			root,
				cwd,
	kindpats,			kindpats,
	ctx=ctx,			ctx=ctx,
	listsubrepos=listsubrepos,			listsubrepos=listsubrepos,
	badfn=None,			badfn=None,
	)			)
	m = differencematcher(m, em)			m = differencematcher(m, em)
	return m			return m

mercurial/subrepo.py

	def filedata(self, name, decode):			def filedata(self, name, decode):
	"""return file data, optionally passed through repo decoders"""			"""return file data, optionally passed through repo decoders"""
	raise NotImplementedError			raise NotImplementedError

	def fileflags(self, name):			def fileflags(self, name):
	"""return file flags"""			"""return file flags"""
	return b''			return b''

	def matchfileset(self, expr, badfn=None):			def matchfileset(self, cwd, expr, badfn=None):
	"""Resolve the fileset expression for this repo"""			"""Resolve the fileset expression for this repo"""
	return matchmod.never(badfn=badfn)			return matchmod.never(badfn=badfn)

	def printfiles(self, ui, m, uipathfn, fm, fmt, subrepos):			def printfiles(self, ui, m, uipathfn, fm, fmt, subrepos):
	"""handle the files command for this subrepo"""			"""handle the files command for this subrepo"""
	return 1			return 1

	def archive(self, archiver, prefix, match=None, decode=True):			def archive(self, archiver, prefix, match=None, decode=True):
	if self._ctx.rev() is None:			if self._ctx.rev() is None:
	ctx = self._repo[None]			ctx = self._repo[None]
	else:			else:
	rev = self._state[1]			rev = self._state[1]
	ctx = self._repo[rev]			ctx = self._repo[rev]
	return cmdutil.files(ui, ctx, m, uipathfn, fm, fmt, subrepos)			return cmdutil.files(ui, ctx, m, uipathfn, fm, fmt, subrepos)

	@annotatesubrepoerror			@annotatesubrepoerror
	def matchfileset(self, expr, badfn=None):			def matchfileset(self, cwd, expr, badfn=None):
	if self._ctx.rev() is None:			if self._ctx.rev() is None:
	ctx = self._repo[None]			ctx = self._repo[None]
	else:			else:
	rev = self._state[1]			rev = self._state[1]
	ctx = self._repo[rev]			ctx = self._repo[rev]

	matchers = [ctx.matchfileset(expr, badfn=badfn)]			matchers = [ctx.matchfileset(cwd, expr, badfn=badfn)]

	for subpath in ctx.substate:			for subpath in ctx.substate:
	sub = ctx.sub(subpath)			sub = ctx.sub(subpath)

	try:			try:
	sm = sub.matchfileset(expr, badfn=badfn)			sm = sub.matchfileset(cwd, expr, badfn=badfn)
	pm = matchmod.prefixdirmatcher(subpath, sm, badfn=badfn)			pm = matchmod.prefixdirmatcher(subpath, sm, badfn=badfn)
	matchers.append(pm)			matchers.append(pm)
	except error.LookupError:			except error.LookupError:
	self.ui.status(			self.ui.status(
	_(b"skipping missing subrepository: %s\n")			_(b"skipping missing subrepository: %s\n")
	% self.wvfs.reljoin(reporelpath(self), subpath)			% self.wvfs.reljoin(reporelpath(self), subpath)
	)			)
	if len(matchers) == 1:			if len(matchers) == 1:

tests/test-fix.t

	$ echo modified > bar			$ echo modified > bar
	$ hg fix -w bar			$ hg fix -w bar
	$ cat bar			$ cat bar
	$TESTTMP/subprocesscwd			$TESTTMP/subprocesscwd

	Apparently fixing p1() and its descendants doesn't include wdir() unless			Apparently fixing p1() and its descendants doesn't include wdir() unless
	explicitly stated.			explicitly stated.

	BROKEN: fileset matches aren't relative to repo.root for commits

	$ hg fix -r '.::'			$ hg fix -r '.::'
	$ hg cat -r . ../quux			$ hg cat -r . ../quux
	quux			quux
	$ hg cat -r tip ../quux			$ hg cat -r tip ../quux
	quux			fs: $TESTTMP/subprocesscwd
	$ cat ../quux			$ cat ../quux
	quux			quux
				mharbison72AuthorUnsubmitted Done I found a couple of things surprising here: `.::` doesn't include fixing wdir() `.` isn't updated unless `-w` is specified `-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`? mharbison72: I found a couple of things surprising here: 1) `.::` doesn't include fixing wdir() 2) `.`…
				martinvonzUnsubmitted Not Done `.::` 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: > `.::` doesn't include fixing wdir() Yes, that's how revsets work (as you noted) > `.` isn't…
				hooperUnsubmitted Not Done Martin and I talked about this, and I think it would make sense to: 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. 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. Maybe remove the -r flag since it's a bit of a footgun. 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". hooper: Martin and I talked about this, and I think it would make sense to: 1. Update/fix the wdir when…

	Clean files are not fixed unless explicitly named			Clean files are not fixed unless explicitly named
	$ echo 'dirty' > ../quux			$ echo 'dirty' > ../quux

	BROKEN: fileset matches aren't relative to repo.root for wdir

	$ hg fix --working-dir			$ hg fix --working-dir
	$ cat ../quux			$ cat ../quux
	dirty			fs: $TESTTMP/subprocesscwd

	$ cd ../..			$ cd ../..

	Tools configured without a pattern are ignored. It would be too dangerous to			Tools configured without a pattern are ignored. It would be too dangerous to
	run them on all files, because this might happen while testing a configuration			run them on all files, because this might happen while testing a configuration
	that also deletes all of the file content. There is no reasonable subset of the			that also deletes all of the file content. There is no reasonable subset of the
	files to use as a default. Users should be explicit about what files are			files to use as a default. Users should be explicit about what files are
	affected by a tool. This test also confirms that we don't crash when the			affected by a tool. This test also confirms that we don't crash when the