[PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head

[PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Jonathan Tan]

887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
2020-08-18) introduced the ability to disable writing to FETCH_HEAD
during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
when this ability is used. This message is misleading in this case,
because FETCH_HEAD is not written. Also, because "fetch" is used to
lazy-fetch missing objects in a partial clone, this significantly
clutters up the output in that case since the objects to be fetched are
potentially numerous. Therefore, suppress this message when
--no-write-fetch-head is passed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is on origin/jt/lazy-fetch.

This might be important for Stolee's maintenance prefetch patch [1] too
- presumably we don't want to show FETCH_HEAD there, as it would be
misleading and would clutter in the same way (albeit to a lesser
extent).

[1] https://lore.kernel.org/git/da64c51a8182ec13aeed8f0157079fb29a09ee85.1598380599.git.gitgitgadget@gmail.com/
---
 builtin/fetch.c          | 3 ++-
 t/t0410-partial-clone.sh | 7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 320ba9471d..442df05f5a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1023,11 +1023,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				rc |= update_local_ref(ref, what, rm, &note,
 						       summary_width);
 				free(ref);
-			} else
+			} else if (write_fetch_head) {
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
+			}
 			if (note.len) {
 				if (verbosity >= 0 && !shown_url) {
 					fprintf(stderr, _("From %.*s\n"),
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index d681e90640..584a039b85 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
 '
 
 test_expect_success 'fetching of missing objects' '
-	rm -rf repo &&
+	rm -rf repo err &&
 	test_create_repo server &&
 	test_commit -C server foo &&
 	git -C server repack -a -d --write-bitmap-index &&
@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "origin" &&
-	git -C repo cat-file -p "$HASH" &&
+	git -C repo cat-file -p "$HASH" 2>err &&
+
+	# Ensure that no spurious FETCH_HEAD messages are written
+	! grep FETCH_HEAD err &&
 
 	# Ensure that the .promisor file is written, and check that its
 	# associated packfile contains the object
-- 
2.28.0.402.g5ffc5be6b7-goog

Re: [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
> 2020-08-18) introduced the ability to disable writing to FETCH_HEAD
> during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
> when this ability is used. This message is misleading in this case,
> because FETCH_HEAD is not written.

Thanks for noticing, but I wonder if we should keep this for users
of dry-run, which tends to give more output to what _would_ have
been done if it were not dry-run?

> This might be important for Stolee's maintenance prefetch patch [1] too
> - presumably we don't want to show FETCH_HEAD there, as it would be
> misleading and would clutter in the same way (albeit to a lesser
> extent).

Yes, that makes sense.

> Also, because "fetch" is used to
> lazy-fetch missing objects in a partial clone, this significantly
> clutters up the output in that case since the objects to be fetched are
> potentially numerous. Therefore, suppress this message when
> --no-write-fetch-head is passed.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> This is on origin/jt/lazy-fetch.

I think this patch, with possible correction for "we are not writing
FETCH_HEAD but we do want to show what would have been done when the
user asked --dry-run", should be done as a brown-paper-bag bugfix
directly on jc/no-update-fetch-head topic and merged quicly down to
'master', instead of taken hostage of some other topic in flight,
but it would soon already be a week since jt/lazy-fetch was merged
to 'next', so it probably is OK to apply on jt/lazy-fetch, and it
indeed is easier to manage that way.

> [1] https://lore.kernel.org/git/da64c51a8182ec13aeed8f0157079fb29a09ee85.1598380599.git.gitgitgadget@gmail.com/
> ---
>  builtin/fetch.c          | 3 ++-
>  t/t0410-partial-clone.sh | 7 +++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 320ba9471d..442df05f5a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1023,11 +1023,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  				rc |= update_local_ref(ref, what, rm, &note,
>  						       summary_width);
>  				free(ref);
> -			} else
> +			} else if (write_fetch_head) {
>  				format_display(&note, '*',
>  					       *kind ? kind : "branch", NULL,
>  					       *what ? what : "HEAD",
>  					       "FETCH_HEAD", summary_width);
> +			}
>  			if (note.len) {
>  				if (verbosity >= 0 && !shown_url) {
>  					fprintf(stderr, _("From %.*s\n"),
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index d681e90640..584a039b85 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
>  '
>  
>  test_expect_success 'fetching of missing objects' '
> -	rm -rf repo &&
> +	rm -rf repo err &&
>  	test_create_repo server &&
>  	test_commit -C server foo &&
>  	git -C server repack -a -d --write-bitmap-index &&
> @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
>  
>  	git -C repo config core.repositoryformatversion 1 &&
>  	git -C repo config extensions.partialclone "origin" &&
> -	git -C repo cat-file -p "$HASH" &&
> +	git -C repo cat-file -p "$HASH" 2>err &&
> +
> +	# Ensure that no spurious FETCH_HEAD messages are written
> +	! grep FETCH_HEAD err &&

Test also --dry-run, but that perhaps needs to be done outside the
context of partial-clone.  The above "lazy fetching should be silent
and should not bother users with mention of FETCH_HEAD" is good test
in the context of partial-clone, though.

jc/no-update-fetch-head added its own test to t/t5510, and both the
"output lacks FETCH_HEAD when --no-write-fetch-head is given" test
and the "output still mentions FETCH_HEAD with --dry-run" test
belong there.

Thanks.

[PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Jonathan Tan]

887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
2020-08-18) introduced the ability to disable writing to FETCH_HEAD
during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
when this ability is used. This message is misleading in this case,
because FETCH_HEAD is not written. Also, because "fetch" is used to
lazy-fetch missing objects in a partial clone, this significantly
clutters up the output in that case since the objects to be fetched are
potentially numerous.

Therefore, suppress this message when --no-write-fetch-head is passed
(but not when --dry-run is set).

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is on origin/jt/lazy-fetch.

Junio writes [1]:

> Test also --dry-run, but that perhaps needs to be done outside the
> context of partial-clone.  The above "lazy fetching should be silent
> and should not bother users with mention of FETCH_HEAD" is good test
> in the context of partial-clone, though.
>
> jc/no-update-fetch-head added its own test to t/t5510, and both the
> "output lacks FETCH_HEAD when --no-write-fetch-head is given" test
> and the "output still mentions FETCH_HEAD with --dry-run" test
> belong there.

Ah, thanks for catching that. Here's an updated version.

[1] https://lore.kernel.org/git/xmqq7dtcaqob.fsf@gitster.c.googlers.com/
---
 builtin/fetch.c          |  8 +++++++-
 t/t0410-partial-clone.sh |  7 +++++--
 t/t5510-fetch.sh         | 18 ++++++++++--------
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 320ba9471d..c6c4689250 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				rc |= update_local_ref(ref, what, rm, &note,
 						       summary_width);
 				free(ref);
-			} else
+			} else if (write_fetch_head || dry_run) {
+				/*
+				 * Display fetches written to FETCH_HEAD (or
+				 * would be written to FETCH_HEAD, if --dry-run
+				 * is set).
+				 */
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
+			}
 			if (note.len) {
 				if (verbosity >= 0 && !shown_url) {
 					fprintf(stderr, _("From %.*s\n"),
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index d681e90640..584a039b85 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
 '
 
 test_expect_success 'fetching of missing objects' '
-	rm -rf repo &&
+	rm -rf repo err &&
 	test_create_repo server &&
 	test_commit -C server foo &&
 	git -C server repack -a -d --write-bitmap-index &&
@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "origin" &&
-	git -C repo cat-file -p "$HASH" &&
+	git -C repo cat-file -p "$HASH" 2>err &&
+
+	# Ensure that no spurious FETCH_HEAD messages are written
+	! grep FETCH_HEAD err &&
 
 	# Ensure that the .promisor file is written, and check that its
 	# associated packfile contains the object
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 2a1abe91f0..759aec9305 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -543,16 +543,18 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '
 
 '
 
-test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' '
-	rm -f .git/FETCH_HEAD &&
-	git fetch --dry-run . &&
-	! test -f .git/FETCH_HEAD
+test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --dry-run . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	grep FETCH_HEAD err
 '
 
-test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' '
-	rm -f .git/FETCH_HEAD &&
-	git fetch --no-write-fetch-head . &&
-	! test -f .git/FETCH_HEAD
+test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --no-write-fetch-head . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	! grep FETCH_HEAD err
 '
 
 test_expect_success '--write-fetch-head gets defeated by --dry-run' '
-- 
2.28.0.402.g5ffc5be6b7-goog

Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Junio C Hamano]

Thanks; this round looks good.

Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Jonathan Nieder]

Jonathan Tan wrote:

> 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
> 2020-08-18) introduced the ability to disable writing to FETCH_HEAD
> during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
> when this ability is used. This message is misleading in this case,
> because FETCH_HEAD is not written. Also, because "fetch" is used to
> lazy-fetch missing objects in a partial clone, this significantly
> clutters up the output in that case since the objects to be fetched are
> potentially numerous.
>
> Therefore, suppress this message when --no-write-fetch-head is passed
> (but not when --dry-run is set).
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
[...]
>  builtin/fetch.c          |  8 +++++++-
>  t/t0410-partial-clone.sh |  7 +++++--
>  t/t5510-fetch.sh         | 18 ++++++++++--------
>  3 files changed, 22 insertions(+), 11 deletions(-)

Thanks for fixing it, and sorry I didn't catch it during initial
review.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 320ba9471d..c6c4689250 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  				rc |= update_local_ref(ref, what, rm, &note,
>  						       summary_width);
>  				free(ref);
> -			} else
> +			} else if (write_fetch_head || dry_run) {
> +				/*
> +				 * Display fetches written to FETCH_HEAD (or
> +				 * would be written to FETCH_HEAD, if --dry-run

nit: to fix the parallel construction, s/would/that would/ or
s/written/that were written/

> +				 * is set).
> +				 */
>  				format_display(&note, '*',
>  					       *kind ? kind : "branch", NULL,
>  					       *what ? what : "HEAD",
>  					       "FETCH_HEAD", summary_width);
> +			}
>  			if (note.len) {
>  				if (verbosity >= 0 && !shown_url) {
>  					fprintf(stderr, _("From %.*s\n"),
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index d681e90640..584a039b85 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
>  '
>  
>  test_expect_success 'fetching of missing objects' '
> -	rm -rf repo &&
> +	rm -rf repo err &&
>  	test_create_repo server &&
>  	test_commit -C server foo &&
>  	git -C server repack -a -d --write-bitmap-index &&
> @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
>  
>  	git -C repo config core.repositoryformatversion 1 &&
>  	git -C repo config extensions.partialclone "origin" &&
> -	git -C repo cat-file -p "$HASH" &&
> +	git -C repo cat-file -p "$HASH" 2>err &&
> +
> +	# Ensure that no spurious FETCH_HEAD messages are written
> +	! grep FETCH_HEAD err &&

This test feels very specific to the bug being addressed.  That said,
since we *also* have a test below for the generic behavior I don't mind.

>  
>  	# Ensure that the .promisor file is written, and check that its
>  	# associated packfile contains the object
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 2a1abe91f0..759aec9305 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -543,16 +543,18 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '
>  
>  '
>  
> -test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' '
> -	rm -f .git/FETCH_HEAD &&
> -	git fetch --dry-run . &&
> -	! test -f .git/FETCH_HEAD
> +test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '
> +	rm -f .git/FETCH_HEAD err &&
> +	git fetch --dry-run . 2>err &&
> +	! test -f .git/FETCH_HEAD &&
> +	grep FETCH_HEAD err
>  '
>  
> -test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' '
> -	rm -f .git/FETCH_HEAD &&
> -	git fetch --no-write-fetch-head . &&
> -	! test -f .git/FETCH_HEAD
> +test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '
> +	rm -f .git/FETCH_HEAD err &&
> +	git fetch --no-write-fetch-head . 2>err &&
> +	! test -f .git/FETCH_HEAD &&
> +	! grep FETCH_HEAD err

Nice.

With or without the following squashed in,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/builtin/fetch.c w/builtin/fetch.c
index c6c46892503..387eeb1ec08 100644
--- i/builtin/fetch.c
+++ w/builtin/fetch.c
@@ -1025,9 +1025,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*
-				 * Display fetches written to FETCH_HEAD (or
-				 * would be written to FETCH_HEAD, if --dry-run
-				 * is set).
+				 * Display fetches that wrote to FETCH_HEAD (or,
+				 * if --dry-run is set, that would write to
+				 * FETCH_HEAD).
 				 */
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,

Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>>  builtin/fetch.c          |  8 +++++++-
>>  t/t0410-partial-clone.sh |  7 +++++--
>>  t/t5510-fetch.sh         | 18 ++++++++++--------
>>  3 files changed, 22 insertions(+), 11 deletions(-)
>
> Thanks for fixing it, and sorry I didn't catch it during initial
> review.
> ...
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 320ba9471d..c6c4689250 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>>  				rc |= update_local_ref(ref, what, rm, &note,
>>  						       summary_width);
>>  				free(ref);
>> -			} else
>> +			} else if (write_fetch_head || dry_run) {
>> +				/*
>> +				 * Display fetches written to FETCH_HEAD (or
>> +				 * would be written to FETCH_HEAD, if --dry-run
>
> nit: to fix the parallel construction, s/would/that would/ or
> s/written/that were written/

True.

>> +				 * is set).
>> +				 */
>>  				format_display(&note, '*',
>>  					       *kind ? kind : "branch", NULL,
>>  					       *what ? what : "HEAD",
>>  					       "FETCH_HEAD", summary_width);
>> +			}

Strictly speaking, I suspect that this is still broken when the user
says "fetch --no-write-fetch-head --dry-run" in which case we should
skip this block.

And to fix it properly, we would probably need to keep track of
three things semi-independently.

 - were we told this is a "dry-run"? (current 'dry_run' variable)

 - were we told not to store fetch-head? (missing)

 - after all, are we going to write or not write fetch-head (current
   'write_fetch_head' variable)

And the conditional to protect this block would be fixed to use only
the second and new "have we seen --no-fetch-head on the command
line?" variable, and ignore the settings of the dry_run variable, I
think.

Thanks.

[PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Jonathan Tan]

887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
2020-08-18) introduced the ability to disable writing to FETCH_HEAD
during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
when this ability is used. This message is misleading in this case,
because FETCH_HEAD is not written. Also, because "fetch" is used to
lazy-fetch missing objects in a partial clone, this significantly
clutters up the output in that case since the objects to be fetched are
potentially numerous.

