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
>
