Re: [PATCH 2/2] send-pack: check atomic push before running GPG

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2020, 23:44 UTC
Message-ID: <xmqqv9gdthgj.fsf@gitster.c.googlers.com>
In-Reply-To: <20200916114958.1123-1-worldhello.net@gmail.com>

Jiang Xin <worldhello.net@gmail.com> writes:

Show 6 quoted lines

> In the original "Finally, tell the other end" block, the function
> `check_to_send_update()` is also called for non-PGP-signed push.
> The 'ref->status' changed by the "Clear the status" block won't 
> make any difference for the return value of the function
> `check_to_send_update()`. Refs even with status REF_STATUS_OK and
> REF_STATUS_EXPECTING_REPORT will be sent to the server side.

Ah, yes, I re-read the code in check_to_send_update() and you're right that it does the right thing. I however strongly suspect it just happens to do the right thing by accident and not by design.

I'd prefer to see a bit more tightening done to the function to clarify the handling of these two values that are omitted from the case arms in the switch statement, perhaps like this, as a preliminary clean-up.

As a further clean-up, we probably should stop relying on the 'default' label.

There are other REF_STATUS values that are not handled explicitly, among which REF_STATUS_ATOMIC_PUSH_FAILED looks like the most troublesome one.

The function will return 0 (success) for ATOMIC_PUSH_FAILED, but the current ordering of the codeflow makes sure check_to_send_update() is *not* called after ref->status is turned into that value and that would be the only thing that may be ensuring the correctness. There may be other ones we are not handling quite right.

 send-pack.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/send-pack.c b/send-pack.c
index 632f1580ca..347fb15633 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -244,7 +244,12 @@ static int check_to_send_update(const struct ref *ref, const struct send_pack_ar
 		return CHECK_REF_STATUS_REJECTED;
 	case REF_STATUS_UPTODATE:
 		return CHECK_REF_UPTODATE;
+
 	default:
+	case REF_STATUS_EXPECTING_REPORT:
+		/* already passed checks on the local side */
+	case REF_STATUS_OK:
+		/* of course this is OK */
 		return 0;
 	}
 }