[v2,1/2] sequencer: allow disabling conflict advice

Commit Message

Philippe Blain March 10, 2024, 7:51 p.m. UTC

From: Philippe Blain <[e-mail]>

Allow disabling the advice shown when a squencer operation results in a
merge conflict through a new config 'advice.mergeConflict', which is
named generically such that it can be used by other commands eventually.

Note that we use 'advise_if_enabled' for each message in the second hunk
in sequencer.c, instead of using 'if (show_hints &&
advice_enabled(...)', because the former instructs the user how to
disable the advice, which is more user-friendly.

Update the tests accordingly. Note that the body of the second test in
t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must
escape them in the added line.

Signed-off-by: Philippe Blain <[e-mail]>
---
 Documentation/config/advice.txt |  2 ++
 advice.c                        |  1 +
 advice.h                        |  1 +
 sequencer.c                     | 33 ++++++++++++++++++---------------
 t/t3501-revert-cherry-pick.sh   |  1 +
 t/t3507-cherry-pick-conflict.sh |  2 ++
 6 files changed, 25 insertions(+), 15 deletions(-)

Comments

Kristoffer Haugsbakk March 11, 2024, 10:29 a.m. UTC | #1

Hi Philippe

On Sun, Mar 10, 2024, at 20:51, Philippe Blain via GitGitGadget wrote:
> diff --git a/Documentation/config/advice.txt
> b/Documentation/config/advice.txt
> index c7ea70f2e2e..a1178284b23 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -56,6 +56,8 @@ advice.*::
>  		Advice on how to set your identity configuration when
>  		your information is guessed from the system username and
>  		domain name.
> +	mergeConflict::
> +		Advice shown when various commands stop because of conflicts.

Given that topic kh/branch-ref-syntax-advice is in `next`, maybe this
should be changed to “Shown when”?[1]

Philippe Blain March 16, 2024, 7:33 p.m. UTC | #2

Hi Kristoffer,

Le 2024-03-11 à 06:29, Kristoffer Haugsbakk a écrit :
> Hi Philippe
> 
> On Sun, Mar 10, 2024, at 20:51, Philippe Blain via GitGitGadget wrote:
>> diff --git a/Documentation/config/advice.txt
>> b/Documentation/config/advice.txt
>> index c7ea70f2e2e..a1178284b23 100644
>> --- a/Documentation/config/advice.txt
>> +++ b/Documentation/config/advice.txt
>> @@ -56,6 +56,8 @@ advice.*::
>>  		Advice on how to set your identity configuration when
>>  		your information is guessed from the system username and
>>  		domain name.
>> +	mergeConflict::
>> +		Advice shown when various commands stop because of conflicts.
> 
> Given that topic kh/branch-ref-syntax-advice is in `next`, maybe this
> should be changed to “Shown when”?[1]
> 
> 

Patch

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..a1178284b23 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -56,6 +56,8 @@  advice.*::
 		Advice on how to set your identity configuration when
 		your information is guessed from the system username and
 		domain name.
+	mergeConflict::
+		Advice shown when various commands stop because of conflicts.
 	nestedTag::
 		Advice shown if a user attempts to recursively tag a tag object.
 	pushAlreadyExists::
