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

From: Joel Klinghed <redacted>

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.

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.
    
    Signed-off-by: Joel Klinghed the_jk@spawned.biz

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

 builtin/commit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43b..4c5286840c5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -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)
+				use_editor = 0;
 		}
 	}
 

base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
-- 
gitgitgadget

On Wed, Aug 11, 2021 at 01:49:18PM +0000, Joel Klinghed via GitGitGadget wrote:

From: Joel Klinghed <redacted>

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"?

-Peff

On Wed, Aug 11, 2021, at 22:24, Jeff King wrote:

On Wed, Aug 11, 2021 at 01:49:18PM +0000, Joel Klinghed via GitGitGadget wrote:

quoted

From: Joel Klinghed <redacted>

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"?

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.

/JK

On Thu, Aug 12, 2021 at 12:10:28AM +0200, Joel Klinghed wrote:

quoted

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.

Ah, thanks for explaining. That makes sense.

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

This is what I worked up, in case it helps.

diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 38a532d81c..3e4364066f 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh

@@ -940,4 +940,22 @@ EOF
 	)
 '
 
+test_expect_success 'commit --fixup respects --edit' '
+	echo broken >foo.c &&
+	git add foo.c &&
+	git commit -m "wip of foo.c" &&
+	echo fixed >foo.c &&
+	(
+		test_set_editor "$TEST_DIRECTORY/t7500/add-content" &&
+		git commit --fixup HEAD --edit foo.c
+	) &&
+	cat >expect <<-\EOF &&
+	fixup! wip of foo.c
+
+	commit message
+	EOF
+	git log -1 --pretty=format:%B HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done

-Peff

On 2021-08-11 at 22:10:28, Joel Klinghed wrote:

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.

Yeah, I do exactly the same thing.  I want to write a nice explanation
of the change I made for the reviewer, but I don't want to preserve it
for the future.

I recently had a series that was 33 commits after squashing but with 117
before squashing, thanks to a series of very thorough and thoughtful
reviews, so in this kind of scenario, it can really be kind to the
reviewer to help them match up the change with their comment.

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.

Thanks for catching this.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

From: Joel Klinghed <redacted>

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.

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.
    
    Signed-off-by: Joel Klinghed the_jk@spawned.biz

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

Range-diff vs v1:

 1:  b3e44c6ccde ! 1:  0ee926d4149 commit: restore --edit when combined with --fixup
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
       		}
       	}
       
     +
     + ## t/t7500-commit-template-squash-signoff.sh ##
     +@@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -m"something" -m"extra"' '
     + 
     + extra"
     + '
     ++test_expect_success 'commit --fixup --edit' '
     ++	commit_for_rebase_autosquash_setup &&
     ++	cat >e-append <<-\EOF &&
     ++	#!/bin/sh
     ++	sed -e "2a\\
     ++something\\
     ++extra" <"$1" >"$1-"
     ++	mv "$1-" "$1"
     ++	EOF
     ++	chmod 755 e-append &&
     ++	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"


 builtin/commit.c                          |  3 ++-
 t/t7500-commit-template-squash-signoff.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43b..4c5286840c5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -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)
+				use_editor = 0;
 		}
 	}
 

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

@@ -281,6 +281,21 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 
 extra"
 '
+test_expect_success 'commit --fixup --edit' '
+	commit_for_rebase_autosquash_setup &&
+	cat >e-append <<-\EOF &&
+	#!/bin/sh
+	sed -e "2a\\
+something\\
+extra" <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF
+	chmod 755 e-append &&
+	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
-- 
gitgitgadget

From: Joel Klinghed <redacted>

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.

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.
    
    Signed-off-by: Joel Klinghed the_jk@spawned.biz

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

Range-diff vs v2:

 1:  0ee926d4149 ! 1:  6bc5d8bbe61 commit: restore --edit when combined with --fixup
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
       			fixup_commit = fixup_message;
       			fixup_prefix = "fixup";
      -			use_editor = 0;
     -+			if (0 > edit_flag)
     ++			if (edit_flag < 0)
      +				use_editor = 0;
       		}
       	}
     @@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -
       '
      +test_expect_success 'commit --fixup --edit' '
      +	commit_for_rebase_autosquash_setup &&
     -+	cat >e-append <<-\EOF &&
     -+	#!/bin/sh
     ++	write_script e-append <<-\EOF &&
      +	sed -e "2a\\
      +something\\
      +extra" <"$1" >"$1-"
      +	mv "$1-" "$1"
      +	EOF
     -+	chmod 755 e-append &&
      +	EDITOR="./e-append" git commit --fixup HEAD~1 --edit &&
      +	commit_msg_is "fixup! target message subject linesomething
      +extra"


 builtin/commit.c                          |  3 ++-
 t/t7500-commit-template-squash-signoff.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43b..560aecd21b1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -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 (edit_flag < 0)
