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

[Brandon Williams <bmwill@google.com>]

On 04/28, Stefan Beller wrote:
> + 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")

Yeah I suppose, I was having a difficult time breaking this largest
patch into multiple.

> 
> > +
> > +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 ?

Yep.

> 
> > @@ -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

Yeah agreed.  I make this change in the earlier patch.

> 
> >  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

-- 
Brandon Williams