diff mbox series

[1/2] fetch-pack: call prepare_shallow_info only if v0

Message ID d2eb101709cc9b300e38151b14c97f96a2b14188.1553546216.git.jonathantanmy@google.com (mailing list archive)
State New, archived
Headers show
Series Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully | expand

Commit Message

Jonathan Tan March 25, 2019, 8:43 p.m. UTC
In fetch_pack(), be clearer that there is no shallow information before
the fetch when v2 is used - memset the struct shallow_info to 0 instead
of calling prepare_shallow_info().

This patch is in preparation for a future patch in which a v2 fetch
might call prepare_shallow_info() after shallow info has been retrieved
during the fetch, so I needed to ensure that prepare_shallow_info() is
not called before the fetch.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit.h     |  4 ++++
 fetch-pack.c | 10 +++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Jeff King March 26, 2019, 5 a.m. UTC | #1
On Mon, Mar 25, 2019 at 01:43:22PM -0700, Jonathan Tan wrote:

> In fetch_pack(), be clearer that there is no shallow information before
> the fetch when v2 is used - memset the struct shallow_info to 0 instead
> of calling prepare_shallow_info().
> 
> This patch is in preparation for a future patch in which a v2 fetch
> might call prepare_shallow_info() after shallow info has been retrieved
> during the fetch, so I needed to ensure that prepare_shallow_info() is
> not called before the fetch.

Makes sense.

I wondered here:

> diff --git a/fetch-pack.c b/fetch-pack.c
> index e69993b2eb..a0eb268dfc 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1648,13 +1648,17 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  		packet_flush(fd[1]);
>  		die(_("no matching remote head"));
>  	}
> -	prepare_shallow_info(&si, shallow);
> -	if (version == protocol_v2)
> +	if (version == protocol_v2) {
> +		if (shallow && shallow->nr)
> +			BUG("Protocol V2 does not provide shallows at this point in the fetch");
> +		memset(&si, 0, sizeof(si));
>  		ref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,
>  					   pack_lockfile);

...who actually might set "shallow". Specifically, I was curious whether
a v2 server could erroneously send us shallow lines, which would trigger
our BUG(), when in fact we should be complaining about the server.

But the answer is no, they have no opportunity. AFAICT, this would only
be set by a call to get_remote_heads(), which we do only for v0/v1
protocols. So there's no way to trigger the BUG. Good.

-Peff
diff mbox series

Patch

diff --git a/commit.h b/commit.h
index 42728c2906..a3f2b2eddb 100644
--- a/commit.h
+++ b/commit.h
@@ -257,6 +257,10 @@  extern void setup_alternate_shallow(struct lock_file *shallow_lock,
 extern const char *setup_temporary_shallow(const struct oid_array *extra);
 extern void advertise_shallow_grafts(int);
 
+/*
+ * Initialize with prepare_shallow_info() or zero-initialize (equivalent to
+ * prepare_shallow_info with a NULL oid_array).
+ */
 struct shallow_info {
 	struct oid_array *shallow;
 	int *ours, nr_ours;
diff --git a/fetch-pack.c b/fetch-pack.c
index e69993b2eb..a0eb268dfc 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1648,13 +1648,17 @@  struct ref *fetch_pack(struct fetch_pack_args *args,
 		packet_flush(fd[1]);
 		die(_("no matching remote head"));
 	}
-	prepare_shallow_info(&si, shallow);
-	if (version == protocol_v2)
+	if (version == protocol_v2) {
+		if (shallow && shallow->nr)
+			BUG("Protocol V2 does not provide shallows at this point in the fetch");
+		memset(&si, 0, sizeof(si));
 		ref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,
 					   pack_lockfile);
-	else
+	} else {
+		prepare_shallow_info(&si, shallow);
 		ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
 					&si, pack_lockfile);
+	}
 	reprepare_packed_git(the_repository);
 
 	if (!args->cloning && args->deepen) {