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

[Stefan Beller <sbeller@google.com>]

On Thu, Feb 4, 2016 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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.


ok

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

"Note that the !command form is intentionally ignored here for
security reasons."

However you can overwrite the update field in .git/config to be "! [foo]",
which we also need to support, so let's keep it a string for now?

>
> Thanks,
> Jonathan