Avoiding errors when partial cloning a tagged blob

When cloning a repository with a tagged blob (like the Git repository) with --filter=blob:none, the following message appears:

error: missing object referenced by 'refs/tags/junio-gpg-pub'

and the resulting repository also fails fsck.

Patch 1 fixes the protocol documentation and the server side of Git, and patch 2 makes clone error out when such a situation occurs.

An argument could be made that we should not merge patch 2 just yet due to the fact that some server implementations (such as Git and JGit) still exhibit the old behavior, and the resulting clones (albeit failing fsck) are still usable, because when attempting to load the blob, Git will automatically fetch it. I'm on the fence about this, and have included patch 2 in this patch set nevertheless for completeness.

Jonathan Tan (2):
upload-pack: send refs' objects despite "filter"
clone: check connectivity even if clone is partial

 Documentation/technical/pack-protocol.txt |  4 +-
 builtin/clone.c                           |  2 +-
 list-objects.c                            |  6 +--
 object.h                                  |  2 +-
 revision.c                                |  1 +
 revision.h                                |  3 +-
 t/t5317-pack-objects-filter-objects.sh    | 16 ++++++
 t/t5616-partial-clone.sh                  | 64 +++++++++++++++++++++++
 8 files changed, 91 insertions(+), 7 deletions(-)

2.18.0.203.gfac676dfb9-goog

A filter line in a request to upload-pack filters out objects regardless of whether they are directly referenced by a "want" line or not. This means that cloning with "--filter=blob:none" (or another filter that excludes blobs) from a repository with at least one ref pointing to a blob (for example, the Git repository itself) results in output like the following:

error: missing object referenced by 'refs/tags/junio-gpg-pub'

and if that particular blob is not referenced by a fetched tree, the resulting clone fails fsck because there is no object from the remote to vouch that the missing object is a promisor object.

Update both the protocol and the upload-pack implementation to include all explicitly specified "want" objects in the packfile regardless of the filter specification.

---
 Documentation/technical/pack-protocol.txt |  4 +++-
 list-objects.c                            |  6 +++---
 object.h                                  |  2 +-
 revision.c                                |  1 +
 revision.h                                |  3 ++-
 t/t5317-pack-objects-filter-objects.sh    | 16 ++++++++++++++++
 t/t5616-partial-clone.sh                  | 16 ++++++++++++++++
 7 files changed, 42 insertions(+), 6 deletions(-)

index 7fee6b780a..508a344cf1 100644
@@ -284,7 +284,9 @@ information is sent back to the client in the next step.
 The client can optionally request that pack-objects omit various
 objects from the packfile using one of several filtering techniques.
 These are intended for use with partial clone and partial fetch
-operations.  See `rev-list` for possible "filter-spec" values.
+operations. An object that does not meet a filter-spec value is
+omitted unless explicitly requested in a 'want' line. See `rev-list`
+for possible filter-spec values.
 
 Once all the 'want's and 'shallow's (and optional 'deepen') are
 transferred, clients MUST send a flush-pkt, to tell the server side

index 3eec510357..be4118889a 100644
@@ -47,7 +47,7 @@ static void process_blob(struct rev_info *revs,
 
 	pathlen = path->len;
 	strbuf_addstr(path, name);
-	if (filter_fn)
+	if (!(obj->flags & USER_GIVEN) && filter_fn)
 		r = filter_fn(LOFS_BLOB, obj,
 			      path->buf, &path->buf[pathlen],
 			      filter_data);

@@ -132,7 +132,7 @@ static void process_tree(struct rev_info *revs,
 	}
 
 	strbuf_addstr(base, name);
-	if (filter_fn)
+	if (!(obj->flags & USER_GIVEN) && filter_fn)
 		r = filter_fn(LOFS_BEGIN_TREE, obj,
 			      base->buf, &base->buf[baselen],
 			      filter_data);

@@ -171,7 +171,7 @@ static void process_tree(struct rev_info *revs,
 				     cb_data, filter_fn, filter_data);
 	}
 
