Re: [WIP/PATCH 1/9] submodule: prepare for recursive checkout of submodules

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 03.02.2014 23:23, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> This commit adds the functions and files needed for configuration,
> 
> Please just say "Add the functions and files needed for ...".

Roger that.

>> +++ b/Documentation/recurse-submodules-update.txt
>> @@ -0,0 +1,8 @@
>> +--[no-]recurse-submodules::
>> +	Using --recurse-submodules will update the work tree of all
>> +	initialized submodules according to the commit recorded in the
>> +	superproject if their update configuration is set to checkout'. If
> 
> That single quote does not seem to be closing any matching quote.
> 
> The phrase "according to" feels a bit too fuzzy.  Merging the commit
> to what is checked out is one possible implementation of "according to".
> Applying the diff between the commit and what is checked out to work
> tree is another.  Resetting the work tree files to exactly match the
> commit would be yet another.
> 
> I think "update the work trees to the commit" (i.e. lose the
> "according") would be the closest to what you are trying to say
> here.
> 
>> +	local modifications in a submodule would be overwritten the checkout
>> +	will fail unless forced. Without this option or with
>> +	--no-recurse-submodules is, the work trees of submodules will not be
>> +	updated, only the hash recorded in the superproject will be updated.
> 
> It is unclear what happens if their update configuration is set to
> something other than 'checkout'.

Jonathan already proposed a better description, will use that in the next
round.

>> diff --git a/submodule.c b/submodule.c
>> index 613857e..b3eb28d 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
>> ...
>> +int option_parse_update_submodules(const struct option *opt,
>> +				   const char *arg, int unset)
>> +{
>> +	if (unset) {
>> +		*(int *)opt->value = RECURSE_SUBMODULES_OFF;
>> +	} else {
>> +		if (arg)
>> +			*(int *)opt->value = parse_update_recurse_submodules_arg(opt->long_name, arg);
>> +		else
>> +			*(int *)opt->value = RECURSE_SUBMODULES_ON;
>> +	}
> 
> You can easily unnest to lose {}
> 
>     if (unset)
>             value = off;
>     else if (arg)
>             value = parse...;
>     else
>             value = on;

Yeah, that's better.

> Also I suspect that git_config_maybe_bool() natively knows how to
> handle arg==NULL, so
> 
>     if (unset)
> 	value = off;
>     else
> 	value = parse...;
> 
> is sufficient?

Will try.