commit: restore --edit when combined with --fixup

From: Joel Klinghed <[email protected]>

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.

Quoted 8 lines · Joel Klinghed via GitGitGadget

From: Joel Klinghed <[email protected]>

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.

This is definitely a change in behavior due to 494d314a05 (commit: add amend suboption to --fixup to create amend! commit, 2021-03-15). That was in v2.32.0, so it's not a regression in the upcoming v2.33 which needs to be handled in the next few days.

My inclination is to call it a regression and restore the original behavior. But when I was going to suggest that you add a test, it made me wonder: what would we be testing for?

If the user says "git commit --fixup HEAD --edit", it seems reasonable for them to expect that the editor is run, and that is easy to check. But what are they planning to edit? If they modify the subject line of the commit, it will wreck the "fixup!" mechanism. If they modify the body (which starts blank), it's going to be discarded by the fixup operation.

Is the goal that they might leave notes for themselves, which they can view in the meantime before they run "rebase --autosquash"?

Quoted 3 lines

Is the goal that they might leave notes for themselves, which they can view in the meantime before they run "rebase --autosquash"?

At my work we use "fixup!" when pushing fixes to a review, using the modified body to outline what issue the commit is addressing, helping the reviewers to see intent and not just the end result. All "fixup!" are then ofc. squashed before integration. Same can be achieved with -m but --edit is generally easier to work with in my experience.

I've also once or twice used it to avoid a "fixup!" of a "fixup!" instead of looking up the original target commit hash but that's just me being lazy.

I'll add a test to check for previous behavior.

At my work we use "fixup!" when pushing fixes to a review, using the modified body to outline what issue the commit is addressing, helping the reviewers to see intent and not just the end result. All "fixup!" are then ofc. squashed before integration. Same can be achieved with -m but --edit is generally easier to work with in my experience.

I've also once or twice used it to avoid a "fixup!" of a "fixup!" instead of looking up the original target commit hash but that's just me being lazy.

I'll add a test to check for previous behavior.

builtin/commit.c +1 -0

+				use_editor = 0;
 		}
 	}
 

t/t7500-commit-template-squash-signoff.sh +1 -0

index 7d02f79c0de..d71c7812180 100755
@@ -281,6 +281,21 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 
 extra"
 '
+test_expect_success 'commit --fixup --edit' '

builtin/commit.c static int parse_and_validate_options(int argc, const char *argv[], · line 1336

Quoted 10 lines · Joel Klinghed via GitGitGadget

builtin/commit.c +1 -1

index 190d215d43b..4c5286840c5 100644
@@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		} else {
 			fixup_commit = fixup_message;
 			fixup_prefix = "fixup";
-			use_editor = 0;
+			if (0 > edit_flag)

Writing this as

			if (edit_flag < 0)

makes it far easier to immediately see that we are talking about a nagetive edit_flag.

builtin/commit.c static int parse_and_validate_options(int argc, const char *argv[], · line 1340

Quoted 4 lines

builtin/commit.c +1 -0

+				use_editor = 0;
 		}
 	}
 

Use write_script helper from test-lib-functions.sh here and lose the hardcoded reference to /bin/sh.

Quoted 4 lines

Writing this as

			if (edit_flag < 0)

Quoted 3 lines

makes it far easier to immediately see that we are talking about a nagetive edit_flag.

Agree, I'll change it.
I was unsure of the style and copied from the earlier condition:

	if (0 <= edit_flag)
		use_editor = edit_flag;

From: Joel Klinghed <[email protected]>

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.

builtin/commit.c +0 -0

index 190d215d43b..560aecd21b1 100644

t/t7500-commit-template-squash-signoff.sh +0 -0

index 7d02f79c0de..a48fe859235 100755
@@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 

t/t7500-commit-template-squash-signoff.sh +5 -0

+	EDITOR="./e-append" 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

Hi Joel

(quoted diff) +1 -1

