Re: [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2020, 19:43 UTC
Message-ID: <xmqqh80n6zvp.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAKPyHN0_AVOo_6bdHvy_J9ebnBpSD2NECBiLZ7g=4TcMvfZgYw@mail.gmail.com>

Bert Wesarg <bert.wesarg@googlemail.com> writes:

Show 52 quoted lines

> Dear Junio,
>
> On Wed, Jan 22, 2020 at 12:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>>
>> > When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
>> > for the type, 2018-08-04) landed in Git, it had the side effect that
>> > not only 'pull --rebase=<type>' accepted the single-letter abbreviations
>> > but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.
>> >
>> > Secondly, 'git remote rename' did not honor these single-letter
>> > abbreviations when reading the 'branch.*.rebase' configurations.
>>
>> Hmph, do you mean s/Secondly/However/ instead?
>
> thanks, that now reads smoothly.
>
>> > @@ -305,17 +304,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>> >                               space = strchr(value, ' ');
>> >                       }
>> >                       string_list_append(&info->merge, xstrdup(value));
>> > -             } else {
>> > -                     int v = git_parse_maybe_bool(value);
>> > -                     if (v >= 0)
>> > -                             info->rebase = v;
>> > -                     else if (!strcmp(value, "preserve"))
>> > -                             info->rebase = NORMAL_REBASE;
>> > -                     else if (!strcmp(value, "merges"))
>> > -                             info->rebase = REBASE_MERGES;
>> > -                     else if (!strcmp(value, "interactive"))
>> > -                             info->rebase = INTERACTIVE_REBASE;
>> > -             }
>> > +             } else
>> > +                     info->rebase = rebase_parse_value(value);
>>
>> Here, we never had info->rebase == REBASE_INVALID.  The field was
>> left intact when the configuration file had a rebase type that is
>> not known to this version of git.  Now it has become possible that
>> info->rebase to be REBASE_INVALID.  Would the code after this part
>> returns be prepared to handle it, and if so how?  At least I think
>> it deserves a comment here, or in rebase_parse_value(), to say (1)
>> that unknown rebase value is treated as false for most of the code
>> that do not need to differentiate between false and unknown, and (2)
>> that assigning a negative value to REBASE_INVALID and always
>> checking if the value is the same or greater than REBASE_TRUE helps
>> to maintain the convention.
>
> Its true that we never had 'info->rebase == REBASE_INVALID', but the
> previous code also considered unknown values as false. 'info' is
> allocated with 'xcalloc', thus 'info->rebase' defaults to false. Thus
> it remains false.

Yes, that is why I was not opposed to the new code. It was just that it was not clear, without some comments I suggested in the latter half of my paragraph you responded above, why it is correct to unconditionally assign to info->rebase and the code the control reaches after this part gets executed does not need any adjustment and simply "works".

Thinking about it again, I think the two points I thought need highlighting in the above belong to the in-code comment for the new helper rebase_parse_value().

    *** in rebase.h ***
    enum rebase_type {
            REBASE_INVALID = -1,
            REBASE_FALSE = 0,
            REBASE_TRUE,
            REBASE_PRESERVE,
            REBASE_MERGES,
            REBASE_INTERACTIVE
    };

    /*
     * Parses textual value for pull.rebase, branch.<name>.rebase, etc.
     * Unrecognised value yields REBASE_INVALID, which traditionally is
     * treated the same way as REBASE_FALSE.
     *
     * The callers that care if (any) rebase is requested should say
     *   if (REBASE_TRUE <= rebase_parse_value(string))
     *
     * The callers that want to differenciate an unrecognised value and
     * false can do so by treating _INVALID and _FALSE differently.
     */
    enum rebase_type rebase_parse_value(const char *value);

or something like that, perhaps.