Re: [PATCH v7 06/10] pkt-line: add functions to read/write flush terminated packet streams

[Lars Schneider <larsxschneider@gmail.com>]

> On 08 Sep 2016, at 23:49, Stefan Beller <sbeller@google.com> wrote:
> 
> On Thu, Sep 8, 2016 at 11:21 AM,  <larsxschneider@gmail.com> wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> write_packetized_from_fd() and write_packetized_from_buf() write a
>> stream of packets. All content packets use the maximal packet size
>> except for the last one. After the last content packet a `flush` control
>> packet is written.
> 
> I presume we need both write_* things in a later patch; can you clarify why
> we need both of them?

Since 9035d7 Git streams from fd to required filters and from buf to
non-required filters. The Git filter protocol v2 makes use of all that,
too.

https://github.com/git/git/commit/9035d75a2be9d80d82676504d69553245017f6d4


>> +       if (paket_len < 0) {
>> +               if (oldalloc == 0)
>> +                       strbuf_release(sb_out);
> 
> So if old alloc is 0, we release it, which is documented as
> /**
> * Release a string buffer and the memory it used. You should not use the
> * string buffer after using this function, unless you initialize it again.
> */
> 
>> +               else
>> +                       strbuf_setlen(sb_out, oldlen);
> 
> Otherwise we just set the length back, such that it looks like before.
> 
> So as a caller the strbuf is in a different state in case of error
> depending whether
> the strbuf already had some data in it. I think it would be better if
> we only did
> `strbuf_setlen(sb_out, oldlen);` here, such that the caller can
> strbuf_release it
> unconditionally.

I tried to mimic the behavior of strbuf_read() [1]. The error handling
was introduced in 2fc647 [2] to ease error handling:

"This allows for easier error handling, as callers only need to call
strbuf_release() if A) the command succeeded or B) if they would have had
to do so anyway because they added something to the strbuf themselves."

Does this convince you to keep the proposed error handling? If yes, then
I would add a comment to the function to document that behavior explicitly!

[1] https://github.com/git/git/blob/cda1bbd474805e653dda8a71d4ea3790e2a66cbb/strbuf.c#L377-L383
[2] https://github.com/git/git/commit/2fc647004ac7016128372a85db8245581e493812


> Or to make things more confusing, you could use strbuf_reset in case of 0,
> as that is a strbuf_setlen internally. ;)


Thanks,
Lars