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

[Jonathan Nieder <jrnieder@gmail.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.

[...]
> +++ 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?

[...]
> --- 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