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

From: Bert Wesarg <bert.wesarg@googlemail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types
Date: Wed, 22 Jan 2020 08:34:46 +0100	[thread overview]
Message-ID: <CAKPyHN0_AVOo_6bdHvy_J9ebnBpSD2NECBiLZ7g=4TcMvfZgYw@mail.gmail.com> (raw)
In-Reply-To: <xmqqv9p475ns.fsf@gitster-ct.c.googlers.com>

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.

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