Re: [PATCHv11 1/7] submodule-config: keep update strategy around

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2016, 19:28 UTC
Message-ID: <xmqq37sphl83.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1455320080-14648-2-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 14 quoted lines

> @@ -340,6 +342,16 @@ static int parse_config(const char *var, const char *value, void *data)
>  			free((void *) submodule->url);
>  			submodule->url = xstrdup(value);
>  		}
> +	} else if (!strcmp(item.buf, "update")) {
> +		if (!value)
> +			ret = config_error_nonbool(var);
> +		else if (!me->overwrite &&
> +			 submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)
> +			warn_multiple_config(me->commit_sha1, submodule->name,
> +					     "update");
> +		else if (parse_submodule_update_strategy(value,
> +			 &submodule->update_strategy) < 0)
> +				die(_("invalid value for %s"), var);

Mental note. This takes "value" that comes from the config API; the pre-context in this hunk treats it as a transient piece of memory and uses xstrdup() on it before storing it away in submodule->url.

Show 14 quoted lines

> +int parse_submodule_update_strategy(const char *value,
> +		struct submodule_update_strategy *dst)
> +{
> +	dst->command = NULL;
> +	if (!strcmp(value, "none"))
> +		dst->type = SM_UPDATE_NONE;
> +	else if (!strcmp(value, "checkout"))
> +		dst->type = SM_UPDATE_CHECKOUT;
> +	else if (!strcmp(value, "rebase"))
> +		dst->type = SM_UPDATE_REBASE;
> +	else if (!strcmp(value, "merge"))
> +		dst->type = SM_UPDATE_MERGE;
> +	else if (skip_prefix(value, "!", &dst->command))
> +		dst->type = SM_UPDATE_COMMAND;

This however uses skip_prefix() on value, making dst->command store a pointer into that transient piece of memory.

That looks inconsistent, doesn't it?  I think this part should be

	else if (value[0] == '!') {
		dst->type = SM_UPDATE_COMMAND;
                dst->command = xstrdup(value + 1);
        }

or something like that. I.e. dst->command should own the piece of memory it points at, no?