Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 18, 2021, 18:29 UTC
Message-ID: <f2085c1b-c6aa-0183-e090-8b2da3cc57c0@gmail.com>
In-Reply-To: <CAPSFM5eZ=_yaeNmF_SnL2jx+Tz4HdgROF=SjhbL=UbB75eFHxA@mail.gmail.com>

Hi Charvi

On 17/01/2021 03:39, Charvi Mendiratta wrote:

Show 30 quoted lines

> Hi Phillip and Taylor,
> 
>> [...]
>>>>> @@ -224,7 +223,7 @@ test_expect_success 'auto squash that matches longer sha1' '
>>>>>         git cat-file blob HEAD^:file1 >actual &&
>>>>>         test_cmp expect actual &&
>>>>>         git cat-file commit HEAD^ >commit &&
>>>>> -     grep squash commit >actual &&
>>>>> +     grep "extra para" commit >actual &&
>>>>>         test_line_count = 1 actual
>>>>>    '
>>>>
>>>> Worth checking that "squash" doesn't appear in an uncommented part of
>>>> actual? Or better yet, checking that "# squash ..." _does_ appear.
>>>>
>>>> I.e., that we would leave this as:
>>>>
>>>>       -   grep squash commit >actual &&
>>>>       +   grep "^# squash" commit >actual &&
>>>>       +   grep "extra para" commit >actual &&
>>
>> This test is checking the message that gets committed, not the contents
>> of the file passed to the editor. I like the idea of checking that the
>> squash! line is indeed commented out, but we'd need to test it with
>>
>> grep -v squash
>>
> 
> It seems to me that you suggest to use 'grep -v squash' instead of
> grep "^#squash".

That's correct

> So I added to check the test as here:
> 
>               -   grep squash commit >actual &&
>               +   grep -v "squash" commit >actual &&

There is no need to redirect the output of this one - we don't expect any output and the test will fail if there is so we want to see what the output is.

Show 13 quoted lines

>               +   grep "extra para" commit >actual &&
> 
>> Looking at the changes to the tests in this commit it highlights the
>> fact that I don't think we ever check exactly what the user sees in
>> their editor. We do add such a test for the new `fixup -C` functionality
>> in a later patch but perhaps we should improve the test coverage of the
>> squash message presented to the user before then.
> 
> I agree and in this test  it's now just checking if the commit message body of
> "squash!" i.e  line "extra para", is added in commit message or not. So, I am
> doubtful if the above is the right way to test whether squash! line is commented
> out or not , as "grep "extra para" commit >actual &&" will rewrite the
> 'actual' file.

The test above gives us reassurance that "squash! ..." does not make it into the commit message which is important. We'd want a separate test to check the message that is presented to the user however looking at patch 7 the test 'sequence of fixup, fixup -C & squash --signoff works' checks that a "squash! ..." subject line gets commented out so I wouldn't worry about an additional test here

Best Wishes

Phillip

> Thanks and Regards,
> Charvi
>