Re: [PATCH v3 0/2] Allow disabling advice shown after merge conflicts

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> ## 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@xxxxxxxxxxxxx>
      +    Helped-by: Junio C Hamano <gitster@xxxxxxxxx>
           Signed-off-by: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
## 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);






[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux