[PATCH 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully

[PATCH 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully

[Jonathan Tan]

This is built on master+jt/test-protocol-version, but should apply
cleanly to master.

After [1] and this patch set, there is only one more small thing to fix
or mark GIT_TEST_PROTOCOL_VERSION=0 at master+jt/test-protocol-version.

This is very similar to [2], which was neither reviewed nor merged,
perhaps because I had another patch [3] that I declared to supersede [2]
and that was merged as bd0b42aed3 ("fetch-pack: do not take shallow lock
unnecessarily", 2019-01-10). But it turns out that [3] was not enough to
make all tests pass with GIT_TEST_PROTOCOL_VERSION=2, hence this patch
set.

[1] https://public-inbox.org/git/20190322190139.151189-1-jonathantanmy@google.com/
[2] https://public-inbox.org/git/20181220195349.92214-1-jonathantanmy@google.com/
[3] https://public-inbox.org/git/20190110193645.34080-1-jonathantanmy@google.com/

Jonathan Tan (2):
  fetch-pack: call prepare_shallow_info only if v0
  fetch-pack: respect --no-update-shallow in v2

 commit.h     |  4 ++++
 fetch-pack.c | 54 +++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 47 insertions(+), 11 deletions(-)

-- 
2.21.0.155.ge902e9bcae.dirty

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

[Jonathan Tan]

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

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) {
-- 
2.21.0.155.ge902e9bcae.dirty

[PATCH 2/2] fetch-pack: respect --no-update-shallow in v2

[Jonathan Tan]

In protocol v0, when sending "shallow" lines, the server distinguishes
between lines caused by the remote repo being shallow and lines caused
by client-specified depth settings. Unless "--update-shallow" is
specified, there is a difference in behavior: refs that reach the former
"shallow" lines, but not the latter, are rejected. But in v2, the server
does not, and the client treats all "shallow" lines like lines caused by
client-specified depth settings.

Full restoration of v0 functionality is not possible without protocol
change, but we can implement a heuristic: if we specify any depth
setting, treat all "shallow" lines like lines caused by client-specified
depth settings (that is, unaffected by "--no-update-shallow"), but
otherwise, treat them like lines caused by the remote repo being shallow
(that is, affected by "--no-update-shallow"). This restores most of v0
behavior, except in the case where a client fetches from a shallow
repository with depth settings.

This patch causes a test that previously failed with
GIT_TEST_PROTOCOL_VERSION=2 to pass.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index a0eb268dfc..672c79c91a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1253,9 +1253,13 @@ static int process_acks(struct fetch_negotiator *negotiator,
 }
 
 static void receive_shallow_info(struct fetch_pack_args *args,
-				 struct packet_reader *reader)
+				 struct packet_reader *reader,
+				 struct shallow_info *si)
 {
-	int line_received = 0;
+	struct oid_array *shallows;
+	int unshallow_received = 0;
+
+	shallows = xcalloc(1, sizeof(*shallows));
 
 	process_section_header(reader, "shallow-info", 0);
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
@@ -1265,8 +1269,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 		if (skip_prefix(reader->line, "shallow ", &arg)) {
 			if (get_oid_hex(arg, &oid))
 				die(_("invalid shallow line: %s"), reader->line);
-			register_shallow(the_repository, &oid);
-			line_received = 1;
+			oid_array_append(shallows, &oid);
 			continue;
 		}
 		if (skip_prefix(reader->line, "unshallow ", &arg)) {
@@ -1279,7 +1282,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 				die(_("error in object: %s"), reader->line);
 			if (unregister_shallow(&oid))
 				die(_("no shallow found: %s"), reader->line);
-			line_received = 1;
+			unshallow_received = 1;
 			continue;
 		}
 		die(_("expected shallow/unshallow, got %s"), reader->line);
@@ -1289,11 +1292,35 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 	    reader->status != PACKET_READ_DELIM)
 		die(_("error processing shallow info: %d"), reader->status);
 
-	if (line_received) {
+	if (args->deepen || unshallow_received) {
+		/*
+		 * Treat these as shallow lines caused by our depth settings.
+		 * In v0, these lines cannot cause refs to be rejected; do the
+		 * same.
+		 */
+		int i;
+
+		for (i = 0; i < shallows->nr; i++)
+			register_shallow(the_repository, &shallows->oid[i]);
+		oid_array_clear(shallows);
+		free(shallows);
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
 		args->deepen = 1;
+	} else if (shallows->nr) {
+		/*
+		 * Treat these as shallow lines caused by the remote being
+		 * shallow. In v0, remote refs that reach these objects are
+		 * rejected (unless --update-shallow is set); do the same.
+		 */
+		prepare_shallow_info(si, shallows);
+		if (si->nr_ours || si->nr_theirs)
+			alternate_shallow_file =
+				setup_temporary_shallow(si->shallow);
+		else
+			alternate_shallow_file = NULL;
 	} else {
+		free(shallows);
 		alternate_shallow_file = NULL;
 	}
 }
@@ -1337,6 +1364,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				    int fd[2],
 				    const struct ref *orig_ref,
 				    struct ref **sought, int nr_sought,
+				    struct shallow_info *si,
 				    char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
@@ -1411,7 +1439,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		case FETCH_GET_PACK:
 			/* Check for shallow-info section */
 			if (process_section_header(&reader, "shallow-info", 1))
-				receive_shallow_info(args, &reader);
+				receive_shallow_info(args, &reader, si);
 
 			if (process_section_header(&reader, "wanted-refs", 1))
 				receive_wanted_refs(&reader, sought, nr_sought);
@@ -1653,7 +1681,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 			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);
+					   &si, pack_lockfile);
 	} else {
 		prepare_shallow_info(&si, shallow);
 		ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
-- 
2.21.0.155.ge902e9bcae.dirty

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

[Jeff King]

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

Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2

[Jeff King]

On Mon, Mar 25, 2019 at 01:43:23PM -0700, Jonathan Tan wrote:

> In protocol v0, when sending "shallow" lines, the server distinguishes
> between lines caused by the remote repo being shallow and lines caused
> by client-specified depth settings. Unless "--update-shallow" is
> specified, there is a difference in behavior: refs that reach the former
> "shallow" lines, but not the latter, are rejected. But in v2, the server
> does not, and the client treats all "shallow" lines like lines caused by
> client-specified depth settings.
> 
> Full restoration of v0 functionality is not possible without protocol
> change,

That's rather unfortunate. Is this because the v2 ls-refs phase is
separate, and that's when a v0 server would tell us about its shallows?
It looks like in v2 it comes in a separate "shallow-info" section.

What would the protocol change look like?  Would we just need a
capability to instruct the server to mark the two different types of
shallow distinctly? Or do we actually need to convey the information
separately (e.g., in the ls-refs phase)?

None of that matters for your patch here, but I'm just wondering what
the path forward is.

> but we can implement a heuristic: if we specify any depth
> setting, treat all "shallow" lines like lines caused by client-specified
> depth settings (that is, unaffected by "--no-update-shallow"), but
> otherwise, treat them like lines caused by the remote repo being shallow
> (that is, affected by "--no-update-shallow"). This restores most of v0
> behavior, except in the case where a client fetches from a shallow
> repository with depth settings.

That seems like the best we can do without the protocol change. And even
if we adjust the protocol, we need some fallback behavior for existing
v2 servers, so this is worth doing.

>  fetch-pack.c | 44 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 8 deletions(-)

The patch looks reasonable to me, though I am far from an expert on the
shallow bits of the protocol. One thing I did notice:

>  static void receive_shallow_info(struct fetch_pack_args *args,
> -				 struct packet_reader *reader)
> +				 struct packet_reader *reader,
> +				 struct shallow_info *si)
>  {
> -	int line_received = 0;
> +	struct oid_array *shallows;
> +	int unshallow_received = 0;
> +
> +	shallows = xcalloc(1, sizeof(*shallows));

This has to be heap-allocated, since we pass off ownership to "si"
(sometimes). But in the v0 case, it comes from the transport's
&data->shallows of a local variable in cmd_fetch_pack(), and we never
free it. So I think this oid_array ends up getting leaked.

Perhaps it's worth passing down the shallows array we get from the
caller of fetch_pack(). Something like the patch below (I think it is
never NULL, which means in your patch 1 you can simplify the conditional
for the BUG).

diff --git a/fetch-pack.c b/fetch-pack.c
index 672c79c91a..af2ee47a83 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1254,13 +1254,11 @@ static int process_acks(struct fetch_negotiator *negotiator,
 
 static void receive_shallow_info(struct fetch_pack_args *args,
 				 struct packet_reader *reader,
+				 struct oid_array *shallows,
 				 struct shallow_info *si)
 {
-	struct oid_array *shallows;
 	int unshallow_received = 0;
 
-	shallows = xcalloc(1, sizeof(*shallows));
-
 	process_section_header(reader, "shallow-info", 0);
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
 		const char *arg;
@@ -1303,7 +1301,6 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 		for (i = 0; i < shallows->nr; i++)
 			register_shallow(the_repository, &shallows->oid[i]);
 		oid_array_clear(shallows);
-		free(shallows);
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
 		args->deepen = 1;
@@ -1320,7 +1317,6 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 		else
 			alternate_shallow_file = NULL;
 	} else {
-		free(shallows);
 		alternate_shallow_file = NULL;
 	}
 }
@@ -1365,6 +1361,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				    const struct ref *orig_ref,
 				    struct ref **sought, int nr_sought,
 				    struct shallow_info *si,
+				    struct oid_array *shallows,
 				    char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
@@ -1439,7 +1436,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		case FETCH_GET_PACK:
 			/* Check for shallow-info section */
 			if (process_section_header(&reader, "shallow-info", 1))
-				receive_shallow_info(args, &reader, si);
+				receive_shallow_info(args, &reader, shallows, si);
 
 			if (process_section_header(&reader, "wanted-refs", 1))
 				receive_wanted_refs(&reader, sought, nr_sought);
@@ -1681,7 +1678,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 			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,
-					   &si, pack_lockfile);
+					   &si, shallow, pack_lockfile);
 	} else {
 		prepare_shallow_info(&si, shallow);
 		ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,

Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2

[Duy Nguyen]

On Tue, Mar 26, 2019 at 12:20 PM Jeff King <peff@peff.net> wrote:
>
> On Mon, Mar 25, 2019 at 01:43:23PM -0700, Jonathan Tan wrote:
>
> > In protocol v0, when sending "shallow" lines, the server distinguishes
> > between lines caused by the remote repo being shallow and lines caused
> > by client-specified depth settings. Unless "--update-shallow" is
> > specified, there is a difference in behavior: refs that reach the former
> > "shallow" lines, but not the latter, are rejected. But in v2, the server
> > does not, and the client treats all "shallow" lines like lines caused by
> > client-specified depth settings.
> >
> > Full restoration of v0 functionality is not possible without protocol
> > change,
>
> That's rather unfortunate. Is this because the v2 ls-refs phase is
> separate, and that's when a v0 server would tell us about its shallows?
> It looks like in v2 it comes in a separate "shallow-info" section.
>
> What would the protocol change look like?  Would we just need a
> capability to instruct the server to mark the two different types of
> shallow distinctly? Or do we actually need to convey the information
> separately (e.g., in the ls-refs phase)?
>
> None of that matters for your patch here, but I'm just wondering what
> the path forward is.

If it helps (because I'm still catching up with v2 to actually help
review), this case is for cloning from a shallow repo. The commit that
outlines how .git/shallow is updated is 58babfffde (shallow.c: the 8
steps to select new commits for .git/shallow, 2013-12-05).

Since the first shallow info is about the shape of the remote repo
(where refs are the tips), ls-refs sounds like the right place to
include the information. In other words, ls-refs currently tells the
tip/top of the repo, what's missing is the piece about "the bottom"
(shallow cut points).

> > but we can implement a heuristic: if we specify any depth
> > setting, treat all "shallow" lines like lines caused by client-specified
> > depth settings (that is, unaffected by "--no-update-shallow"), but
> > otherwise, treat them like lines caused by the remote repo being shallow
> > (that is, affected by "--no-update-shallow"). This restores most of v0
> > behavior, except in the case where a client fetches from a shallow
> > repository with depth settings.
>
> That seems like the best we can do without the protocol change. And even
> if we adjust the protocol, we need some fallback behavior for existing
> v2 servers, so this is worth doing.

Are people actually doing this (i.e. cloning from or  pushing to a
shallow repo)? I added this with the intention that a big shallow repo
(e.g. one year long history) is served as the common source to reduce
server loads and everything, while the full/big repo is available but
rarely needed. I never saw anyone complain about it (so, likely not
using it).

The description of --update-shallow probably should mention this
fallback behavior? --update-shallow was not default because I feared
the local repo could be cut short by unsolicited shallow requests from
the server side, and it looks like --update-shallow is default (by
mistake) in v2? Maybe I worried for nothing. I dunno.
-- 
Duy

Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2

[Jeff King]

On Tue, Mar 26, 2019 at 05:14:11PM +0700, Duy Nguyen wrote:

> > That seems like the best we can do without the protocol change. And even
> > if we adjust the protocol, we need some fallback behavior for existing
> > v2 servers, so this is worth doing.
> 
> Are people actually doing this (i.e. cloning from or  pushing to a
> shallow repo)? I added this with the intention that a big shallow repo
> (e.g. one year long history) is served as the common source to reduce
> server loads and everything, while the full/big repo is available but
> rarely needed. I never saw anyone complain about it (so, likely not
> using it).

I don't think I've ever seen anybody serve fetches out of a shallow
clone in practice (I don't think we ever seriously considered them at
GitHub, but given their general incompatibility with reachability
bitmaps, I suspect it would cause more performance problems than it
solves).

I've always imagined people do it for one-offs. E.g., they have a
shallow clone, and fetch out of that to a temporary copy. That may be
less useful these days with the advent of separate worktrees.

-Peff

Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2

[Duy Nguyen]

On Tue, Mar 26, 2019 at 10:18 PM Jeff King <peff@peff.net> wrote:
>
> On Tue, Mar 26, 2019 at 05:14:11PM +0700, Duy Nguyen wrote:
>
> > > That seems like the best we can do without the protocol change. And even
> > > if we adjust the protocol, we need some fallback behavior for existing
> > > v2 servers, so this is worth doing.
> >
> > Are people actually doing this (i.e. cloning from or  pushing to a
> > shallow repo)? I added this with the intention that a big shallow repo
> > (e.g. one year long history) is served as the common source to reduce
> > server loads and everything, while the full/big repo is available but
> > rarely needed. I never saw anyone complain about it (so, likely not
> > using it).
>
> I don't think I've ever seen anybody serve fetches out of a shallow
> clone in practice (I don't think we ever seriously considered them at
> GitHub, but given their general incompatibility with reachability
> bitmaps, I suspect it would cause more performance problems than it
> solves).

Yeah. I still think it scales better long term because you can control
this history depth in the common case instead of trying to make git
work well with 30+ years worth of commits. I might try to make
reachability bitmaps and commit-graph work with shallow clones one
day, but that's pretty much in the "hey this is fun, are you bored?"
category.

> I've always imagined people do it for one-offs. E.g., they have a
> shallow clone, and fetch out of that to a temporary copy. That may be
> less useful these days with the advent of separate worktrees.
-- 
Duy

Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2

[Jonathan Tan]

> On Mon, Mar 25, 2019 at 01:43:23PM -0700, Jonathan Tan wrote:
> 
> > In protocol v0, when sending "shallow" lines, the server distinguishes
> > between lines caused by the remote repo being shallow and lines caused
> > by client-specified depth settings. Unless "--update-shallow" is
> > specified, there is a difference in behavior: refs that reach the former
> > "shallow" lines, but not the latter, are rejected. But in v2, the server
> > does not, and the client treats all "shallow" lines like lines caused by
> > client-specified depth settings.
> > 
> > Full restoration of v0 functionality is not possible without protocol
> > change,
> 
> That's rather unfortunate. Is this because the v2 ls-refs phase is
> separate, and that's when a v0 server would tell us about its shallows?
> It looks like in v2 it comes in a separate "shallow-info" section.

That's right. In v2, it comes in "shallow-info", which happens right
before the server sends the packfile.

> What would the protocol change look like?  Would we just need a
> capability to instruct the server to mark the two different types of
> shallow distinctly? Or do we actually need to convey the information
> separately (e.g., in the ls-refs phase)?
> 
> None of that matters for your patch here, but I'm just wondering what
> the path forward is.

Conveying it in the ls-refs would work.

> > but we can implement a heuristic: if we specify any depth
> > setting, treat all "shallow" lines like lines caused by client-specified
> > depth settings (that is, unaffected by "--no-update-shallow"), but
> > otherwise, treat them like lines caused by the remote repo being shallow
> > (that is, affected by "--no-update-shallow"). This restores most of v0
> > behavior, except in the case where a client fetches from a shallow
> > repository with depth settings.
> 
> That seems like the best we can do without the protocol change. And even
> if we adjust the protocol, we need some fallback behavior for existing
> v2 servers, so this is worth doing.

Thanks.

> The patch looks reasonable to me, though I am far from an expert on the
> shallow bits of the protocol. One thing I did notice:
> 
> >  static void receive_shallow_info(struct fetch_pack_args *args,
> > -				 struct packet_reader *reader)
> > +				 struct packet_reader *reader,
> > +				 struct shallow_info *si)
> >  {
> > -	int line_received = 0;
> > +	struct oid_array *shallows;
> > +	int unshallow_received = 0;
> > +
> > +	shallows = xcalloc(1, sizeof(*shallows));
> 
> This has to be heap-allocated, since we pass off ownership to "si"
> (sometimes). But in the v0 case, it comes from the transport's
> &data->shallows of a local variable in cmd_fetch_pack(), and we never
> free it. So I think this oid_array ends up getting leaked.

Thanks for the catch.

> Perhaps it's worth passing down the shallows array we get from the
> caller of fetch_pack(). Something like the patch below (I think it is
> never NULL, which means in your patch 1 you can simplify the conditional
> for the BUG).

[snip patch]

You're right that it is never NULL - I have removed that check. As for
passing down the shallows array that we get from the caller of
fetch_pack(), that would get confusing because we end up modifying the
shallows array in some code paths, and the transport is sometimes reused
(for example, when backfilling tags). I have instead made a
shallows_scratch variable in fetch_pack(), and made it pass it down
(like in the diff you provided).

[PATCH v2 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully

[Jonathan Tan]

Updated to remove the unnecessary NULL check and memory leak that Peff
noticed [1].

(Only commit 2/2 is changed, so the range-diff only shows 1 commit.)

[1] https://public-inbox.org/git/20190326052011.GB1933@sigill.intra.peff.net/

Jonathan Tan (2):
  fetch-pack: call prepare_shallow_info only if v0
  fetch-pack: respect --no-update-shallow in v2

 commit.h     |  4 ++++
 fetch-pack.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 10 deletions(-)

Range-diff against v1:
1:  c4d2f409e2 ! 1:  943b1cbc61 fetch-pack: respect --no-update-shallow in v2
    @@ -33,13 +33,11 @@
      static void receive_shallow_info(struct fetch_pack_args *args,
     -				 struct packet_reader *reader)
     +				 struct packet_reader *reader,
    ++				 struct oid_array *shallows,
     +				 struct shallow_info *si)
      {
     -	int line_received = 0;
    -+	struct oid_array *shallows;
     +	int unshallow_received = 0;
    -+
    -+	shallows = xcalloc(1, sizeof(*shallows));
      
      	process_section_header(reader, "shallow-info", 0);
      	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
    @@ -77,8 +75,6 @@
     +
     +		for (i = 0; i < shallows->nr; i++)
     +			register_shallow(the_repository, &shallows->oid[i]);
    -+		oid_array_clear(shallows);
    -+		free(shallows);
      		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
      					NULL);
      		args->deepen = 1;
    @@ -95,14 +91,13 @@
     +		else
     +			alternate_shallow_file = NULL;
      	} else {
    -+		free(shallows);
      		alternate_shallow_file = NULL;
      	}
    - }
     @@
      				    int fd[2],
      				    const struct ref *orig_ref,
      				    struct ref **sought, int nr_sought,
    ++				    struct oid_array *shallows,
     +				    struct shallow_info *si,
      				    char **pack_lockfile)
      {
    @@ -112,16 +107,36 @@
      			/* Check for shallow-info section */
      			if (process_section_header(&reader, "shallow-info", 1))
     -				receive_shallow_info(args, &reader);
    -+				receive_shallow_info(args, &reader, si);
    ++				receive_shallow_info(args, &reader, shallows, si);
      
      			if (process_section_header(&reader, "wanted-refs", 1))
      				receive_wanted_refs(&reader, sought, nr_sought);
     @@
    + {
    + 	struct ref *ref_cpy;
    + 	struct shallow_info si;
    ++	struct oid_array shallows_scratch = OID_ARRAY_INIT;
    + 
    + 	fetch_pack_setup();
    + 	if (nr_sought)
    +@@
    + 		die(_("no matching remote head"));
    + 	}
    + 	if (version == protocol_v2) {
    +-		if (shallow && shallow->nr)
    ++		if (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);
    -+					   &si, pack_lockfile);
    ++					   &shallows_scratch, &si,
    + 					   pack_lockfile);
      	} else {
      		prepare_shallow_info(&si, shallow);
    - 		ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
    +@@
    + 	update_shallow(args, sought, nr_sought, &si);
    + cleanup:
    + 	clear_shallow_info(&si);
    ++	oid_array_clear(&shallows_scratch);
    + 	return ref_cpy;
    + }
    + 