diff --git a/advice.c b/advice.c
index 6e9098ff089..ecce0f5a803 100644
--- a/advice.c
+++ b/advice.c
@@ -57,6 +57,7 @@  static struct {
 	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated" },
 	[ADVICE_IGNORED_HOOK]				= { "ignoredHook" },
 	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity" },
+	[ADVICE_MERGE_CONFLICT]				= { "mergeConflict" },
 	[ADVICE_NESTED_TAG]				= { "nestedTag" },
 	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning" },
 	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..89117cdeb77 100644
--- a/advice.h
+++ b/advice.h
@@ -26,6 +26,7 @@  enum advice_type {
 	ADVICE_IGNORED_HOOK,
 	ADVICE_IMPLICIT_IDENTITY,
 	ADVICE_NESTED_TAG,
+	ADVICE_MERGE_CONFLICT,
 	ADVICE_OBJECT_NAME_WARNING,
 	ADVICE_PUSH_ALREADY_EXISTS,
 	ADVICE_PUSH_FETCH_FIRST,
diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..d61bbe37c8c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -467,7 +467,7 @@  static void print_advice(struct repository *r, int show_hint,
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		advise("%s\n", msg);
+		advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s\n", msg);
 		/*
 		 * A conflict has occurred but the porcelain
 		 * (typically rebase --interactive) wants to take care
@@ -480,22 +480,25 @@  static void print_advice(struct repository *r, int show_hint,
 
 	if (show_hint) {
 		if (opts->no_commit)
-			advise(_("after resolving the conflicts, mark the corrected paths\n"
-				 "with 'git add <paths>' or 'git rm <paths>'"));
+			advise_if_enabled(ADVICE_MERGE_CONFLICT,
+					  _("after resolving the conflicts, mark the corrected paths\n"
+					    "with 'git add <paths>' or 'git rm <paths>'"));
 		else if (opts->action == REPLAY_PICK)
-			advise(_("After resolving the conflicts, mark them with\n"
-				 "\"git add/rm <pathspec>\", then run\n"
-				 "\"git cherry-pick --continue\".\n"
-				 "You can instead skip this commit with \"git cherry-pick --skip\".\n"
-				 "To abort and get back to the state before \"git cherry-pick\",\n"
-				 "run \"git cherry-pick --abort\"."));
+			advise_if_enabled(ADVICE_MERGE_CONFLICT,
+					  _("After resolving the conflicts, mark them with\n"
+					    "\"git add/rm <pathspec>\", then run\n"
+					    "\"git cherry-pick --continue\".\n"
+					    "You can instead skip this commit with \"git cherry-pick --skip\".\n"
+					    "To abort and get back to the state before \"git cherry-pick\",\n"
+					    "run \"git cherry-pick --abort\"."));
 		else if (opts->action == REPLAY_REVERT)
-			advise(_("After resolving the conflicts, mark them with\n"
-				 "\"git add/rm <pathspec>\", then run\n"
-				 "\"git revert --continue\".\n"
-				 "You can instead skip this commit with \"git revert --skip\".\n"
-				 "To abort and get back to the state before \"git revert\",\n"
-				 "run \"git revert --abort\"."));
+			advise_if_enabled(ADVICE_MERGE_CONFLICT,
+					  _("After resolving the conflicts, mark them with\n"
+					    "\"git add/rm <pathspec>\", then run\n"
+					    "\"git revert --continue\".\n"
+					    "You can instead skip this commit with \"git revert --skip\".\n"
+					    "To abort and get back to the state before \"git revert\",\n"
+					    "run \"git revert --abort\"."));
 		else
 			BUG("unexpected pick action in print_advice()");
 	}
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index aeab689a98d..43c579ea53a 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -170,6 +170,7 @@  test_expect_success 'advice from failed revert' '
 	hint: You can instead skip this commit with "git revert --skip".
 	hint: To abort and get back to the state before "git revert",
 	hint: run "git revert --abort".
+	hint: Disable this message with "git config advice.mergeConflict false"
 	EOF
 	test_commit --append --no-tag "double-add dream" dream dream &&
 	test_must_fail git revert HEAD^ 2>actual &&
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index c88d597b126..f3947b400a3 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -60,6 +60,7 @@  test_expect_success 'advice from failed cherry-pick' '
 	hint: You can instead skip this commit with "git cherry-pick --skip".
 	hint: To abort and get back to the state before "git cherry-pick",
 	hint: run "git cherry-pick --abort".
+	hint: Disable this message with "git config advice.mergeConflict false"
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
@@ -74,6 +75,7 @@  test_expect_success 'advice from failed cherry-pick --no-commit' "
 	error: could not apply \$picked... picked
 	hint: after resolving the conflicts, mark the corrected paths
 	hint: with 'git add <paths>' or 'git rm <paths>'
+	hint: Disable this message with \"git config advice.mergeConflict false\"
 	EOF
 	test_must_fail git cherry-pick --no-commit picked 2>actual &&