Re: [PATCH v2 3/8] fetch-pack: directly end negotiation if ACK ready

[Brandon Williams <bmwill@google.com>]

On 06/06, Jonathan Tan wrote:
> 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 do this.
> 
> This means that the memory in priority queue will be reclaimed only upon
> program exit, similar to the cases in which "ACK %s ready" is not

This seems fine for now though ideally we would remove the global
priority queue and have it live on the stack somewhere in the call stack
and it could be cleared unconditionally before returning.

> received. (A related problem occurs when do_fetch_pack() is invoked a
> second time in the same process with a possibly non-empty priority
> queue, but this will be solved in a subsequent patch in this patch set.)
> 
> [1] The rationale is further described in the originating commit
> f2cba9299b ("fetch-pack: Finish negotation if remote replies "ACK %s
> ready"", 2011-03-14).
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 2812499a5..09f5c83c4 100644
> --- 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:
> @@ -1300,7 +1300,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;
>  		}
> -- 
> 2.17.0.768.g1526ddbba1.dirty
> 

-- 
Brandon Williams