diff mbox

[v3,0/2] Allow disabling advice shown after merge conflicts

Message ID pull.1682.v3.git.1710623790.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Derrick Stolee via GitGitGadget March 16, 2024, 9:16 p.m. UTC
This series introduces a new config 'advice.mergeConflict' and uses it to
allow disabling the advice shown when 'git rebase', 'git cherry-pick', 'git
revert', 'git rebase --apply' and 'git am' stop because of conflicts.

Thanks everyone for the reviews!

Changes since v2:

 * expanded the commit messages to explain why the tests for 'git rebase' do
   not need to be adjusted
 * adjusted the wording of the new 'advice.mergeConflict' in the doc, as
   suggested by Kristoffer for uniformity with his series which is already
   merged to 'master' (b09a8839a4 (Merge branch
   'kh/branch-ref-syntax-advice', 2024-03-15)).
 * checked all new output manually and consequently adjusted the code in 1/2
   to avoid a lonely 'hint: ' line.
 * adjusted the addition in advice.h in 1/2 to put the new enum
   alphabetically, as noticed by Rubén.
 * added misssing newlines in 2/2 as noticed by Phillip and tweaked by
   Junio.
 * rebased on master (2953d95d40 (The eighth batch, 2024-03-15)), to avoid
   conflicts in 'Documentation/config/advice.txt' due to Kristoffer's merged
   series

Changes since v1:

 * renamed the new advice to 'advice.mergeConflict' to make it non-sequencer
   specific
 * added 2/2 which uses the advice in builtin/am, which covers 'git rebase
   --apply' and 'git am'

Note that the code path where 'git rebase --apply' stops because of
conflicts is not covered by the tests but I tested it manually using this
diff:


Philippe Blain (2):
  sequencer: allow disabling conflict advice
  builtin/am: allow disabling conflict advice

 Documentation/config/advice.txt |  2 ++
 advice.c                        |  1 +
 advice.h                        |  1 +
 builtin/am.c                    | 14 +++++++++-----
 sequencer.c                     | 33 ++++++++++++++++++---------------
 t/t3501-revert-cherry-pick.sh   |  1 +
 t/t3507-cherry-pick-conflict.sh |  2 ++
 t/t4150-am.sh                   |  8 ++++----
 t/t4254-am-corrupt.sh           |  2 +-
 9 files changed, 39 insertions(+), 25 deletions(-)


base-commit: 2953d95d402b6bff1a59c4712f4d46f1b9ea137f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1682

