[PATCH] merge: add resolve --prep, which outputs conflicted comparison files
Phil Cohen
phillco at fb.com
Fri Feb 24 22:54:00 UTC 2017
Hi Jun,
Interesting, thanks. So if I understand correctly, if --tool accepted
a Python function, to replicate the functionality described here, the
user could write a simple script that wrote out the four* versions to
some location on disk and printed their paths as JSON (as you noted,
they could also output the contents directly in some encoding, but
that's up the user.)
It sounds like the issue of `resolve` always overwriting the file in
the working copy is a nuisance, and will probably be more of one in
the future if we people use #1 to extract information about the merge
as we are doing. It might not be an issue for us but we should
probably fix/change that too?
[Let me know if this email is poorly formatted, it's my first reply to
the list.]
*Including generating the version with the conflict markers.
On Thu, Feb 23, 2017 at 2:57 PM, Jun Wu <quark at fb.com> wrote:
> 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
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
More information about the Mercurial-devel
mailing list