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

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

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
>