-- 
2.21.0.155.ge902e9bcae.dirty

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

[Jonathan Tan]

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

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) {
-- 
2.21.0.155.ge902e9bcae.dirty

[PATCH v2 2/2] fetch-pack: respect --no-update-shallow in v2

[Jonathan Tan]

In protocol v0, when sending "shallow" lines, the server distinguishes
between lines caused by the remote repo being shallow and lines caused
by client-specified depth settings. Unless "--update-shallow" is
specified, there is a difference in behavior: refs that reach the former
"shallow" lines, but not the latter, are rejected. But in v2, the server
does not, and the client treats all "shallow" lines like lines caused by
client-specified depth settings.

Full restoration of v0 functionality is not possible without protocol
change, but we can implement a heuristic: if we specify any depth
setting, treat all "shallow" lines like lines caused by client-specified
depth settings (that is, unaffected by "--no-update-shallow"), but
otherwise, treat them like lines caused by the remote repo being shallow
(that is, affected by "--no-update-shallow"). This restores most of v0
behavior, except in the case where a client fetches from a shallow
repository with depth settings.

This patch causes a test that previously failed with
GIT_TEST_PROTOCOL_VERSION=2 to pass.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index a0eb268dfc..4831eb6a25 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1253,9 +1253,11 @@ static int process_acks(struct fetch_negotiator *negotiator,
 }
 
 static void receive_shallow_info(struct fetch_pack_args *args,
-				 struct packet_reader *reader)
+				 struct packet_reader *reader,
+				 struct oid_array *shallows,
+				 struct shallow_info *si)
 {
-	int line_received = 0;
+	int unshallow_received = 0;
 
 	process_section_header(reader, "shallow-info", 0);
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
@@ -1265,8 +1267,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 		if (skip_prefix(reader->line, "shallow ", &arg)) {
 			if (get_oid_hex(arg, &oid))
 				die(_("invalid shallow line: %s"), reader->line);
-			register_shallow(the_repository, &oid);
-			line_received = 1;
+			oid_array_append(shallows, &oid);
 			continue;
 		}
 		if (skip_prefix(reader->line, "unshallow ", &arg)) {
@@ -1279,7 +1280,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 				die(_("error in object: %s"), reader->line);
 			if (unregister_shallow(&oid))
 				die(_("no shallow found: %s"), reader->line);
-			line_received = 1;
+			unshallow_received = 1;
 			continue;
 		}
 		die(_("expected shallow/unshallow, got %s"), reader->line);
@@ -1289,10 +1290,31 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 	    reader->status != PACKET_READ_DELIM)
 		die(_("error processing shallow info: %d"), reader->status);
 
