[PATCH] merge: add resolve --prep, which outputs conflicted comparison files
Jun Wu
quark at fb.com
Thu Feb 23 17:57:00 EST 2017
Congratulations on your first patch to the list!
But I think we have better ways to achieve the same goal that we may prefer
them to this patch.
1) Better way to provide conflicted file contents - in-python merge tools
From a high-level, the patch tries to solve the problem:
- Get all paths and file contents involved in merge conflict resolution
in an efficient way.
We have "--list" and "--tool" already to fetch the data in a less efficient
way. I'm not a big fan of a new flag doing what we can do today.
I think a better to achieve the "efficient" goal is to make "--tool"
accept Python functions, just like what we do for hooks. If the signature
of the Python function accepts file contents directly, we can even avoid
writing temporary files - even more efficient than this patch.
2) Better "-T json" - templates
I think it's a bit hacky to only support JSON in a hard-coded format.
Ideally I think we want a very-flexible template version. The template
seems to fit with "--list" (so we don't need a new flag), and it may look
like:
- --list -T '{status} {path}\n'
# something similar to what we have today
- --list -T '... {patch-local}\n{diff-other}\n'
# show patches, code reading the output could read the working copy and
# apply patches to reconstruct file contents
- --list -T '... {temp-path-local}\n{temp-path-other}\n'
# create temporary files only when requested
- --list -T json:field1,field2,....
# choose what field needed for the JSON output
Although this looks promising, this project seems too big. I guess "1)" is
a reasonable way to go.
In additional, I think "hg resolve" needs to provide a way to not rewrite
unresolved files in the working copy. It could be done via a
"--dry-run"-alike flag, which is probably a separate patch.
Excerpts from Phil Cohen's message of 2017-02-22 22:49:06 -0800:
> # HG changeset patch
> # User Phil Cohen <phillco at fb.com>
> # Date 1487831905 28800
> # Wed Feb 22 22:38:25 2017 -0800
> # Node ID 77da232f2d689cdb9545e060405e8776f2193488
> # Parent 96eaefd350aec869047d9e2da90913ae698463df
> merge: add resolve --prep, which outputs conflicted comparison files
>
> Normally when resolving merge conflicts, `hg resolve` loops over each specified
> file, generating .orig, ~other.xxxxxx, and ~base.xxxxxx files and then launching
> the user's editor (along with a path to the output file with the conflict
> markers) and waiting for it to complete.
>
> We'd like to enable random-access to the list of the conflicted files so the
> user can flip more easily between conflicted files. This commit introduces
> `resolve --prep`, which generates all of these files up-front and outputs the
> result as JSON, which could be consumed by an IDE. (We’re not sure if it makes
> sense to have a human readable version yet, so we’re leaving that functionality
> out until a use case demands it.) The intention is that the user will fix all
> the conflicts and then run `resolve --mark`.
>
> Unlike the existing flow, which writes these files to a temporary directory,
> these files are stored in `.hg/merge/prep` and get deleted when `.hg/merge`
> does. Like the old flow, they have a randomized portion of the filename to
> prevent collisions. Technically each call to `resolve --prep` will generate a
> new set of files but we consider the cost of this to be low.
>
> No change is made to the existing merge flow as we decided it was not worth
> touching the merge state to reuse the same files.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -33,6 +33,7 @@
> error,
> exchange,
> extensions,
> + formatter,
> graphmod,
> hbisect,
> help,
> @@ -4241,7 +4242,8 @@
> ('l', 'list', None, _('list state of files needing merge')),
> ('m', 'mark', None, _('mark files as resolved')),
> ('u', 'unmark', None, _('mark files as unresolved')),
> - ('n', 'no-status', None, _('hide status prefix'))]
> + ('n', 'no-status', None, _('hide status prefix')),
> + ('prep', 'prep', None, _('lists paths to comparison file paths'))]
The description reads a bit strange to me. But maybe not to a native
speaker.
> + mergetoolopts + walkopts + formatteropts,
> _('[OPTION]... [FILE]...'),
> inferrepo=True)
> @@ -4287,8 +4289,8 @@
> Returns 0 on success, 1 if any files fail a resolve attempt.
> """
>
> - flaglist = 'all mark unmark list no_status'.split()
> - all, mark, unmark, show, nostatus = \
> + flaglist = 'all mark unmark list no_status prep'.split()
> + all, mark, unmark, show, nostatus, prep = \
> [opts.get(o) for o in flaglist]
>
> if (show and (mark or unmark)) or (mark and unmark):
> @@ -4315,6 +4317,28 @@
> fm.end()
> return 0
>
> + if prep:
> + fm = ui.formatter('resolve', opts)
> + if not isinstance(fm, formatter.jsonformatter):
> + raise error.Abort(_('--prep requires `-T json`'))
This is the first sub-commend that only works with "-T json". So it's not
terminal-user-faceing. Maybe a debug command, or change the flag description
to hide it, by adding "(ADVANCED)", or "(EXPERIMENTAL)".
> + ms = mergemod.mergestate.read(repo)
> + m = scmutil.match(repo[None], pats, opts)
> + wctx = repo[None]
> +
> + paths = {}
> + for f in ms:
> + if not m(f):
> + continue
> +
> + val = ms.prep(f, wctx)
> + if val is not None:
> + paths[f] = val
> +
> + fm.startitem()
> + fm.write('conflicts', '%s\n', paths)
> + fm.end()
> + return 0
> +
> with repo.wlock():
> ms = mergemod.mergestate.read(repo)
>
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -567,6 +567,35 @@
> "o": " [%s]" % labels[1],
> }
>
> +# If repo_dir is None, a temp dir is used
> +def temp(prefix, ctx, repo, repo_dir=None):
> + fullbase, ext = os.path.splitext(ctx.path())
> + pre = "%s~%s." % (os.path.basename(fullbase), prefix)
> + data = repo.wwritedata(ctx.path(), ctx.data())
> +
> + if repo_dir:
> + repo.vfs.makedirs(repo_dir)
> + (fd, name) = repo.vfs.mkstemp(prefix=pre, suffix=ext, dir=repo_dir)
> + f = repo.vfs(name, pycompat.sysstr("wb"))
> + else:
> + (fd, name) = tempfile.mkstemp(prefix=pre, suffix=ext)
> + f = os.fdopen(fd, pycompat.sysstr("wb"))
> +
> + f.write(data)
> + f.close()
> + return repo.vfs.join(name)
> +
> +def gentempfiles(repo, fcd, fco, fca):
> + back = None if fcd.isabsent() else \
> + scmutil.origpath(repo.ui, repo, repo.wjoin(fcd.path()))
> +
> + return {
> + 'workingcopy': repo.wjoin(fcd.path()),
> + 'base': temp("base", fca, repo, "merge/prep"),
> + 'other': temp("other", fco, repo, "merge/prep"),
> + 'original': back
> + }
> +
> def _filemerge(premerge, repo, mynode, orig, fcd, fco, fca, labels=None):
> """perform a 3-way merge in the working directory
>
> @@ -580,16 +609,6 @@
> Returns whether the merge is complete, the return value of the merge, and
> a boolean indicating whether the file was deleted from disk."""
>
> - def temp(prefix, ctx):
> - fullbase, ext = os.path.splitext(ctx.path())
> - pre = "%s~%s." % (os.path.basename(fullbase), prefix)
> - (fd, name) = tempfile.mkstemp(prefix=pre, suffix=ext)
> - data = repo.wwritedata(ctx.path(), ctx.data())
> - f = os.fdopen(fd, pycompat.sysstr("wb"))
> - f.write(data)
> - f.close()
> - return name
> -
Usually the practice is to prefer small patches [1]. Like moving a function
could be a single patch. That'll make review easier.
[1] https://www.mercurial-scm.org/wiki/ContributingChanges
> if not fco.cmp(fcd): # files identical?
> return True, None, False
>
> @@ -637,8 +656,8 @@
> return True, 1, False
>
> a = repo.wjoin(fd)
> - b = temp("base", fca)
> - c = temp("other", fco)
> + b = temp("base", fca, repo)
> + c = temp("other", fco, repo)
> if not fcd.isabsent():
> back = scmutil.origpath(ui, repo, a)
> if premerge:
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -456,6 +456,24 @@
> def extras(self, filename):
> return self._stateextras.setdefault(filename, {})
>
> + def _prep(self, dfile, wctx):
> + if self[dfile] in 'rd':
> + return None
> + stateentry = self._state[dfile]
> + state, hash, lfile, afile, anode, ofile, onode, flags = stateentry
> + octx = self._repo[self._other]
> + extras = self.extras(dfile)
> + anccommitnode = extras.get('ancestorlinknode')
> + if anccommitnode:
> + actx = self._repo[anccommitnode]
> + else:
> + actx = None
> + fcd = self._filectxorabsent(hash, wctx, dfile)
> + fco = self._filectxorabsent(onode, octx, ofile)
> + fca = self._repo.filectx(afile, fileid=anode, changeid=actx)
> +
> + return filemerge.gentempfiles(self._repo, fcd, fco, fca)
> +
> def _resolve(self, preresolve, dfile, wctx):
> """rerun merge process for file path `dfile`"""
> if self[dfile] in 'rd':
> @@ -543,6 +561,9 @@
> Returns whether the merge is complete, and the exit code."""
> return self._resolve(True, dfile, wctx)
>
> + def prep(self, dfile, wctx):
> + return self._prep(dfile, wctx)
> +
> def resolve(self, dfile, wctx):
> """run merge process (assuming premerge was run) for dfile
>
> diff --git a/tests/test-resolve-prep.t b/tests/test-resolve-prep.t
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-resolve-prep.t
> @@ -0,0 +1,83 @@
> +1) Make the repo
> + $ hg init
> +
> +2) Can't run prep outside a conflict
> + $ hg resolve --prep -T json
> + abort: no files or directories specified
> + (use --all to re-merge all unresolved files)
> + [255]
> +
> +3) Make a simple conflict
> + $ echo "Unconflicted base, F1" > F1
> + $ echo "Unconflicted base, F2" > F2
> + $ hg add .
> + adding F1
> + adding F2
> + $ hg commit -m "initial commit"
> + $ echo "First conflicted version, F1" > F1
> + $ echo "First conflicted version, F2" > F2
> + $ hg commit -m "first version, a"
> + $ hg bookmark a
> + $ hg checkout .~1
> + 2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> + (leaving bookmark a)
> + $ echo "Second conflicted version, F1" > F1
> + $ echo "Second conflicted version, F2" > F2
> + $ hg commit -m "second version, b"
> + created new head
> + $ hg bookmark b
> + $ hg log -G -T '({rev}) {desc}\nbookmark: {bookmarks}\nfiles: {files}\n\n'
> + @ (2) second version, b
> + | bookmark: b
> + | files: F1 F2
> + |
> + | o (1) first version, a
> + |/ bookmark: a
> + | files: F1 F2
> + |
> + o (0) initial commit
> + bookmark:
> + files: F1 F2
> +
> +
> +
> + $ hg merge a
> + merging F1
> + merging F2
> + warning: conflicts while merging F1! (edit, then use 'hg resolve --mark')
> + warning: conflicts while merging F2! (edit, then use 'hg resolve --mark')
> + 0 files updated, 0 files merged, 0 files removed, 2 files unresolved
> + use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
> + [1]
> +
> +4) Must pass '-T json':
> + $ hg resolve --prep --all
> + abort: --prep requires `-T json`
> + [255]
> +
> +5) Get the paths:
> + $ hg resolve --prep --all -T json
> + [
> + {
> + "conflicts": {"F1": {"base": "$TESTTMP/.hg/merge/prep/F1~base.??????", "original": "$TESTTMP/F1.orig", "other": "$TESTTMP/.hg/merge/prep/F1~other.??????", "workingcopy": "$TESTTMP/F1"}, "F2": {"base": "$TESTTMP/.hg/merge/prep/F2~base.??????", "original": "$TESTTMP/F2.orig", "other": "$TESTTMP/.hg/merge/prep/F2~other.??????", "workingcopy": "$TESTTMP/F2"}} (glob)
> + }
> + ]
> +
> +6) Ensure the paths point to the right contents:
> + $ getpath() { # Usage: getpath <path> <version>
> + > local script="import sys, json; print json.load(sys.stdin)[0][\"conflicts\"][\"$1\"][\"$2\"]"
> + > local result=$(hg resolve --prep --all -T json | python -c "$script")
> + > echo "$result"
> + > }
> + $ cat $(getpath "F1" "base")
> + Unconflicted base, F1
> + $ cat $(getpath "F1" "other")
> + First conflicted version, F1
> + $ cat $(getpath "F1" "original")
> + Second conflicted version, F1
> + $ cat $(getpath "F2" "base")
> + Unconflicted base, F2
> + $ cat $(getpath "F2" "other")
> + First conflicted version, F2
> + $ cat $(getpath "F2" "original")
> + Second conflicted version, F2
More information about the Mercurial-devel
mailing list