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

From: Charvi Mendiratta <charvi077@gmail.com>
To: Phillip Wood <phillip.wood123@gmail.com>
Cc: Taylor Blau <me@ttaylorr.com>, git <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
Date: Sun, 17 Jan 2021 09:09:41 +0530	[thread overview]
Message-ID: <CAPSFM5eZ=_yaeNmF_SnL2jx+Tz4HdgROF=SjhbL=UbB75eFHxA@mail.gmail.com> (raw)
In-Reply-To: <abc9334d-a9ec-a041-aa04-16cb4f01372a@gmail.com>

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". So I added to check the test as here:

             -   grep squash commit >actual &&
             +   grep -v "squash" commit >actual &&
             +   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.

Thanks and Regards,
Charvi