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

On Fri, Oct 07, 2016 at 11:16:31AM -0700, Stefan Beller wrote:

quoted

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

Thanks. So I do not completely get what you are suggesting: args or kept
it the way it is? Since in the end you are saying it is ok here ;) I
mainly chose this name because I am substituting the argv variable which
is already called 'argv' with this array. That might also be the reason
why in so many locations with struct child_processe's we have the 'argv'
name: Because they initially started with the old-style NULL terminated
array.

I am fine with it either way. Just tell me what you like :)

Cheers Heiko