Re: [PATCH v5] commit: restore --edit when combined with --fixup

Signed-off-by: Joel Klinghed <redacted>
---
     commit: restore --edit when combined with --fixup
     
     Recent changes to --fixup, adding amend suboption, caused the --edit
     flag to be ignored as use_editor was always set to zero.
     
     Restore edit_flag having higher priority than fixup_message when
     deciding the value of use_editor by only changing the default if
     edit_flag is not set.
     
     Changes since v1: Added test verifying that --fixup --edit brings up
     editor.
     
     Changes since v2: Clarify if condition and use write_script helper in
     test.
     
     Changes since v3: Simplify test.
     
     Changes since v4: Using cleaner fix by Phillip Wood instead and added an
     explicit verification to a test for --fixup without --edit.
     
     Signed-off-by: Joel Klinghed the_jk@spawned.biz

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1014%2Fthejk%2Ffixup_edit-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1014

Range-diff vs v4:

  1:  0c0cb647e03 ! 1:  1c608daf0cd commit: restore --edit when combined with --fixup
      @@ Commit message
           --edit flag to be ignored as use_editor was always set to zero.
       
           Restore edit_flag having higher priority than fixup_message when
      -    deciding the value of use_editor by only changing the default
      -    if edit_flag is not set.
      +    deciding the value of use_editor by moving the edit flag condition
      +    later in the method.
       
           Signed-off-by: Joel Klinghed [off-list ref]
       
        ## builtin/commit.c ##
       @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
      - 		} else {
      - 			fixup_commit = fixup_message;
      - 			fixup_prefix = "fixup";
      --			use_editor = 0;
      -+			if (edit_flag < 0)
      -+				use_editor = 0;
      +
      + 	if (logfile || have_option_m || use_message)
      + 		use_editor = 0;
      +-	if (0 <= edit_flag)
      +-		use_editor = edit_flag;
      +
      + 	/* Sanity check options */
      + 	if (amend && !current_head)
      +@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
        		}
        	}
        
      ++	if (0 <= edit_flag)
      ++		use_editor = edit_flag;
      ++
      + 	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
      +
      + 	handle_untracked_files_arg(s);
       
        ## t/t7500-commit-template-squash-signoff.sh ##
      +@@ t/t7500-commit-template-squash-signoff.sh: EOF
      +
      + test_expect_success 'commit --fixup provides correct one-line commit message' '
      + 	commit_for_rebase_autosquash_setup &&
      +-	git commit --fixup HEAD~1 &&
      ++	EDITOR="echo ignored >>" git commit --fixup HEAD~1 &&
      + 	commit_msg_is "fixup! target message subject line"
      + '
      +
       @@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -m"something" -m"extra"' '
        
        extra"


  builtin/commit.c                          | 5 +++--
  t/t7500-commit-template-squash-signoff.sh | 9 ++++++++-
  2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43b..854903ad113 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1246,8 +1246,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
  
  	if (logfile || have_option_m || use_message)
  		use_editor = 0;
-	if (0 <= edit_flag)
-		use_editor = edit_flag;
  
  	/* Sanity check options */
  	if (amend && !current_head)

@@ -1337,6 +1335,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
  		}
  	}
  
+	if (0 <= edit_flag)
+		use_editor = edit_flag;
+
  	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
  
  	handle_untracked_files_arg(s);

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 7d02f79c0de..8515736003a 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh

@@ -270,7 +270,7 @@ EOF
  
  test_expect_success 'commit --fixup provides correct one-line commit message' '
  	commit_for_rebase_autosquash_setup &&
-	git commit --fixup HEAD~1 &&
+	EDITOR="echo ignored >>" git commit --fixup HEAD~1 &&
  	commit_msg_is "fixup! target message subject line"
  '
  

@@ -281,6 +281,13 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
  
  extra"
  '
+test_expect_success 'commit --fixup --edit' '
+	commit_for_rebase_autosquash_setup &&
+	EDITOR="printf \"something\nextra\" >>" git commit --fixup HEAD~1 --edit &&
+	commit_msg_is "fixup! target message subject linesomething
+extra"
+'
+
  get_commit_msg () {
  	rev="$1" &&
  	git log -1 --pretty=format:"%B" "$rev"

base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb