Le 16/10/2020 à 21:19, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@xxxxxxxxx> writes:
>
>> +#include "cache.h"
>> +#include "builtin.h"
>> +#include "merge-strategies.h"
>> +
>> +static const char builtin_merge_resolve_usage[] =
>> + "git merge-resolve <bases>... -- <head> <remote>";
>> +
>> +int cmd_merge_resolve(int argc, const char **argv, const char *prefix)
>> +{
>> + int i, is_baseless = 1, sep_seen = 0;
>> + const char *head = NULL;
>> + struct commit_list *bases = NULL, *remote = NULL;
>> + struct commit_list **next_base = &bases;
>> +
>> + if (argc < 5)
>> + usage(builtin_merge_resolve_usage);
>> +
>> + setup_work_tree();
>> + if (repo_read_index(the_repository) < 0)
>> + die("invalid index");
>> +
>> + /* The first parameters up to -- are merge bases; the rest are
>> + * heads. */
>
> Style (I won't repeat).
>
>> + for (i = 1; i < argc; i++) {
>> + if (strcmp(argv[i], "--") == 0)
>
> if (!strcmp(...))
>
> is more typical than comparing with "== 0".
>
>> + sep_seen = 1;
>> + else if (strcmp(argv[i], "-h") == 0)
>> + usage(builtin_merge_resolve_usage);
>> + else if (sep_seen && !head)
>> + head = argv[i];
>> + else if (remote) {
>> + /* Give up if we are given two or more remotes.
>> + * Not handling octopus. */
>> + return 2;
>> + } else {
>> + struct object_id oid;
>> +
>> + get_oid(argv[i], &oid);
>> + is_baseless &= sep_seen;
>> +
>> + if (!oideq(&oid, the_hash_algo->empty_tree)) {
>
> What is this business about an empty tree about?
>
I don’t remember my intent here -- perhaps I wanted to avoid merges on
empty trees… I’ll remove that from here and merge-octopus.c.
>> + struct commit *commit;
>> + commit = lookup_commit_or_die(&oid, argv[i]);
>> +
>> + if (sep_seen)
>> + commit_list_append(commit, &remote);
>> + else
>> + next_base = commit_list_append(commit, next_base);
>> + }
>> + }
>> + }
>> +
>> + /* Give up if this is a baseless merge. */
>> + if (is_baseless)
>> + return 2;
>
> This is quite convoluted.
>
> The original is much more straight-forward. We just said "grab
> everything before we see '--' and call them bases; immediately after
> '--' is HEAD and everything else is remote. Now do we have any
> base? Otherwise we cannot handle it".
>
> I cannot see an equivalence to it in the rewritten result, with the
> bit operation with is_baseless and sep_seen. Wouldn't it be the
> matter of checking if next_base is NULL, or is there something more
> subtle that deserves in-code comment going on?
>
After re-reading this many, many weeks later, I can confirm that this is
convoluted, and that there is a much better way to perform some checks…
for instance, checking if `bases' is NULL instead of having
`is_baseless', or checking after the loop if `remotes->next' is not NULL
to verify if there is multiple remotes.
> Thanks.
>
Alban
