Bert Wesarg <bert.wesarg@xxxxxxxxxxxxxx> writes:
> Dear Junio,
>
> On Wed, Jan 22, 2020 at 12:26 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> Bert Wesarg <bert.wesarg@xxxxxxxxxxxxxx> 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.
