Re: [PATCH v5 2/8] upload-pack: implement ref-in-want

From: Brandon Williams <bmwill@google.com>
Date: Jun 27, 2018, 18:05 UTC
Message-ID: <20180627180557.GG19910@google.com>
In-Reply-To: <xmqqsh59i6jx.fsf@gitster-ct.c.googlers.com>

On 06/26, Junio C Hamano wrote:

Show 23 quoted lines

> Brandon Williams <bmwill@google.com> writes:
> 
> > +    wanted-refs section
> > +	* This section is only included if the client has requested a
> > +	  ref using a 'want-ref' line and if a packfile section is also
> > +	  included in the response.
> > +
> > +	* Always begins with the section header "wanted-refs".
> > +
> > +	* The server will send a ref listing ("<oid> <refname>") for
> > +	  each reference requested using 'want-ref' lines.
> > +
> > +	* The server SHOULD NOT send any refs which were not requested
> > +	  using 'want-ref' lines and a client MUST ignore refs which
> > +	  weren't requested.
> 
> Just being curious, but the above feels the other way around.  Why
> are we being more lenient to writers of broken server than writers
> of broken clients?  The number of installations they need to take
> back and replace is certainly lower for the former, which means
> that, if exchanges of unsoliclited refs are unwanted, clients should
> notice and barf (or warn) if the server misbehaves, and the server
> should be forbidden from sending unsolicited refs, no?

Ok so should I change the server part to "MUST NOT" and the client part to "SHOULD"? And I can add code to die when we see refs that weren't requested, but i feel like if we add an ability to request a pattern in the future this will completely change, which is why I currently have a client just ignoring anything else.

Show 29 quoted lines

> 
> 
> > diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> > new file mode 100755
> > index 000000000..0ef182970
> > --- /dev/null
> > +++ b/t/t5703-upload-pack-ref-in-want.sh
> > @@ -0,0 +1,153 @@
> > +#!/bin/sh
> > +
> > +test_description='upload-pack ref-in-want'
> > +
> > +. ./test-lib.sh
> > +
> > +get_actual_refs() {
> 
> Style.  "get_actual_refs () {"
> 
> > +	sed -n '/wanted-refs/,/0001/p' <out | sed '1d;$d' | test-pkt-line unpack >actual_refs
> 
> Unnecessary piping of sed output into another sed invocation?
> 
> 	sed -n -e '/wanted-refs/,/0001/{
> 		/wanted-refs/d
> 		/0001/d
> 		p
> 	}'
> 
> or something like that?

Yeah thanks for the help with sed :)

-- 
Brandon Williams