On Fri, Feb 17, 2017 at 10:36 AM, Jacob Keller <jacob.keller@xxxxxxxxx> wrote:
> On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller <sbeller@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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.
Yes the comment was not updated in the last round of patches and is
now out of context.
> 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?
Please disregard the comment; I'll remove it in the next reroll.
The submodule API is in such a way that
config_update_recurse_submodules
>
> 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?
Yes that is what I had in mind. See previous comment, in a later series
we could extend that to other strategies such as "revert-in-submodules"
for git-revert or "rebase", "merge" as we curreently have for
"git submodule update".
> Maybe this should be called something like
> recurse_into_submodules_in_worktree() though that is pretty verbose.
I like that. (It's less than double the number of characters, so it's
fine, isn't it?)
Maybe we can abbreviate worktree by "wt" ans "submodules by subs:
/* recurse into submodules in the worktree? */
int rec_subs_wt;
That looks short enough to qualify as non-Java.
> 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.
Mh, makes sense. Maybe I tricked myself into premature optimization,
because I'd expect most of the users not caring about submodules, such
that we want to have a *really* cheap way of saying "no, not interesting in
submodules", which is what this method mainly offers.
Junio also remarked this and the following
"is_active_submodule_with_strategy" to be bad design.
I'll redo those, such that the caller decides what to do with each strategy.
>
>> +
>> +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.
0 is already taken as SM_UPDATE_UNSPECIFIED,
so maybe we'd introduce a new update command
SM_UPDATE_IGNORE = -1 or rather use
SM_UPDATE_NONE instead.
>
>> 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.
correct, I'll reword that.
>
>> + */
>> +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
>>