This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2889

filemerge: make the 'local' path match the format that 'base' and 'other' use
ClosedPublic

Authored by spectral on Mar 16 2018, 8:00 PM.

Download Raw Diff

Details

Reviewers

yuja

Group Reviewers

hg-reviewers

Commits

rHG1e30a26a65d0: filemerge: make the 'local' path match the format that 'base' and 'other' use

Summary

If we pass a separate '$output' arg to the merge tool, we produce four files:
local, base, other, and output. In this situation, 'output' will be the
original filename, 'base' and 'other' are temporary files, and previously
'local' would be the backup file (so if 'output' was foo.txt, 'local' would be
foo.txt.orig).

This change makes it so that 'local' follows the same pattern as 'base' and
'other' - it will be a temporary file either in the
experimental.mergetempdirprefix-controlled directory with a name like
foo~local.txt, or in the normal system-wide temp dir with a name like
foo~local.RaNd0m.txt.

For the cases where the merge tool does not use an '$output' arg, 'local' is
still the destination filename, and 'base' and 'other' are unchanged.

The hope is that this is much easier for people to reason about; rather than
having a tool like Meld pop up with three panes, one of them with the filename
"foo.txt.orig", one with the filename "foo.txt", and one with
"foo~other.StuFf2.txt", we can (when the merge temp dir stuff is enabled) make
it show up as "foo~local.txt", "foo.txt" and "foo~other.txt", respectively.

This also opens the door to future customization, such as getting the
operation-provided labels and a hash prefix into the filenames (so we see
something like "foo~dest.abc123", "foo.txt", and "foo~src.d4e5f6").

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

spectral created this revision.Mar 16 2018, 8:00 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMar 16 2018, 8:00 PM

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

Not reviewed yet, but this can't be applied cleanly. Can you rebase?

This revision now requires changes to proceed.Mar 19 2018, 10:33 AM

In D2889#46482, @yuja wrote:

Not reviewed yet, but this can't be applied cleanly. Can you rebase?

This series should rebase cleanly onto a4a95bd7 (which was accepted by pulkit separately from the rest of the series). I've rebase these three remaining ones and pushed.

Ah, there's a "Request Review" that I'd never noticed before, choosing that to hopefully get this out of "Requires Revision"

Can't apply.

% hg up -C a4a95bd7
2 files updated, 0 files merged, 0 files removed, 0 files unresolved

% hg pimp D2889
devel-warn: accessing unregistered config item: 'phabricator.batchsize' at: /home/yuya/work/hghacks/mercurial/mercurial/ui.py:624 (configwith)
devel-warn: accessing unregistered config item: 'phabricator.url' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:124 (readurltoken)
devel-warn: accessing unregistered config item: 'phabricator.token' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:111 (readlegacytoken)
phabricator.token is deprecated - please migrate to the phabricator.auth section.
devel-warn: accessing unregistered config item: 'phabricator.curlcmd' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:162 (callconduit)
applying patch from stdin
devel-warn: accessing unregistered config item: 'phabricator.url' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:124 (readurltoken)
devel-warn: accessing unregistered config item: 'phabricator.token' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:111 (readlegacytoken)
devel-warn: accessing unregistered config item: 'phabricator.curlcmd' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:162 (callconduit)
devel-warn: accessing unregistered config item: 'phabricator.url' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:124 (readurltoken)
devel-warn: accessing unregistered config item: 'phabricator.token' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:111 (readlegacytoken)
devel-warn: accessing unregistered config item: 'phabricator.curlcmd' at: /home/yuya/work/hghacks/mercurial/contrib/phabricator.py:162 (callconduit)
patching file tests/test-merge-tools.t
Hunk #2 FAILED at 1599
1 out of 2 hunks FAILED -- saving rejects to file tests/test-merge-tools.t.rej
patching file mercurial/filemerge.py
Hunk #1 FAILED at 511
Hunk #3 FAILED at 663
2 out of 3 hunks FAILED -- saving rejects to file mercurial/filemerge.py.rej
abort: patch failed to apply

