Re: [PATCHv8 1/9] submodule-config: keep update strategy around

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 5, 2016, 00:59 UTC
Message-ID: <20160205005946.GA19501@google.com>
In-Reply-To: <1454623776-3347-2-git-send-email-sbeller@google.com>

Hi,

It's been a while since I looked at this series. Hopefully I can come at it with some fresh eyes. Thanks for your perseverance.

Stefan Beller wrote:

> We need the submodule update strategies in a later patch.

This description doesn't explain what the patch will do or why parse_config didn't already retain the value. If I look back at this patch later and want to understand why it was written, what would I want to know?

It could say

	Currently submodule.<name>.update is only handled by git-submodule.sh.
	C code will start to need to make use of that value as more of the
	functionality of git-submodule.sh moves into library code in C.

	Add the update field to 'struct submodule' and populate it so it can
	be read as sm->update.

[...]

Show 15 quoted lines

> +++ b/submodule-config.c
> @@ -311,6 +312,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 != NULL)
> +			warn_multiple_config(me->commit_sha1, submodule->name,
> +					     "update");
> +		else {
> +			free((void *) submodule->update);
> +			submodule->update = xstrdup(value);
> +		}

(not about this patch) This code is repetitive. Would there be a way to share code between the parsing of different per-submodule settings?

[...]

Show 7 quoted lines

> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -14,6 +14,7 @@ struct submodule {
>  	const char *url;
>  	int fetch_recurse;
>  	const char *ignore;
> +	const char *update;

gitmodules(5) tells me the only allowed values are checkout, rebase, merge, and none. I wouldn't know at a glance how to match against those in calling code. Can this be an enum?

Thanks, Jonathan