Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2023, 07:31 UTC
Message-ID: <xmqqbkmxbort.fsf@gitster.g>
In-Reply-To: <3a2d4559-fce2-80f3-bafd-5eb8ac1a7eff@amd.com>

Luben Tuikov <luben.tuikov@amd.com> writes:

Show 8 quoted lines

>> +test_expect_success $PREREQ "--validate hook supports header argument" '
>> +	write_script my-hooks/sendemail-validate <<-\EOF &&
>> +	if test -s "$2"
>> +	then
>> +		cat "$2" >actual
>> +		exit 1
>> +	fi
>> +	EOF

If "$2" is not given, or an empty "$2" is given, is that an error? I am wondering if the lack of "else" clause (and the hook exits with success when "$2" is an empty file) here is intentional.

>> +	cat actual | replace_variable_fields \
>> +	>actual-headers &&

Do not cat a single file into a pipe. You can instead redirect out of the file to whatever is reading from the pipe. I.e.

	replace_variable_fields <actual >actual-headers &&

>> +	test_cmp expected-headers actual-headers
>> +'

OK. We make sure the presence and the order of the fields in the output just like all the other tests in this file do (which I think may be a bit too much---there is no strong reason to insist that "Subject:" comes before or after "Date:" or is spelled "Subject:" and not "subject:" or "SUBJECT:"---but that is a problem shared with many other existing tests in this file and this patch is not making it much worse).

Show 7 quoted lines

>>  for enc in 7bit 8bit quoted-printable base64
>>  do
>>  	test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '
>
> As Junio and I discussed in the v5 2/2 patch review, here we may want to
> do something like this: Add a custom header to the SMTP envelope and then make
> sure that that is present when the hook checks $2.

Adding a custom header test is also fine, but I am OK with what we see above, to verify the headers just the same way as existing tests.