{"thread":{"id":"48833","subject":"[PATCH 1/2] upload-pack: send refs' objects despite \"filter\"","startedAt":"2018-07-06T19:34:22Z","lastAt":"2018-07-09T20:07:56Z","messageCount":12,"participants":["Jonathan Tan","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"351796","messageId":"e122c58f6febf265f9eadbfbac34a2f5729dffe2.1530905323.git.jonathantanmy@google.com","threadId":"48833","inReplyTo":"cover.1530905323.git.jonathantanmy@google.com","subject":"[PATCH 1/2] upload-pack: send refs' objects despite \"filter\"","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-06T19:34:09Z","receivedAt":"2018-07-06T19:34:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A filter line in a request to upload-pack filters out objects regardless\nof whether they are directly referenced by a \"want\" line or not. This\nmeans that cloning with \"--filter=blob:none\" (or another filter that\nexcludes blobs) from a repository with at least one ref pointing to a\nblob (for example, the Git repository itself) results in output like the\nfollowing:\n\n error: missing object referenced by 'refs/tags/junio-gpg-pub'\n\nand if that particular blob is not referenced by a fetched tree, the\nresulting clone fails fsck because there is no object from the remote to\nvouch that the missing object is a promisor object.\n\nUpdate both the protocol and the upload-pack implementation to include\nall explicitly specified \"want\" objects in the packfile regardless of\nthe filter specification.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/pack-protocol.txt | 4 +++-\n list-objects.c | 6 +++---\n object.h | 2 +-\n revision.c | 1 +\n revision.h | 3 ++-\n t/t5317-pack-objects-filter-objects.sh | 16 ++++++++++++++++\n t/t5616-partial-clone.sh | 16 ++++++++++++++++\n 7 files changed, 42 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 7fee6b780a..508a344cf1 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -284,7 +284,9 @@ information is sent back to the client in the next step.\n The client can optionally request that pack-objects omit various\n objects from the packfile using one of several filtering techniques.\n These are intended for use with partial clone and partial fetch\n-operations. See `rev-list` for possible \"filter-spec\" values.\n+operations. An object that does not meet a filter-spec value is\n+omitted unless explicitly requested in a 'want' line. See `rev-list`\n+for possible filter-spec values.\n \n Once all the 'want's and 'shallow's (and optional 'deepen') are\n transferred, clients MUST send a flush-pkt, to tell the server side\ndiff --git a/list-objects.c b/list-objects.c\nindex 3eec510357..be4118889a 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -47,7 +47,7 @@ static void process_blob(struct rev_info *revs,\n \n \tpathlen = path->len;\n \tstrbuf_addstr(path, name);\n-\tif (filter_fn)\n+\tif (!(obj->flags & USER_GIVEN) && filter_fn)\n \t\tr = filter_fn(LOFS_BLOB, obj,\n \t\t\t path->buf, &path->buf[pathlen],\n \t\t\t filter_data);\n@@ -132,7 +132,7 @@ static void process_tree(struct rev_info *revs,\n \t}\n \n \tstrbuf_addstr(base, name);\n-\tif (filter_fn)\n+\tif (!(obj->flags & USER_GIVEN) && filter_fn)\n \t\tr = filter_fn(LOFS_BEGIN_TREE, obj,\n \t\t\t base->buf, &base->buf[baselen],\n \t\t\t filter_data);\n@@ -171,7 +171,7 @@ static void process_tree(struct rev_info *revs,\n \t\t\t\t cb_data, filter_fn, filter_data);\n \t}\n \n-\tif (filter_fn) {\n+\tif (!(obj->flags & USER_GIVEN) && filter_fn) {\n \t\tr = filter_fn(LOFS_END_TREE, obj,\n \t\t\t base->buf, &base->buf[baselen],\n \t\t\t filter_data);\ndiff --git a/object.h b/object.h\nindex 5c13955000..cf8da6ba83 100644\n--- a/object.h\n+++ b/object.h\n@@ -27,7 +27,7 @@ struct object_array {\n \n /*\n * object flag allocation:\n- * revision.h: 0---------10 26\n+ * revision.h: 0---------10 2526\n * fetch-pack.c: 0----5\n * walker.c: 0-2\n * upload-pack.c: 4 11----------------19\ndiff --git a/revision.c b/revision.c\nindex 40fd91ff2b..1b37da988d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -172,6 +172,7 @@ static void add_pending_object_with_path(struct rev_info *revs,\n \t\tstrbuf_release(&buf);\n \t\treturn; /* do not add the commit itself */\n \t}\n+\tobj->flags |= USER_GIVEN;\n \tadd_object_array_with_path(obj, name, &revs->pending, mode, path);\n }\n \ndiff --git a/revision.h b/revision.h\nindex b8c47b98e2..dc8f96366e 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -19,8 +19,9 @@\n #define SYMMETRIC_LEFT\t(1u<<8)\n #define PATCHSAME\t(1u<<9)\n #define BOTTOM\t\t(1u<<10)\n+#define USER_GIVEN\t(1u<<25) /* given directly by the user */\n #define TRACK_LINEAR\t(1u<<26)\n-#define ALL_REV_FLAGS\t(((1u<<11)-1) | TRACK_LINEAR)\n+#define ALL_REV_FLAGS\t(((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)\n \n #define DECORATE_SHORT_REFS\t1\n #define DECORATE_FULL_REFS\t2\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 1b0acc383b..6710c8bc8c 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -160,6 +160,22 @@ test_expect_success 'verify blob:limit=1k' '\n \ttest_cmp observed expected\n '\n \n+test_expect_success 'verify explicitly specifying oversized blob in input' '\n+\tgit -C r2 ls-files -s large.1000 large.10000 \\\n+\t\t| awk -f print_2.awk \\\n+\t\t| sort >expected &&\n+\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n+\tHEAD\n+\t$(git -C r2 rev-parse HEAD:large.10000)\n+\tEOF\n+\tgit -C r2 index-pack ../filter.pack &&\n+\tgit -C r2 verify-pack -v ../filter.pack \\\n+\t\t| grep blob \\\n+\t\t| awk -f print_1.awk \\\n+\t\t| sort >observed &&\n+\ttest_cmp observed expected\n+'\n+\n test_expect_success 'verify blob:limit=1m' '\n \tgit -C r2 ls-files -s large.1000 large.10000 \\\n \t\t| awk -f print_2.awk \\\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex cee5565367..8a2bf86491 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -154,4 +154,20 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack -\n \tgrep \"git index-pack.*--fsck-objects\" trace\n '\n \n+test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '\n+\trm -rf src dst &&\n+\tgit init src &&\n+\ttest_commit -C src x &&\n+\ttest_config -C src uploadpack.allowfilter 1 &&\n+\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n+\n+\t# Create a tag pointing to a blob.\n+\tBLOB=$(echo blob-contents | git -C src hash-object --stdin -w) &&\n+\tgit -C src tag myblob \"$BLOB\" &&\n+\n+\tgit clone --filter=\"blob:none\" \"file://$(pwd)/src\" dst 2>err &&\n+\t! grep \"does not point to a valid object\" err &&\n+\tgit -C dst fsck\n+'\n+\n test_done\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"351797","messageId":"09abde498b73c49fbf71c0dbb7680d23bd63e3b2.1530905323.git.jonathantanmy@google.com","threadId":"48833","inReplyTo":"cover.1530905323.git.jonathantanmy@google.com","subject":"[PATCH 2/2] clone: check connectivity even if clone is partial","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-06T19:34:10Z","receivedAt":"2018-07-06T19:34:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The commit that introduced the partial clone feature - 548719fbdc\n(\"clone: partial clone\", 2017-12-08) - excluded connectivity checks\nfor partial clones, but this also meant that it is possible for a clone\nto succeed, yet not have all objects either present or promised.\nSpecifically, if cloning with --filter=blob:none from a repository that\nhas a tag pointing to a blob, and the blob is not sent in the packfile,\nthe clone will pass, even if the blob is not referenced by any tree in\nthe packfile.\n\nTurn on connectivity checks for partial clone.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/clone.c | 2 +-\n t/t5616-partial-clone.sh | 48 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 8f86d99c51..fa53550758 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1201,7 +1201,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n \t\t\t branch_top.buf, reflog_msg.buf, transport,\n-\t\t\t !is_local && !filter_options.choice);\n+\t\t\t !is_local);\n \n \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n \ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 8a2bf86491..44d8e80171 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -170,4 +170,52 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if norm\n \tgit -C dst fsck\n '\n \n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+# Converts bytes into a form suitable for inclusion in a sed command. For\n+# example, \"printf 'ab\\r\\n' | hex_unpack\" results in '\\x61\\x62\\x0d\\x0a'.\n+sed_escape () {\n+\tperl -e '$/ = undef; $input = <>; print unpack(\"H2\" x length($input), $input)' |\n+\t\tsed 's/\\(..\\)/\\\\x\\1/g'\n+}\n+\n+test_expect_success 'upon cloning, check that all refs point to objects' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\trm -rf \"$SERVER\" repo &&\n+\ttest_create_repo \"$SERVER\" &&\n+\ttest_commit -C \"$SERVER\" foo &&\n+\ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n+\ttest_config -C \"$SERVER\" uploadpack.allowanysha1inwant 1 &&\n+\n+\t# Create a tag pointing to a blob.\n+\tBLOB=$(echo blob-contents | git -C \"$SERVER\" hash-object --stdin -w) &&\n+\tgit -C \"$SERVER\" tag myblob \"$BLOB\" &&\n+\n+\t# Craft a packfile not including that blob.\n+\tgit -C \"$SERVER\" rev-parse HEAD |\n+\t\tgit -C \"$SERVER\" pack-objects --stdout >incomplete.pack &&\n+\n+\t# Replace the existing packfile with the crafted one. The protocol\n+\t# requires that the packfile be sent in sideband 1, hence the extra\n+\t# \\x01 byte at the beginning.\n+\tprintf \"1,/packfile/!c %04x\\\\\\\\x01%s0000\" \\\n+\t\t\"$(($(wc -c \"$HTTPD_ROOT_PATH/one-time-sed\" &&\n+\n+\t# Use protocol v2 because the sed command looks for the \"packfile\"\n+\t# section header.\n+\ttest_config -C \"$SERVER\" protocol.version 2 &&\n+\ttest_must_fail git -c protocol.version=2 clone \\\n+\t\t--filter=blob:none $HTTPD_URL/one_time_sed/server repo 2>err &&\n+\n+\tgrep \"did not send all necessary objects\" err &&\n+\n+\t# Ensure that the one-time-sed script was used.\n+\t! test -e \"$HTTPD_ROOT_PATH/one-time-sed\"\n+'\n+\n+stop_httpd\n+\n test_done\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"351798","messageId":"cover.1530905323.git.jonathantanmy@google.com","threadId":"48833","inReplyTo":null,"subject":"[PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-06T19:34:08Z","receivedAt":"2018-07-06T19:37:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cloning a repository with a tagged blob (like the Git repository)\nwith --filter=blob:none, the following message appears:\n\n error: missing object referenced by 'refs/tags/junio-gpg-pub'\n\nand the resulting repository also fails fsck.\n\nPatch 1 fixes the protocol documentation and the server side of Git, and\npatch 2 makes clone error out when such a situation occurs.\n\nAn argument could be made that we should not merge patch 2 just yet due\nto the fact that some server implementations (such as Git and JGit)\nstill exhibit the old behavior, and the resulting clones (albeit failing\nfsck) are still usable, because when attempting to load the blob, Git\nwill automatically fetch it. I'm on the fence about this, and have\nincluded patch 2 in this patch set nevertheless for completeness.\n\nJonathan Tan (2):\n upload-pack: send refs' objects despite \"filter\"\n clone: check connectivity even if clone is partial\n\n Documentation/technical/pack-protocol.txt | 4 +-\n builtin/clone.c | 2 +-\n list-objects.c | 6 +--\n object.h | 2 +-\n revision.c | 1 +\n revision.h | 3 +-\n t/t5317-pack-objects-filter-objects.sh | 16 ++++++\n t/t5616-partial-clone.sh | 64 +++++++++++++++++++++++\n 8 files changed, 91 insertions(+), 7 deletions(-)\n\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"351799","messageId":"20180706193847.160161-1-jonathantanmy@google.com","threadId":"48833","inReplyTo":"cover.1530905323.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-06T19:38:47Z","receivedAt":"2018-07-06T19:38:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> When cloning a repository with a tagged blob (like the Git repository)\n> with --filter=blob:none, the following message appears:\n> \n> error: missing object referenced by 'refs/tags/junio-gpg-pub'\n> \n> and the resulting repository also fails fsck.\n> \n> Patch 1 fixes the protocol documentation and the server side of Git, and\n> patch 2 makes clone error out when such a situation occurs.\n> \n> An argument could be made that we should not merge patch 2 just yet due\n> to the fact that some server implementations (such as Git and JGit)\n> still exhibit the old behavior, and the resulting clones (albeit failing\n> fsck) are still usable, because when attempting to load the blob, Git\n> will automatically fetch it. I'm on the fence about this, and have\n> included patch 2 in this patch set nevertheless for completeness.\n> \n> Jonathan Tan (2):\n> upload-pack: send refs' objects despite \"filter\"\n> clone: check connectivity even if clone is partial\n\nForgot to mention: this patch set is based on bw/ref-in-want because I\nneeded its one-time-sed functionality.\n"},{"id":"351803","messageId":"xmqqk1q8jfak.fsf@gitster-ct.c.googlers.com","threadId":"48833","inReplyTo":"20180706193847.160161-1-jonathantanmy@google.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T20:10:59Z","receivedAt":"2018-07-06T20:11:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> When cloning a repository with a tagged blob (like the Git repository)\n>> with --filter=blob:none, the following message appears:\n>> \n>> error: missing object referenced by 'refs/tags/junio-gpg-pub'\n>> \n>> and the resulting repository also fails fsck.\n\nHmph, the approach taken by these two patches smells bad.\n\nWhen a blob is deliberately omitted with --fitler=blob:none, the\nfsck that encounters an entry in a tree object that is about that\nexpected-to-be-and-actually-is-missing blob does *not* complain and\nthat is by design, right? Naïvely, I may expect fsck to follow the\nsame principle--when it encounters a reference to an object that is\ndeliberately missing, refrain from complaining, regardless of the\nplace the offending reference was found, be it inside a tree object\nor a ref.\n\nPerhaps that line of consistent behaviour may be impossible due to\nthe way the reference is stored inside a tree object and a ref?\nE.g. a reference to an expected-to-be-missing blob still lets us\nknow that the missing object is expected to be a blob, but a ref\nonly stores the object name and not its type, so we can tell it is\nmissing, but we cannot say it is OK to be missing because we expect\nit to be a blob, or something?\n"},{"id":"351816","messageId":"20180706212546.178115-1-jonathantanmy@google.com","threadId":"48833","inReplyTo":"xmqqk1q8jfak.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-06T21:25:46Z","receivedAt":"2018-07-06T21:25:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Hmph, the approach taken by these two patches smells bad.\n> \n> When a blob is deliberately omitted with --fitler=blob:none, the\n> fsck that encounters an entry in a tree object that is about that\n> expected-to-be-and-actually-is-missing blob does *not* complain and\n> that is by design, right? Naïvely, I may expect fsck to follow the\n> same principle--when it encounters a reference to an object that is\n> deliberately missing, refrain from complaining, regardless of the\n> place the offending reference was found, be it inside a tree object\n> or a ref.\n\nThe difference between the two is that this tree object is from a\npackfile with an associated \".promisor\" file, indicating that the\ndestinations of any \"dangling\" links are promisor objects (and thus,\nfsck and others will not complain about those). We don't have such a\nmechanism for refs.\n\nThis was a tradeoff between the desire to still detect repository\ncorruption whenever we can, and to not impact performance greatly when\noperating on a partial repository.\n\nBesides the solution in this patch set, we could (1) allow any ref to\npoint to a missing object, (2) allow a subset of refs to point to a\nmissing object, or (3) have another place in $GIT_DIR that can record\nthe hashes of promisor objects. (1) seems contrary to the repository\ncorruption detection goal that we have. (2) is plausible, but we would\nhave to figure out what to record - it's fine to record the RHS of the\ndefault refspec when we clone, but I don't know what we should record\nwhen the user subsequently runs \"git fetch origin\nrefs/heads/master:refs/heads/master\". As for (3), that would require\nanother repository extension since we wouldn't want an old Git to fetch\ninto a new Git repository without updating the promisor-hashes file -\nbut at this point, it seems easier to update the protocol instead. Any\ncomments are welcome, and I'll think about this some more.\n"},{"id":"351818","messageId":"20180706212957.GD6195@aiede.svl.corp.google.com","threadId":"48833","inReplyTo":"xmqqk1q8jfak.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-06T21:29:57Z","receivedAt":"2018-07-06T21:30:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n>> Jonathan Tan writes:\n\n>>> When cloning a repository with a tagged blob (like the Git repository)\n>>> with --filter=blob:none, the following message appears:\n>>>\n>>> error: missing object referenced by 'refs/tags/junio-gpg-pub'\n>>>\n>>> and the resulting repository also fails fsck.\n[...]\n> Naïvely, I may expect fsck to follow the\n> same principle--when it encounters a reference to an object that is\n> deliberately missing, refrain from complaining, regardless of the\n> place the offending reference was found, be it inside a tree object\n> or a ref.\n>\n> Perhaps that line of consistent behaviour may be impossible due to\n> the way the reference is stored inside a tree object and a ref?\n\nYes, exactly. I believe this is why we did not treat refs as\npromisors when we introduced promised objects.\n\nWe could revisit that decision and make refs into promisors, storing\nany extra metadata on the side. That would be a more invasive change\nthan this series does, though. In the model used today, where refs\nare not promisors, this series is the logical thing to do.\n\n> E.g. a reference to an expected-to-be-missing blob still lets us\n> know that the missing object is expected to be a blob, but a ref\n> only stores the object name and not its type, so we can tell it is\n> missing, but we cannot say it is OK to be missing because we expect\n> it to be a blob, or something?\n\nThe main downside of saying that filters should not be applied to the\nobjects named directly in a \"want\" is that such objects could be large.\n\nThe upsides are that\n\n- it makes the protocol more consistent with the current invariants\n maintained by \"git fsck\"\n\n- down the line, it should make operations like \"fetch just this one\n tree\" a little simpler, since you can use\n\n filter blob:none\n filter tree:none\n want \n\nLater we could introduce a separate kind of \"want\" for which filters\napply to the object named directly, too, to provide more flexibility\nfor the client. I can imagine cases where a person would want each of\nthose two behaviors, so I don't think we are incurring unnecessary\ncomplexity by always sending the object named in a \"want\" without such\nan explicit filtering request. So I'm comfortable with the direction\nthis series goes in, though I haven't looked at the patches in detail.\n\nThanks,\nJonathan\n"},{"id":"351827","messageId":"xmqqy3eogg98.fsf@gitster-ct.c.googlers.com","threadId":"48833","inReplyTo":"20180706212957.GD6195@aiede.svl.corp.google.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T22:18:27Z","receivedAt":"2018-07-06T22:18:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The upsides are that\n> ...\n> - down the line, it should make operations like \"fetch just this one\n> tree\" a little simpler, since you can use\n>\n> filter blob:none\n> filter tree:none\n> want \n\n;-) \n\nI think this example, especially without the first line, would have\na practical use, i.e. \"grab the contents of this directory without\nrecursing\", and as such is a very convincing argument to support the\napproach taken by the solution presented here.\n\n> .... So I'm comfortable with the direction\n> this series goes in, though I haven't looked at the patches in detail.\n\nLikewise. Thanks.\n"},{"id":"351951","messageId":"xmqqpnzwfl4a.fsf@gitster-ct.c.googlers.com","threadId":"48833","inReplyTo":"20180706212957.GD6195@aiede.svl.corp.google.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T16:07:49Z","receivedAt":"2018-07-09T16:07:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>>>> When cloning a repository with a tagged blob (like the Git repository)\n>>>> with --filter=blob:none, the following message appears:\n>>>>\n>>>> error: missing object referenced by 'refs/tags/junio-gpg-pub'\n>>>>\n>>>> and the resulting repository also fails fsck.\n> [...]\n>> Naïvely, I may expect fsck to follow the\n>> same principle--when it encounters a reference to an object that is\n>> deliberately missing, refrain from complaining, regardless of the\n>> place the offending reference was found, be it inside a tree object\n>> or a ref.\n>>\n>> Perhaps that line of consistent behaviour may be impossible due to\n>> the way the reference is stored inside a tree object and a ref?\n>\n> Yes, exactly. I believe this is why we did not treat refs as\n> promisors when we introduced promised objects.\n>\n> We could revisit that decision and make refs into promisors, storing\n> any extra metadata on the side. That would be a more invasive change\n> than this series does, though. In the model used today, where refs\n> are not promisors, this series is the logical thing to do.\n\nI think that refs in files-backend is a bit hard to update to\nannotate with extra information like \"is this merely promising or a\ntip of the connectivity graph?\" and agree that we should not make\nrefs promisors.\n\nSuch an otherwise unreferenced blob could be made into a promised\nobject by the promisor repository, though, I think. It can include\na synthetic tree that points at such a blob in the pack stream data\nif it knows that the resulting pack will be marked with the\n.promisor bit, and by doing so, there is no need to update the\nclient side, no?\n\n"},{"id":"351957","messageId":"20180709171007.53406-1-jonathantanmy@google.com","threadId":"48833","inReplyTo":"xmqqpnzwfl4a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T17:10:07Z","receivedAt":"2018-07-09T17:10:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> I think that refs in files-backend is a bit hard to update to\n> annotate with extra information like \"is this merely promising or a\n> tip of the connectivity graph?\" and agree that we should not make\n> refs promisors.\n\nOK, we agree on this :-)\n\n> Such an otherwise unreferenced blob could be made into a promised\n> object by the promisor repository, though, I think. It can include\n> a synthetic tree that points at such a blob in the pack stream data\n> if it knows that the resulting pack will be marked with the\n> .promisor bit, and by doing so, there is no need to update the\n> client side, no?\n\nYes, that is possible, but I think the client side still needs to be\nupdated, for the same reason as in my implementation (the client should\ncheck that the server sent all necessary objects - the blob itself in my\ncase, and the synthetic tree in yours).\n\nI think that if we were planning to introduce a synthetic tree, though,\nit would be better just to include the object itself (as in my\nimplementation). In both solutions, both the protocol and the server\nneed to be updated, but existing clients will still work (minus the\nchecking that would still be desirable with both solutions).\n"},{"id":"351993","messageId":"xmqqy3ekcihe.fsf@gitster-ct.c.googlers.com","threadId":"48833","inReplyTo":"cover.1530905323.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T19:33:01Z","receivedAt":"2018-07-09T19:33:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When cloning a repository with a tagged blob (like the Git repository)\n> with --filter=blob:none, the following message appears:\n>\n> error: missing object referenced by 'refs/tags/junio-gpg-pub'\n>\n> and the resulting repository also fails fsck.\n>\n> Patch 1 fixes the protocol documentation and the server side of Git, and\n> patch 2 makes clone error out when such a situation occurs.\n>\n> An argument could be made that we should not merge patch 2 just yet due\n> to the fact that some server implementations (such as Git and JGit)\n> still exhibit the old behavior, and the resulting clones (albeit failing\n> fsck) are still usable, because when attempting to load the blob, Git\n> will automatically fetch it. I'm on the fence about this, and have\n> included patch 2 in this patch set nevertheless for completeness.\n\nI think the latter is probably a good thing to nudge the server\nimplementations in the right direction by gently poking them ;-)\n\nThe patches textually apply cleanly on 'master' but apparently it\nneeds some fixes in jt/partial-clone-fsck-connectivity topic to\nfunction correctly?\n\n"},{"id":"352004","messageId":"20180709200749.190861-1-jonathantanmy@google.com","threadId":"48833","inReplyTo":"xmqqy3ekcihe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T20:07:49Z","receivedAt":"2018-07-09T20:07:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > An argument could be made that we should not merge patch 2 just yet due\n> > to the fact that some server implementations (such as Git and JGit)\n> > still exhibit the old behavior, and the resulting clones (albeit failing\n> > fsck) are still usable, because when attempting to load the blob, Git\n> > will automatically fetch it. I'm on the fence about this, and have\n> > included patch 2 in this patch set nevertheless for completeness.\n> \n> I think the latter is probably a good thing to nudge the server\n> implementations in the right direction by gently poking them ;-)\n\nOK, I'll keep patch 2 then.\n\n> The patches textually apply cleanly on 'master' but apparently it\n> needs some fixes in jt/partial-clone-fsck-connectivity topic to\n> function correctly?\n\nI forgot to mention in the original e-mail that this is on\nbw/ref-in-want. (I mentioned it in an e-mail I sent later [1].) I've\nchecked and basing it on both bw/ref-in-want and\njt/partial-clone-fsck-connectivity (which is based on bw/ref-in-want\nitself) both work, so you can choose either one.\n\n[1] https://public-inbox.org/git/20180706193847.160161-1-jonathantanmy@google.com/\n"}]}