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

Hi Charvi

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

Hi Phillip and Taylor,

quoted

[...]

quoted

quoted

quoted

@@ -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 &&

quoted

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