Hi,

Stefan Beller wrote:

> Signed-off-by: Stefan Beller <[email protected]>
> ---
>  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 <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to