-	if (filter_fn) {
+	if (!(obj->flags & USER_GIVEN) && filter_fn) {
 		r = filter_fn(LOFS_END_TREE, obj,
 			      base->buf, &base->buf[baselen],
 			      filter_data);

index 5c13955000..cf8da6ba83 100644
@@ -27,7 +27,7 @@ struct object_array {
 
 /*
  * object flag allocation:
- * revision.h:               0---------10                                26
+ * revision.h:               0---------10                              2526
  * fetch-pack.c:             0----5
  * walker.c:                 0-2
  * upload-pack.c:                4       11----------------19

index 40fd91ff2b..1b37da988d 100644
@@ -172,6 +172,7 @@ static void add_pending_object_with_path(struct rev_info *revs,
 		strbuf_release(&buf);
 		return; /* do not add the commit itself */
 	}
+	obj->flags |= USER_GIVEN;
 	add_object_array_with_path(obj, name, &revs->pending, mode, path);
 }
 

index b8c47b98e2..dc8f96366e 100644
@@ -19,8 +19,9 @@
 #define SYMMETRIC_LEFT	(1u<<8)
 #define PATCHSAME	(1u<<9)
 #define BOTTOM		(1u<<10)
+#define USER_GIVEN	(1u<<25) /* given directly by the user */
 #define TRACK_LINEAR	(1u<<26)
-#define ALL_REV_FLAGS	(((1u<<11)-1) | TRACK_LINEAR)
+#define ALL_REV_FLAGS	(((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)
 
 #define DECORATE_SHORT_REFS	1
 #define DECORATE_FULL_REFS	2

index 1b0acc383b..6710c8bc8c 100755
@@ -160,6 +160,22 @@ test_expect_success 'verify blob:limit=1k' '
 	test_cmp observed expected
 '
 
+test_expect_success 'verify explicitly specifying oversized blob in input' '
+	git -C r2 ls-files -s large.1000 large.10000 \
+		| awk -f print_2.awk \
+		| sort >expected &&
+	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	HEAD
+	$(git -C r2 rev-parse HEAD:large.10000)
+	EOF
+	git -C r2 index-pack ../filter.pack &&
+	git -C r2 verify-pack -v ../filter.pack \
+		| grep blob \
+		| awk -f print_1.awk \
+		| sort >observed &&
+	test_cmp observed expected
+'
+
 test_expect_success 'verify blob:limit=1m' '
 	git -C r2 ls-files -s large.1000 large.10000 \
 		| awk -f print_2.awk \

index cee5565367..8a2bf86491 100755
@@ -154,4 +154,20 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack -
 	grep "git index-pack.*--fsck-objects" trace
 '
 
+test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
+	rm -rf src dst &&
+	git init src &&
+	test_commit -C src x &&
+	test_config -C src uploadpack.allowfilter 1 &&
+	test_config -C src uploadpack.allowanysha1inwant 1 &&
+
+	# Create a tag pointing to a blob.
+	BLOB=$(echo blob-contents | git -C src hash-object --stdin -w) &&
+	git -C src tag myblob "$BLOB" &&
+
+	git clone --filter="blob:none" "file://$(pwd)/src" dst 2>err &&
+	! grep "does not point to a valid object" err &&
+	git -C dst fsck
+'
+
 test_done

2.18.0.203.gfac676dfb9-goog

The commit that introduced the partial clone feature - 548719fbdc ("clone: partial clone", 2017-12-08) - excluded connectivity checks for partial clones, but this also meant that it is possible for a clone to succeed, yet not have all objects either present or promised. Specifically, if cloning with --filter=blob:none from a repository that has a tag pointing to a blob, and the blob is not sent in the packfile, the clone will pass, even if the blob is not referenced by any tree in the packfile.

Turn on connectivity checks for partial clone.

---
 builtin/clone.c          |  2 +-
 t/t5616-partial-clone.sh | 48 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

index 8f86d99c51..fa53550758 100644
@@ -1201,7 +1201,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
 			   branch_top.buf, reflog_msg.buf, transport,
-			   !is_local && !filter_options.choice);
+			   !is_local);
 
 	update_head(our_head_points_at, remote_head, reflog_msg.buf);
 

2.18.0.203.gfac676dfb9-goog

Forgot to mention: this patch set is based on bw/ref-in-want because I needed its one-time-sed functionality.

Hmph, the approach taken by these two patches smells bad.

