Re: [PATCH v3] send-pack: run GPG after atomic push checking

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2020, 23:02 UTC
Message-ID: <xmqqpn6hid5c.fsf@gitster.c.googlers.com>
In-Reply-To: <20200919144750.95812-1-hanxin.hx@alibaba-inc.com>

"Han Xin" <hanxin.hx@alibaba-inc.com> writes:

Show 8 quoted lines

> The refs update commands can be sent to the server side in two different
> ways: GPG-signed or unsigned.  We should run these two operations in the
> same "Finally, tell the other end!" code block, but they are seperated
> by the "Clear the status for each ref" code block.  This will result in
> a slight performance loss, because the failed atomic push will still
> perform unnecessary preparations for shallow advertise and GPG-signed
> commands buffers, and user may have to be bothered by the (possible) GPG
> passphrase input when there is nothing to sign.

The above sounds as if we care about the performace loss and that is the main motivation behind this change. Intended? I have an impression that it is a hard-sell as a "performance improvement" patch, as the saved cycles are negligible compared to everything else that is done in the flow, and more importantly, it optimizes for the wrong case (i.e. it fails more efficiently).

Show 5 quoted lines

> Add a new test case to t5534 to ensure GPG will not be called when the
> GPG-signed atomic push fails.
>
> Signed-off-by: Han Xin <hanxin.hx@alibaba-inc.com>
> ---

...

Show 5 quoted lines

> +	test_must_fail env PATH="$TRASH_DIRECTORY:$PATH" git push \
> +			--signed --atomic --porcelain \
> +			dst noop ff noff >out 2>&1 &&
> +
> +	test_i18ngrep ! "gpg failed to sign" out &&

OK, that is much less brittle than the "output must match these lines exactly" test we saw earlier.

Show 9 quoted lines

> +	sed -n -e "/^To dst/,$ p" out >actual &&
> +	cat >expect <<-EOF &&
> +	To dst
> +	=	refs/heads/noop:refs/heads/noop	[up to date]
> +	!	refs/heads/ff:refs/heads/ff	[rejected] (atomic push failed)
> +	!	refs/heads/noff:refs/heads/noff	[rejected] (non-fast-forward)
> +	Done
> +	EOF
> +	test_i18ncmp expect actual

Didn't you mean to remove this part, which makes the whole test more brittle than necessary?

Thanks.