From: Fabian Stelzer <fs@gigacodes.de>
Date: Thu, 03 Mar 2022 11:51:14 GMT
Subject: Re: [PATCH v3 1/3] gpg-interface/gpgsm: fix for v2.3
Message-ID: <20220303115114.7pbnggjccku44wqa@fs>
In-Reply-To: <xmqqh78gkvsm.fsf@gitster.g>

On 02.03.2022 11:18, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> writes:
>
>> Checking if signing was successful will now accept '[GNUPG]:
>> SIG_CREATED' on any beginning of a line. Not just explictly the second
>> one anymore.
>
>"the second or subsequent one", I would think, but the code change
>looks correct anyway.
>
>> Switch to gpg's `--with-colons` output format to make
>> parsing more robust.  This avoids issues where the
>> human-readable output from gpg commands changes.
>
>Does this refer only to how parsing in tests is done?

If only refers to the test prerequisite actually. I'll update the message.

>
>> Adjust error messages checking in tests for v2.3 specific output changes.
>
>Does this refer only to the change to 4202 where "failed to find
>the" and the colon after "certificate" are made optional, so that
>the regexp can read messages from both pre- and post-2.3 versions?
>
>> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
>> index 3e7ee1386a..6bc083ca77 100644
>> --- a/t/lib-gpg.sh
>> +++ b/t/lib-gpg.sh
>> @@ -72,12 +72,10 @@ test_lazy_prereq GPGSM '
>>  		--passphrase-fd 0 --pinentry-mode loopback \
>>  		--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
>>
>> -	gpgsm --homedir "${GNUPGHOME}" -K |
>> -	grep fingerprint: |
>> -	cut -d" " -f4 |
>> -	tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" &&
>> +	gpgsm --homedir "${GNUPGHOME}" -K --with-colons |
>> +	awk -F ":" "/^fpr:/ {printf \"%s S relax\\n\", \$10}" \
>> +		>"${GNUPGHOME}/trustlist.txt" &&
>
>The old iteration had (fpr|fingerprint) which appeared as if it were
>catering to both pre- and post-2.3 versions, but "with colons", all
>versions we care about would say "fpr" and that is the reason why we
>no longer have such an alternative here?  Just checking my
>understanding.

Correct. The `with-colons` always uses fpr pre and post 2.3

>
>> -	echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
>
>This removal is because...?  I do not recall seeing the explanation
>in the proposed log message.

Switching to awk allows us to integrate this trailing info into the awk 
expression itself making this extra echo unnecessary.

>
>>  	echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
>>  	       -u committer@example.com -o /dev/null --sign -
>>  '
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 55fac64446..d599bf4b11 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -2037,7 +2037,7 @@ test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 miss
>>  	git merge --no-ff -m msg signed_tag_x509_nokey &&
>>  	GNUPGHOME=. git log --graph --show-signature -n1 plain-x509-nokey >actual &&
>>  	grep "^|\\\  merged tag" actual &&
>> -	grep "^| | gpgsm: certificate not found" actual
>> +	grep -Ei "^| | gpgsm:( failed to find the)? certificate:? not found" actual
>>  '
>
>OK.  It might be easier to read if we give two expressions
>separately and say "we can take either of these", i.e.
>
>	# the former is from pre-2.3, the latter is from 2.3 and later
>	grep -e "^| | gpgsm: certificate not found" \
>	     -e "^| | gpgsm: failed to find the certificate: not found" \
>	     actual
>
>Thanks for working on this update.

Easy enough. Initially I used a subshell and 2 grep calls but this is 
obviously easier. I prefer the static strings over the regex as well.

I'll send a new patch probably tomorrow and try to improve the commit 
message.

Thanks