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

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

[Joel Klinghed via GitGitGadget]

From: Joel Klinghed <the_jk@spawned.biz>

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

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

[Jeff King]

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

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

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

[Joel Klinghed]

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

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

[Jeff King]

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

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Joel Klinghed via GitGitGadget]

From: Joel Klinghed <the_jk@spawned.biz>

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

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

[Junio C Hamano]

"Joel Klinghed via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

Writing this as

			if (edit_flag < 0)

makes it far easier to immediately see that we are talking about a
nagetive 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 &&

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

> +	EDITOR="./e-append" git commit --fixup HEAD~1 --edit &&
> +	commit_msg_is "fixup! target message subject linesomething
> +extra"
> +'


Thanks.

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

[Joel Klinghed]

On Thu, Aug 12, 2021, at 07:21, Junio C Hamano wrote:
> "Joel Klinghed via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > 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)
> 
> Writing this as
> 
> 			if (edit_flag < 0)
> 
> 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;

> > +				use_editor = 0;
> >  		}
> >  	}
> >  

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

Ah, missed that one.

Thanks.

/JK

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

[Junio C Hamano]

"Joel Klinghed" <the_jk@spawned.biz> writes:

>> Writing this as
>> 
>> 			if (edit_flag < 0)
>> 
>> 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;

There are two valid schools of thought when it comes to comparison.

Some folks consider that a comparison between a variable and a
constant is a statement about the variable, hence the expression
should be

		if (VARIRABLE comparison-operator CONSTANT)

They will write things like:

		if (edit_flag >= 0)
		if (edit_flag < 0)

Other folks consider that textual order of the comparison should
match the actual order of the things being compared, as if they are
arranged on a number line, hence the expression should be

		if (SMALLER < LARGER)

no matter which one is variable and which one is constant.

They will write:

		if (0 <= edit_flag)
		if (edit_flag < 0)

The case in question, asserting that edit_flag is negative, is what
both camps agree how to write.

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

[Joel Klinghed via GitGitGadget]

From: Joel Klinghed <the_jk@spawned.biz>

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

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

[Phillip Wood]

Hi Joel

On 12/08/2021 09:02, Joel Klinghed via GitGitGadget wrote:
> From: Joel Klinghed <the_jk@spawned.biz>
> 
> 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

> 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;
>   		}
>   	}
>   

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.

> 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

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

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

[Joel Klinghed]

On Thu, Aug 12, 2021, at 11:32, Phillip Wood wrote:
> 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.
 
> > 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

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

[Phillip Wood]

On 12/08/2021 11:01, Joel Klinghed wrote:
> On Thu, Aug 12, 2021, at 11:32, Phillip Wood wrote:
>> 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
>   
>>> 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
> 

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

[Joel Klinghed]

On Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:
> On 12/08/2021 11:01, Joel Klinghed 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.
> 
> 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.

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

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

[Phillip Wood]

Hi Joel

On 13/08/2021 16:35, Joel Klinghed wrote:
> 
> 
> On Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:
>> On 12/08/2021 11:01, Joel Klinghed 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.
>>
>> 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"

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

[Joel Klinghed]

On Sat, Aug 14, 2021, at 17:20, Phillip Wood wrote:
> On 13/08/2021 16:35, Joel Klinghed 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.
> 
> 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.

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

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

/JK

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

[Joel Klinghed via GitGitGadget]

From: Joel Klinghed <the_jk@spawned.biz>

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

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

[Joel Klinghed via GitGitGadget]

From: Joel Klinghed <the_jk@spawned.biz>

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 <the_jk@spawned.biz>
---
    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 <the_jk@spawned.biz>
      
       ## 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

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

[Phillip Wood]

Hi Joel

On 14/08/2021 22:40, Joel Klinghed via GitGitGadget wrote:
> From: Joel Klinghed <the_jk@spawned.biz>
> 
> 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

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