Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules

[Stefan Beller <sbeller@google.com>]

+ Heiko, who touched the pushing code end of last year.

On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams <bmwill@google.com> wrote:
> There are currently two instances (fetch and push) where we want to
> determine if submodules have changed given some revision specification.
> These two instances don't use the same logic to generate a list of
> changed submodules and as a result there is a fair amount of code
> duplication.
>
> This patch refactors these two code paths such that they both use the
> same logic to generate a list of changed submodules.  This also makes it
> easier for future callers to be able to reuse this logic as they only
> need to create an argv_array with the revision specification to be using
> during the revision walk.

Thanks for doing some refactoring. :)

> -static struct oid_array *submodule_commits(struct string_list *submodules,
> -                                           const char *path)
> ...

> -static void free_submodules_oids(struct string_list *submodules)
> -{
> ...

These are just moved north, no change in code.
If you want to be extra nice to reviewers this could have been an extra patch,
as it makes reviewing easier if you just have to look at the lines of code with
one goal ("shuffling code around, no change intended" vs "make a change
to improve things with no bad side effects")



> +
> +static void collect_changed_submodules_cb(struct diff_queue_struct *q,
> +                                         struct diff_options *options,
> +                                         void *data)
> +{

This one combines both collect_submodules_from_diff and
submodule_collect_changed_cb ?

> @@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,
>         return ret;
>  }
>
> -static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
> -{
> -       int is_present = 0;
> -       if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
> -               /* Even if the submodule is checked out and the commit is
> -                * present, make sure it is reachable from a ref. */
> -               struct child_process cp = CHILD_PROCESS_INIT;
> -               const char *argv[] = {"rev-list", "-n", "1", NULL, "--not", "--all", NULL};
> -               struct strbuf buf = STRBUF_INIT;
> -
> -               argv[3] = sha1_to_hex(sha1);
> -               cp.argv = argv;
> -               prepare_submodule_repo_env(&cp.env_array);
> -               cp.git_cmd = 1;
> -               cp.no_stdin = 1;
> -               cp.dir = path;
> -               if (!capture_command(&cp, &buf, 1024) && !buf.len)
> -                       is_present = 1;

Oh, I see where the nit in patch 5/6 is coming from. Another note
on that: The hint is way off. The hint should be on the order of
GIT_SHA1_HEXSZ

>  int find_unpushed_submodules(struct oid_array *commits,
>                 const char *remotes_name, struct string_list *needs_pushing)
> ...

>  static void calculate_changed_submodule_paths(void)
> ...

These are both nicely clean now.

Thanks,
Stefan