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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 16, 2017, 21:02 UTC
Message-ID: <xmqqd1ehx30c.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170216003811.18273-9-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 32 quoted lines

> 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.

Show 10 quoted lines

> +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?

Show 6 quoted 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.