fetch-pack: directly end negotiation if ACK ready

jonathantanmy · gitster · commit 21bcf6e429e0 · 2018-06-15T08:44:23.000-07:00
When "ACK %s ready" is received, find_common() clears rev_list in an attempt to stop further "have" lines from being sent [1]. It is much more readable to explicitly break from the loop instead. So explicitly break from the loop, and make the clearing of the rev_list happen unconditionally. [1] The rationale is further described in the originating commit f2cba92 ("fetch-pack: Finish negotation if remote replies "ACK %s ready"", 2011-03-14). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/fetch-pack.c b/fetch-pack.c
@@ -517,10 +517,8 @@ static int find_common(struct fetch_pack_args *args,
 					mark_common(commit, 0, 1);
 					retval = 0;
 					got_continue = 1;
-					if (ack == ACK_ready) {
-						clear_prio_queue(&rev_list);
+					if (ack == ACK_ready)
 						got_ready = 1;
-					}
 					break;
 					}
 				}
@@ -530,6 +528,8 @@ static int find_common(struct fetch_pack_args *args,
 				print_verbose(args, _("giving up"));
 				break; /* give up */
 			}
+			if (got_ready)
+				break;
 		}
 	}
 done:
@@ -1096,6 +1096,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		die(_("git fetch-pack: fetch failed."));
 
  all_done:
+	clear_prio_queue(&rev_list);
 	return ref;
 }
 
@@ -1300,7 +1301,6 @@ static int process_acks(struct packet_reader *reader, struct oidset *common)
 		}
 
 		if (!strcmp(reader->line, "ready")) {
-			clear_prio_queue(&rev_list);
 			received_ready = 1;
 			continue;
 		}
@@ -1441,6 +1441,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		}
 	}
 
+	clear_prio_queue(&rev_list);
 	oidset_clear(&common);
 	return ref;
 }

Original file line number	Diff line number	Diff line change
`@@ -517,10 +517,8 @@ static int find_common(struct fetch_pack_args *args,`
`517`	`517`	`mark_common(commit, 0, 1);`
`518`	`518`	`retval = 0;`
`519`	`519`	`got_continue = 1;`
`520`		`- if (ack == ACK_ready) {`
`521`		`- clear_prio_queue(&rev_list);`
	`520`	`+ if (ack == ACK_ready)`
`522`	`521`	`got_ready = 1;`
`523`		`- }`
`524`	`522`	`break;`
`525`	`523`	`}`
`526`	`524`	`}`
`@@ -530,6 +528,8 @@ static int find_common(struct fetch_pack_args *args,`
`530`	`528`	`print_verbose(args, _("giving up"));`
`531`	`529`	`break; /* give up */`
`532`	`530`	`}`
	`531`	`+ if (got_ready)`
	`532`	`+ break;`
`533`	`533`	`}`
`534`	`534`	`}`
`535`	`535`	`done:`
`@@ -1096,6 +1096,7 @@ static struct ref do_fetch_pack(struct fetch_pack_args args,`
`1096`	`1096`	`die(_("git fetch-pack: fetch failed."));`
`1097`	`1097`
`1098`	`1098`	`all_done:`
	`1099`	`+ clear_prio_queue(&rev_list);`
`1099`	`1100`	`return ref;`
`1100`	`1101`	`}`
`1101`	`1102`
`@@ -1300,7 +1301,6 @@ static int process_acks(struct packet_reader reader, struct oidset common)`
`1300`	`1301`	`}`
`1301`	`1302`
`1302`	`1303`	`if (!strcmp(reader->line, "ready")) {`
`1303`		`- clear_prio_queue(&rev_list);`
`1304`	`1304`	`received_ready = 1;`
`1305`	`1305`	`continue;`
`1306`	`1306`	`}`
`@@ -1441,6 +1441,7 @@ static struct ref do_fetch_pack_v2(struct fetch_pack_args args,`
`1441`	`1441`	`}`
`1442`	`1442`	`}`
`1443`	`1443`
	`1444`	`+ clear_prio_queue(&rev_list);`
`1444`	`1445`	`oidset_clear(&common);`
`1445`	`1446`	`return ref;`
`1446`	`1447`	`}`