{"thread":{"id":"52170","subject":"[PATCH] fetch: remove fetch_if_missing=0","startedAt":"2019-11-01T20:38:38Z","lastAt":"2019-11-08T07:40:10Z","messageCount":12,"participants":["Jonathan Tan","Jonathan Nieder","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"385279","messageId":"20191101203830.231676-1-jonathantanmy@google.com","threadId":"52170","inReplyTo":null,"subject":"[PATCH] fetch: remove fetch_if_missing=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-01T20:38:30Z","receivedAt":"2019-11-01T20:38:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In fetch_pack() (and all functions it calls), pass\nOBJECT_INFO_SKIP_FETCH_OBJECT whenever we query an object that could be\na tree or blob that we do not want to be lazy-fetched even if it is\nabsent. Thus, the only lazy-fetches occurring for trees and blobs are\nwhen resolving deltas.\n\nThus, we can remove fetch_if_missing=0 from builtin/fetch.c. Remove\nthis, and also add a test ensuring that such objects are not\nlazy-fetched. (We might be able to remove fetch_if_missing=0 from other\nplaces too, but I have limited myself to builtin/fetch.c in this commit\nbecause I have not written tests for the other commands yet.)\n\nNote that commits and tags may still be lazy-fetched. I limited myself\nto objects that could be trees or blobs here because Git does not\nsupport creating such commit- and tag-excluding clones yet, and even if\nsuch a clone were manually created, Git does not have good support for\nfetching a single commit (when fetching a commit, it and all its\nancestors would be sent).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nI've verified that this also solves the bug explained in:\nhttps://public-inbox.org/git/20191007181825.13463-1-jonathantanmy@google.com/\n\nQuoting what I wrote from there:\n\n> (Alternatively, we\n> could add the equivalent of OBJECT_INFO_SKIP_FETCH_OBJECT to functions\n> like parse_commit() that are used by files like negotiator/default.c, or\n> split up commit parsing into object reading - which already has that\n> flag - and commit parsing.)\n\nThis commit adds OBJECT_INFO_SKIP_FETCH_OBJECT to functions, but not the\nparse_commit() mentioned - as stated above, this commit only handles\nobjects that could be trees or blobs.\n---\n builtin/fetch.c          |  5 ++-\n fetch-pack.c             |  3 +-\n t/t5616-partial-clone.sh | 69 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 73 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0c345b5dfe..5ff7367dd7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1074,7 +1074,8 @@ static int check_exist_and_connected(struct ref *ref_map)\n \t * we need all direct targets to exist.\n \t */\n \tfor (r = rm; r; r = r->next) {\n-\t\tif (!has_object_file(&r->old_oid))\n+\t\tif (!has_object_file_with_flags(&r->old_oid,\n+\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\treturn -1;\n \t}\n \n@@ -1755,8 +1756,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"fetch\");\n \n-\tfetch_if_missing = 0;\n-\n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n \tfor (i = 1; i < argc; i++)\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0130b44112..37178e2d34 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -673,7 +673,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tstruct object *o;\n \n \t\tif (!has_object_file_with_flags(&ref->old_oid,\n-\t\t\t\t\t\tOBJECT_INFO_QUICK))\n+\t\t\t\t\t\tOBJECT_INFO_QUICK |\n+\t\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\tcontinue;\n \t\to = parse_object(the_repository, &ref->old_oid);\n \t\tif (!o)\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 79f7b65f8c..1081ed2de0 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -296,6 +296,75 @@ test_expect_success 'partial clone with unresolvable sparse filter fails cleanly\n \ttest_i18ngrep \"unable to parse sparse filter data in\" err\n '\n \n+setup_triangle () {\n+\trm -rf big-blob.txt server client promisor-remote &&\n+\n+\ttouch big-blob.txt &&\n+\tfor i in $(seq 1 100)\n+\tdo\n+\t\techo line $i >>big-blob.txt\n+\tdone &&\n+\n+\t# Create a server with 2 commits: a commit with a big blob and a child\n+\t# commit with an incremental change. Also, create a partial clone\n+\t# client that only contains the first commit.\n+\tgit init server &&\n+\tgit -C server config --local uploadpack.allowfilter 1 &&\n+\tcp big-blob.txt server &&\n+\tgit -C server add big-blob.txt &&\n+\tgit -C server commit -m \"initial\" &&\n+\tgit clone --bare --filter=tree:0 \"file://$(pwd)/server\" client &&\n+\techo another line >>server/big-blob.txt &&\n+\tgit -C server commit -am \"append line to big blob\" &&\n+\n+\t# Create a promisor remote that only contains the blob from the first\n+\t# commit, and set it as the promisor remote of client. Thus, whenever\n+\t# the client lazy fetches, the lazy fetch will succeed only if it is\n+\t# for this blob.\n+\tgit init promisor-remote &&\n+\ttest_commit -C promisor-remote one && # so that ref advertisement is not empty\n+\tgit -C promisor-remote config --local uploadpack.allowanysha1inwant 1 &&\n+\tgit -C promisor-remote hash-object -w --stdin <big-blob.txt &&\n+\tgit -C client remote set-url origin \"file://$(pwd)/promisor-remote\"\n+}\n+\n+test_expect_success 'fetch lazy-fetches only to resolve deltas' '\n+\tsetup_triangle &&\n+\n+\t# Exercise to make sure it works. Git will not fetch anything from the\n+\t# promisor remote other than for the big blob (because it needs to\n+\t# resolve the delta).\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client \\\n+\t\tfetch \"file://$(pwd)/server\" master &&\n+\n+\t# Verify the assumption that the client needed to fetch the delta base\n+\t# to resolve the delta.\n+\tgit hash-object big-blob.txt >hash &&\n+\tgrep \"want $(cat hash)\" trace\n+'\n+\n+test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n+\tsetup_triangle &&\n+\n+\tgit -C server config --local protocol.version 2 &&\n+\tgit -C client config --local protocol.version 2 &&\n+\tgit -C promisor-remote config --local protocol.version 2 &&\n+\n+\t# Exercise to make sure it works. Git will not fetch anything from the\n+\t# promisor remote other than for the big blob (because it needs to\n+\t# resolve the delta).\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client \\\n+\t\tfetch \"file://$(pwd)/server\" master &&\n+\n+\t# Verify that protocol version 2 was used.\n+\tgrep \"fetch< version 2\" trace &&\n+\n+\t# Verify the assumption that the client needed to fetch the delta base\n+\t# to resolve the delta.\n+\tgit hash-object big-blob.txt >hash &&\n+\tgrep \"want $(cat hash)\" trace\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.24.0.rc1.363.gb1bccd3e3d-goog\n\n"},{"id":"385281","messageId":"20191101220537.GA249573@google.com","threadId":"52170","inReplyTo":"20191101203830.231676-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: remove fetch_if_missing=0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-11-01T22:05:37Z","receivedAt":"2019-11-01T22:05:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> In fetch_pack() (and all functions it calls), pass\n> OBJECT_INFO_SKIP_FETCH_OBJECT whenever we query an object that could be\n> a tree or blob that we do not want to be lazy-fetched even if it is\n> absent. Thus, the only lazy-fetches occurring for trees and blobs are\n> when resolving deltas.\n>\n> Thus, we can remove fetch_if_missing=0 from builtin/fetch.c. Remove\n> this, and also add a test ensuring that such objects are not\n> lazy-fetched. (We might be able to remove fetch_if_missing=0 from other\n> places too, but I have limited myself to builtin/fetch.c in this commit\n> because I have not written tests for the other commands yet.)\n\nHooray!  Thanks much, this looks easier to maintain.\n\n> Note that commits and tags may still be lazy-fetched. I limited myself\n> to objects that could be trees or blobs here because Git does not\n> support creating such commit- and tag-excluding clones yet, and even if\n> such a clone were manually created, Git does not have good support for\n> fetching a single commit (when fetching a commit, it and all its\n> ancestors would be sent).\n\nIs there a place we could put a NEEDSWORK comment to avoid confusion\nwhen debugging if this gets introduced later?\n\nEven if not, this seems like a sensible choice.\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> I've verified that this also solves the bug explained in:\n> https://public-inbox.org/git/20191007181825.13463-1-jonathantanmy@google.com/\n\nMight be worth mentioning the example from there in the commit message\nas well, to help explain the context behind the change.\n\nI would still be in favor of applying that more conservative change to\n\"master\", even this late in the -rc cycle.\n\n[...]\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1074,7 +1074,8 @@ static int check_exist_and_connected(struct ref *ref_map)\n>  \t * we need all direct targets to exist.\n>  \t */\n>  \tfor (r = rm; r; r = r->next) {\n> -\t\tif (!has_object_file(&r->old_oid))\n> +\t\tif (!has_object_file_with_flags(&r->old_oid,\n> +\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n\nYes.\n\n[...]\n> @@ -1755,8 +1756,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \n>  \tpacket_trace_identity(\"fetch\");\n>  \n> -\tfetch_if_missing = 0;\n> -\n\nThis is the scary part, but in an \"uncomfortably exciting\" sense rather\nthan a worrying one.  Thanks for adding a test.\n\n[...]\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -673,7 +673,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \t\tstruct object *o;\n>  \n>  \t\tif (!has_object_file_with_flags(&ref->old_oid,\n> -\t\t\t\t\t\tOBJECT_INFO_QUICK))\n> +\t\t\t\t\t\tOBJECT_INFO_QUICK |\n> +\t\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n\nShould we make OBJECT_INFO_QUICK always imply\nOBJECT_INFO_SKIP_FETCH_OBJECT?  I would suspect that if we are willing to\navoid checking thoroughly locally, checking remotely would be even more\nundesirable.\n\n[...]\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -296,6 +296,75 @@ test_expect_success 'partial clone with unresolvable sparse filter fails cleanly\n>  \ttest_i18ngrep \"unable to parse sparse filter data in\" err\n>  '\n>  \n> +setup_triangle () {\n> +\trm -rf big-blob.txt server client promisor-remote &&\n> +\n> +\ttouch big-blob.txt &&\n\nTests seem to prefer spelling this as\n\n\t>big-blob.txt &&\n\nbecause that specifes the content of the file.\n\n> +\tfor i in $(seq 1 100)\n> +\tdo\n> +\t\techo line $i >>big-blob.txt\n> +\tdone &&\n\nShould this use test_seq for better portability?\n\nnit: can avoid a subshell:\n\n\ttest_seq 1 100 | sed -e 's/^/line /' >big-blob.txt\n\n[...]\n> +test_expect_success 'fetch lazy-fetches only to resolve deltas' '\n> +\tsetup_triangle &&\n> +\n> +\t# Exercise to make sure it works. Git will not fetch anything from the\n> +\t# promisor remote other than for the big blob (because it needs to\n> +\t# resolve the delta).\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client \\\n> +\t\tfetch \"file://$(pwd)/server\" master &&\n> +\n> +\t# Verify the assumption that the client needed to fetch the delta base\n> +\t# to resolve the delta.\n> +\tgit hash-object big-blob.txt >hash &&\n> +\tgrep \"want $(cat hash)\" trace\n\nnit: can avoid using cat:\n\n\thash=$(git hash-object big-blob.txt) &&\n\tgrep \"want $hash\" trace\n\nThanks and hope that helps,\nJonathan\n"},{"id":"385303","messageId":"xmqqv9s23jr5.fsf@gitster-ct.c.googlers.com","threadId":"52170","inReplyTo":"20191101220537.GA249573@google.com","subject":"Re: [PATCH] fetch: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-02T05:55:10Z","receivedAt":"2019-11-02T05:55:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> @@ -1755,8 +1756,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>>  \n>>  \tpacket_trace_identity(\"fetch\");\n>>  \n>> -\tfetch_if_missing = 0;\n>> -\n>\n> This is the scary part, but in an \"uncomfortably exciting\" sense rather\n> than a worrying one.  Thanks for adding a test.\n>\n> [...]\n>> --- a/fetch-pack.c\n>> +++ b/fetch-pack.c\n>> @@ -673,7 +673,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>>  \t\tstruct object *o;\n>>  \n>>  \t\tif (!has_object_file_with_flags(&ref->old_oid,\n>> -\t\t\t\t\t\tOBJECT_INFO_QUICK))\n>> +\t\t\t\t\t\tOBJECT_INFO_QUICK |\n>> +\t\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n>\n> Should we make OBJECT_INFO_QUICK always imply\n> OBJECT_INFO_SKIP_FETCH_OBJECT?  I would suspect that if we are willing to\n> avoid checking thoroughly locally, checking remotely would be even more\n> undesirable.\n\nI think I've seen this mentioned a few times during this cycle in\nthe list archive ;-)\n\n>> +\tfor i in $(seq 1 100)\n>> +\tdo\n>> +\t\techo line $i >>big-blob.txt\n>> +\tdone &&\n>\n> Should this use test_seq for better portability?\n\nYup.\n\n> nit: can avoid a subshell:\n>\n> \ttest_seq 1 100 | sed -e 's/^/line /' >big-blob.txt\n\nYeah, but it costs process start-up and \"sed\" that may be rather\nheavyweight.  At least\n\n\tfor i in $(test_seq ...)\n\tdo\n\t\techo line $i\n\tdone >big-blob.txt\n\nwould save repeated opening and closing the file, I'd think.\n\n>> +\tgit hash-object big-blob.txt >hash &&\n>> +\tgrep \"want $(cat hash)\" trace\n>\n> nit: can avoid using cat:\n>\n> \thash=$(git hash-object big-blob.txt) &&\n> \tgrep \"want $hash\" trace\n>\n> Thanks and hope that helps,\n\nThanks, both.\n\n"},{"id":"385304","messageId":"xmqqpnia3jja.fsf@gitster-ct.c.googlers.com","threadId":"52170","inReplyTo":"20191101220537.GA249573@google.com","subject":"Re: [PATCH] fetch: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-02T05:59:53Z","receivedAt":"2019-11-02T05:59:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> I would still be in favor of applying that more conservative change to\n> \"master\", even this late in the -rc cycle.\n\nI believe <20191023233403.145457-1-jonathantanmy@google.com> is the\nlatest incarnation of the \"conservative\" approach.  I think I can\nbuy that, but let me see if there are interactions with other topics\nfirst.\n\n"},{"id":"385305","messageId":"CAPig+cTqKe7Xz4tZrA+sC7peVdjaVS1wQfJ4fLVVUzVzjHdrmw@mail.gmail.com","threadId":"52170","inReplyTo":"xmqqv9s23jr5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch: remove fetch_if_missing=0","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-11-02T06:11:35Z","receivedAt":"2019-11-02T06:11:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Nov 2, 2019 at 1:55 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> > nit: can avoid a subshell:\n> >\n> >       test_seq 1 100 | sed -e 's/^/line /' >big-blob.txt\n>\n> Yeah, but it costs process start-up and \"sed\" that may be rather\n> heavyweight.  At least\n>\n>         for i in $(test_seq ...)\n>         do\n>                 echo line $i\n>         done >big-blob.txt\n>\n> would save repeated opening and closing the file, I'd think.\n\nMore bikeshedding:\n\n    printf \"line %d\\n\" $(test_seq 1 100) >big-blob.txt\n\nis reasonably concise, perhaps easier to grok than the one-liner\nincorporating 'sed', and shouldn't run afoul of command-line length\nlimitation.\n"},{"id":"385475","messageId":"20191105185322.205317-1-jonathantanmy@google.com","threadId":"52170","inReplyTo":"20191101220537.GA249573@google.com","subject":"Re: [PATCH] fetch: remove fetch_if_missing=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-05T18:53:22Z","receivedAt":"2019-11-05T18:53:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks for your review.\n\n> > Note that commits and tags may still be lazy-fetched. I limited myself\n> > to objects that could be trees or blobs here because Git does not\n> > support creating such commit- and tag-excluding clones yet, and even if\n> > such a clone were manually created, Git does not have good support for\n> > fetching a single commit (when fetching a commit, it and all its\n> > ancestors would be sent).\n> \n> Is there a place we could put a NEEDSWORK comment to avoid confusion\n> when debugging if this gets introduced later?\n> \n> Even if not, this seems like a sensible choice.\n\nDone in the test.\n\n> > I've verified that this also solves the bug explained in:\n> > https://public-inbox.org/git/20191007181825.13463-1-jonathantanmy@google.com/\n> \n> Might be worth mentioning the example from there in the commit message\n> as well, to help explain the context behind the change.\n> \n> I would still be in favor of applying that more conservative change to\n> \"master\", even this late in the -rc cycle.\n\nIf we're applying that change first, then this no longer fixes any bug,\nbut is just a code cleanup. (If we don't apply that change, then I'll\ninclude that example in the commit message.)\n\n> Should we make OBJECT_INFO_QUICK always imply\n> OBJECT_INFO_SKIP_FETCH_OBJECT?  I would suspect that if we are willing to\n> avoid checking thoroughly locally, checking remotely would be even more\n> undesirable.\n\nAs I wrote in [1], the implication does not really go both ways. I think\nit's better to keep them separate. (Or, at least, we don't need to make\nthe decision in this patch.)\n\n[1] https://public-inbox.org/git/20191011220822.154063-1-jonathantanmy@google.com/\n\n> > +test_expect_success 'fetch lazy-fetches only to resolve deltas' '\n> > +\tsetup_triangle &&\n> > +\n> > +\t# Exercise to make sure it works. Git will not fetch anything from the\n> > +\t# promisor remote other than for the big blob (because it needs to\n> > +\t# resolve the delta).\n> > +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client \\\n> > +\t\tfetch \"file://$(pwd)/server\" master &&\n> > +\n> > +\t# Verify the assumption that the client needed to fetch the delta base\n> > +\t# to resolve the delta.\n> > +\tgit hash-object big-blob.txt >hash &&\n> > +\tgrep \"want $(cat hash)\" trace\n> \n> nit: can avoid using cat:\n> \n> \thash=$(git hash-object big-blob.txt) &&\n> \tgrep \"want $hash\" trace\n\nI think it's less error-prone if we always have a \"git\" command on its\nown on a line, to avoid losing its error code. When piped into another\ninvocation, or when command-substituted into an argument (e.g. \"echo\n$(git hash-object foo)\"), we lose its error code.\n"},{"id":"385476","messageId":"20191105185619.207173-1-jonathantanmy@google.com","threadId":"52170","inReplyTo":"20191101203830.231676-1-jonathantanmy@google.com","subject":"[PATCH v2] fetch: remove fetch_if_missing=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-05T18:56:19Z","receivedAt":"2019-11-05T18:56:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In fetch_pack() (and all functions it calls), pass\nOBJECT_INFO_SKIP_FETCH_OBJECT whenever we query an object that could be\na tree or blob that we do not want to be lazy-fetched even if it is\nabsent. Thus, the only lazy-fetches occurring for trees and blobs are\nwhen resolving deltas.\n\nThus, we can remove fetch_if_missing=0 from builtin/fetch.c. Remove\nthis, and also add a test ensuring that such objects are not\nlazy-fetched. (We might be able to remove fetch_if_missing=0 from other\nplaces too, but I have limited myself to builtin/fetch.c in this commit\nbecause I have not written tests for the other commands yet.)\n\nNote that commits and tags may still be lazy-fetched. I limited myself\nto objects that could be trees or blobs here because Git does not\nsupport creating such commit- and tag-excluding clones yet, and even if\nsuch a clone were manually created, Git does not have good support for\nfetching a single commit (when fetching a commit, it and all its\nancestors would be sent).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nChanges from v1:\n- Added NEEDSWORK in test, suggested by Jonathan Nieder\n- Used printf in test, suggested by Eric Sunshine\n\nRange-diff against v1:\n1:  f52c8d4c11 ! 1:  8e93cb8b02 fetch: remove fetch_if_missing=0\n    @@ Commit message\n         ancestors would be sent).\n     \n         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n     \n      ## builtin/fetch.c ##\n     @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n    @@ t/t5616-partial-clone.sh: test_expect_success 'partial clone with unresolvable s\n     +setup_triangle () {\n     +\trm -rf big-blob.txt server client promisor-remote &&\n     +\n    -+\ttouch big-blob.txt &&\n    -+\tfor i in $(seq 1 100)\n    -+\tdo\n    -+\t\techo line $i >>big-blob.txt\n    -+\tdone &&\n    ++\tprintf \"line %d\\n\" $(test_seq 1 100) >big-blob.txt\n     +\n     +\t# Create a server with 2 commits: a commit with a big blob and a child\n     +\t# commit with an incremental change. Also, create a partial clone\n    @@ t/t5616-partial-clone.sh: test_expect_success 'partial clone with unresolvable s\n     +\tgit -C client remote set-url origin \"file://$(pwd)/promisor-remote\"\n     +}\n     +\n    ++# NEEDSWORK: The tests beginning with \"fetch lazy-fetches\" below only\n    ++# test that \"fetch\" avoid fetching trees and blobs, but not commits or\n    ++# tags. Revisit this if Git is ever taught to support partial clones\n    ++# with commits and/or tags filtered out.\n    ++\n     +test_expect_success 'fetch lazy-fetches only to resolve deltas' '\n     +\tsetup_triangle &&\n     +\n\n builtin/fetch.c          |  5 ++-\n fetch-pack.c             |  3 +-\n t/t5616-partial-clone.sh | 70 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 74 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0c345b5dfe..5ff7367dd7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1074,7 +1074,8 @@ static int check_exist_and_connected(struct ref *ref_map)\n \t * we need all direct targets to exist.\n \t */\n \tfor (r = rm; r; r = r->next) {\n-\t\tif (!has_object_file(&r->old_oid))\n+\t\tif (!has_object_file_with_flags(&r->old_oid,\n+\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\treturn -1;\n \t}\n \n@@ -1755,8 +1756,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"fetch\");\n \n-\tfetch_if_missing = 0;\n-\n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n \tfor (i = 1; i < argc; i++)\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0130b44112..37178e2d34 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -673,7 +673,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tstruct object *o;\n \n \t\tif (!has_object_file_with_flags(&ref->old_oid,\n-\t\t\t\t\t\tOBJECT_INFO_QUICK))\n+\t\t\t\t\t\tOBJECT_INFO_QUICK |\n+\t\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\tcontinue;\n \t\to = parse_object(the_repository, &ref->old_oid);\n \t\tif (!o)\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 79f7b65f8c..e785fba7ed 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -296,6 +296,76 @@ test_expect_success 'partial clone with unresolvable sparse filter fails cleanly\n \ttest_i18ngrep \"unable to parse sparse filter data in\" err\n '\n \n+setup_triangle () {\n+\trm -rf big-blob.txt server client promisor-remote &&\n+\n+\tprintf \"line %d\\n\" $(test_seq 1 100) >big-blob.txt\n+\n+\t# Create a server with 2 commits: a commit with a big blob and a child\n+\t# commit with an incremental change. Also, create a partial clone\n+\t# client that only contains the first commit.\n+\tgit init server &&\n+\tgit -C server config --local uploadpack.allowfilter 1 &&\n+\tcp big-blob.txt server &&\n+\tgit -C server add big-blob.txt &&\n+\tgit -C server commit -m \"initial\" &&\n+\tgit clone --bare --filter=tree:0 \"file://$(pwd)/server\" client &&\n+\techo another line >>server/big-blob.txt &&\n+\tgit -C server commit -am \"append line to big blob\" &&\n+\n+\t# Create a promisor remote that only contains the blob from the first\n+\t# commit, and set it as the promisor remote of client. Thus, whenever\n+\t# the client lazy fetches, the lazy fetch will succeed only if it is\n+\t# for this blob.\n+\tgit init promisor-remote &&\n+\ttest_commit -C promisor-remote one && # so that ref advertisement is not empty\n+\tgit -C promisor-remote config --local uploadpack.allowanysha1inwant 1 &&\n+\tgit -C promisor-remote hash-object -w --stdin <big-blob.txt &&\n+\tgit -C client remote set-url origin \"file://$(pwd)/promisor-remote\"\n+}\n+\n+# NEEDSWORK: The tests beginning with \"fetch lazy-fetches\" below only\n+# test that \"fetch\" avoid fetching trees and blobs, but not commits or\n+# tags. Revisit this if Git is ever taught to support partial clones\n+# with commits and/or tags filtered out.\n+\n+test_expect_success 'fetch lazy-fetches only to resolve deltas' '\n+\tsetup_triangle &&\n+\n+\t# Exercise to make sure it works. Git will not fetch anything from the\n+\t# promisor remote other than for the big blob (because it needs to\n+\t# resolve the delta).\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client \\\n+\t\tfetch \"file://$(pwd)/server\" master &&\n+\n+\t# Verify the assumption that the client needed to fetch the delta base\n+\t# to resolve the delta.\n+\tgit hash-object big-blob.txt >hash &&\n+\tgrep \"want $(cat hash)\" trace\n+'\n+\n+test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n+\tsetup_triangle &&\n+\n+\tgit -C server config --local protocol.version 2 &&\n+\tgit -C client config --local protocol.version 2 &&\n+\tgit -C promisor-remote config --local protocol.version 2 &&\n+\n+\t# Exercise to make sure it works. Git will not fetch anything from the\n+\t# promisor remote other than for the big blob (because it needs to\n+\t# resolve the delta).\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client \\\n+\t\tfetch \"file://$(pwd)/server\" master &&\n+\n+\t# Verify that protocol version 2 was used.\n+\tgrep \"fetch< version 2\" trace &&\n+\n+\t# Verify the assumption that the client needed to fetch the delta base\n+\t# to resolve the delta.\n+\tgit hash-object big-blob.txt >hash &&\n+\tgrep \"want $(cat hash)\" trace\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.24.0.rc1.363.gb1bccd3e3d-goog\n\n"},{"id":"385477","messageId":"20191105185845.GA94320@google.com","threadId":"52170","inReplyTo":"20191105185322.205317-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: remove fetch_if_missing=0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-11-05T18:58:45Z","receivedAt":"2019-11-05T18:58:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> I think it's less error-prone if we always have a \"git\" command on its\n> own on a line, to avoid losing its error code. When piped into another\n> invocation, or when command-substituted into an argument (e.g. \"echo\n> $(git hash-object foo)\"), we lose its error code.\n\nSure, but assignment is a special case:\n\n\tif myvar=$(false)\n\tthen\n\t\techo yes\n\telse\n\t\techo no\n\tfi\n\nprints \"no\".\n\nSee \"Don't use command substitution in a way that discards git's exit\ncode\" in t/README for more details.\n\nThanks,\nJonathan\n"},{"id":"385485","messageId":"CAPig+cQ6iyJacniLDF_kuc1FhPMXYmL3V1DzdOPa6bX0h2FHFA@mail.gmail.com","threadId":"52170","inReplyTo":"20191105185619.207173-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch: remove fetch_if_missing=0","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-11-05T20:06:13Z","receivedAt":"2019-11-05T20:06:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 5, 2019 at 1:56 PM Jonathan Tan <jonathantanmy@google.com> wrote:\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> @@ -296,6 +296,76 @@ test_expect_success 'partial clone with unresolvable sparse filter fails cleanly\n> +setup_triangle () {\n> +       rm -rf big-blob.txt server client promisor-remote &&\n> +\n> +       printf \"line %d\\n\" $(test_seq 1 100) >big-blob.txt\n\nBroken &&-chain.\n\n> +       # Create a server with 2 commits: a commit with a big blob and a child\n> +       # commit with an incremental change. Also, create a partial clone\n> +       # client that only contains the first commit.\n> +       git init server &&\n> +       git -C server config --local uploadpack.allowfilter 1 &&\n> +       cp big-blob.txt server &&\n> +       git -C server add big-blob.txt &&\n> +       git -C server commit -m \"initial\" &&\n> +       git clone --bare --filter=tree:0 \"file://$(pwd)/server\" client &&\n> +       echo another line >>server/big-blob.txt &&\n> +       git -C server commit -am \"append line to big blob\" &&\n> +\n> +       # Create a promisor remote that only contains the blob from the first\n> +       # commit, and set it as the promisor remote of client. Thus, whenever\n> +       # the client lazy fetches, the lazy fetch will succeed only if it is\n> +       # for this blob.\n> +       git init promisor-remote &&\n> +       test_commit -C promisor-remote one && # so that ref advertisement is not empty\n> +       git -C promisor-remote config --local uploadpack.allowanysha1inwant 1 &&\n> +       git -C promisor-remote hash-object -w --stdin <big-blob.txt &&\n> +       git -C client remote set-url origin \"file://$(pwd)/promisor-remote\"\n> +}\n"},{"id":"385507","messageId":"xmqq7e4dydzv.fsf@gitster-ct.c.googlers.com","threadId":"52170","inReplyTo":"20191105185619.207173-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T01:45:08Z","receivedAt":"2019-11-06T01:45:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> In fetch_pack() (and all functions it calls), pass\n> OBJECT_INFO_SKIP_FETCH_OBJECT whenever we query an object that could be\n> a tree or blob that we do not want to be lazy-fetched even if it is\n> absent. Thus, the only lazy-fetches occurring for trees and blobs are\n> when resolving deltas.\n>\n> Thus, we can remove fetch_if_missing=0 from builtin/fetch.c. Remove\n> this, and also add a test ensuring that such objects are not\n> lazy-fetched. (We might be able to remove fetch_if_missing=0 from other\n> places too, but I have limited myself to builtin/fetch.c in this commit\n> because I have not written tests for the other commands yet.)\n>\n> Note that commits and tags may still be lazy-fetched. I limited myself\n> to objects that could be trees or blobs here because Git does not\n> support creating such commit- and tag-excluding clones yet, and even if\n> such a clone were manually created, Git does not have good support for\n> fetching a single commit (when fetching a commit, it and all its\n> ancestors would be sent).\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Changes from v1:\n> - Added NEEDSWORK in test, suggested by Jonathan Nieder\n> - Used printf in test, suggested by Eric Sunshine\n\nNice.  The less we use big \"plug the machinery to lazily fetch for\nnow\" switch, the better, I think.\n\n> +setup_triangle () {\n> +\trm -rf big-blob.txt server client promisor-remote &&\n> +\n> +\tprintf \"line %d\\n\" $(test_seq 1 100) >big-blob.txt\n\nI'll tweak this line while queueing with a trailing \" &&\" (no need\nto resend only for this).\n"},{"id":"385759","messageId":"xmqqr22inagr.fsf@gitster-ct.c.googlers.com","threadId":"52170","inReplyTo":"20191105185619.207173-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-08T06:33:40Z","receivedAt":"2019-11-08T06:33:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 0c345b5dfe..5ff7367dd7 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1074,7 +1074,8 @@ static int check_exist_and_connected(struct ref *ref_map)\n>  \t * we need all direct targets to exist.\n>  \t */\n>  \tfor (r = rm; r; r = r->next) {\n> -\t\tif (!has_object_file(&r->old_oid))\n> +\t\tif (!has_object_file_with_flags(&r->old_oid,\n> +\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n>  \t\t\treturn -1;\n>  \t}\n>  \n> @@ -1755,8 +1756,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \n>  \tpacket_trace_identity(\"fetch\");\n>  \n> -\tfetch_if_missing = 0;\n> -\n>  \t/* Record the command line for the reflog */\n>  \tstrbuf_addstr(&default_rla, \"fetch\");\n>  \tfor (i = 1; i < argc; i++)\n\nAs this is on a stale codebase, let's base the fix on top of\nc32ca691c2 or later.  The part of the patch for builtin/fetch.c\nadjusted for that decision would look like attached.\n\nAn interesting thing is that c32ca691c2^2 that moved the assignment\nto this big red switch variable around causes 3-way merge to fail in\na miserable way.  The \"moving around\" would involve removing from\nthe same location as the rebased patch below removes, plus adding\nthe assignment elsewhere, so \"both sides removed the assignment from\nthis hunk, so take that\" would correctly leave the original assignment\nwe see in the second hunk above removed, but fails to notice that\nthe assignment made elsewhere (the result of \"moving around\" patch)\nis no longer needed, because \"c32ca691c2^2 added one and this change\ndoes not do anything there, so take the addition\" cleanly resolves\nto an incorrect merge result.\n\n builtin/fetch.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 863c858fde..5ff7367dd7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1074,7 +1074,8 @@ static int check_exist_and_connected(struct ref *ref_map)\n \t * we need all direct targets to exist.\n \t */\n \tfor (r = rm; r; r = r->next) {\n-\t\tif (!has_object_file(&r->old_oid))\n+\t\tif (!has_object_file_with_flags(&r->old_oid,\n+\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\treturn -1;\n \t}\n \n@@ -1822,8 +1823,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tfetch_if_missing = 0;\n-\n \tif (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n"},{"id":"385762","messageId":"xmqqmud6n7e5.fsf@gitster-ct.c.googlers.com","threadId":"52170","inReplyTo":"xmqqr22inagr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] fetch: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-08T07:40:02Z","receivedAt":"2019-11-08T07:40:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> An interesting thing is that c32ca691c2^2 that moved the assignment\n> to this big red switch variable around causes 3-way merge to fail in\n> a miserable way.  The \"moving around\" would involve removing from\n> the same location as the rebased patch below removes, plus adding\n> the assignment elsewhere, so \"both sides removed the assignment from\n> this hunk, so take that\" would correctly leave the original assignment\n> we see in the second hunk above removed, but fails to notice that\n> the assignment made elsewhere (the result of \"moving around\" patch)\n> is no longer needed, because \"c32ca691c2^2 added one and this change\n> does not do anything there, so take the addition\" cleanly resolves\n> to an incorrect merge result.\n\nTotal tangent.  One frustrating thing is that we do this a bit\nbetter at the tree level merge.  After read-tree does three-way\nmerge at the tree level, what is passed to the merge-recursive\nmachinery has \"side A added\" and \"side A removed\" left unresolved,\nso that the post-processing phase could try to match them up and say\n\"aha, side A moved that path elsewhere while side B just removed,\nwhich is a conflict\".  \n\nI wish if xdiff/xmerge.c could learn a similar trick, but the\nnecessary change feels quite involved, error prone and too magical.\n\n\n\n"}]}