[v2,1/3] fetch set_head: move warn advice into advise_if_enabled

Commit Message

Bence Ferdinandy Dec. 4, 2024, 10:39 a.m. UTC

Advice about what to do when getting a warning is typed out explicitly
twice and is printed as regular output. The output is also tested for.
Extract the advice message into a single place and use a wrapper
function, so if later the advice is made more chatty the signature only
needs to be changed in once place. Remove the testing for the advice
output in the tests.

Signed-off-by: Bence Ferdinandy <[e-mail]>
---

Notes:
    v2: new patch, with the refactor moved out from the following one

 advice.c         |  1 +
 advice.h         |  1 +
 builtin/fetch.c  | 17 +++++++++++++----
 t/t5510-fetch.sh |  2 --
 4 files changed, 15 insertions(+), 6 deletions(-)

Comments

Junio C Hamano Dec. 4, 2024, 7:28 p.m. UTC | #1

Bence Ferdinandy <[e-mail]> writes:

> +static void set_head_advice_msg(const char *remote, const char *head_name)
> +{
> +	const char message_advice_set_head[] =
> +	N_("Run 'git remote set-head %s %s' to follow the change, or set\n"
> +	   "'remote.%s.followRemoteHEAD' configuration option to a different value\n"
> +	   "if you do not want to see this message.");
> +
> +	advise_if_enabled(ADVICE_FETCH_SET_HEAD_WARN, _(message_advice_set_head),
> +			remote, head_name, remote);
> +}
> ...
>  	else if (updateres && buf_prev->len) {
>  		printf("'HEAD' at '%s' is '%s', "
>  			"but we have a detached HEAD pointing to '%s' locally.\n",
>  			remote, head_name, buf_prev->buf);
> -		printf("Run 'git remote set-head %s %s' to follow the change.\n",
> -			remote, head_name);
> +		set_head_advice_msg(remote, head_name);
>  	}
>  	strbuf_release(&buf_prefix);
>  }

Looking good.

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index ccb74428bc..5d7ee1b550 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -123,7 +123,6 @@ test_expect_success "fetch test followRemoteHEAD warn no change" '
>  		git fetch >output &&
>  		echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
>  			"but we have ${SQ}other${SQ} locally." >expect &&
> -		echo "Run ${SQ}git remote set-head origin main${SQ} to follow the change." >>expect &&
>  		test_cmp expect output &&

OK.  We just lose this part of the test, not because the advice
messages are squelched but because they go to the standard error
stream.  An obvious alternative is to capture it in a separate
file and grep, i.e.

	git fetch >output 2>error &&
	echo ... >expect &&
	test_grep "Run ${SQ}git remote set-head" error &&
	test_cmp expect output

If we were testing that the advice mechanism is used, do that with
and without the ADVICE_FETCH_SET_HEAD_WARN squelched, but probably
it is a bit too much, in the sense that we are not in the business
of testing that the advice mechanism works correctly in this series.

Patch

diff --git a/advice.c b/advice.c
index 6b879d805c..66461fdce9 100644
--- a/advice.c
+++ b/advice.c
@@ -53,6 +53,7 @@  static struct {
 	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
 	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
 	[ADVICE_DIVERGING]				= { "diverging" },
+	[ADVICE_FETCH_SET_HEAD_WARN]			= { "fetchRemoteHEADWarn" },
 	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates" },
 	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch" },
 	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated" },
diff --git a/advice.h b/advice.h
index d7466bc0ef..cf2284ec43 100644
--- a/advice.h
+++ b/advice.h
@@ -20,6 +20,7 @@  enum advice_type {
 	ADVICE_COMMIT_BEFORE_MERGE,
 	ADVICE_DETACHED_HEAD,
 	ADVICE_DIVERGING,
+	ADVICE_FETCH_SET_HEAD_WARN,
 	ADVICE_FETCH_SHOW_FORCED_UPDATES,
 	ADVICE_FORCE_DELETE_BRANCH,
 	ADVICE_GRAFT_FILE_DEPRECATED,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 62769d1686..087beb4c92 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1579,6 +1579,17 @@  static const char *strip_refshead(const char *name){
 	return name;
 }
 
+static void set_head_advice_msg(const char *remote, const char *head_name)
+{
+	const char message_advice_set_head[] =
+	N_("Run 'git remote set-head %s %s' to follow the change, or set\n"
+	   "'remote.%s.followRemoteHEAD' configuration option to a different value\n"
+	   "if you do not want to see this message.");
+
+	advise_if_enabled(ADVICE_FETCH_SET_HEAD_WARN, _(message_advice_set_head),
+			remote, head_name, remote);
+}
+
 static void report_set_head(const char *remote, const char *head_name,
 			struct strbuf *buf_prev, int updateres) {
 	struct strbuf buf_prefix = STRBUF_INIT;
@@ -1590,15 +1601,13 @@  static void report_set_head(const char *remote, const char *head_name,
 	if (prev_head && strcmp(prev_head, head_name)) {
 		printf("'HEAD' at '%s' is '%s', but we have '%s' locally.\n",
 			remote, head_name, prev_head);
-		printf("Run 'git remote set-head %s %s' to follow the change.\n",
-			remote, head_name);
+		set_head_advice_msg(remote, head_name);
 	}
 	else if (updateres && buf_prev->len) {
 		printf("'HEAD' at '%s' is '%s', "
 			"but we have a detached HEAD pointing to '%s' locally.\n",
 			remote, head_name, buf_prev->buf);
-		printf("Run 'git remote set-head %s %s' to follow the change.\n",
-			remote, head_name);
+		set_head_advice_msg(remote, head_name);
 	}
 	strbuf_release(&buf_prefix);
 }
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ccb74428bc..5d7ee1b550 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -123,7 +123,6 @@  test_expect_success "fetch test followRemoteHEAD warn no change" '
 		git fetch >output &&
 		echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
 			"but we have ${SQ}other${SQ} locally." >expect &&
-		echo "Run ${SQ}git remote set-head origin main${SQ} to follow the change." >>expect &&
 		test_cmp expect output &&
 		head=$(git rev-parse refs/remotes/origin/HEAD) &&
 		branch=$(git rev-parse refs/remotes/origin/other) &&
@@ -160,7 +159,6 @@  test_expect_success "fetch test followRemoteHEAD warn detached" '
 		echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \
 			"but we have a detached HEAD pointing to" \
 			"${SQ}${HEAD}${SQ} locally." >expect &&
-		echo "Run ${SQ}git remote set-head origin main${SQ} to follow the change." >>expect &&
 		test_cmp expect output
 	)
 '