Range-diff vs v2:

 1:  a2ce6fd24c2 ! 1:  6005c1e9890 sequencer: allow disabling conflict advice
     @@ Commit message
          merge conflict through a new config 'advice.mergeConflict', which is
          named generically such that it can be used by other commands eventually.
      
     +    Remove that final '\n' in the first hunk in sequencer.c to avoid an
     +    otherwise empty 'hint: ' line before the line 'hint: Disable this
     +    message with "git config advice.mergeConflict false"' which is
     +    automatically added by 'advise_if_enabled'.
     +
          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
     @@ Commit message
      
          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.
     +    escape them in the added line. Note that t5520-pull.sh, which checks
     +    that we display the advice for 'git rebase' (via 'git pull --rebase')
     +    does not have to be updated because it only greps for a specific line in
     +    the advice message.
      
          Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
      
       ## Documentation/config/advice.txt ##
      @@ Documentation/config/advice.txt: advice.*::
     - 		Advice on how to set your identity configuration when
     - 		your information is guessed from the system username and
     - 		domain name.
     + 		Shown when the user's information is guessed from the
     + 		system username and domain name, to tell the user how to
     + 		set their identity configuration.
      +	mergeConflict::
     -+		Advice shown when various commands stop because of conflicts.
     ++		Shown when various commands stop because of conflicts.
       	nestedTag::
     - 		Advice shown if a user attempts to recursively tag a tag object.
     + 		Shown when a user attempts to recursively tag a tag object.
       	pushAlreadyExists::
      
       ## advice.c ##
     @@ advice.c: static struct {
      
       ## advice.h ##
      @@ advice.h: enum advice_type {
     + 	ADVICE_GRAFT_FILE_DEPRECATED,
       	ADVICE_IGNORED_HOOK,
       	ADVICE_IMPLICIT_IDENTITY,
     - 	ADVICE_NESTED_TAG,
      +	ADVICE_MERGE_CONFLICT,
     + 	ADVICE_NESTED_TAG,
       	ADVICE_OBJECT_NAME_WARNING,
       	ADVICE_PUSH_ALREADY_EXISTS,
     - 	ADVICE_PUSH_FETCH_FIRST,
      
       ## sequencer.c ##
      @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
     @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
       
       	if (msg) {
      -		advise("%s\n", msg);
     -+		advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s\n", msg);
     ++		advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s", msg);
       		/*
       		 * A conflict has occurred but the porcelain
       		 * (typically rebase --interactive) wants to take care
 2:  3235542cc6f ! 2:  73d07c8b6a7 builtin/am: allow disabling conflict advice
     @@ Commit message
          on stderr instead of stdout. In t4150, redirect stdout to 'out' and
          stderr to 'err', since this is less confusing. In t4254, as we are
          testing a specific failure mode of 'git am', simply disable the advice.
     +    Note that we are not testing that this advice is shown in 'git rebase'
     +    for the apply backend since 2ac0d6273f (rebase: change the default
     +    backend from "am" to "merge", 2020-02-15).
      
     +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
      
       ## builtin/am.c ##
     @@ builtin/am.c: static const char *msgnum(const struct am_state *state)
       
      -		printf_ln(_("When you have resolved this problem, run \"%s --continue\"."), cmdline);
      -		printf_ln(_("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline);
     -+		strbuf_addf(&sb, _("When you have resolved this problem, run \"%s --continue\"."), cmdline);
     -+		strbuf_addf(&sb, _("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline);
     ++		strbuf_addf(&sb, _("When you have resolved this problem, run \"%s --continue\".\n"), cmdline);
     ++		strbuf_addf(&sb, _("If you prefer to skip this patch, run \"%s --skip\" instead.\n"), cmdline);
       
       		if (advice_enabled(ADVICE_AM_WORK_DIR) &&
       		    is_empty_or_missing_file(am_path(state, "patch")) &&
       		    !repo_index_has_changes(the_repository, NULL, NULL))
      -			printf_ln(_("To record the empty patch as an empty commit, run \"%s --allow-empty\"."), cmdline);
     -+			strbuf_addf(&sb, _("To record the empty patch as an empty commit, run \"%s --allow-empty\"."), cmdline);
     ++			strbuf_addf(&sb, _("To record the empty patch as an empty commit, run \"%s --allow-empty\".\n"), cmdline);
       
      -		printf_ln(_("To restore the original branch and stop patching, run \"%s --abort\"."), cmdline);
      +		strbuf_addf(&sb, _("To restore the original branch and stop patching, run \"%s --abort\"."), cmdline);

Comments

Junio C Hamano March 18, 2024, 4:31 p.m. UTC | #1
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This series introduces a new config 'advice.mergeConflict' and uses it to
> allow disabling the advice shown when 'git rebase', 'git cherry-pick', 'git
> revert', 'git rebase --apply' and 'git am' stop because of conflicts.
>
> Thanks everyone for the reviews!
>
> Changes since v2:
>
>  * expanded the commit messages to explain why the tests for 'git rebase' do
>    not need to be adjusted
>  * adjusted the wording of the new 'advice.mergeConflict' in the doc, as
>    suggested by Kristoffer for uniformity with his series which is already
>    merged to 'master' (b09a8839a4 (Merge branch
>    'kh/branch-ref-syntax-advice', 2024-03-15)).
>  * checked all new output manually and consequently adjusted the code in 1/2
>    to avoid a lonely 'hint: ' line.
>  * adjusted the addition in advice.h in 1/2 to put the new enum
>    alphabetically, as noticed by Rubén.
>  * added misssing newlines in 2/2 as noticed by Phillip and tweaked by
>    Junio.
>  * rebased on master (2953d95d40 (The eighth batch, 2024-03-15)), to avoid
>    conflicts in 'Documentation/config/advice.txt' due to Kristoffer's merged
>    series

Looking good; will queue.  Thanks.
Phillip Wood March 25, 2024, 10:48 a.m. UTC | #2
Hi Philippe

On 16/03/2024 21:16, Philippe Blain via GitGitGadget wrote:
> Changes since v2:
> 
>   * expanded the commit messages to explain why the tests for 'git rebase' do
>     not need to be adjusted
>   * adjusted the wording of the new 'advice.mergeConflict' in the doc, as
>     suggested by Kristoffer for uniformity with his series which is already
>     merged to 'master' (b09a8839a4 (Merge branch
>     'kh/branch-ref-syntax-advice', 2024-03-15)).
>   * checked all new output manually and consequently adjusted the code in 1/2
>     to avoid a lonely 'hint: ' line.
>   * adjusted the addition in advice.h in 1/2 to put the new enum
>     alphabetically, as noticed by Rubén.
>   * added misssing newlines in 2/2 as noticed by Phillip and tweaked by
>     Junio.
>   * rebased on master (2953d95d40 (The eighth batch, 2024-03-15)), to avoid
>     conflicts in 'Documentation/config/advice.txt' due to Kristoffer's merged >     series
> [...] 
> Note that the code path where 'git rebase --apply' stops because of
> conflicts is not covered by the tests but I tested it manually using this
> diff:
> 
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 47534f1062..34eac2e6f4 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -374,7 +374,7 @@ test_pull_autostash_fail ()
>       echo conflicting >>seq.txt &&
>       test_tick &&
>       git commit -m "Create conflict" seq.txt &&
> -	test_must_fail git pull --rebase . seq 2>err >out &&
> +	test_must_fail git -c rebase.backend=apply pull --rebase . seq 2>err >out &&
>       test_grep "Resolve all conflicts manually" err
>   '

Thanks for being so thorough, this version looks good to me

Best Wishes

Phillip

> 
> Philippe Blain (2):
>    sequencer: allow disabling conflict advice
>    builtin/am: allow disabling conflict advice
> 
>   Documentation/config/advice.txt |  2 ++
>   advice.c                        |  1 +
>   advice.h                        |  1 +
>   builtin/am.c                    | 14 +++++++++-----
>   sequencer.c                     | 33 ++++++++++++++++++---------------
>   t/t3501-revert-cherry-pick.sh   |  1 +
>   t/t3507-cherry-pick-conflict.sh |  2 ++
>   t/t4150-am.sh                   |  8 ++++----
>   t/t4254-am-corrupt.sh           |  2 +-
>   9 files changed, 39 insertions(+), 25 deletions(-)
> 
> 
> base-commit: 2953d95d402b6bff1a59c4712f4d46f1b9ea137f
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1682
> 
> Range-diff vs v2:
> 
>   1:  a2ce6fd24c2 ! 1:  6005c1e9890 sequencer: allow disabling conflict advice
>       @@ Commit message
>            merge conflict through a new config 'advice.mergeConflict', which is
>            named generically such that it can be used by other commands eventually.
>        
>       +    Remove that final '\n' in the first hunk in sequencer.c to avoid an
>       +    otherwise empty 'hint: ' line before the line 'hint: Disable this
>       +    message with "git config advice.mergeConflict false"' which is
>       +    automatically added by 'advise_if_enabled'.
>       +
>            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
>       @@ Commit message
>        
>            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.
>       +    escape them in the added line. Note that t5520-pull.sh, which checks
>       +    that we display the advice for 'git rebase' (via 'git pull --rebase')
>       +    does not have to be updated because it only greps for a specific line in
>       +    the advice message.
>        
>            Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>        
>         ## Documentation/config/advice.txt ##
>        @@ Documentation/config/advice.txt: advice.*::
>       - 		Advice on how to set your identity configuration when
>       - 		your information is guessed from the system username and
>       - 		domain name.
>       + 		Shown when the user's information is guessed from the
>       + 		system username and domain name, to tell the user how to
>       + 		set their identity configuration.
>        +	mergeConflict::
>       -+		Advice shown when various commands stop because of conflicts.
>       ++		Shown when various commands stop because of conflicts.
>         	nestedTag::
>       - 		Advice shown if a user attempts to recursively tag a tag object.
>       + 		Shown when a user attempts to recursively tag a tag object.
>         	pushAlreadyExists::
>        
>         ## advice.c ##
>       @@ advice.c: static struct {
>        
>         ## advice.h ##
>        @@ advice.h: enum advice_type {
>       + 	ADVICE_GRAFT_FILE_DEPRECATED,
>         	ADVICE_IGNORED_HOOK,
>         	ADVICE_IMPLICIT_IDENTITY,
>       - 	ADVICE_NESTED_TAG,
>        +	ADVICE_MERGE_CONFLICT,
>       + 	ADVICE_NESTED_TAG,
>         	ADVICE_OBJECT_NAME_WARNING,
>         	ADVICE_PUSH_ALREADY_EXISTS,
>       - 	ADVICE_PUSH_FETCH_FIRST,
>        
>         ## sequencer.c ##
>        @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
>       @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
>         
>         	if (msg) {
>        -		advise("%s\n", msg);
>       -+		advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s\n", msg);
>       ++		advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s", msg);
>         		/*
>         		 * A conflict has occurred but the porcelain
>         		 * (typically rebase --interactive) wants to take care
>   2:  3235542cc6f ! 2:  73d07c8b6a7 builtin/am: allow disabling conflict advice
>       @@ Commit message
>            on stderr instead of stdout. In t4150, redirect stdout to 'out' and
>            stderr to 'err', since this is less confusing. In t4254, as we are
>            testing a specific failure mode of 'git am', simply disable the advice.
>       +    Note that we are not testing that this advice is shown in 'git rebase'
>       +    for the apply backend since 2ac0d6273f (rebase: change the default
>       +    backend from "am" to "merge", 2020-02-15).
>        
>       +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>       +    Helped-by: Junio C Hamano <gitster@pobox.com>
>            Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>        
>         ## builtin/am.c ##
>       @@ builtin/am.c: static const char *msgnum(const struct am_state *state)
>         
>        -		printf_ln(_("When you have resolved this problem, run \"%s --continue\"."), cmdline);
>        -		printf_ln(_("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline);
>       -+		strbuf_addf(&sb, _("When you have resolved this problem, run \"%s --continue\"."), cmdline);
>       -+		strbuf_addf(&sb, _("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline);
>       ++		strbuf_addf(&sb, _("When you have resolved this problem, run \"%s --continue\".\n"), cmdline);
>       ++		strbuf_addf(&sb, _("If you prefer to skip this patch, run \"%s --skip\" instead.\n"), cmdline);
>         
>         		if (advice_enabled(ADVICE_AM_WORK_DIR) &&
>         		    is_empty_or_missing_file(am_path(state, "patch")) &&
>         		    !repo_index_has_changes(the_repository, NULL, NULL))
>        -			printf_ln(_("To record the empty patch as an empty commit, run \"%s --allow-empty\"."), cmdline);
>       -+			strbuf_addf(&sb, _("To record the empty patch as an empty commit, run \"%s --allow-empty\"."), cmdline);
>       ++			strbuf_addf(&sb, _("To record the empty patch as an empty commit, run \"%s --allow-empty\".\n"), cmdline);
>         
>        -		printf_ln(_("To restore the original branch and stop patching, run \"%s --abort\"."), cmdline);
>        +		strbuf_addf(&sb, _("To restore the original branch and stop patching, run \"%s --abort\"."), cmdline);
>
Junio C Hamano March 25, 2024, 4:57 p.m. UTC | #3
Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Philippe
>
> On 16/03/2024 21:16, Philippe Blain via GitGitGadget wrote:
>> Changes since v2:
>>   * expanded the commit messages to explain why the tests for 'git
>> rebase' do
>>     not need to be adjusted
>>   * adjusted the wording of the new 'advice.mergeConflict' in the doc, as
>>     suggested by Kristoffer for uniformity with his series which is already
>>     merged to 'master' (b09a8839a4 (Merge branch
>>     'kh/branch-ref-syntax-advice', 2024-03-15)).
>>   * checked all new output manually and consequently adjusted the code in 1/2
>>     to avoid a lonely 'hint: ' line.
>>   * adjusted the addition in advice.h in 1/2 to put the new enum
>>     alphabetically, as noticed by Rubén.
>>   * added misssing newlines in 2/2 as noticed by Phillip and tweaked by
>>     Junio.
>>   * rebased on master (2953d95d40 (The eighth batch, 2024-03-15)), to avoid
>>     conflicts in 'Documentation/config/advice.txt' due to Kristoffer's merged >     series
>> [...] Note that the code path where 'git rebase --apply' stops
>> because of
>> conflicts is not covered by the tests but I tested it manually using this
>> diff:
>> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
>> index 47534f1062..34eac2e6f4 100755
>> --- a/t/t5520-pull.sh
>> +++ b/t/t5520-pull.sh
>> @@ -374,7 +374,7 @@ test_pull_autostash_fail ()
>>       echo conflicting >>seq.txt &&
>>       test_tick &&
>>       git commit -m "Create conflict" seq.txt &&
>> -	test_must_fail git pull --rebase . seq 2>err >out &&
>> +	test_must_fail git -c rebase.backend=apply pull --rebase . seq 2>err >out &&
>>       test_grep "Resolve all conflicts manually" err
>>   '
>
> Thanks for being so thorough, this version looks good to me

Yup, these look good.  Let's mark the topic for 'next'.

Thanks, both.
diff mbox

Patch

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 47534f1062..34eac2e6f4 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -374,7 +374,7 @@  test_pull_autostash_fail ()
     echo conflicting >>seq.txt &&
     test_tick &&
     git commit -m "Create conflict" seq.txt &&
-	test_must_fail git pull --rebase . seq 2>err >out &&
+	test_must_fail git -c rebase.backend=apply pull --rebase . seq 2>err >out &&
     test_grep "Resolve all conflicts manually" err
 '