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