spectral updated this revision to Diff 7189.Mar 21 2018, 4:48 PM

yuja accepted this revision.Mar 22 2018, 10:34 AM

This revision is now accepted and ready to land.Mar 22 2018, 10:34 AM

Closed by commit rHG1e30a26a65d0: filemerge: make the 'local' path match the format that 'base' and 'other' use (authored by spectral). · Explain WhyMar 22 2018, 10:46 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/filemerge.py (46 lines)
M			tests/test-merge-tools.t (4 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	7087		Mar 16 2018, 8:00 PM	★	★
Diff 2	7189		Mar 21 2018, 4:48 PM	★	★
Diff 3	7253	rHG1e30a26a65d077b3503cd746ee73fb99f3fb8033	Mar 21 2018, 3:36 PM	★	★

Status	Author	Revision
Closed	spectral	D2889 filemerge: make the 'local' path match the format that 'base' and 'other' use
Closed	spectral	D2888 filemerge: use a single temp dir instead of temp files
Closed	spectral	D2887 filemerge: move temp file unlinks to _maketempfiles
Closed	spectral	D2886 filemerge: give some variables in _xmerge more descriptive names

Diff 7087

mercurial/filemerge.py

	def _xmerge(repo, mynode, orig, fcd, fco, fca, toolconf, files, labels=None):			def _xmerge(repo, mynode, orig, fcd, fco, fca, toolconf, files, labels=None):
	tool, toolpath, binary, symlink = toolconf			tool, toolpath, binary, symlink = toolconf
	if fcd.isabsent() or fco.isabsent():			if fcd.isabsent() or fco.isabsent():
	repo.ui.warn(_('warning: %s cannot merge change/delete conflict '			repo.ui.warn(_('warning: %s cannot merge change/delete conflict '
	'for %s\n') % (tool, fcd.path()))			'for %s\n') % (tool, fcd.path()))
	return False, 1, None			return False, 1, None
	unused, unused, unused, back = files			unused, unused, unused, back = files
	localpath = _workingpath(repo, fcd)			localpath = _workingpath(repo, fcd)
	with _maketempfiles(repo, fco, fca) as temppaths:			args = _toolstr(repo.ui, tool, "args")
	basepath, otherpath = temppaths
				with _maketempfiles(repo, fco, fca, repo.wvfs.join(back.path()),
				"$output" in args) as temppaths:
				basepath, otherpath, localoutputpath = temppaths
	outpath = ""			outpath = ""
	mylabel, otherlabel = labels[:2]			mylabel, otherlabel = labels[:2]
	if len(labels) >= 3:			if len(labels) >= 3:
	baselabel = labels[2]			baselabel = labels[2]
	else:			else:
	baselabel = 'base'			baselabel = 'base'
	env = {'HG_FILE': fcd.path(),			env = {'HG_FILE': fcd.path(),
	'HG_MY_NODE': short(mynode),			'HG_MY_NODE': short(mynode),
	'HG_OTHER_NODE': short(fco.changectx().node()),			'HG_OTHER_NODE': short(fco.changectx().node()),
	'HG_BASE_NODE': short(fca.changectx().node()),			'HG_BASE_NODE': short(fca.changectx().node()),
	'HG_MY_ISLINK': 'l' in fcd.flags(),			'HG_MY_ISLINK': 'l' in fcd.flags(),
	'HG_OTHER_ISLINK': 'l' in fco.flags(),			'HG_OTHER_ISLINK': 'l' in fco.flags(),
	'HG_BASE_ISLINK': 'l' in fca.flags(),			'HG_BASE_ISLINK': 'l' in fca.flags(),
	'HG_MY_LABEL': mylabel,			'HG_MY_LABEL': mylabel,
	'HG_OTHER_LABEL': otherlabel,			'HG_OTHER_LABEL': otherlabel,
	'HG_BASE_LABEL': baselabel,			'HG_BASE_LABEL': baselabel,
	}			}
	ui = repo.ui			ui = repo.ui

	args = _toolstr(ui, tool, "args")
	if "$output" in args:			if "$output" in args:
	# read input from backup, write to original			# read input from backup, write to original
	outpath = localpath			outpath = localpath
	localpath = repo.wvfs.join(back.path())			localpath = localoutputpath
	replace = {'local': localpath, 'base': basepath, 'other': otherpath,			replace = {'local': localpath, 'base': basepath, 'other': otherpath,
	'output': outpath, 'labellocal': mylabel,			'output': outpath, 'labellocal': mylabel,
	'labelother': otherlabel, 'labelbase': baselabel}			'labelother': otherlabel, 'labelbase': baselabel}
	args = util.interpolate(br'\$', replace, args,			args = util.interpolate(br'\$', replace, args,
	lambda s: util.shellquote(util.localpath(s)))			lambda s: util.shellquote(util.localpath(s)))
	cmd = toolpath + ' ' + args			cmd = toolpath + ' ' + args
	if _toolbool(ui, tool, "gui"):			if _toolbool(ui, tool, "gui"):
	repo.ui.status(_('running merge tool %s for file %s\n') %			repo.ui.status(_('running merge tool %s for file %s\n') %
	util.writefile(back, fcd.data())			util.writefile(back, fcd.data())
	else:			else:
	util.copyfile(a, back)			util.copyfile(a, back)
	# A arbitraryfilectx is returned, so we can run the same functions on			# A arbitraryfilectx is returned, so we can run the same functions on
	# the backup context regardless of where it lives.			# the backup context regardless of where it lives.
	return context.arbitraryfilectx(back, repo=repo)			return context.arbitraryfilectx(back, repo=repo)

	@contextlib.contextmanager			@contextlib.contextmanager
	def _maketempfiles(repo, fco, fca):			def _maketempfiles(repo, fco, fca, localpath, uselocalpath):
	"""Writes out `fco` and `fca` as temporary files, so an external merge			"""Writes out `fco` and `fca` as temporary files, and (if not None) copies
	tool may use them.			`localpath` to another temporary file, so an external merge tool may use
				them.
	"""			"""
	tmproot = None			tmproot = None
	tmprootprefix = repo.ui.config('experimental', 'mergetempdirprefix')			tmprootprefix = repo.ui.config('experimental', 'mergetempdirprefix')
	if tmprootprefix:			if tmprootprefix:
	tmproot = tempfile.mkdtemp(prefix=tmprootprefix)			tmproot = tempfile.mkdtemp(prefix=tmprootprefix)

	def temp(prefix, ctx):			def maketempfrompath(prefix, path):
	fullbase, ext = os.path.splitext(ctx.path())			fullbase, ext = os.path.splitext(path)
	pre = "%s~%s" % (os.path.basename(fullbase), prefix)			pre = "%s~%s" % (os.path.basename(fullbase), prefix)
	if tmproot:			if tmproot:
	name = os.path.join(tmproot, pre)			name = os.path.join(tmproot, pre)
	if ext:			if ext:
	name += ext			name += ext
	f = open(name, r"wb")			f = open(name, r"wb")
	else:			else:
	(fd, name) = tempfile.mkstemp(prefix=pre + '.', suffix=ext)			fd, name = tempfile.mkstemp(prefix=pre + '.', suffix=ext)
	f = os.fdopen(fd, r"wb")			f = os.fdopen(fd, r"wb")
				return f, name

				def tempfromcontext(prefix, ctx):
				f, name = maketempfrompath(prefix, ctx.path())
	data = repo.wwritedata(ctx.path(), ctx.data())			data = repo.wwritedata(ctx.path(), ctx.data())
	f.write(data)			f.write(data)
	f.close()			f.close()
	return name			return name

	b = temp("base", fca)			b = tempfromcontext("base", fca)
	c = temp("other", fco)			c = tempfromcontext("other", fco)
				d = localpath
				if uselocalpath:
				if d.endswith('.orig'):
				d, _ = os.path.splitext(d)
				f, d = maketempfrompath("local", d)
				with open(localpath, 'rb') as src:
				f.write(src.read())
				f.close()

	try:			try:
	yield b, c			yield b, c, d
	finally:			finally:
	if tmproot:			if tmproot:
	shutil.rmtree(tmproot)			shutil.rmtree(tmproot)
	else:			else:
	util.unlink(b)			util.unlink(b)
	util.unlink(c)			util.unlink(c)
				# if not uselocalpath, d is the 'orig'/backup file which we
				# shouldn't delete.
				if d and uselocalpath:
				util.unlink(d)

	def _filemerge(premerge, repo, wctx, mynode, orig, fcd, fco, fca, labels=None):			def _filemerge(premerge, repo, wctx, mynode, orig, fcd, fco, fca, labels=None):
	"""perform a 3-way merge in the working directory			"""perform a 3-way merge in the working directory

	premerge = whether this is a premerge			premerge = whether this is a premerge
	mynode = parent node before merge			mynode = parent node before merge
	orig = original local filename before merge			orig = original local filename before merge
	fco = other file context			fco = other file context

tests/test-merge-tools.t

	Verify naming of temporary files and that extension is preserved:			Verify naming of temporary files and that extension is preserved:

	$ hg update -q -C 1			$ hg update -q -C 1
	$ hg mv f f.txt			$ hg mv f f.txt
	$ hg ci -qm "f.txt"			$ hg ci -qm "f.txt"
	$ hg update -q -C 2			$ hg update -q -C 2
	$ hg merge -y -r tip --tool echo --config merge-tools.echo.args='$base $local $other $output'			$ hg merge -y -r tip --tool echo --config merge-tools.echo.args='$base $local $other $output'
	merging f and f.txt to f.txt			merging f and f.txt to f.txt
	/f~base.?????? $TESTTMP/f.txt.orig /f~other.??????.txt $TESTTMP/f.txt (glob)			/f~base.?????? /f~local.??????.txt */f~other.??????.txt $TESTTMP/f.txt (glob)
	0 files updated, 1 files merged, 0 files removed, 0 files unresolved			0 files updated, 1 files merged, 0 files removed, 0 files unresolved
	(branch merge, don't forget to commit)			(branch merge, don't forget to commit)

	Verify naming of temporary files and that extension is preserved			Verify naming of temporary files and that extension is preserved
	(experimental.mergetempdirprefix version):			(experimental.mergetempdirprefix version):

	$ hg update -q -C 1			$ hg update -q -C 1
	$ hg mv f f.txt			$ hg mv f f.txt
	$ hg ci -qm "f.txt"			$ hg ci -qm "f.txt"
	$ hg update -q -C 2			$ hg update -q -C 2
	$ hg merge -y -r tip --tool echo \			$ hg merge -y -r tip --tool echo \
	> --config merge-tools.echo.args='$base $local $other $output' \			> --config merge-tools.echo.args='$base $local $other $output' \
	> --config experimental.mergetempdirprefix=$TESTTMP/hgmerge.			> --config experimental.mergetempdirprefix=$TESTTMP/hgmerge.
	merging f and f.txt to f.txt			merging f and f.txt to f.txt
	$TESTTMP/hgmerge.??????/f~base $TESTTMP/f.txt.orig $TESTTMP/hgmerge.??????/f~other.txt $TESTTMP/f.txt (glob)			$TESTTMP/hgmerge.??????/f~base $TESTTMP/hgmerge.??????/f~local.txt $TESTTMP/hgmerge.??????/f~other.txt $TESTTMP/f.txt (glob)
	0 files updated, 1 files merged, 0 files removed, 0 files unresolved			0 files updated, 1 files merged, 0 files removed, 0 files unresolved
	(branch merge, don't forget to commit)			(branch merge, don't forget to commit)
	Check that debugpicktool examines which merge tool is chosen for			Check that debugpicktool examines which merge tool is chosen for
	specified file as expected			specified file as expected

	$ beforemerge			$ beforemerge
	[merge-tools]			[merge-tools]
	false.whatever=			false.whatever=