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

[Jacob Keller <jacob.keller@gmail.com>]

On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller <sbeller@google.com> wrote:
> 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;
> +}

Ok, so here, we're just checking whether the value is
RECURSE_SUBMODULES_ON. The comment doesn't make sense to me at all.
What is "update" and why "can't" it be those values? How is this code
treating thise values as OFF exept for an explicit ON?

At first I thought this comment was related to check in the previous
patch. I think I see the patch is "recurse submodules = true" or
"recurse submodules = checkout" as a specific strategy? Ie:
recurse-submodules=checkout" means "recurse into submodules and update
them using checkout strategy?

Ok this starts to make a bit more sense. However, it's still somewhat
confusing to me.

Maybe this should be called something like
recurse_into_submodules_in_worktree() though that is pretty verbose.

I'm not sure I have a good name really. But I wonder why we need this
in the first place. Basically, we set RECURSE_SUBMODULES_* value which
could be OFF, ON, or even future extensions of CHECKOUT, REBASE,
MERGE, etc?

But shouldn't we just return the mode, and let the later code decide
"oh. actually I don't support this mode". For now, obviously we
wouldn't set any of the new modes yet.

> +
> +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;
> +
> +       sub = submodule_from_path(null_sha1, ce->name);
> +       if (!sub)
> +               return 0;
> +
> +       return sub->update_strategy.type == strategy;
> +}
> +

I liked Junio's suggestion where this just returns the strategy that
it can use, or 0 if it shouldn't be updated. Then, other code just
decides: Yes, I can use this strategy or no I cannot.

Should this be tied in with the strategy used by the
recurse_submodules above? ie: when/if we support recursing using other
strategies, shouldn't we make these match here, so that if the recurse
mode is "checkout" we don't checkout a submodule that was configured
to be rebased? Or do you want to blindly apply checkout to all
submodules even if they don't have strategy?

I assume you do not, since you check here with passing a strategy
value, and then see if it matches.

this could be named something like:

get_active_submodule_strategy() and return the strategy directly. It
would then return 0 in any case where it shouldn't be updated. Later
code can then check the strategy.

>  static int has_remote(const char *refname, const struct object_id *oid,
>                       int flags, void *cb_data)
>  {
> diff --git a/submodule.h b/submodule.h
> index b4e60c08d2..46d9f0f293 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -65,6 +65,19 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,
>                 const struct diff_options *opt);
>  extern void set_config_fetch_recurse_submodules(int value);
>  extern void set_config_update_recurse_submodules(int value);
> +
> +/*
> + * Traditionally Git ignored changes made for submodules.
> + * This function checks if we are interested in the given submodule
> + * for any kind of operation.

This comment seems a bit weird.

> + */
> +extern int touch_submodules_in_worktree(void);
> +/*
> + * Check if the given ce entry is a submodule with the given update
> + * strategy configured.

I like Junio's suggestion of this "getting the current configured
strategy for a submodule. When we aren't set to recurse into
submodules we (obviously) return that we have no strategy since we're
not going to update it at all.

> + */
> +extern int is_active_submodule_with_strategy(const struct cache_entry *ce,
> +                                            enum submodule_update_type strategy);
>  extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
>  extern int fetch_populated_submodules(const struct argv_array *options,
>                                const char *prefix, int command_line_option,
> --
> 2.12.0.rc1.16.ge4278d41a0.dirty
>