Re: [PATCH/RFC/GSOC] make git-pull a builtin

Hi,

On Thu, Mar 19, 2015 at 6:26 AM, Junio C Hamano [off-list ref] wrote:

Paul Tan [off-list ref] writes:

quoted

+/* Global vars since they are used often */
+static char *head_name;
+static const char *head_name_short;
+static unsigned char head_sha1[20];
+static int head_flags;
+
+enum rebase_type {
+     REBASE_FALSE = 0,
+     REBASE_TRUE = 1,
+     REBASE_PRESERVE = 2
+};
+
+/**
+ * Parse rebase config/option value and return corresponding int
+ */
+static int parse_rebase(const char *arg)
+{
+     if (!strcmp(arg, "true"))
+             return REBASE_TRUE;
+     else if (!strcmp(arg, "false"))
+             return REBASE_FALSE;
+     else if (!strcmp(arg, "preserve"))
+             return REBASE_PRESERVE;
+     else
+             return -1; /* Invalid value */
+}

Even though the original does not use bool-or-string-config, we
would want to do the same by doing something like

        case (config_maybe_bool()) {
        case 0:
                return REBASE_FALSE;
        case 1:
                return REBASE_TRUE;
        default:
                if (!strcmp(arg, "preserve"))
                        return REBASE_PRESERVE;
                return -1;
        }

and then use that in rebase_config_default().

If you mean letting "yes", "on", "no", "off" be accepted on the
command line as well, then yes I guess it will be a good idea.

quoted

+
+/**
+ * Returns default rebase option value
+ */
+static int rebase_config_default(void)
+{
+     struct strbuf name = STRBUF_INIT;
+     const char *value = NULL;
+     int boolval;
+
+     strbuf_addf(&name, "branch.%s.rebase", head_name_short);
+     if (git_config_get_value(name.buf, &value))
+             git_config_get_value("pull.rebase", &value);

What happens when neither is defined?

quoted

+     strbuf_release(&name);
+     if (!value)
+             return REBASE_FALSE;

Hmph, are you sure about this?  Isn't this "[pull] rebase" that does
not have "= value", in which case pull.rebase is "true"?

You cannot use NULL as the sentinel value to tell that you did not
find either branch.*.rebase nor pull.rebase (in which case you want
to default to 'false').  Either of them can be spelled as an
equal-less true, which you will see as value==NULL, and you want to
take that as 'true'.

        const char *value = "false";
        ...
        if (get_value(..., &value))
                get_value(..., &value));
        strbuf_release(&name);
        if (!value)
                return REBASE_TRUE;
        return parse_rebase(value);

or something along that line, perhaps?

Whoops, didn't take into account the possibility that the config value
could be NULL. Thanks.

quoted

+     boolval = git_config_maybe_bool("pull.rebase", value);
+     if (boolval >= 0)
+             return boolval ? REBASE_TRUE : REBASE_FALSE;
+     else if (value && !strcmp(value, "preserve"))
+             return REBASE_PRESERVE;

Is value something you need to free before returning from this
function?

From my reading if config.c, the memory of value comes from the

config_set the_config_set (the config cache), so there is no need to
free it.

quoted

+static int parse_opt_recurse_submodules(const struct option *opt, const char *arg, int unset)
+{
+     if (!arg)
+             *(int *)opt->value = unset ? RS_NO : RS_YES;
+     else if (!strcmp(arg, "no"))
+             *(int *)opt->value = RS_NO;
+     else if (!strcmp(arg, "yes"))
+             *(int *)opt->value = RS_YES;
+     else if (!strcmp(arg, "on-demand"))
+             *(int *)opt->value = RS_ON_DEMAND;
+     else
+             return -1;
+     return 0;

I suspect that maybe-bool-or-string comment applies equally here for
the UI consistency.

Yup, I'll keep that in mind.

I'll stop here for now.  Thanks.

Thanks.