From: Phillip Wood Date: Mon, 18 Jan 2021 18:29:26 GMT Subject: Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message Message-ID: In-Reply-To: Hi Charvi On 17/01/2021 03:39, Charvi Mendiratta wrote: > 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. > + 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 >