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

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

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
>