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

[Junio C Hamano <gitster@pobox.com>]

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 . &&