Re: [PATCH v2 2/3] serialize collection of refs that contain submodule changes

From: Stefan Beller <sbeller@google.com>
Date: Oct 7, 2016, 18:16 UTC
Message-ID: <CAGZ79kZ5CPTuW2fARDs3BUt89b8H_=P3otZv+Vm5nTV70NLWtg@mail.gmail.com>
In-Reply-To: <81bdbf6a1295c17c1b9233c91da6e5eb4583785e.1475851621.git.hvoigt@hvoigt.net>

On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:

Show 24 quoted lines

> We are iterating over each pushed ref and want to check whether it
> contains changes to submodules. Instead of immediately checking each ref
> lets first collect them and then do the check for all of them in one
> revision walk.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  submodule.c | 36 +++++++++++++++++++++---------------
>  submodule.h |  5 +++--
>  transport.c | 29 +++++++++++++++++++++--------
>  3 files changed, 45 insertions(+), 25 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 59c9d15905..5044afc2f8 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct object_id *oid,
>         return 1;
>  }
>
> +static int append_hash_to_argv(const unsigned char sha1[20], void *data)
> +{
> +       struct argv_array *argv = (struct argv_array *) data;
> +       argv_array_push(argv, sha1_to_hex(sha1));

Nit of the day: When using the struct child-process, we have the oldstyle argv NULL terminated array as well as the new style args argv_array. So in that context we'd prefer `args` as a name for argv_array as that helps to distinguish from the old array type. Here however `argv` seems to be a reasonable name, in fact whenever we do not deal with child processes, we seem to not like the `args` name:

    $ git grep argv_array |wc -l
    577
    $ git grep argv_array |grep args |wc -l
    293

The rest looks good to me. :)