On Thu, Feb 4, 2016 at 4:59 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html