+				use_editor = 0;
 		}
 	}
 

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

@@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 
 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
+	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
-- 
gitgitgadget

Hi Joel

On 12/08/2021 09:02, Joel Klinghed via GitGitGadget wrote:

From: Joel Klinghed <redacted>

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.

Thanks for fixing this

Commit 494d314a05 ("commit: add amend suboption to --fixup to create 
amend! commit", 2021-03-15) that broke this has the following hunk above 
this change

@@ -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.

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

+	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

On Thu, Aug 12, 2021, at 11:32, Phillip Wood wrote:

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

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

@@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
  
  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

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

From: Joel Klinghed <redacted>

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.

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.
    
    Signed-off-by: Joel Klinghed the_jk@spawned.biz

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

Range-diff vs v3:

 1:  6bc5d8bbe61 ! 1:  0c0cb647e03 commit: restore --edit when combined with --fixup
     @@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -
       '
      +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
     -+	EDITOR="./e-append" git commit --fixup HEAD~1 --edit &&
     ++	EDITOR="printf \"something\nextra\" >>" git commit --fixup HEAD~1 --edit &&
      +	commit_msg_is "fixup! target message subject linesomething
      +extra"
      +'


 builtin/commit.c                          | 3 ++-
 t/t7500-commit-template-squash-signoff.sh | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43b..560aecd21b1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -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 (edit_flag < 0)
+				use_editor = 0;
 		}
 	}
 

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

@@ -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
-- 
gitgitgadget

On 12/08/2021 11:01, Joel Klinghed wrote:

On Thu, Aug 12, 2021, at 11:32, Phillip Wood wrote:

quoted

Hi Joel

@@ -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.

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)?

diff --git a/builtin/commit.c b/builtin/commit.c
index 243c626307..67a84ff6e4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -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."));

@@ -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.

Best wishes

Phillip

quoted

quoted

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

@@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
   
   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

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

On Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:

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.

An alternative would be:
+       if (logfile || have_option_m || use_message || fixup_message)
+               use_editor = 0;
+       if (0 <= edit_flag)
+               use_editor = edit_flag;

That would fix the above cases but in 
"commit: add amend suboption to --fixup to create amend! commit"
the implementation left
git commit --fixup amend:<hash>  // Editor
and I didn't want to change that. But if the default should be no editor
here as well then the above would be a better patch.

/JK

Hi Joel

On 13/08/2021 16:35, Joel Klinghed wrote:

On Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:

quoted

On 12/08/2021 11:01, Joel Klinghed wrote:

quoted

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)?

diff --git a/builtin/commit.c b/builtin/commit.c
index 243c626307..67a84ff6e4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -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."));

@@ -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.

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)

diff --git a/builtin/commit.c b/builtin/commit.c
index 243c626307..7c9b1e7be3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1253,8 +1253,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)

@@ -1344,6 +1342,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 54c2082acb..3fa674e52d 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="printf \"something\nextra\" >>" git commit --fixup HEAD~1 &&
         commit_msg_is "fixup! target message subject line"
  '
  

@@ -281,6 +281,14 @@ 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"

On Sat, Aug 14, 2021, at 17:20, Phillip Wood wrote:

On 13/08/2021 16:35, Joel Klinghed wrote:

quoted

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)

Ah, my bad. I misunderstood and thought your first patch was to
be applied without my fixes. This way is cleaner, no doubt.

Good idea to make --fixup without edit behavior verification explicit.

/JK

From: Joel Klinghed <redacted>

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 <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
-- 
gitgitgadget

Hi Joel

On 14/08/2021 22:40, Joel Klinghed via GitGitGadget wrote:

From: Joel Klinghed <redacted>

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.

This version looks good, thanks for revising it

Best Wishes

Phillip