From: Jonathan Nieder <jrnieder@gmail.com>
Date: Tue, 09 Feb 2016 21:49:37 GMT
Subject: Re: [PATCHv9 1/6] submodule-config: keep update strategy around
Message-ID: <20160209214937.GG28749@google.com>
In-Reply-To: <1455051274-15256-2-git-send-email-sbeller@google.com>

Hi,

Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule-config.c | 22 ++++++++++++++++++++++
>  submodule-config.h | 11 +++++++++++
>  2 files changed, 33 insertions(+)

Yay, this is much clearer.  Thanks for indulging.

[...]
> +++ b/submodule-config.c
[...]
> @@ -311,6 +313,26 @@ static int parse_config(const char *var, const char *value, void *data)
> +			else if (!skip_prefix(value, "!", &submodule->update_command))
> +				die(_("invalid value for %s"), var);

Should this say
			else if (skip_prefix(value, "!", &submodule->update_command))
				submodule->update = SM_UPDATE_COMMAND;
			else
				die(...);

?

[...]
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -4,6 +4,15 @@
>  #include "hashmap.h"
>  #include "strbuf.h"
>  
> +enum submodule_update_type {
> +	SM_UPDATE_CHECKOUT,
> +	SM_UPDATE_REBASE,
> +	SM_UPDATE_MERGE,
> +	SM_UPDATE_NONE,
> +	SM_UPDATE_COMMAND,
> +	SM_UPDATE_UNSPECIFIED
> +};

If _UNSPECIFIED is equal to 0, that would futureproof this better
against zero-initialized submodule structs without ->update explicitly
set.

After those two changes and the whitespace change Junio suggested,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>