When a blob is deliberately omitted with --fitler=blob:none, the fsck that encounters an entry in a tree object that is about that expected-to-be-and-actually-is-missing blob does *not* complain and that is by design, right? Naïvely, I may expect fsck to follow the same principle--when it encounters a reference to an object that is deliberately missing, refrain from complaining, regardless of the place the offending reference was found, be it inside a tree object or a ref.

Perhaps that line of consistent behaviour may be impossible due to the way the reference is stored inside a tree object and a ref? E.g. a reference to an expected-to-be-missing blob still lets us know that the missing object is expected to be a blob, but a ref only stores the object name and not its type, so we can tell it is missing, but we cannot say it is OK to be missing because we expect it to be a blob, or something?

The difference between the two is that this tree object is from a packfile with an associated ".promisor" file, indicating that the destinations of any "dangling" links are promisor objects (and thus, fsck and others will not complain about those). We don't have such a mechanism for refs.

This was a tradeoff between the desire to still detect repository corruption whenever we can, and to not impact performance greatly when operating on a partial repository.

Besides the solution in this patch set, we could (1) allow any ref to point to a missing object, (2) allow a subset of refs to point to a missing object, or (3) have another place in $GIT_DIR that can record the hashes of promisor objects. (1) seems contrary to the repository corruption detection goal that we have. (2) is plausible, but we would have to figure out what to record - it's fine to record the RHS of the default refspec when we clone, but I don't know what we should record when the user subsequently runs "git fetch origin refs/heads/master:refs/heads/master". As for (3), that would require another repository extension since we wouldn't want an old Git to fetch into a new Git repository without updating the promisor-hashes file - but at this point, it seems easier to update the protocol instead. Any comments are welcome, and I'll think about this some more.

Hi,

[...]

Yes, exactly. I believe this is why we did not treat refs as promisors when we introduced promised objects.

We could revisit that decision and make refs into promisors, storing any extra metadata on the side. That would be a more invasive change than this series does, though. In the model used today, where refs are not promisors, this series is the logical thing to do.

The main downside of saying that filters should not be applied to the objects named directly in a "want" is that such objects could be large.

The upsides are that

- it makes the protocol more consistent with the current invariants maintained by "git fsck"

- down the line, it should make operations like "fetch just this one tree" a little simpler, since you can use

filter blob:none
filter tree:none
want <this tree>

Later we could introduce a separate kind of "want" for which filters apply to the object named directly, too, to provide more flexibility for the client. I can imagine cases where a person would want each of those two behaviors, so I don't think we are incurring unnecessary complexity by always sending the object named in a "want" without such an explicit filtering request. So I'm comfortable with the direction this series goes in, though I haven't looked at the patches in detail.

Thanks,
Jonathan

;-)

I think this example, especially without the first line, would have a practical use, i.e. "grab the contents of this directory without recursing", and as such is a very convincing argument to support the approach taken by the solution presented here.

Likewise. Thanks.

I think that refs in files-backend is a bit hard to update to annotate with extra information like "is this merely promising or a tip of the connectivity graph?" and agree that we should not make refs promisors.

Such an otherwise unreferenced blob could be made into a promised object by the promisor repository, though, I think. It can include a synthetic tree that points at such a blob in the pack stream data if it knows that the resulting pack will be marked with the .promisor bit, and by doing so, there is no need to update the client side, no?

OK, we agree on this :-)

Yes, that is possible, but I think the client side still needs to be updated, for the same reason as in my implementation (the client should check that the server sent all necessary objects - the blob itself in my case, and the synthetic tree in yours).

I think that if we were planning to introduce a synthetic tree, though, it would be better just to include the object itself (as in my implementation). In both solutions, both the protocol and the server need to be updated, but existing clients will still work (minus the checking that would still be desirable with both solutions).

I think the latter is probably a good thing to nudge the server implementations in the right direction by gently poking them ;-)

The patches textually apply cleanly on 'master' but apparently it needs some fixes in jt/partial-clone-fsck-connectivity topic to function correctly?

OK, I'll keep patch 2 then.

I forgot to mention in the original e-mail that this is on bw/ref-in-want. (I mentioned it in an e-mail I sent later [1].) I've checked and basing it on both bw/ref-in-want and jt/partial-clone-fsck-connectivity (which is based on bw/ref-in-want itself) both work, so you can choose either one.

[1] https://public-inbox.org/git/[email protected]/