On 16/08/2022 13:17, Johannes Schindelin wrote:

Hi Phillip,

On Wed, 10 Aug 2022, Phillip Wood wrote:

quoted

On 09/08/2022 19:54, Alban Gruin wrote:

quoted

This rewrites `git merge-resolve' from shell to C.  As for `git
merge-one-file', this port is not completely straightforward and removes
calls to external processes to avoid reading and writing the index over
and over again.

   - The call to `update-index -q --refresh' is replaced by a call to
     refresh_index().

   - The call to `read-tree' is replaced by a call to unpack_trees() (and
     all the setup needed).

   - The call to `write-tree' is replaced by a call to
     cache_tree_update().  This call is wrapped in a new function,
     write_tree().  It is made to mimick write_index_as_tree() with
     WRITE_TREE_SILENT flag, but without locking the index; this is taken
     care directly in merge_strategies_resolve().

   - The call to `diff-index ...' is replaced by a call to
     repo_index_has_changes().

   - The call to `merge-index', needed to invoke `git merge-one-file', is
     replaced by a call to the new merge_all_index() function.

The index is read in cmd_merge_resolve(), and is wrote back by
merge_strategies_resolve().  This is to accomodate future applications:
in `git-merge', the index has already been read when the merge strategy
is called, so it would be redundant to read it again when the builtin
will be able to use merge_strategies_resolve() directly.

The parameters of merge_strategies_resolve() will be surprising at first
glance: why using a commit list for `bases' and `remote', where we could
use an oid array, and a pointer to an oid?  Because, in a later commit,
try_merge_strategy() will be able to call merge_strategies_resolve()
directly, and it already uses a commit list for `bases' (`common') and
`remote' (`remoteheads'), and a string for `head_arg'.  To reduce
frictions later, merge_strategies_resolve() takes the same types of
parameters.

git-merge-resolve will happily merge three trees, unfortunately using
lists of commits will break that.

But isn't `merge-resolve` specifically implemented as a merge strategy? I
do not see any contract in Git's documentation that commits to supporting
direct calls to the implementation detail that is `git merge-resolve`:

	$ man git-merge-resolve
	No manual entry for git-merge-resolve

I've certainly got scripts that call "git merge-recursive" with a 
mixture of commits and trees (it's kind of doing an cherry-pick), it 
wouldn't surprise me if someone was doing something weird with 
merge-resolve.

quoted

quoted

+int cmd_merge_resolve(int argc, const char **argv, const char *prefix)
+{
+	int i, sep_seen = 0;
+	const char *head = NULL;
+	struct commit_list *bases = NULL, *remote = NULL;
+	struct commit_list **next_base = &bases;
+	struct repository *r = the_repository;
+
+	if (argc < 5)
+		usage(builtin_merge_resolve_usage);

I think it would be better to call parse_options() and then check argc. That
would give better error messages for unknown options and supports '-h' for
free.

Again, we are talking about a merge strategy, a program that is not meant
to be called directly by the user. Why should we complicate the code by
using the `parse_options` machinery?

I thought it would simplify the implementation of '-h' below. However as 
the script does not support '-h' we should perhaps drop support for that 
and the usage() call if we want a strictly equivalent conversion.

quoted

I think we also need to call git_config(). I see that read-tree respects
submodule.recurse so I think we need the same here. I suspect we should
also be reading the merge config to respect merge.conflictStyle.

Valid concerns. Extra brownie points if you can provide a simple test case
that demonstrates the current behavior.

I'll add it to my todo list.

quoted

quoted

+
+	if (repo_index_has_changes(r, head_tree, &sb)) {
+		error(_("Your local changes to the following files "
+			"would be overwritten by merge:\n  %s"),
+		      sb.buf);

This matches the script but I wonder why that did not check for unstaged
changes.

Any deviations from the scripted behavior should be done on top of this
patch series, unless the deviations make the conversion substantially
cleaner.

I agree. Having thought some more I suspect it is relying on 
unpack_trees() to error out if there are unstaged changes.

Best Wishes

Phillip

Thanks,
Dscho