Re: [PATCH 08/15] submodules: introduce check to see whether to touch a submodule

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> In later patches we introduce the --recurse-submodule flag for commands
> that modify the working directory, e.g. git-checkout.
>
> It is potentially expensive to check if a submodule needs an update,
> because a common theme to interact with submodules is to spawn a child
> process for each interaction.
>
> So let's introduce a function that checks if a submodule needs
> to be checked for an update before attempting the update.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c | 27 +++++++++++++++++++++++++++
>  submodule.h | 13 +++++++++++++
>  2 files changed, 40 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 591f4a694e..2a37e03420 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -548,6 +548,33 @@ void set_config_update_recurse_submodules(int value)
>  	config_update_recurse_submodules = value;
>  }
>  
> +int touch_submodules_in_worktree(void)
> +{
> +	/*
> +	 * Update can't be "none", "merge" or "rebase",
> +	 * treat any value as OFF, except an explicit ON.
> +	 */
> +	return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
> +}

I somehow sense a somewhat misnamed function.

> +int is_active_submodule_with_strategy(const struct cache_entry *ce,
> +				      enum submodule_update_type strategy)
> +{
> +	const struct submodule *sub;
> +
> +	if (!S_ISGITLINK(ce->ce_mode))
> +		return 0;
> +
> +	if (!touch_submodules_in_worktree())
> +		return 0;

Reading this caller alone, it is totally unclear what this !touch is
about.  "We try to touch it by calling this function, and if the
function successfullly touches it, we return without doing anything
else?"

Would it help to avoid confusion, if the helper function is named to
be clearly a boolean?  should_update_submodules_in_worktree() or
something along those lines?

> +	sub = submodule_from_path(null_sha1, ce->name);
> +	if (!sub)
> +		return 0;
> +
> +	return sub->update_strategy.type == strategy;
> +}

I am not sure if this is a good API design; if it were "static int"
contained inside the module I wouldn't care, but wouldn't it be more
natural for the caller of this function to say

	if (get_submodule_update_strategy(ce) == STRATEGY_I_WANT)
		do something;
	else
		do something else;

rather than forced to say:

	if (is_active_submodule_with_strategy(ce, STRATEGY_I_WANT))
		do something;
	else
		do something else;

no?  The caller can easily be extended to

	switch (get_submodule_update_strategy(ce)) {
        case STRATEGY_I_WANT:
	case STRATEGY_I_TOLERATE:
		do something; 
		break;
	default:
		do something else;
		break;
	}                

if the function does not insist taking a single allowed strategy and
return just yes/no as its answer.