Re: [PATCH 3/2] batch check whether submodule needs pushing into one call
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 16, 2016, 17:59 UTC
- Message-ID
- <xmqq8turlo8m.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160915121044.GA96648@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
Show 6 quoted lines
> +static void append_hash_to_argv(const unsigned char sha1[20], void *data)
> {
> - if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
> + struct argv_array *argv = (struct argv_array *) data;
> + argv_array_push(argv, sha1_to_hex(sha1));
> +}Hmph, why do I think I've seen this before in the previous patch?
... scans through this patch and finds that a similar one is
removed ;-)OK. This makes sense.
Show 23 quoted lines
> +static void check_has_hash(const unsigned char sha1[20], void *data)
> +{
> + int *has_hash = (int *) data;
> +
> + if (!lookup_commit_reference(sha1))
> + *has_hash = 0;
> +}
> +
> +static int submodule_has_hashes(const char *path, struct sha1_array *hashes)
> +{
> + int has_hash = 1;
> +
> + if (add_submodule_odb(path))
> + return 0;
> +
> + sha1_array_for_each_unique(hashes, check_has_hash, &has_hash);
> + return has_hash;
> +}
> +
> +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)
> +{
> + if (!submodule_has_hashes(path, hashes))
> return 0;I think you meant well, but this optimization is wrong. A mere presence of an object does not mean that the current tip can reach that object. Imagine you pushed commit A earlier to them at the tip, then pushed commit A~ to them at the tip, which is the current state of the remote of the submodule, and since them they may have GC'ed. They no longer have the commit A.
For that matter, because you are doing this check by pretending as if all the submodule objects are in the object store of the current superproject you are working in, and saying "it exists there in the submodule repository" when the only thing you know is it exists in an object store of either the submodule repository, the superproject repository, or any of the other submodule repositories, you really cannot tell much from a mere presence of an object. Not just the remote of the submodule repository you are interested in, but the submodule repository you are interested in itself, may not have that object.
Drop the previous two helper functions and this short-cut.
Show 29 quoted lines
> if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
> struct child_process cp = CHILD_PROCESS_INIT;
> - const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
> +
> + argv_array_push(&cp.args, "rev-list");
> + sha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);
> + argv_array_pushl(&cp.args, "--not", "--remotes", "-n", "1" , NULL);
> +
> struct strbuf buf = STRBUF_INIT;
> int needs_pushing = 0;
>
> - argv[1] = sha1_to_hex(sha1);
> - cp.argv = argv;
> prepare_submodule_repo_env(&cp.env_array);
> cp.git_cmd = 1;
> cp.no_stdin = 1;
> cp.out = -1;
> cp.dir = path;
> if (start_command(&cp))
> - die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s",
> - sha1_to_hex(sha1), path);
> + die("Could not run 'git rev-list <hashes> --not --remotes -n 1' command in submodule %s",
> + path);
> if (strbuf_read(&buf, cp.out, 41))
> needs_pushing = 1;
> finish_command(&cp);
> @@ -601,21 +628,6 @@ static void find_unpushed_submodule_commits(struct commit *commit,
> diff_tree_combined_merge(commit, 1, &rev);
> }Good. This is the optimization I alluded to in the review of the first one in the series.