list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Jiang Xin <>
Cc: Han Xin <>,
	Han Xin <>,
	Git List <>,
	Jiang Xin <>
Subject: Re: [PATCH 2/2] send-pack: check atomic push before running GPG
Date: Wed, 16 Sep 2020 16:44:28 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (Jiang Xin's message of "Wed, 16 Sep 2020 07:49:58 -0400")

Jiang Xin <> writes:

> 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'

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
+		/* already passed checks on the local side */
+		/* of course this is OK */
 		return 0;

  reply	other threads:[~2020-09-16 23:44 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-15  9:58 [PATCH 1/2] t5534: new test case for atomic signed push Han Xin
2020-09-15  9:58 ` [PATCH 2/2] send-pack: check atomic push before running GPG Han Xin
2020-09-15 21:02   ` Junio C Hamano
2020-09-15 21:40     ` Junio C Hamano
2020-09-16  1:53     ` Jiang Xin
2020-09-16  4:37       ` Junio C Hamano
2020-09-16 11:49         ` Jiang Xin
2020-09-16 23:44           ` Junio C Hamano [this message]
2020-09-18  4:50             ` [PATCH v2] send-pack: run GPG after atomic push checking Han Xin
2020-09-19  0:02               ` Junio C Hamano
2020-09-19 14:47                 ` [PATCH v3] " Han Xin
2020-09-19 23:02                   ` Junio C Hamano
2020-09-20  6:20                     ` [PATCH v4] " Han Xin
2020-09-16 17:35         ` [PATCH 2/2] send-pack: check atomic push before running GPG 韩欣(炽天)
2020-09-15 20:31 ` [PATCH 1/2] t5534: new test case for atomic signed push Junio C Hamano
2020-09-16  0:34   ` brian m. carlson
2020-09-15 20:34 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 2/2] send-pack: check atomic push before running GPG' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).