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

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

Am 04.02.2014 01:01, schrieb Jonathan Nieder:
> Jens Lehmann wrote:
>> --- /dev/null
>> +++ 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
>> +	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.
> 
> Tweaks:
> 
>  * Spelling out "--no-recurse-submodules, --recurse-submodules" (imitating
>    e.g. --decorate in git-log(1))
> 
>  * Shortening, using imperative mood
>  
>  * Skipping description of safety check, since it matches how checkout
>    works in general
> 
> That would make
> 
> 	--no-recurse-submodules::
> 	--recurse-submodules::
> 		Perform the checkout in submodules, too.  This only affects
> 		submodules with update strategy `checkout` (which is the
> 		default update strategy; see `submodule.<name>.update` in
> 		link:gitmodules[5]).
> 	+
> 	The default behavior is to update submodule entries in the superproject
> 	index and to leave the inside of submodules alone.  That behavior can also
> 	be requested explicitly with --no-recurse-submodules.

Much better, thanks!

> Ideas for further work:
> 
>  * The safety check probably deserves a new section where it could be
>    described in detail alongside a description of the corresponding check
>    for plain checkout.  Then the description of the -f option could
>    point to that section.

Good idea.

>  * What happens when update = merge, rebase, or !command?  I think
>    skipping them for now like suggested above is fine, but:
> 
>    - It would be even better to error out when there are changes to carry
>      over with update = merge or rebase

In the first round I'd rather do nothing (just like we do now) for merge
or rebase. These two should be tackled in a follow up series (especially
as I currently do not think everybody agrees on the desired behavior when
the branch config is set yet)

>    - Better still to perform the rebase when update = rebase
> 
>    - I have no idea what update = merge should do for non-fast-forward
>      moves

The same it does for checkout when we would overwrite local changes:
error out before doing anything and let the user sort things out?

>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -16,6 +16,8 @@ static struct string_list config_name_for_path;
>>  static struct string_list config_fetch_recurse_submodules_for_name;
>>  static struct string_list config_ignore_for_name;
>>  static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
>> +static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
>> +static int option_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
> 
> Confusingly, config_update_recurse_submodules is set using the
> --recurse-submodules-default option, not configuration.  There's
> precedent for that in fetch.recurseSubmodules handling, but perhaps
> a comment would help --- something like
> 
> 	/*
> 	 * When no --recurse-submodules option was passed, should git fetch
> 	 * from submodules where submodule.<name>.fetchRecurseSubmodules
> 	 * doesn't indicate what to do?
> 	 *
> 	 * Controlled by fetch.recurseSubmodules.  The default is determined by
> 	 * the --recurse-submodules-default option, which propagates
> 	 * --recurse-submodules from the parent git process when recursing.
> 	 */
> 	static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
> 
> 	/*
> 	 * When no --recurse-submodules option was passed, should git update
> 	 * the index and worktree within submodules (and in turn their
> 	 * submodules, etc)?
> 	 *
> 	 * Controlled by the --recurse-submodules-default option, which
> 	 * propagates --recurse-submodules from the parent git process
> 	 * when recursing.
> 	 */
> 	static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;

Makes lots of sense.

> [...]
>> @@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
>>  	}
>>  }
>>
>> +int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
>> +{
>> +	switch (git_config_maybe_bool(opt, arg)) {
>> +	case 1:
>> +		return RECURSE_SUBMODULES_ON;
>> +	case 0:
>> +		return RECURSE_SUBMODULES_OFF;
>> +	default:
>> +		if (!strcmp(arg, "checkout"))
>> +			return RECURSE_SUBMODULES_ON;
> 
> Hm, is this arg == checkout case futureproofing for when
> --recurse-submodules learns to handle submodules without
> 'update = checkout', too?

Right.

> Is it safe to leave it out for now?

Yes it is.

> [...]
>> +int submodule_needs_update(const char *path)
> 
> Return value convention: 1 means "do update"; 0 means "don't update".
> 
> Some day later I suppose 2 or -1 could mean "error out".  Ok.
> 
> Naming nit: needs_update sounds like it's checking if there was a
> change at that path.  How about something like submodule_should_update(),
> !submodule_ignore_for_update(), or update_should_recurse_into_submodule()?

Good point, will do.

> [...]
>> @@ -589,6 +633,12 @@ int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
>>  	return ret;
>>  }
>>
>> +void set_config_update_recurse_submodules(int default_value, int option_value)
>> +{
>> +	config_update_recurse_submodules = default_value;
>> +	option_update_recurse_submodules = option_value;
>> +}
> 
> Could option_parse_update_submodules set
> option_update_recurse_submodules directly?  Alternatively, could this
> function examine option_value so that submodule.c would only need one
> variable?
> 
> 	if (option_value == RECURSE_SUBMODULES_DEFAULT)
> 		update_recurse_submodules = default_value;
> 	else
> 		update_recurse_submodules = option_value;
> 
> If .gitmodules some day grows a submodule.<name>.checkoutRecurseSubmodules
> option then it would be convenient to have the option that overrides and
> the default tracked separately.  Is that the idea here?

Correct. I intend to add a global and per-submodule "autoupdate" setting
just like those we have for fetch.

> I might try writing a dummy command to test this basic --recurse-submodules
> option handling as a separate patch.

Hmm, I haven't thought of that. So far I was testing this in the regular
test cases and intended to add that to the test framework. Will think
about that.