-	if (line_received) {
+	if (args->deepen || unshallow_received) {
+		/*
+		 * Treat these as shallow lines caused by our depth settings.
+		 * In v0, these lines cannot cause refs to be rejected; do the
+		 * same.
+		 */
+		int i;
+
+		for (i = 0; i < shallows->nr; i++)
+			register_shallow(the_repository, &shallows->oid[i]);
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
 		args->deepen = 1;
+	} else if (shallows->nr) {
+		/*
+		 * Treat these as shallow lines caused by the remote being
+		 * shallow. In v0, remote refs that reach these objects are
+		 * rejected (unless --update-shallow is set); do the same.
+		 */
+		prepare_shallow_info(si, shallows);
+		if (si->nr_ours || si->nr_theirs)
+			alternate_shallow_file =
+				setup_temporary_shallow(si->shallow);
+		else
+			alternate_shallow_file = NULL;
 	} else {
 		alternate_shallow_file = NULL;
 	}
@@ -1337,6 +1359,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				    int fd[2],
 				    const struct ref *orig_ref,
 				    struct ref **sought, int nr_sought,
+				    struct oid_array *shallows,
+				    struct shallow_info *si,
 				    char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
@@ -1411,7 +1435,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		case FETCH_GET_PACK:
 			/* Check for shallow-info section */
 			if (process_section_header(&reader, "shallow-info", 1))
-				receive_shallow_info(args, &reader);
+				receive_shallow_info(args, &reader, shallows, si);
 
 			if (process_section_header(&reader, "wanted-refs", 1))
 				receive_wanted_refs(&reader, sought, nr_sought);
@@ -1625,6 +1649,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 {
 	struct ref *ref_cpy;
 	struct shallow_info si;
+	struct oid_array shallows_scratch = OID_ARRAY_INIT;
 
 	fetch_pack_setup();
 	if (nr_sought)
@@ -1649,10 +1674,11 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 		die(_("no matching remote head"));
 	}
 	if (version == protocol_v2) {
-		if (shallow && shallow->nr)
+		if (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,
+					   &shallows_scratch, &si,
 					   pack_lockfile);
 	} else {
 		prepare_shallow_info(&si, shallow);
@@ -1680,6 +1706,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 	update_shallow(args, sought, nr_sought, &si);
 cleanup:
 	clear_shallow_info(&si);
+	oid_array_clear(&shallows_scratch);
 	return ref_cpy;
 }
 
-- 
2.21.0.155.ge902e9bcae.dirty

Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2

[Jeff King]

On Tue, Mar 26, 2019 at 10:37:06AM -0700, Jonathan Tan wrote:

> > Perhaps it's worth passing down the shallows array we get from the
> > caller of fetch_pack(). Something like the patch below (I think it is
> > never NULL, which means in your patch 1 you can simplify the conditional
> > for the BUG).
> 
> [snip patch]
> 
> You're right that it is never NULL - I have removed that check. As for
> passing down the shallows array that we get from the caller of
> fetch_pack(), that would get confusing because we end up modifying the
> shallows array in some code paths, and the transport is sometimes reused
> (for example, when backfilling tags). I have instead made a
> shallows_scratch variable in fetch_pack(), and made it pass it down
> (like in the diff you provided).

Yeah, I confess to having spent quite a few minutes trying to figure out
the difference between "shallows" and "shallow_info", whether one
wrote into the other, and who was responsible for filling each in. So I
will not complain if you have a way of writing it that is less
confusing. :)