Therefore, suppress this message when --no-write-fetch-head is passed.
In order to do this, a new variable that keeps track of what the user
said explicitly about FETCH_HEAD (which may or may not have the same
value as the existing one tracking whether FETCH_HEAD is written) is
introduced.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is on origin/jt/lazy-fetch.

OK - updated the code, added a test for the "--dry-run
--no-write-fetch-head" case, and updated commit message and code
comment.
---
 builtin/fetch.c          | 21 +++++++++++++++++----
 t/t0410-partial-clone.sh |  7 +++++--
 t/t5510-fetch.sh         | 25 +++++++++++++++++--------
 3 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 320ba9471d..9addd1f2d4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -56,6 +56,7 @@ static int prune_tags = -1; /* unspecified */
 #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */
 
 static int all, append, dry_run, force, keep, multiple, update_head_ok;
+static int user_specified_write_fetch_head = 1;
 static int write_fetch_head = 1;
 static int verbosity, deepen_relative, set_upstream;
 static int progress = -1;
@@ -164,7 +165,7 @@ static struct option builtin_fetch_options[] = {
 		    PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
 	OPT_BOOL(0, "dry-run", &dry_run,
 		 N_("dry run")),
-	OPT_BOOL(0, "write-fetch-head", &write_fetch_head,
+	OPT_BOOL(0, "write-fetch-head", &user_specified_write_fetch_head,
 		 N_("write fetched references to the FETCH_HEAD file")),
 	OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")),
 	OPT_BOOL('u', "update-head-ok", &update_head_ok,
@@ -1023,11 +1024,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				rc |= update_local_ref(ref, what, rm, &note,
 						       summary_width);
 				free(ref);
-			} else
+			} else if (user_specified_write_fetch_head) {
+				/*
+				 * If the user specified --write-fetch-head
+				 * (or, equivalently, did not specify
+				 * --no-write-fetch-head), inform the user that
+				 * this ref was written to FETCH_HEAD (or, if
+				 * --dry-run was specified, would have been
+				 *  written).
+				 */
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
+			}
 			if (note.len) {
 				if (verbosity >= 0 && !shown_url) {
 					fprintf(stderr, _("From %.*s\n"),
@@ -1822,8 +1832,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (depth || deepen_since || deepen_not.nr)
 		deepen = 1;
 
-	/* FETCH_HEAD never gets updated in --dry-run mode */
-	if (dry_run)
+	/*
+	 * FETCH_HEAD never gets updated in --dry-run mode, nor if user passed
+	 * --no-write-fetch-head
+	 */
+	if (dry_run || !user_specified_write_fetch_head)
 		write_fetch_head = 0;
 
 	if (all) {
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index d681e90640..584a039b85 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
 '
 
 test_expect_success 'fetching of missing objects' '
-	rm -rf repo &&
+	rm -rf repo err &&
 	test_create_repo server &&
 	test_commit -C server foo &&
 	git -C server repack -a -d --write-bitmap-index &&
@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "origin" &&
-	git -C repo cat-file -p "$HASH" &&
+	git -C repo cat-file -p "$HASH" 2>err &&
+
+	# Ensure that no spurious FETCH_HEAD messages are written
+	! grep FETCH_HEAD err &&
 
 	# Ensure that the .promisor file is written, and check that its
 	# associated packfile contains the object
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 2a1abe91f0..83b2504519 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -543,16 +543,25 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '
 
 '
 
-test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' '
-	rm -f .git/FETCH_HEAD &&
-	git fetch --dry-run . &&
-	! test -f .git/FETCH_HEAD
+test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --dry-run . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	grep FETCH_HEAD err
 '
 
-test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' '
-	rm -f .git/FETCH_HEAD &&
-	git fetch --no-write-fetch-head . &&
-	! test -f .git/FETCH_HEAD
+test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --no-write-fetch-head . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	! grep FETCH_HEAD err
+'
+
+test_expect_success '--no-write-fetch-head and --dry-run does not touch FETCH_HEAD, and does not print what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --dry-run --no-write-fetch-head . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	! grep FETCH_HEAD err
 '
 
 test_expect_success '--write-fetch-head gets defeated by --dry-run' '
-- 
2.28.0.526.ge36021eeef-goog

Re: [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> OK - updated the code, added a test for the "--dry-run
> --no-write-fetch-head" case, and updated commit message and code
> comment.

Unfortunately our actions crossed X-< and the previous one that was
good enough is already in 'master', together with the lazy fetch
topic.

Let's turn this into an incremental fix only for "Ouch, we still say
FETCH_HEAD when both --dry-run and --no-write-fetch-head are given"
bug.

Thanks.

-- >8 --
Subject: [PATCH] fetch: fix --dry-run --no-write-fetch-head interaction
From: Jonathan Tan <jonathantanmy@google.com>

Recently we introduced "--[no-]-write-fetch-head" option to tell
"git fetch" not to write FETCH_HEAD file.  The command reported that
FETCH_HEAD file is written, even with the "--no-write-fetch-head"
option.

db3c293e (fetch: no FETCH_HEAD display if --no-write-fetch-head,
2020-09-02) tried to squelch this, but the fix was not sufficient.
Because we never write the FETCH_HEAD file when "--dry-run" is
given, the addition of "--[no-]write-fetch-head" option was made by
directly fliping the internal variable 'write_fetch_head' (which
defaults to 'on') to 'off' upon seeing "--dry-run", which allowed
the condition to decide if we write FETCH_HEAD to be a simple
reference to the variable.  But now, we need to tell if the user
explicitly asked "--no-write-fetch-head" with "--dry-run" to
decide when to show the report about FETCH_HEAD correctly.

Introduce an extra 'user_specified_write_fetch_head' variable, which
is 'on' by default and is turned 'off' with '--no-write-fetch-head'.
The 'write_fetch_head' variable that decides if we actually write
FETCH_HEAD remains there, retaining its meaning, but use this new
variable and 'dry_run' to decide if we report about FETCH_HEAD.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

 builtin/fetch.c  | 21 ++++++++++++++-------
 t/t5510-fetch.sh |  7 +++++++
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git c/builtin/fetch.c w/builtin/fetch.c
index c6c4689250..9addd1f2d4 100644
--- c/builtin/fetch.c
+++ w/builtin/fetch.c
@@ -56,6 +56,7 @@ static int prune_tags = -1; /* unspecified */
 #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */
 
 static int all, append, dry_run, force, keep, multiple, update_head_ok;
+static int user_specified_write_fetch_head = 1;
 static int write_fetch_head = 1;
 static int verbosity, deepen_relative, set_upstream;
 static int progress = -1;
@@ -164,7 +165,7 @@ static struct option builtin_fetch_options[] = {
 		    PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
 	OPT_BOOL(0, "dry-run", &dry_run,
 		 N_("dry run")),
-	OPT_BOOL(0, "write-fetch-head", &write_fetch_head,
+	OPT_BOOL(0, "write-fetch-head", &user_specified_write_fetch_head,
 		 N_("write fetched references to the FETCH_HEAD file")),
 	OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")),
 	OPT_BOOL('u', "update-head-ok", &update_head_ok,
@@ -1023,11 +1024,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				rc |= update_local_ref(ref, what, rm, &note,
 						       summary_width);
 				free(ref);
-			} else if (write_fetch_head || dry_run) {
+			} else if (user_specified_write_fetch_head) {
 				/*
-				 * Display fetches written to FETCH_HEAD (or
-				 * would be written to FETCH_HEAD, if --dry-run
-				 * is set).
+				 * If the user specified --write-fetch-head
+				 * (or, equivalently, did not specify
+				 * --no-write-fetch-head), inform the user that
+				 * this ref was written to FETCH_HEAD (or, if
+				 * --dry-run was specified, would have been
+				 *  written).
 				 */
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
@@ -1828,8 +1832,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (depth || deepen_since || deepen_not.nr)
 		deepen = 1;
 
-	/* FETCH_HEAD never gets updated in --dry-run mode */
-	if (dry_run)
+	/*
+	 * FETCH_HEAD never gets updated in --dry-run mode, nor if user passed
+	 * --no-write-fetch-head
+	 */
+	if (dry_run || !user_specified_write_fetch_head)
 		write_fetch_head = 0;
 
 	if (all) {
diff --git c/t/t5510-fetch.sh w/t/t5510-fetch.sh
index 759aec9305..83b2504519 100755
--- c/t/t5510-fetch.sh
+++ w/t/t5510-fetch.sh
@@ -557,6 +557,13 @@ test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does n
 	! grep FETCH_HEAD err
 '
 
+test_expect_success '--no-write-fetch-head and --dry-run does not touch FETCH_HEAD, and does not print what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --dry-run --no-write-fetch-head . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	! grep FETCH_HEAD err
+'
+
 test_expect_success '--write-fetch-head gets defeated by --dry-run' '
 	rm -f .git/FETCH_HEAD &&
 	git fetch --dry-run --write-fetch-head . &&

Re: [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > OK - updated the code, added a test for the "--dry-run
> > --no-write-fetch-head" case, and updated commit message and code
> > comment.
> 
> Unfortunately our actions crossed X-< and the previous one that was
> good enough is already in 'master', together with the lazy fetch
> topic.
> 
> Let's turn this into an incremental fix only for "Ouch, we still say
> FETCH_HEAD when both --dry-run and --no-write-fetch-head are given"
> bug.
> 
> Thanks.
> 
> -- >8 --
> Subject: [PATCH] fetch: fix --dry-run --no-write-fetch-head interaction
> From: Jonathan Tan <jonathantanmy@google.com>
> 
> Recently we introduced "--[no-]-write-fetch-head" option to tell
> "git fetch" not to write FETCH_HEAD file.  The command reported that
> FETCH_HEAD file is written, even with the "--no-write-fetch-head"
> option.
> 
> db3c293e (fetch: no FETCH_HEAD display if --no-write-fetch-head,
> 2020-09-02) tried to squelch this, but the fix was not sufficient.
> Because we never write the FETCH_HEAD file when "--dry-run" is
> given, the addition of "--[no-]write-fetch-head" option was made by
> directly fliping the internal variable 'write_fetch_head' (which
> defaults to 'on') to 'off' upon seeing "--dry-run", which allowed
> the condition to decide if we write FETCH_HEAD to be a simple
> reference to the variable.  But now, we need to tell if the user
> explicitly asked "--no-write-fetch-head" with "--dry-run" to
> decide when to show the report about FETCH_HEAD correctly.
> 
> Introduce an extra 'user_specified_write_fetch_head' variable, which
> is 'on' by default and is turned 'off' with '--no-write-fetch-head'.
> The 'write_fetch_head' variable that decides if we actually write
> FETCH_HEAD remains there, retaining its meaning, but use this new
> variable and 'dry_run' to decide if we report about FETCH_HEAD.
> 
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---

Thanks - this looks good.