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

Bert Wesarg [off-list ref] writes:

Dear Junio,

On Wed, Jan 22, 2020 at 12:26 AM Junio C Hamano [off-list ref] wrote:

quoted

Bert Wesarg [off-list ref] writes:

quoted

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.

quoted

quoted

@@ -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.