@@ -1170,7 +1206,7 @@ static int parse_and_validate_options(int argc, 
 const char *argv[],
         if (force_author && renew_authorship)
                 die(_("Using both --reset-author and --author does not 
 make sense"));
 
-       if (logfile || have_option_m || use_message || fixup_message)
+       if (logfile || have_option_m || use_message)
                 use_editor = 0;

         if (0 <= edit_flag)
                 use_editor = edit_flag;

I think it should have moved those last two context lines that set use_editor to below the part that you are fixing. Then the use_editor = 0 line that you are changing continues to work as before. (As you see there are quite a few legacy Yoda conditions in this file, nowadays we avoid adding new ones). I'm not sure if it is worth re working this patch to do that, but it does have the advantage of only testing edit_flag once.

Quoted 6 lines

t/t7500-commit-template-squash-signoff.sh +0 -0

index 7d02f79c0de..a48fe859235 100755
@@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 

t/t7500-commit-template-squash-signoff.sh test_expect_success 'commit --fixup -m"something" -m"extra"' ' · line 291

Quoted 10 lines

t/t7500-commit-template-squash-signoff.sh +8 -0

  extra"
  '
+test_expect_success 'commit --fixup --edit' '
+	commit_for_rebase_autosquash_setup &&
+	write_script e-append <<-\EOF &&
+	sed -e "2a\\
+something\\
+extra" <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF

Again I'm not sure it is worth changing it now but for future reference this is a rather complicated way of appending to the commit message. The test file has an example using set_fake_editor() together with FAKE_COMMIT_AMEND just below where you have added this test or you can just have

     EDITOR="echo something extra >>" git commit --fixup=HEAD~1 --edit

Best Wishes

Phillip

Quoted 14 lines · Phillip Wood

Hi Joel

(quoted diff) +1 -1

@@ -1170,7 +1206,7 @@ static int parse_and_validate_options(int argc,
 const char *argv[],
          if (force_author && renew_authorship)
                  die(_("Using both --reset-author and --author does not
 make sense"));
 
-       if (logfile || have_option_m || use_message || fixup_message)
+       if (logfile || have_option_m || use_message)
                  use_editor = 0;

          if (0 <= edit_flag)
                  use_editor = edit_flag;

Quoted 7 lines

I think it should have moved those last two context lines that set use_editor to below the part that you are fixing. Then the use_editor = 0 line that you are changing continues to work as before. (As you see there are quite a few legacy Yoda conditions in this file, nowadays we avoid adding new ones). I'm not sure if it is worth re working this patch to do that, but it does have the advantage of only testing edit_flag once.

I looked at moving the condition to one place but as use_editor = 0 is only set for --fixup if there isn't a suboption specified I didn't want to have to duplicate the check for a suboption when deciding if use_editor should default to zero.

Quoted 24 lines

Quoted 7 lines

t/t7500-commit-template-squash-signoff.sh +0 -0

index 7d02f79c0de..a48fe859235 100755
@@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 

t/t7500-commit-template-squash-signoff.sh test_expect_success 'commit --fixup -m"something" -m"extra"' ' · line 291

Quoted 10 lines

t/t7500-commit-template-squash-signoff.sh +8 -0

   extra"
   '
+test_expect_success 'commit --fixup --edit' '
+	commit_for_rebase_autosquash_setup &&
+	write_script e-append <<-\EOF &&
+	sed -e "2a\\
+something\\
+extra" <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF

Again I'm not sure it is worth changing it now but for future reference this is a rather complicated way of appending to the commit message. The test file has an example using set_fake_editor() together with FAKE_COMMIT_AMEND just below where you have added this test or you can just have

Quoted 6 lines

      EDITOR="echo something extra >>" git commit --fixup=HEAD~1 --edit

Best Wishes

Phillip

Yeah, especially getting sed in a POSIX and BSD compatible mode took some doing. I wanted to have a similar output to the test above this one, with a line break between something and extra, and frankly, my shell-foo lacked for getting either FAKE_COMMIT_AMEND or EDITOR="... >>" to include a newline. But looking at it again, I realize that EDITOR="printf \"something\nextra\" >>" works just fine.

/JK

Quoted 4 lines · Joel Klinghed

I looked at moving the condition to one place but as use_editor = 0 is only set for --fixup if there isn't a suboption specified I didn't want to have to duplicate the check for a suboption when deciding if use_editor should default to zero.

I don't think you need to duplicate the check for a suboption, can't you just do this on top of master (i.e without you patch applied)?

builtin/commit.c +0 -5

index 243c626307..67a84ff6e4 100644
@@ -1251,11 +1251,6 @@ static int parse_and_validate_options(int argc, 
 const char *argv[],
         if (force_author && renew_authorship)
                 die(_("Using both --reset-author and --author does not 
 make sense"));
 
-       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)
                 die(_("You have nothing to amend."));

builtin/commit.c +5 -0

@@ -1344,6 +1339,11 @@ 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;
+
         cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
 

         handle_untracked_files_arg(s);

I chose to move the other clause that sets use_editor as well so they stay together.

Quoted 41 lines · Phillip Wood

Quoted 4 lines · Joel Klinghed

I looked at moving the condition to one place but as use_editor = 0 is only set for --fixup if there isn't a suboption specified I didn't want to have to duplicate the check for a suboption when deciding if use_editor should default to zero.

I don't think you need to duplicate the check for a suboption, can't you just do this on top of master (i.e without you patch applied)?

builtin/commit.c +0 -5

index 243c626307..67a84ff6e4 100644
@@ -1251,11 +1251,6 @@ static int parse_and_validate_options(int argc,
 const char *argv[],
          if (force_author && renew_authorship)
                  die(_("Using both --reset-author and --author does not
 make sense"));
 
-       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)
                  die(_("You have nothing to amend."));

builtin/commit.c +5 -0

@@ -1344,6 +1339,11 @@ 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;
+
          cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
 

          handle_untracked_files_arg(s);

Quoted 3 lines

I chose to move the other clause that sets use_editor as well so they stay together.

With the above change use_editor no longer defaults to 0 for --fixup as it used to do.
My expected behavior (based on old versions):
git commit --fixup <hash> /// No editor
git commit --fixup <hash> --edit /// Editor
As far as I can see your change would display an editor in both cases.

Quoted 7 lines · Joel Klinghed

With the above change use_editor no longer defaults to 0 for --fixup as it used to do.
My expected behavior (based on old versions):
git commit --fixup <hash> /// No editor
git commit --fixup <hash> --edit /// Editor
As far as I can see your change would display an editor in both cases.

I've just tested it and it works as expected. However moving the 'if (logfile...)' breaks the test "commit --squash works with -c" so we need to just move the second if clause. This is what I have on top of master (i.e. without your patch so a plain fixup is still setting use_editor=0)

t/t7500-commit-template-squash-signoff.sh +0 -0

index 54c2082acb..3fa674e52d 100755
@@ -270,7 +270,7 @@ EOF
 
  test_expect_success 'commit --fixup provides correct one-line commit 

message' '

         commit_for_rebase_autosquash_setup &&

t/t7500-commit-template-squash-signoff.sh +1 -1

-       git commit --fixup HEAD~1 &&
+       EDITOR="printf \"something\nextra\" >>" git commit --fixup 

HEAD~1 &&
commit_msg_is "fixup! target message subject line"
'

From: Joel Klinghed <[email protected]>

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 moving the edit flag condition later in the method.

Signed-off-by Joel Klinghed <[email protected]>

---
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 [email protected]

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 <[email protected]>
       
        ## 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(-)

builtin/commit.c +3 -2

index 190d215d43b..854903ad113 100644
@@ -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);

t/t7500-commit-template-squash-signoff.sh +8 -1

index 7d02f79c0de..8515736003a 100755
@@ -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