Re: [PATCH v8 08/14] merge-resolve: rewrite in C

[Phillip Wood <phillip.wood123@gmail.com>]

On 16/08/2022 13:17, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Wed, 10 Aug 2022, Phillip Wood wrote:
> 
>> On 09/08/2022 19:54, Alban Gruin wrote:
>>> 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.
>>> +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.

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

>>> +
>>> +	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