| Message ID | f0cfbc03c27658e54a73c46570c5153961ed85b6.1588031728.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Protocol v2 in_vain fixes | expand |
Jonathan Tan wrote: > process_acks() returns 0, 1, or 2, depending on whether "ready" was > received and if not, whether at least one commit was found to be common. > Replace these magic numbers with a documented enum. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > fetch-pack.c | 35 ++++++++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 7 deletions(-) Yay! > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1268,9 +1268,29 @@ static int process_section_header(struct packet_reader *reader, > return ret; > } > > -static int process_acks(struct fetch_negotiator *negotiator, > - struct packet_reader *reader, > - struct oidset *common) > +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) Thanks for the clear comments. > { > /* received */ > int received_ready = 0; > @@ -1320,7 +1340,8 @@ static int process_acks(struct fetch_negotiator *negotiator, > die(_("expected no other sections to be sent after no 'ready'")); > > /* return 0 if no common, 1 if there are common, or 2 if ready */ This comment can be removed now, which is a nice bonus. > - return received_ready ? 2 : (received_ack ? 1 : 0); > + return received_ready ? READY : > + (received_ack ? COMMON_FOUND : NO_COMMON_FOUND); > } > > static void receive_shallow_info(struct fetch_pack_args *args, > @@ -1508,13 +1529,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, > case FETCH_PROCESS_ACKS: > /* Process ACKs/NAKs */ > switch (process_acks(negotiator, &reader, &common)) { > - case 2: > + case READY: > state = FETCH_GET_PACK; > break; > - case 1: > + case COMMON_FOUND: > in_vain = 0; > /* fallthrough */ > - default: > + case NO_COMMON_FOUND: > state = FETCH_SEND_REQUEST; > break; > } With the extraneous comment removed, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Thanks. diff --git i/fetch-pack.c w/fetch-pack.c index 45547a621e6..7b0eef98db5 100644 --- i/fetch-pack.c +++ w/fetch-pack.c @@ -1339,7 +1339,6 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator, if (!received_ready && reader->status != PACKET_READ_FLUSH) die(_("expected no other sections to be sent after no 'ready'")); - /* return 0 if no common, 1 if there are common, or 2 if ready */ return received_ready ? READY : (received_ack ? COMMON_FOUND : NO_COMMON_FOUND); }
Jonathan Nieder <jrnieder@gmail.com> writes: >> /* return 0 if no common, 1 if there are common, or 2 if ready */ > > This comment can be removed now, which is a nice bonus. > >> - return received_ready ? 2 : (received_ack ? 1 : 0); >> + return received_ready ? READY : >> + (received_ack ? COMMON_FOUND : NO_COMMON_FOUND); >> } >> >> static void receive_shallow_info(struct fetch_pack_args *args, >> @@ -1508,13 +1529,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, >> case FETCH_PROCESS_ACKS: >> /* Process ACKs/NAKs */ >> switch (process_acks(negotiator, &reader, &common)) { >> - case 2: >> + case READY: >> state = FETCH_GET_PACK; >> break; >> - case 1: >> + case COMMON_FOUND: >> in_vain = 0; >> /* fallthrough */ >> - default: >> + case NO_COMMON_FOUND: >> state = FETCH_SEND_REQUEST; >> break; >> } > > With the extraneous comment removed, > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Yes, the result reads very clearly. Locally amended. Thanks.
On Tue, 28 Apr 2020 09:54:19 -0700 Junio C Hamano <gitster@pobox.com> wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: > > >> /* return 0 if no common, 1 if there are common, or 2 if ready */ > > > > This comment can be removed now, which is a nice bonus. It is still present in the pu branch. Is there other branch where I should look? Thanks Michal
Michal Suchánek <msuchanek@suse.de> writes: > On Tue, 28 Apr 2020 09:54:19 -0700 > Junio C Hamano <gitster@pobox.com> wrote: > >> Jonathan Nieder <jrnieder@gmail.com> writes: >> >> >> /* return 0 if no common, 1 if there are common, or 2 if ready */ >> > >> > This comment can be removed now, which is a nice bonus. > It is still present in the pu branch. > > Is there other branch where I should look? Some branch that is pushed out _after_ I push out, perhaps?
diff --git a/fetch-pack.c b/fetch-pack.c index 0b07b3ee73..45547a621e 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1268,9 +1268,29 @@ static int process_section_header(struct packet_reader *reader, return ret; } -static int process_acks(struct fetch_negotiator *negotiator, - struct packet_reader *reader, - struct oidset *common) +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) { /* received */ int received_ready = 0; @@ -1320,7 +1340,8 @@ static int process_acks(struct fetch_negotiator *negotiator, die(_("expected no other sections to be sent after no 'ready'")); /* return 0 if no common, 1 if there are common, or 2 if ready */ - return received_ready ? 2 : (received_ack ? 1 : 0); + return received_ready ? READY : + (received_ack ? COMMON_FOUND : NO_COMMON_FOUND); } static void receive_shallow_info(struct fetch_pack_args *args, @@ -1508,13 +1529,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, case FETCH_PROCESS_ACKS: /* Process ACKs/NAKs */ switch (process_acks(negotiator, &reader, &common)) { - case 2: + case READY: state = FETCH_GET_PACK; break; - case 1: + case COMMON_FOUND: in_vain = 0; /* fallthrough */ - default: + case NO_COMMON_FOUND: state = FETCH_SEND_REQUEST; break; }
process_acks() returns 0, 1, or 2, depending on whether "ready" was received and if not, whether at least one commit was found to be common. Replace these magic numbers with a documented enum. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- fetch-pack.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-)