From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sat, 14 Aug 2021 15:20:37 GMT
Subject: Re: [PATCH v3] commit: restore --edit when combined with --fixup
Message-ID: <ec5f6698-46e9-c8c8-057d-b04851cb9265@gmail.com>
In-Reply-To: <1fc066c5-a085-4865-9eb9-853dfcbe33c2@www.fastmail.com>

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"