-Peff

Re: [PATCH v2 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully

[Jeff King]

On Tue, Mar 26, 2019 at 10:53:24AM -0700, Jonathan Tan wrote:

> Updated to remove the unnecessary NULL check and memory leak that Peff
> noticed [1].
> 
> (Only commit 2/2 is changed, so the range-diff only shows 1 commit.)

But isn't this line:

>     + 	if (version == protocol_v2) {
>     +-		if (shallow && shallow->nr)
>     ++		if (shallow->nr)
>       			BUG("Protocol V2 does not provide shallows at this point in the fetch");

added by patch 1? It's added with "shallow &&" in patch 1, and then
modified in patch 2.

I think the "it's never NULL" property is true even before this series,
right?

-Peff

Re: [PATCH v2 2/2] fetch-pack: respect --no-update-shallow in v2

[Jeff King]

On Tue, Mar 26, 2019 at 10:53:26AM -0700, Jonathan Tan wrote:

> @@ -1625,6 +1649,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  {
>  	struct ref *ref_cpy;
>  	struct shallow_info si;
> +	struct oid_array shallows_scratch = OID_ARRAY_INIT;
>  
>  	fetch_pack_setup();
>  	if (nr_sought)
> @@ -1649,10 +1674,11 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  		die(_("no matching remote head"));
>  	}
>  	if (version == protocol_v2) {
> -		if (shallow && shallow->nr)
> +		if (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,
> +					   &shallows_scratch, &si,
>  					   pack_lockfile);
>  	} else {
>  		prepare_shallow_info(&si, shallow);
> @@ -1680,6 +1706,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  	update_shallow(args, sought, nr_sought, &si);
>  cleanup:
>  	clear_shallow_info(&si);
> +	oid_array_clear(&shallows_scratch);
>  	return ref_cpy;
>  }

Yeah, this is much clearer than what I posted earlier. Thanks for
thinking it through.

The patches look good to me, modulo the timing of the NULL thing I
mentioned elsewhere.

-Peff

[PATCH v3 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully

[Jonathan Tan]

Peff says in [1]:

> But isn't this line:
> 
> >     + 	if (version == protocol_v2) {
> >     +-		if (shallow && shallow->nr)
> >     ++		if (shallow->nr)
> >       			BUG("Protocol V2 does not provide shallows at this point in the fetch");
> 
> added by patch 1? It's added with "shallow &&" in patch 1, and then
> modified in patch 2.
> 
> I think the "it's never NULL" property is true even before this series,
> right?

Ah...yes you're right. I've updated it here.

Thanks for your review.

[1] https://public-inbox.org/git/20190326182047.GB24105@sigill.intra.peff.net/

Jonathan Tan (2):
  fetch-pack: call prepare_shallow_info only if v0
  fetch-pack: respect --no-update-shallow in v2

 commit.h     |  4 ++++
 fetch-pack.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 10 deletions(-)

Range-diff against v2:
1:  d2eb101709 ! 1:  64f44a18ad fetch-pack: call prepare_shallow_info only if v0
    @@ -38,7 +38,7 @@
     -	prepare_shallow_info(&si, shallow);
     -	if (version == protocol_v2)
     +	if (version == protocol_v2) {
    -+		if (shallow && shallow->nr)
    ++		if (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,
2:  943b1cbc61 ! 2:  3f65698610 fetch-pack: respect --no-update-shallow in v2
    @@ -120,11 +120,6 @@
      	fetch_pack_setup();
      	if (nr_sought)
     @@
    - 		die(_("no matching remote head"));
    - 	}
    - 	if (version == protocol_v2) {
    --		if (shallow && shallow->nr)
    -+		if (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,
-- 
2.21.0.155.ge902e9bcae.dirty

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

[Jonathan Tan]

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

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..ebb24ada24 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->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) {
-- 
2.21.0.155.ge902e9bcae.dirty

[PATCH v3 2/2] fetch-pack: respect --no-update-shallow in v2

[Jonathan Tan]

In protocol v0, when sending "shallow" lines, the server distinguishes
between lines caused by the remote repo being shallow and lines caused
by client-specified depth settings. Unless "--update-shallow" is
specified, there is a difference in behavior: refs that reach the former
"shallow" lines, but not the latter, are rejected. But in v2, the server
does not, and the client treats all "shallow" lines like lines caused by
client-specified depth settings.

Full restoration of v0 functionality is not possible without protocol
change, but we can implement a heuristic: if we specify any depth
setting, treat all "shallow" lines like lines caused by client-specified
depth settings (that is, unaffected by "--no-update-shallow"), but
otherwise, treat them like lines caused by the remote repo being shallow
(that is, affected by "--no-update-shallow"). This restores most of v0
behavior, except in the case where a client fetches from a shallow
repository with depth settings.

This patch causes a test that previously failed with
GIT_TEST_PROTOCOL_VERSION=2 to pass.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index ebb24ada24..4831eb6a25 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1253,9 +1253,11 @@ static int process_acks(struct fetch_negotiator *negotiator,
 }
 
 static void receive_shallow_info(struct fetch_pack_args *args,
-				 struct packet_reader *reader)
+				 struct packet_reader *reader,
+				 struct oid_array *shallows,
+				 struct shallow_info *si)
 {
-	int line_received = 0;
+	int unshallow_received = 0;
 
 	process_section_header(reader, "shallow-info", 0);
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
@@ -1265,8 +1267,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 		if (skip_prefix(reader->line, "shallow ", &arg)) {
 			if (get_oid_hex(arg, &oid))
 				die(_("invalid shallow line: %s"), reader->line);
-			register_shallow(the_repository, &oid);
-			line_received = 1;
+			oid_array_append(shallows, &oid);
 			continue;
 		}
 		if (skip_prefix(reader->line, "unshallow ", &arg)) {
@@ -1279,7 +1280,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 				die(_("error in object: %s"), reader->line);
 			if (unregister_shallow(&oid))
 				die(_("no shallow found: %s"), reader->line);
-			line_received = 1;
+			unshallow_received = 1;
 			continue;
 		}
 		die(_("expected shallow/unshallow, got %s"), reader->line);
@@ -1289,10 +1290,31 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 	    reader->status != PACKET_READ_DELIM)
 		die(_("error processing shallow info: %d"), reader->status);
 
-	if (line_received) {
+	if (args->deepen || unshallow_received) {
+		/*
+		 * Treat these as shallow lines caused by our depth settings.
+		 * In v0, these lines cannot cause refs to be rejected; do the
+		 * same.
+		 */
+		int i;
+
+		for (i = 0; i < shallows->nr; i++)
+			register_shallow(the_repository, &shallows->oid[i]);
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
 		args->deepen = 1;
+	} else if (shallows->nr) {
+		/*
+		 * Treat these as shallow lines caused by the remote being
+		 * shallow. In v0, remote refs that reach these objects are
+		 * rejected (unless --update-shallow is set); do the same.
+		 */
+		prepare_shallow_info(si, shallows);
+		if (si->nr_ours || si->nr_theirs)
+			alternate_shallow_file =
+				setup_temporary_shallow(si->shallow);
+		else
+			alternate_shallow_file = NULL;
 	} else {
 		alternate_shallow_file = NULL;
 	}
@@ -1337,6 +1359,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				    int fd[2],
 				    const struct ref *orig_ref,
 				    struct ref **sought, int nr_sought,
+				    struct oid_array *shallows,
+				    struct shallow_info *si,
 				    char **pack_lockfile)
 {
 	struct ref *ref = copy_ref_list(orig_ref);
@@ -1411,7 +1435,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		case FETCH_GET_PACK:
 			/* Check for shallow-info section */
 			if (process_section_header(&reader, "shallow-info", 1))
-				receive_shallow_info(args, &reader);
+				receive_shallow_info(args, &reader, shallows, si);
 
 			if (process_section_header(&reader, "wanted-refs", 1))
 				receive_wanted_refs(&reader, sought, nr_sought);
@@ -1625,6 +1649,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 {
 	struct ref *ref_cpy;
 	struct shallow_info si;
+	struct oid_array shallows_scratch = OID_ARRAY_INIT;
 
 	fetch_pack_setup();
 	if (nr_sought)
@@ -1653,6 +1678,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 			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,
+					   &shallows_scratch, &si,
 					   pack_lockfile);
 	} else {
 		prepare_shallow_info(&si, shallow);
@@ -1680,6 +1706,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 	update_shallow(args, sought, nr_sought, &si);
 cleanup:
 	clear_shallow_info(&si);
+	oid_array_clear(&shallows_scratch);
 	return ref_cpy;
 }
 
-- 
2.21.0.155.ge902e9bcae.dirty