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.
While my change may set 'info->rebase' implicitly to 'REBASE_INVALID'
I also changed all truth value checks to '>= REBASE_TRUE'. Therefore,
(and I must admit) incidentally, I did not introduced a function
change. Both versions handle unknown '.rebase' values as false.
If this is the expected behavior, I will add a comment to the line,
with that finding. If not, I will map 'REBASE_INVALID' to
'REBASE_TRUE' in that case.
Bert
