Re: [PATCH v7 09/15] merge-resolve: rewrite in C

[Alban Gruin <alban.gruin@gmail.com>]

Hi Johannes,

Le 23/03/2021 à 23:21, Johannes Schindelin a écrit :
> Hi Alban,
> 
> On Wed, 17 Mar 2021, Alban Gruin wrote:
> 
>> diff --git a/merge-strategies.c b/merge-strategies.c
>> index 2717af51fd..a51700dae5 100644
>> --- a/merge-strategies.c
>> +++ b/merge-strategies.c
>> @@ -272,3 +275,95 @@ int merge_all_index(struct index_state *istate, int oneshot, int quiet,
>>
>>  	return err;
>>  }
>> +
>> +static int fast_forward(struct repository *r, struct tree_desc *t,
>> +			int nr, int aggressive)
>> +{
>> +	struct unpack_trees_options opts;
>> +	struct lock_file lock = LOCK_INIT;
>> +
>> +	refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
>> +	repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
> 
> Shouldn't we lock the index first, and _then_ refresh it? I guess not,
> seeing as we don't do that either in `cmd_status()`: there, we also
> refresh the index and _then_ lock it.
> 

Yeah, I don't think I saw a lock/refresh sequence, but I may be wrong.

>> +
>> +	memset(&opts, 0, sizeof(opts));
>> +	opts.head_idx = 1;
>> +	opts.src_index = r->index;
>> +	opts.dst_index = r->index;
>> +	opts.merge = 1;
>> +	opts.update = 1;
>> +	opts.aggressive = aggressive;
>> +
>> +	if (nr == 1)
>> +		opts.fn = oneway_merge;
>> +	else if (nr == 2) {
>> +		opts.fn = twoway_merge;
>> +		opts.initial_checkout = is_index_unborn(r->index);
>> +	} else if (nr >= 3) {
>> +		opts.fn = threeway_merge;
>> +		opts.head_idx = nr - 1;
>> +	}
> 
> Given the function's name `fast_forward()`, I have to admit that I
> somewhat stumbled over these merges.
>> +
>> +	if (unpack_trees(nr, t, &opts))
>> +		return -1;
>> +

I just noticed that the lock is not released if there is an error here.

>> +	if (write_locked_index(r->index, &lock, COMMIT_LOCK))
>> +		return error(_("unable to write new index file"));
>> +
>> +	return 0;
>> +}
>> +
>> +static int add_tree(struct tree *tree, struct tree_desc *t)
>> +{
>> +	if (parse_tree(tree))
>> +		return -1;
>> +
>> +	init_tree_desc(t, tree->buffer, tree->size);
>> +	return 0;
>> +}
> 
> This is a really trivial helper, but it is used a couple times below, so
> it makes sense to have it encapsulated in a separate function.
> 
>> +
>> +int merge_strategies_resolve(struct repository *r,
>> +			     struct commit_list *bases, const char *head_arg,
>> +			     struct commit_list *remote)
> 
> Since it is a list, and since the original variable in the shell script
> had been named in the plural form, let's do the same here: `remotes`.
> 

This one is supposed to contain only one commit, so I'm not really
conviced that this parameter should be in the plural form.

>> +{
>> +	struct tree_desc t[MAX_UNPACK_TREES];
>> +	struct object_id head, oid;
>> +	struct commit_list *i;
>> +	int nr = 0;
>> +
>> +	if (head_arg)
>> +		get_oid(head_arg, &head);
>> +
>> +	puts(_("Trying simple merge."));
> 
> Good. Usually I would recommend to print this to `stderr`, but the
> original script prints it to `stdout`, so we should do that here, too.
> 
>> +
>> +	for (i = bases; i && i->item; i = i->next) {
>> +		if (add_tree(repo_get_commit_tree(r, i->item), t + (nr++)))
>> +			return 2;
> 
> Since we're talking about a library function, not a `cmd_*()` function,
> the return value on error should probably be negative.
> 
> Even better would be to let the function return an `enum` that contains
> labels with more intuitive meaning than "2".
> 
> It _is_ the expected exit code when calling `git merge-resolve`, of course
> (because of the `|| exit 2` after that `read-tree` call), but I wonder
> whether a better layer for that `2` would be the `cmd_merge_resolve()`
> function, letting `merge_strategies_resolve()` report failures in a more
> fine-grained fashion.
> 

Right -- I'll see what I can do here.

>> +	}
>> +
>> +	if (head_arg) {
> 
> It would probably be easier to read if the `if (head_arg)` clause above
> was merged into this here clause.
> 
>> +		struct tree *tree = parse_tree_indirect(&head);
>> +		if (add_tree(tree, t + (nr++)))
>> +			return 2;
>> +	}
>> +
>> +	if (remote && add_tree(repo_get_commit_tree(r, remote->item), t + (nr++)))
>> +		return 2;
> 
> You get away with assuming that `remotes` only contains at most a single
> entry because `cmd_merge_resolve()` verified it.
> 
> However, as the intention is to use this as a library function, I think
> the input validation needs to be moved here instead of relying on all
> callers to verify that they send at most one "remote" ref.
> 
> Other than that, this patch looks good to me.
> 
Well, this condition checks that there is one commit, and if so, uses it
to call add_tree().  I don't see the mistake here.

Cheers,
Alban

> Thanks,
> Dscho
>