Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call

[Stefan Beller <sbeller@google.com>]

On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> We run a command for each sha1 change in a submodule. This is
> unnecessary since we can simply batch all sha1's we want to check into
> one command. Lets do it so we can speedup the check when many submodule
> changes are in need of checking.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  submodule.c | 63 +++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 34 insertions(+), 29 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 5044afc2f8..a05c2a34b1 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -529,27 +529,49 @@ static int append_hash_to_argv(const unsigned char sha1[20], void *data)
>         return 0;
>  }
>
> -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
> +static int check_has_hash(const unsigned char sha1[20], void *data)
>  {
> -       if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
> +       int *has_hash = (int *) data;
> +
> +       if (!lookup_commit_reference(sha1))
> +               *has_hash = 0;
> +
> +       return 0;
> +}
> +
> +static int submodule_has_hashes(const char *path, struct sha1_array *hashes)
> +{
> +       int has_hash = 1;
> +
> +       if (add_submodule_odb(path))
> +               return 0;
> +
> +       sha1_array_for_each_unique(hashes, check_has_hash, &has_hash);
> +       return has_hash;
> +}
> +
> +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)
> +{
> +       if (!submodule_has_hashes(path, hashes))

So the above is an implicit lookup already, but we did that before,
too, so it's fine.

> @@ -658,13 +665,11 @@ int find_unpushed_submodules(struct sha1_array *hashes,
>         argv_array_clear(&argv);
>
>         for (i = 0; i < submodules.nr; i++) {
> -               struct string_list_item *item = &submodules.items[i];
> -               struct collect_submodule_from_sha1s_data data;
> -               data.submodule_path = item->string;
> -               data.needs_pushing = needs_pushing;
> -               sha1_array_for_each_unique((struct sha1_array *) item->util,
> -                               collect_submodules_from_sha1s,
> -                               &data);
> +               struct string_list_item *submodule = &submodules.items[i];
> +               struct sha1_array *hashes = (struct sha1_array *) submodule->util;
> +
> +               if (submodule_needs_pushing(submodule->string, hashes))
> +                       string_list_insert(needs_pushing, submodule->string);

That makes sense.

Thanks!
Stefan