git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/6] fetch-pack: refactor process_acks()
Date: Thu, 08 Apr 2021 22:08:15 -0700	[thread overview]
Message-ID: <xmqqsg40niyo.fsf@gitster.g> (raw)
In-Reply-To: <6a9f78df1a3513df04ac3275cd1feccfb6cf87b1.1617929278.git.jonathantanmy@google.com> (Jonathan Tan's message of "Thu, 8 Apr 2021 18:09:59 -0700")

Jonathan Tan <jonathantanmy@google.com> writes:

> A subsequent commit will need part, but not all, of the functionality in
> process_acks(), so move some of its functionality to its sole caller
> do_fetch_pack_v2(). As a side effect, the resulting code is also
> shorter.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 70 +++++++++++++++++-----------------------------------
>  1 file changed, 22 insertions(+), 48 deletions(-)

Nicely done.

I first found the patch hard to follow, but that wasn't due to this
patch, but because I wasn't all that familiar with the original
anymore (even though I do recall futzing with the codepath that
involves the in_vain stuff myself at some point in the past).

And indeed the resulting code is easy to see what is going on (I'd
need a lot more concentration than I can muster this late in the
night to be able to tell if the behaviour does not change with the
patch, though).

> diff --git a/fetch-pack.c b/fetch-pack.c
> index 2318ebe680..9f3901cdba 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1351,35 +1351,11 @@ static int process_section_header(struct packet_reader *reader,
>  	return ret;
>  }
>  
> -enum common_found {
> -	/*
> -	 * No commit was found to be possessed by both the client and the
> -	 * server, and "ready" was not received.
> -	 */
> -	NO_COMMON_FOUND,
> -
> -	/*
> -	 * At least one commit was found to be possessed by both the client and
> -	 * the server, and "ready" was not received.
> -	 */
> -	COMMON_FOUND,
> -
> -	/*
> -	 * "ready" was received, indicating that the server is ready to send
> -	 * the packfile without any further negotiation.
> -	 */
> -	READY
> -};
> -
> -static enum common_found process_acks(struct fetch_negotiator *negotiator,
> -				      struct packet_reader *reader,
> -				      struct oidset *common)
> +static int process_ack(struct fetch_negotiator *negotiator,
> +		       struct packet_reader *reader,
> +		       struct object_id *common_oid,
> +		       int *received_ready)
>  {
> -	/* received */
> -	int received_ready = 0;
> -	int received_ack = 0;
> -
> -	process_section_header(reader, "acknowledgments", 0);
>  	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
>  		const char *arg;
>  
> @@ -1387,20 +1363,17 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,
>  			continue;
>  
>  		if (skip_prefix(reader->line, "ACK ", &arg)) {
> -			struct object_id oid;
> -			received_ack = 1;
> -			if (!get_oid_hex(arg, &oid)) {
> +			if (!get_oid_hex(arg, common_oid)) {
>  				struct commit *commit;
> -				oidset_insert(common, &oid);
> -				commit = lookup_commit(the_repository, &oid);
> +				commit = lookup_commit(the_repository, common_oid);
>  				if (negotiator)
>  					negotiator->ack(negotiator, commit);
>  			}
> -			continue;
> +			return 1;
>  		}
>  
>  		if (!strcmp(reader->line, "ready")) {
> -			received_ready = 1;
> +			*received_ready = 1;
>  			continue;
>  		}
>  
> @@ -1418,13 +1391,12 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,
>  	 * sent. Therefore, a DELIM is expected if "ready" is sent, and a FLUSH
>  	 * otherwise.
>  	 */
> -	if (received_ready && reader->status != PACKET_READ_DELIM)
> +	if (*received_ready && reader->status != PACKET_READ_DELIM)
>  		die(_("expected packfile to be sent after 'ready'"));
> -	if (!received_ready && reader->status != PACKET_READ_FLUSH)
> +	if (!*received_ready && reader->status != PACKET_READ_FLUSH)
>  		die(_("expected no other sections to be sent after no 'ready'"));
>  
> -	return received_ready ? READY :
> -		(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);
> +	return 0;
>  }
>  
>  static void receive_shallow_info(struct fetch_pack_args *args,
> @@ -1573,6 +1545,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  	struct fetch_negotiator negotiator_alloc;
>  	struct fetch_negotiator *negotiator;
>  	int seen_ack = 0;
> +	struct object_id common_oid;
> +	int received_ready = 0;
>  	struct string_list packfile_uris = STRING_LIST_INIT_DUP;
>  	int i;
>  	struct strvec index_pack_args = STRVEC_INIT;
> @@ -1631,22 +1605,22 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  			break;
>  		case FETCH_PROCESS_ACKS:
>  			/* Process ACKs/NAKs */
> -			switch (process_acks(negotiator, &reader, &common)) {
> -			case READY:
> +			process_section_header(&reader, "acknowledgments", 0);
> +			while (process_ack(negotiator, &reader, &common_oid,
> +					   &received_ready)) {
> +				in_vain = 0;
> +				seen_ack = 1;
> +				oidset_insert(&common, &common_oid);
> +			}
> +			if (received_ready) {
>  				/*
>  				 * Don't check for response delimiter; get_pack() will
>  				 * read the rest of this response.
>  				 */
>  				state = FETCH_GET_PACK;
> -				break;
> -			case COMMON_FOUND:
> -				in_vain = 0;
> -				seen_ack = 1;
> -				/* fallthrough */
> -			case NO_COMMON_FOUND:
> +			} else {
>  				do_check_stateless_delimiter(args, &reader);
>  				state = FETCH_SEND_REQUEST;
> -				break;
>  			}
>  			break;
>  		case FETCH_GET_PACK:

  reply	other threads:[~2021-04-09  5:09 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-09  1:09 [PATCH 0/6] Push negotiation Jonathan Tan
2021-04-09  1:09 ` [PATCH 1/6] fetch-pack: buffer object-format with other args Jonathan Tan
2021-04-09  4:49   ` Junio C Hamano
2021-04-09 16:24     ` Jonathan Tan
2021-04-09  1:09 ` [PATCH 2/6] fetch-pack: refactor process_acks() Jonathan Tan
2021-04-09  5:08   ` Junio C Hamano [this message]
2021-05-03 16:30   ` Derrick Stolee
2021-04-09  1:10 ` [PATCH 3/6] fetch-pack: refactor add_haves() Jonathan Tan
2021-04-09  5:20   ` Junio C Hamano
2021-04-09  1:10 ` [PATCH 4/6] fetch-pack: refactor command and capability write Jonathan Tan
2021-04-09  5:27   ` Junio C Hamano
2021-04-09  1:10 ` [PATCH 5/6] fetch: teach independent negotiation (no packfile) Jonathan Tan
2021-04-09  5:41   ` Junio C Hamano
2021-04-09 16:38     ` Jonathan Tan
2021-05-03 15:25   ` Derrick Stolee
2021-05-03 15:40     ` Derrick Stolee
2021-05-03 21:52     ` Jonathan Tan
2021-04-09  1:10 ` [PATCH 6/6] send-pack: support push negotiation Jonathan Tan
2021-05-03 15:35   ` Derrick Stolee
2021-05-03 22:02     ` Jonathan Tan
2021-05-04 17:26       ` Derrick Stolee
2021-04-30  5:42 ` [PATCH 0/6] Push negotiation Junio C Hamano
2021-04-30 17:33   ` Derrick Stolee
2021-05-04 21:15 ` [PATCH v2 0/5] " Jonathan Tan
2021-05-04 21:15   ` [PATCH v2 1/5] fetch-pack: refactor process_acks() Jonathan Tan
2021-05-04 21:15   ` [PATCH v2 2/5] fetch-pack: refactor add_haves() Jonathan Tan
2021-05-04 21:16   ` [PATCH v2 3/5] fetch-pack: refactor command and capability write Jonathan Tan
2021-05-04 21:16   ` [PATCH v2 4/5] fetch: teach independent negotiation (no packfile) Jonathan Tan
2021-05-05  1:53     ` Junio C Hamano
2021-05-05 16:42       ` Derrick Stolee
2021-05-06  2:12         ` Junio C Hamano
2021-05-05 16:44     ` Derrick Stolee
2021-05-04 21:16   ` [PATCH v2 5/5] send-pack: support push negotiation Jonathan Tan

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqqsg40niyo.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --subject='Re: [PATCH 2/6] fetch-pack: refactor process_acks()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

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

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git