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

[Phillip Wood <phillip.wood123@gmail.com>]

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
>