Re: [PATCH v3 05/11] merge-resolve: rewrite in C
- From
Alban Gruin <alban.gruin@gmail.com>
- Date
- Nov 6, 2020, 19:53 UTC
- Message-ID
- <f321f01d-8071-9be3-7214-f8506081f17c@gmail.com>
- In-Reply-To
- <xmqqr1pyashe.fsf@gitster.c.googlers.com>
Le 16/10/2020 à 21:19, Junio C Hamano a écrit :
Show 54 quoted lines
> Alban Gruin <alban.gruin@gmail.com> 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.
Show 27 quoted lines
>> + 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