Re: [PATCH v3] commit: restore --edit when combined with --fixup
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Aug 14, 2021, 15:20 UTC
- 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:
Show 54 quoted lines
>
>
> 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"