Re: [PATCH v3 11/35] test-pkt-line: introduce a packet-line test helper

[Brandon Williams]

On 03/02, Jeff King wrote:
> On Fri, Feb 23, 2018 at 01:22:31PM -0800, Brandon Williams wrote:
> 
> > On 02/22, Stefan Beller wrote:
> > > On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  
> > > wrote:
> > > 
> > > > +static void pack_line(const char *line)
> > > > +{
> > > > +   if (!strcmp(line, "") || !strcmp(line, "\n"))
> > > 
> > > From our in-office discussion:
> > > v1/v0 packs pktlines twice in http, which is not possible to
> > > construct using this test helper when using the same string
> > > for the packed and unpacked representation of flush and delim packets,
> > > i.e. test-pkt-line --pack $(test-pkt-line --pack ) would produce
> > > '' instead of '0009\n'.
> > > To fix it we'd have to replace the unpacked versions of these pkts to
> > > something else such as "FLUSH" "DELIM".
> > > 
> > > However as we do not anticipate the test helper to be used in further
> > > tests for v0, this ought to be no big issue.
> > > Maybe someone else cares though?
> > 
> > I'm going to punt and say, if someone cares enough they can update this
> > test-helper when they want to use it for v1/v0 stuff.
> 
> I recently add packetize and depacketize helpers for testing v0 streams;
> see 4414a15002 (t/lib-git-daemon: add network-protocol helpers,
> 2018-01-24). Is it worth folding these together?

I didn't know something like that existed! (of course if it was just
added this year then it didn't exist when I started working on this
stuff).  Yeah its probably a good idea to fold these together, I can
take a look at how your packetize and depacketize helpers work and add
the small amount of functionality that I'd need to replace the helper I
made.

> 
> -Peff

-- 
Brandon Williams

Re: [PATCH v3 11/35] test-pkt-line: introduce a packet-line test helper

[Jeff King]

On Fri, Feb 23, 2018 at 01:22:31PM -0800, Brandon Williams wrote:

> On 02/22, Stefan Beller wrote:
> > On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:
> > 
> > > +static void pack_line(const char *line)
> > > +{
> > > +   if (!strcmp(line, "") || !strcmp(line, "\n"))
> > 
> > From our in-office discussion:
> > v1/v0 packs pktlines twice in http, which is not possible to
> > construct using this test helper when using the same string
> > for the packed and unpacked representation of flush and delim packets,
> > i.e. test-pkt-line --pack $(test-pkt-line --pack ) would produce
> > '' instead of '0009\n'.
> > To fix it we'd have to replace the unpacked versions of these pkts to
> > something else such as "FLUSH" "DELIM".
> > 
> > However as we do not anticipate the test helper to be used in further
> > tests for v0, this ought to be no big issue.
> > Maybe someone else cares though?
> 
> I'm going to punt and say, if someone cares enough they can update this
> test-helper when they want to use it for v1/v0 stuff.

I recently add packetize and depacketize helpers for testing v0 streams;
see 4414a15002 (t/lib-git-daemon: add network-protocol helpers,
2018-01-24). Is it worth folding these together?

-Peff

Re: [PATCH v3 11/35] test-pkt-line: introduce a packet-line test helper

[Brandon Williams]

On 02/22, Stefan Beller wrote:
> On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:
> 
> > +static void pack_line(const char *line)
> > +{
> > +   if (!strcmp(line, "") || !strcmp(line, "\n"))
> 
> From our in-office discussion:
> v1/v0 packs pktlines twice in http, which is not possible to
> construct using this test helper when using the same string
> for the packed and unpacked representation of flush and delim packets,
> i.e. test-pkt-line --pack $(test-pkt-line --pack ) would produce
> '' instead of '0009\n'.
> To fix it we'd have to replace the unpacked versions of these pkts to
> something else such as "FLUSH" "DELIM".
> 
> However as we do not anticipate the test helper to be used in further
> tests for v0, this ought to be no big issue.
> Maybe someone else cares though?

I'm going to punt and say, if someone cares enough they can update this
test-helper when they want to use it for v1/v0 stuff.

-- 
Brandon Williams

Re: [PATCH v3 11/35] test-pkt-line: introduce a packet-line test helper

[Stefan Beller]

On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:

> +static void pack_line(const char *line)
> +{
> +   if (!strcmp(line, "") || !strcmp(line, "\n"))

>From our in-office discussion:
v1/v0 packs pktlines twice in http, which is not possible to
construct using this test helper when using the same string
for the packed and unpacked representation of flush and delim packets,
i.e. test-pkt-line --pack $(test-pkt-line --pack ) would produce
'' instead of '0009\n'.
To fix it we'd have to replace the unpacked versions of these pkts to
something else such as "FLUSH" "DELIM".

However as we do not anticipate the test helper to be used in further
tests for v0, this ought to be no big issue.
Maybe someone else cares though?