Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 27, 2020, 17:27 UTC
Message-ID: <20200427172736.61159-1-jonathantanmy@google.com>
In-Reply-To: <20200426002802.GD877@gmail.com>

> not about this patch: can these return values from process_acks be made
> into an enum with named enumerators?  That would make what's happening
> in the call site more obvious.

That sounds reasonable to me.

> > +	cp trace /tmp/x &&
> 
> Leftover debugging line?

Ah, yes. If Junio can't or won't do it locally then I'll send out another set with this changed.

Show 10 quoted lines

> > +	test_i18ngrep "Total 3 " trace
> 
> Clever.
> 
> In some sense this is a fragile test, since the server could change
> how it reports progress some day.  Would it make sense (perhaps as a
> followup patch) for this to use a trace2 log instead?  For example,
> if we turn on tracing in the server, then since 9ed8790282
> (pack-objects: write objects packed to trace2, 2019-04-11) it will
> report how many objects were in the pack it wrote.

Probably better to have tracing in the client and use that, but this requires us to add that tracing. But in general I agree.

> After removing the "cp trace /tmp/x" line,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Thanks.

Thanks for your review.