Three t4150 tests does not work as expected

Three t4150 tests does not work as expected

[Marcel Telka]

Hi,

I noticed that the following three t4150 tests probably does not work as
expected:

- record as an empty commit when meeting e-mail message that lacks a patch
- record an empty patch as an empty commit in the middle of an am session
- create an non-empty commit when the index IS changed though "--allow-empty" is given

All of them does something like:

	git show HEAD --format="%B" >actual
	grep -f actual expected

While the 'actual' file usually contains something like (four lines
between the 'cut' markers):

-------------------------- cut --------------------------
empty commit

--
2.45.1

-------------------------- cut --------------------------

IOW, there are two empty lines there.  Because of that the grep gets an
empty RE from the 'actual' file and so it matches everything that comes
in the 'expected' file.

Is this the expected behavior?


Thank you.

-- 
+-------------------------------------------+
| Marcel Telka   e-mail:   marcel@telka.sk  |
|                homepage: http://telka.sk/ |
+-------------------------------------------+

Re: Three t4150 tests does not work as expected

[Junio C Hamano]

Marcel Telka <marcel@telka.sk> writes:

> All of them does something like:
>
> 	git show HEAD --format="%B" >actual
> 	grep -f actual expected

Wow, these commands look nonsensical in as many counts as there are
lines in there X-< [*].

It seems that they came from two patches, namely

 7c096b8d (am: support --empty=<option> to handle empty patches, 2021-12-09)
 9e7e41bf (am: support --allow-empty to record specific empty patches, 2021-12-09)

Most likely the author of these lines were confused and meant to use
test_cmp (which by the way should always compare actual against
expect, i.e. "test_cmp expect actual", because when the actual
output differs, that is the order of the diff we want to see in the
output to diagnose the breakage).  I especially suspect that the
author did not mean "read patterns from this file", but was confused
to think that 'f' implies "match literally" somehow (the correct
spelling of that fixed-strings option is -F but still it does the
wrong thing reading from the file---it does not even ensure that
all patterns listed in the file matches).

I am not saying (as I did not check) that doing just "grep -f A B"
-> "test_cmp B A" woudl make things work.  I do not think I trust
that these tests added by the same patch have a sane expectations
X-<.  The outcome these tests try to verify may also need to be
corrected.


[Footnote] 

 * To start with, any dashed option like --format=%B should come
   before a revision argument HEAD.  There are many other
   antipatterns in this test script, which should be cleaned up.