{"thread":{"id":"54180","subject":"[PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head","startedAt":"2020-09-02T19:02:39Z","lastAt":"2020-09-03T21:06:37Z","messageCount":9,"participants":["Jonathan Tan","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"404927","messageId":"20200902190232.317477-1-jonathantanmy@google.com","threadId":"54180","inReplyTo":null,"subject":"[PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-09-02T19:02:32Z","receivedAt":"2020-09-02T19:02:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"887952b8c6 (\"fetch: optionally allow disabling FETCH_HEAD update\",\n2020-08-18) introduced the ability to disable writing to FETCH_HEAD\nduring fetch, but did not suppress the \"<source> -> FETCH_HEAD\" message\nwhen this ability is used. This message is misleading in this case,\nbecause FETCH_HEAD is not written. Also, because \"fetch\" is used to\nlazy-fetch missing objects in a partial clone, this significantly\nclutters up the output in that case since the objects to be fetched are\npotentially numerous. Therefore, suppress this message when\n--no-write-fetch-head is passed.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis is on origin/jt/lazy-fetch.\n\nThis might be important for Stolee's maintenance prefetch patch [1] too\n- presumably we don't want to show FETCH_HEAD there, as it would be\nmisleading and would clutter in the same way (albeit to a lesser\nextent).\n\n[1] https://lore.kernel.org/git/da64c51a8182ec13aeed8f0157079fb29a09ee85.1598380599.git.gitgitgadget@gmail.com/\n---\n builtin/fetch.c          | 3 ++-\n t/t0410-partial-clone.sh | 7 +++++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 320ba9471d..442df05f5a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1023,11 +1023,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n \t\t\t\t\t\t       summary_width);\n \t\t\t\tfree(ref);\n-\t\t\t} else\n+\t\t\t} else if (write_fetch_head) {\n \t\t\t\tformat_display(&note, '*',\n \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n \t\t\t\t\t       *what ? what : \"HEAD\",\n \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n+\t\t\t}\n \t\t\tif (note.len) {\n \t\t\t\tif (verbosity >= 0 && !shown_url) {\n \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex d681e90640..584a039b85 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '\n '\n \n test_expect_success 'fetching of missing objects' '\n-\trm -rf repo &&\n+\trm -rf repo err &&\n \ttest_create_repo server &&\n \ttest_commit -C server foo &&\n \tgit -C server repack -a -d --write-bitmap-index &&\n@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"origin\" &&\n-\tgit -C repo cat-file -p \"$HASH\" &&\n+\tgit -C repo cat-file -p \"$HASH\" 2>err &&\n+\n+\t# Ensure that no spurious FETCH_HEAD messages are written\n+\t! grep FETCH_HEAD err &&\n \n \t# Ensure that the .promisor file is written, and check that its\n \t# associated packfile contains the object\n-- \n2.28.0.402.g5ffc5be6b7-goog\n\n"},{"id":"404935","messageId":"xmqq7dtcaqob.fsf@gitster.c.googlers.com","threadId":"54180","inReplyTo":"20200902190232.317477-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-02T20:07:16Z","receivedAt":"2020-09-02T20:07:24Z","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> 887952b8c6 (\"fetch: optionally allow disabling FETCH_HEAD update\",\n> 2020-08-18) introduced the ability to disable writing to FETCH_HEAD\n> during fetch, but did not suppress the \"<source> -> FETCH_HEAD\" message\n> when this ability is used. This message is misleading in this case,\n> because FETCH_HEAD is not written.\n\nThanks for noticing, but I wonder if we should keep this for users\nof dry-run, which tends to give more output to what _would_ have\nbeen done if it were not dry-run?\n\n> This might be important for Stolee's maintenance prefetch patch [1] too\n> - presumably we don't want to show FETCH_HEAD there, as it would be\n> misleading and would clutter in the same way (albeit to a lesser\n> extent).\n\nYes, that makes sense.\n\n> Also, because \"fetch\" is used to\n> lazy-fetch missing objects in a partial clone, this significantly\n> clutters up the output in that case since the objects to be fetched are\n> potentially numerous. Therefore, suppress this message when\n> --no-write-fetch-head is passed.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> This is on origin/jt/lazy-fetch.\n\nI think this patch, with possible correction for \"we are not writing\nFETCH_HEAD but we do want to show what would have been done when the\nuser asked --dry-run\", should be done as a brown-paper-bag bugfix\ndirectly on jc/no-update-fetch-head topic and merged quicly down to\n'master', instead of taken hostage of some other topic in flight,\nbut it would soon already be a week since jt/lazy-fetch was merged\nto 'next', so it probably is OK to apply on jt/lazy-fetch, and it\nindeed is easier to manage that way.\n\n> [1] https://lore.kernel.org/git/da64c51a8182ec13aeed8f0157079fb29a09ee85.1598380599.git.gitgitgadget@gmail.com/\n> ---\n>  builtin/fetch.c          | 3 ++-\n>  t/t0410-partial-clone.sh | 7 +++++--\n>  2 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 320ba9471d..442df05f5a 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1023,11 +1023,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n>  \t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n>  \t\t\t\t\t\t       summary_width);\n>  \t\t\t\tfree(ref);\n> -\t\t\t} else\n> +\t\t\t} else if (write_fetch_head) {\n>  \t\t\t\tformat_display(&note, '*',\n>  \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n>  \t\t\t\t\t       *what ? what : \"HEAD\",\n>  \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n> +\t\t\t}\n>  \t\t\tif (note.len) {\n>  \t\t\t\tif (verbosity >= 0 && !shown_url) {\n>  \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index d681e90640..584a039b85 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '\n>  '\n>  \n>  test_expect_success 'fetching of missing objects' '\n> -\trm -rf repo &&\n> +\trm -rf repo err &&\n>  \ttest_create_repo server &&\n>  \ttest_commit -C server foo &&\n>  \tgit -C server repack -a -d --write-bitmap-index &&\n> @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '\n>  \n>  \tgit -C repo config core.repositoryformatversion 1 &&\n>  \tgit -C repo config extensions.partialclone \"origin\" &&\n> -\tgit -C repo cat-file -p \"$HASH\" &&\n> +\tgit -C repo cat-file -p \"$HASH\" 2>err &&\n> +\n> +\t# Ensure that no spurious FETCH_HEAD messages are written\n> +\t! grep FETCH_HEAD err &&\n\nTest also --dry-run, but that perhaps needs to be done outside the\ncontext of partial-clone.  The above \"lazy fetching should be silent\nand should not bother users with mention of FETCH_HEAD\" is good test\nin the context of partial-clone, though.\n\njc/no-update-fetch-head added its own test to t/t5510, and both the\n\"output lacks FETCH_HEAD when --no-write-fetch-head is given\" test\nand the \"output still mentions FETCH_HEAD with --dry-run\" test\nbelong there.\n\nThanks.\n"},{"id":"404945","messageId":"20200902210539.1981453-1-jonathantanmy@google.com","threadId":"54180","inReplyTo":"xmqq7dtcaqob.fsf@gitster.c.googlers.com","subject":"[PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-09-02T21:05:39Z","receivedAt":"2020-09-02T21:05:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"887952b8c6 (\"fetch: optionally allow disabling FETCH_HEAD update\",\n2020-08-18) introduced the ability to disable writing to FETCH_HEAD\nduring fetch, but did not suppress the \"<source> -> FETCH_HEAD\" message\nwhen this ability is used. This message is misleading in this case,\nbecause FETCH_HEAD is not written. Also, because \"fetch\" is used to\nlazy-fetch missing objects in a partial clone, this significantly\nclutters up the output in that case since the objects to be fetched are\npotentially numerous.\n\nTherefore, suppress this message when --no-write-fetch-head is passed\n(but not when --dry-run is set).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis is on origin/jt/lazy-fetch.\n\nJunio writes [1]:\n\n> Test also --dry-run, but that perhaps needs to be done outside the\n> context of partial-clone.  The above \"lazy fetching should be silent\n> and should not bother users with mention of FETCH_HEAD\" is good test\n> in the context of partial-clone, though.\n>\n> jc/no-update-fetch-head added its own test to t/t5510, and both the\n> \"output lacks FETCH_HEAD when --no-write-fetch-head is given\" test\n> and the \"output still mentions FETCH_HEAD with --dry-run\" test\n> belong there.\n\nAh, thanks for catching that. Here's an updated version.\n\n[1] https://lore.kernel.org/git/xmqq7dtcaqob.fsf@gitster.c.googlers.com/\n---\n builtin/fetch.c          |  8 +++++++-\n t/t0410-partial-clone.sh |  7 +++++--\n t/t5510-fetch.sh         | 18 ++++++++++--------\n 3 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 320ba9471d..c6c4689250 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n \t\t\t\t\t\t       summary_width);\n \t\t\t\tfree(ref);\n-\t\t\t} else\n+\t\t\t} else if (write_fetch_head || dry_run) {\n+\t\t\t\t/*\n+\t\t\t\t * Display fetches written to FETCH_HEAD (or\n+\t\t\t\t * would be written to FETCH_HEAD, if --dry-run\n+\t\t\t\t * is set).\n+\t\t\t\t */\n \t\t\t\tformat_display(&note, '*',\n \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n \t\t\t\t\t       *what ? what : \"HEAD\",\n \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n+\t\t\t}\n \t\t\tif (note.len) {\n \t\t\t\tif (verbosity >= 0 && !shown_url) {\n \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex d681e90640..584a039b85 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '\n '\n \n test_expect_success 'fetching of missing objects' '\n-\trm -rf repo &&\n+\trm -rf repo err &&\n \ttest_create_repo server &&\n \ttest_commit -C server foo &&\n \tgit -C server repack -a -d --write-bitmap-index &&\n@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"origin\" &&\n-\tgit -C repo cat-file -p \"$HASH\" &&\n+\tgit -C repo cat-file -p \"$HASH\" 2>err &&\n+\n+\t# Ensure that no spurious FETCH_HEAD messages are written\n+\t! grep FETCH_HEAD err &&\n \n \t# Ensure that the .promisor file is written, and check that its\n \t# associated packfile contains the object\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 2a1abe91f0..759aec9305 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -543,16 +543,18 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '\n \n '\n \n-test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' '\n-\trm -f .git/FETCH_HEAD &&\n-\tgit fetch --dry-run . &&\n-\t! test -f .git/FETCH_HEAD\n+test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '\n+\trm -f .git/FETCH_HEAD err &&\n+\tgit fetch --dry-run . 2>err &&\n+\t! test -f .git/FETCH_HEAD &&\n+\tgrep FETCH_HEAD err\n '\n \n-test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' '\n-\trm -f .git/FETCH_HEAD &&\n-\tgit fetch --no-write-fetch-head . &&\n-\t! test -f .git/FETCH_HEAD\n+test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '\n+\trm -f .git/FETCH_HEAD err &&\n+\tgit fetch --no-write-fetch-head . 2>err &&\n+\t! test -f .git/FETCH_HEAD &&\n+\t! grep FETCH_HEAD err\n '\n \n test_expect_success '--write-fetch-head gets defeated by --dry-run' '\n-- \n2.28.0.402.g5ffc5be6b7-goog\n\n"},{"id":"404946","messageId":"xmqqsgbzamz8.fsf@gitster.c.googlers.com","threadId":"54180","inReplyTo":"20200902210539.1981453-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-02T21:27:07Z","receivedAt":"2020-09-02T21:27:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; this round looks good.\n"},{"id":"404948","messageId":"20200902235628.GB4035286@google.com","threadId":"54180","inReplyTo":"20200902210539.1981453-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-09-02T23:56:28Z","receivedAt":"2020-09-02T23:56:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> 887952b8c6 (\"fetch: optionally allow disabling FETCH_HEAD update\",\n> 2020-08-18) introduced the ability to disable writing to FETCH_HEAD\n> during fetch, but did not suppress the \"<source> -> FETCH_HEAD\" message\n> when this ability is used. This message is misleading in this case,\n> because FETCH_HEAD is not written. Also, because \"fetch\" is used to\n> lazy-fetch missing objects in a partial clone, this significantly\n> clutters up the output in that case since the objects to be fetched are\n> potentially numerous.\n>\n> Therefore, suppress this message when --no-write-fetch-head is passed\n> (but not when --dry-run is set).\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n[...]\n>  builtin/fetch.c          |  8 +++++++-\n>  t/t0410-partial-clone.sh |  7 +++++--\n>  t/t5510-fetch.sh         | 18 ++++++++++--------\n>  3 files changed, 22 insertions(+), 11 deletions(-)\n\nThanks for fixing it, and sorry I didn't catch it during initial\nreview.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 320ba9471d..c6c4689250 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n>  \t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n>  \t\t\t\t\t\t       summary_width);\n>  \t\t\t\tfree(ref);\n> -\t\t\t} else\n> +\t\t\t} else if (write_fetch_head || dry_run) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Display fetches written to FETCH_HEAD (or\n> +\t\t\t\t * would be written to FETCH_HEAD, if --dry-run\n\nnit: to fix the parallel construction, s/would/that would/ or\ns/written/that were written/\n\n> +\t\t\t\t * is set).\n> +\t\t\t\t */\n>  \t\t\t\tformat_display(&note, '*',\n>  \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n>  \t\t\t\t\t       *what ? what : \"HEAD\",\n>  \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n> +\t\t\t}\n>  \t\t\tif (note.len) {\n>  \t\t\t\tif (verbosity >= 0 && !shown_url) {\n>  \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index d681e90640..584a039b85 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '\n>  '\n>  \n>  test_expect_success 'fetching of missing objects' '\n> -\trm -rf repo &&\n> +\trm -rf repo err &&\n>  \ttest_create_repo server &&\n>  \ttest_commit -C server foo &&\n>  \tgit -C server repack -a -d --write-bitmap-index &&\n> @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '\n>  \n>  \tgit -C repo config core.repositoryformatversion 1 &&\n>  \tgit -C repo config extensions.partialclone \"origin\" &&\n> -\tgit -C repo cat-file -p \"$HASH\" &&\n> +\tgit -C repo cat-file -p \"$HASH\" 2>err &&\n> +\n> +\t# Ensure that no spurious FETCH_HEAD messages are written\n> +\t! grep FETCH_HEAD err &&\n\nThis test feels very specific to the bug being addressed.  That said,\nsince we *also* have a test below for the generic behavior I don't mind.\n\n>  \n>  \t# Ensure that the .promisor file is written, and check that its\n>  \t# associated packfile contains the object\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 2a1abe91f0..759aec9305 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -543,16 +543,18 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '\n>  \n>  '\n>  \n> -test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' '\n> -\trm -f .git/FETCH_HEAD &&\n> -\tgit fetch --dry-run . &&\n> -\t! test -f .git/FETCH_HEAD\n> +test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '\n> +\trm -f .git/FETCH_HEAD err &&\n> +\tgit fetch --dry-run . 2>err &&\n> +\t! test -f .git/FETCH_HEAD &&\n> +\tgrep FETCH_HEAD err\n>  '\n>  \n> -test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' '\n> -\trm -f .git/FETCH_HEAD &&\n> -\tgit fetch --no-write-fetch-head . &&\n> -\t! test -f .git/FETCH_HEAD\n> +test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '\n> +\trm -f .git/FETCH_HEAD err &&\n> +\tgit fetch --no-write-fetch-head . 2>err &&\n> +\t! test -f .git/FETCH_HEAD &&\n> +\t! grep FETCH_HEAD err\n\nNice.\n\nWith or without the following squashed in,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\ndiff --git i/builtin/fetch.c w/builtin/fetch.c\nindex c6c46892503..387eeb1ec08 100644\n--- i/builtin/fetch.c\n+++ w/builtin/fetch.c\n@@ -1025,9 +1025,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n-\t\t\t\t * Display fetches written to FETCH_HEAD (or\n-\t\t\t\t * would be written to FETCH_HEAD, if --dry-run\n-\t\t\t\t * is set).\n+\t\t\t\t * Display fetches that wrote to FETCH_HEAD (or,\n+\t\t\t\t * if --dry-run is set, that would write to\n+\t\t\t\t * FETCH_HEAD).\n \t\t\t\t */\n \t\t\t\tformat_display(&note, '*',\n \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n"},{"id":"404951","messageId":"xmqqo8mnaa6k.fsf@gitster.c.googlers.com","threadId":"54180","inReplyTo":"20200902235628.GB4035286@google.com","subject":"Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-03T02:03:31Z","receivedAt":"2020-09-03T02:03:38Z","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>>  builtin/fetch.c          |  8 +++++++-\n>>  t/t0410-partial-clone.sh |  7 +++++--\n>>  t/t5510-fetch.sh         | 18 ++++++++++--------\n>>  3 files changed, 22 insertions(+), 11 deletions(-)\n>\n> Thanks for fixing it, and sorry I didn't catch it during initial\n> review.\n> ...\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 320ba9471d..c6c4689250 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n>>  \t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n>>  \t\t\t\t\t\t       summary_width);\n>>  \t\t\t\tfree(ref);\n>> -\t\t\t} else\n>> +\t\t\t} else if (write_fetch_head || dry_run) {\n>> +\t\t\t\t/*\n>> +\t\t\t\t * Display fetches written to FETCH_HEAD (or\n>> +\t\t\t\t * would be written to FETCH_HEAD, if --dry-run\n>\n> nit: to fix the parallel construction, s/would/that would/ or\n> s/written/that were written/\n\nTrue.\n\n>> +\t\t\t\t * is set).\n>> +\t\t\t\t */\n>>  \t\t\t\tformat_display(&note, '*',\n>>  \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n>>  \t\t\t\t\t       *what ? what : \"HEAD\",\n>>  \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n>> +\t\t\t}\n\nStrictly speaking, I suspect that this is still broken when the user\nsays \"fetch --no-write-fetch-head --dry-run\" in which case we should\nskip this block.\n\nAnd to fix it properly, we would probably need to keep track of\nthree things semi-independently.\n\n - were we told this is a \"dry-run\"? (current 'dry_run' variable)\n\n - were we told not to store fetch-head? (missing)\n\n - after all, are we going to write or not write fetch-head (current\n   'write_fetch_head' variable)\n\nAnd the conditional to protect this block would be fixed to use only\nthe second and new \"have we seen --no-fetch-head on the command\nline?\" variable, and ignore the settings of the dry_run variable, I\nthink.\n\nThanks.\n"},{"id":"404980","messageId":"20200903194148.2738968-1-jonathantanmy@google.com","threadId":"54180","inReplyTo":"xmqqo8mnaa6k.fsf@gitster.c.googlers.com","subject":"[PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-09-03T19:41:48Z","receivedAt":"2020-09-03T19:41:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"887952b8c6 (\"fetch: optionally allow disabling FETCH_HEAD update\",\n2020-08-18) introduced the ability to disable writing to FETCH_HEAD\nduring fetch, but did not suppress the \"<source> -> FETCH_HEAD\" message\nwhen this ability is used. This message is misleading in this case,\nbecause FETCH_HEAD is not written. Also, because \"fetch\" is used to\nlazy-fetch missing objects in a partial clone, this significantly\nclutters up the output in that case since the objects to be fetched are\npotentially numerous.\n\nTherefore, suppress this message when --no-write-fetch-head is passed.\nIn order to do this, a new variable that keeps track of what the user\nsaid explicitly about FETCH_HEAD (which may or may not have the same\nvalue as the existing one tracking whether FETCH_HEAD is written) is\nintroduced.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis is on origin/jt/lazy-fetch.\n\nOK - updated the code, added a test for the \"--dry-run\n--no-write-fetch-head\" case, and updated commit message and code\ncomment.\n---\n builtin/fetch.c          | 21 +++++++++++++++++----\n t/t0410-partial-clone.sh |  7 +++++--\n t/t5510-fetch.sh         | 25 +++++++++++++++++--------\n 3 files changed, 39 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 320ba9471d..9addd1f2d4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -56,6 +56,7 @@ static int prune_tags = -1; /* unspecified */\n #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */\n \n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n+static int user_specified_write_fetch_head = 1;\n static int write_fetch_head = 1;\n static int verbosity, deepen_relative, set_upstream;\n static int progress = -1;\n@@ -164,7 +165,7 @@ static struct option builtin_fetch_options[] = {\n \t\t    PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n \t\t N_(\"dry run\")),\n-\tOPT_BOOL(0, \"write-fetch-head\", &write_fetch_head,\n+\tOPT_BOOL(0, \"write-fetch-head\", &user_specified_write_fetch_head,\n \t\t N_(\"write fetched references to the FETCH_HEAD file\")),\n \tOPT_BOOL('k', \"keep\", &keep, N_(\"keep downloaded pack\")),\n \tOPT_BOOL('u', \"update-head-ok\", &update_head_ok,\n@@ -1023,11 +1024,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n \t\t\t\t\t\t       summary_width);\n \t\t\t\tfree(ref);\n-\t\t\t} else\n+\t\t\t} else if (user_specified_write_fetch_head) {\n+\t\t\t\t/*\n+\t\t\t\t * If the user specified --write-fetch-head\n+\t\t\t\t * (or, equivalently, did not specify\n+\t\t\t\t * --no-write-fetch-head), inform the user that\n+\t\t\t\t * this ref was written to FETCH_HEAD (or, if\n+\t\t\t\t * --dry-run was specified, would have been\n+\t\t\t\t *  written).\n+\t\t\t\t */\n \t\t\t\tformat_display(&note, '*',\n \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n \t\t\t\t\t       *what ? what : \"HEAD\",\n \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n+\t\t\t}\n \t\t\tif (note.len) {\n \t\t\t\tif (verbosity >= 0 && !shown_url) {\n \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n@@ -1822,8 +1832,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\t/* FETCH_HEAD never gets updated in --dry-run mode */\n-\tif (dry_run)\n+\t/*\n+\t * FETCH_HEAD never gets updated in --dry-run mode, nor if user passed\n+\t * --no-write-fetch-head\n+\t */\n+\tif (dry_run || !user_specified_write_fetch_head)\n \t\twrite_fetch_head = 0;\n \n \tif (all) {\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex d681e90640..584a039b85 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '\n '\n \n test_expect_success 'fetching of missing objects' '\n-\trm -rf repo &&\n+\trm -rf repo err &&\n \ttest_create_repo server &&\n \ttest_commit -C server foo &&\n \tgit -C server repack -a -d --write-bitmap-index &&\n@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"origin\" &&\n-\tgit -C repo cat-file -p \"$HASH\" &&\n+\tgit -C repo cat-file -p \"$HASH\" 2>err &&\n+\n+\t# Ensure that no spurious FETCH_HEAD messages are written\n+\t! grep FETCH_HEAD err &&\n \n \t# Ensure that the .promisor file is written, and check that its\n \t# associated packfile contains the object\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 2a1abe91f0..83b2504519 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -543,16 +543,25 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '\n \n '\n \n-test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' '\n-\trm -f .git/FETCH_HEAD &&\n-\tgit fetch --dry-run . &&\n-\t! test -f .git/FETCH_HEAD\n+test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '\n+\trm -f .git/FETCH_HEAD err &&\n+\tgit fetch --dry-run . 2>err &&\n+\t! test -f .git/FETCH_HEAD &&\n+\tgrep FETCH_HEAD err\n '\n \n-test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' '\n-\trm -f .git/FETCH_HEAD &&\n-\tgit fetch --no-write-fetch-head . &&\n-\t! test -f .git/FETCH_HEAD\n+test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '\n+\trm -f .git/FETCH_HEAD err &&\n+\tgit fetch --no-write-fetch-head . 2>err &&\n+\t! test -f .git/FETCH_HEAD &&\n+\t! grep FETCH_HEAD err\n+'\n+\n+test_expect_success '--no-write-fetch-head and --dry-run does not touch FETCH_HEAD, and does not print what would be written' '\n+\trm -f .git/FETCH_HEAD err &&\n+\tgit fetch --dry-run --no-write-fetch-head . 2>err &&\n+\t! test -f .git/FETCH_HEAD &&\n+\t! grep FETCH_HEAD err\n '\n \n test_expect_success '--write-fetch-head gets defeated by --dry-run' '\n-- \n2.28.0.526.ge36021eeef-goog\n\n"},{"id":"404985","messageId":"xmqqy2lq8tka.fsf@gitster.c.googlers.com","threadId":"54180","inReplyTo":"20200903194148.2738968-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-03T21:00:05Z","receivedAt":"2020-09-03T21:00:18Z","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> OK - updated the code, added a test for the \"--dry-run\n> --no-write-fetch-head\" case, and updated commit message and code\n> comment.\n\nUnfortunately our actions crossed X-< and the previous one that was\ngood enough is already in 'master', together with the lazy fetch\ntopic.\n\nLet's turn this into an incremental fix only for \"Ouch, we still say\nFETCH_HEAD when both --dry-run and --no-write-fetch-head are given\"\nbug.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] fetch: fix --dry-run --no-write-fetch-head interaction\nFrom: Jonathan Tan <jonathantanmy@google.com>\n\nRecently we introduced \"--[no-]-write-fetch-head\" option to tell\n\"git fetch\" not to write FETCH_HEAD file.  The command reported that\nFETCH_HEAD file is written, even with the \"--no-write-fetch-head\"\noption.\n\ndb3c293e (fetch: no FETCH_HEAD display if --no-write-fetch-head,\n2020-09-02) tried to squelch this, but the fix was not sufficient.\nBecause we never write the FETCH_HEAD file when \"--dry-run\" is\ngiven, the addition of \"--[no-]write-fetch-head\" option was made by\ndirectly fliping the internal variable 'write_fetch_head' (which\ndefaults to 'on') to 'off' upon seeing \"--dry-run\", which allowed\nthe condition to decide if we write FETCH_HEAD to be a simple\nreference to the variable.  But now, we need to tell if the user\nexplicitly asked \"--no-write-fetch-head\" with \"--dry-run\" to\ndecide when to show the report about FETCH_HEAD correctly.\n\nIntroduce an extra 'user_specified_write_fetch_head' variable, which\nis 'on' by default and is turned 'off' with '--no-write-fetch-head'.\nThe 'write_fetch_head' variable that decides if we actually write\nFETCH_HEAD remains there, retaining its meaning, but use this new\nvariable and 'dry_run' to decide if we report about FETCH_HEAD.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n\n builtin/fetch.c  | 21 ++++++++++++++-------\n t/t5510-fetch.sh |  7 +++++++\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git c/builtin/fetch.c w/builtin/fetch.c\nindex c6c4689250..9addd1f2d4 100644\n--- c/builtin/fetch.c\n+++ w/builtin/fetch.c\n@@ -56,6 +56,7 @@ static int prune_tags = -1; /* unspecified */\n #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */\n \n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n+static int user_specified_write_fetch_head = 1;\n static int write_fetch_head = 1;\n static int verbosity, deepen_relative, set_upstream;\n static int progress = -1;\n@@ -164,7 +165,7 @@ static struct option builtin_fetch_options[] = {\n \t\t    PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n \t\t N_(\"dry run\")),\n-\tOPT_BOOL(0, \"write-fetch-head\", &write_fetch_head,\n+\tOPT_BOOL(0, \"write-fetch-head\", &user_specified_write_fetch_head,\n \t\t N_(\"write fetched references to the FETCH_HEAD file\")),\n \tOPT_BOOL('k', \"keep\", &keep, N_(\"keep downloaded pack\")),\n \tOPT_BOOL('u', \"update-head-ok\", &update_head_ok,\n@@ -1023,11 +1024,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n \t\t\t\t\t\t       summary_width);\n \t\t\t\tfree(ref);\n-\t\t\t} else if (write_fetch_head || dry_run) {\n+\t\t\t} else if (user_specified_write_fetch_head) {\n \t\t\t\t/*\n-\t\t\t\t * Display fetches written to FETCH_HEAD (or\n-\t\t\t\t * would be written to FETCH_HEAD, if --dry-run\n-\t\t\t\t * is set).\n+\t\t\t\t * If the user specified --write-fetch-head\n+\t\t\t\t * (or, equivalently, did not specify\n+\t\t\t\t * --no-write-fetch-head), inform the user that\n+\t\t\t\t * this ref was written to FETCH_HEAD (or, if\n+\t\t\t\t * --dry-run was specified, would have been\n+\t\t\t\t *  written).\n \t\t\t\t */\n \t\t\t\tformat_display(&note, '*',\n \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n@@ -1828,8 +1832,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\t/* FETCH_HEAD never gets updated in --dry-run mode */\n-\tif (dry_run)\n+\t/*\n+\t * FETCH_HEAD never gets updated in --dry-run mode, nor if user passed\n+\t * --no-write-fetch-head\n+\t */\n+\tif (dry_run || !user_specified_write_fetch_head)\n \t\twrite_fetch_head = 0;\n \n \tif (all) {\ndiff --git c/t/t5510-fetch.sh w/t/t5510-fetch.sh\nindex 759aec9305..83b2504519 100755\n--- c/t/t5510-fetch.sh\n+++ w/t/t5510-fetch.sh\n@@ -557,6 +557,13 @@ test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does n\n \t! grep FETCH_HEAD err\n '\n \n+test_expect_success '--no-write-fetch-head and --dry-run does not touch FETCH_HEAD, and does not print what would be written' '\n+\trm -f .git/FETCH_HEAD err &&\n+\tgit fetch --dry-run --no-write-fetch-head . 2>err &&\n+\t! test -f .git/FETCH_HEAD &&\n+\t! grep FETCH_HEAD err\n+'\n+\n test_expect_success '--write-fetch-head gets defeated by --dry-run' '\n \trm -f .git/FETCH_HEAD &&\n \tgit fetch --dry-run --write-fetch-head . &&\n"},{"id":"404986","messageId":"20200903210628.2753574-1-jonathantanmy@google.com","threadId":"54180","inReplyTo":"xmqqy2lq8tka.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-09-03T21:06:28Z","receivedAt":"2020-09-03T21:06:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > OK - updated the code, added a test for the \"--dry-run\n> > --no-write-fetch-head\" case, and updated commit message and code\n> > comment.\n> \n> Unfortunately our actions crossed X-< and the previous one that was\n> good enough is already in 'master', together with the lazy fetch\n> topic.\n> \n> Let's turn this into an incremental fix only for \"Ouch, we still say\n> FETCH_HEAD when both --dry-run and --no-write-fetch-head are given\"\n> bug.\n> \n> Thanks.\n> \n> -- >8 --\n> Subject: [PATCH] fetch: fix --dry-run --no-write-fetch-head interaction\n> From: Jonathan Tan <jonathantanmy@google.com>\n> \n> Recently we introduced \"--[no-]-write-fetch-head\" option to tell\n> \"git fetch\" not to write FETCH_HEAD file.  The command reported that\n> FETCH_HEAD file is written, even with the \"--no-write-fetch-head\"\n> option.\n> \n> db3c293e (fetch: no FETCH_HEAD display if --no-write-fetch-head,\n> 2020-09-02) tried to squelch this, but the fix was not sufficient.\n> Because we never write the FETCH_HEAD file when \"--dry-run\" is\n> given, the addition of \"--[no-]write-fetch-head\" option was made by\n> directly fliping the internal variable 'write_fetch_head' (which\n> defaults to 'on') to 'off' upon seeing \"--dry-run\", which allowed\n> the condition to decide if we write FETCH_HEAD to be a simple\n> reference to the variable.  But now, we need to tell if the user\n> explicitly asked \"--no-write-fetch-head\" with \"--dry-run\" to\n> decide when to show the report about FETCH_HEAD correctly.\n> \n> Introduce an extra 'user_specified_write_fetch_head' variable, which\n> is 'on' by default and is turned 'off' with '--no-write-fetch-head'.\n> The 'write_fetch_head' variable that decides if we actually write\n> FETCH_HEAD remains there, retaining its meaning, but use this new\n> variable and 'dry_run' to decide if we report about FETCH_HEAD.\n> \n> Suggested-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n\nThanks - this looks good.\n"}]}