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

On 08 Sep 2016, at 23:49, Stefan Beller [off-list ref] wrote:

On Thu, Sep 8, 2016 at 11:21 AM,  [off-list ref] wrote:

quoted

From: Lars Schneider <redacted>

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

quoted

+       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.
*/

quoted

+               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