Re: [PATCH v8 06/11] pkt-line: add packet_write_gently()

From: Jakub Narębski <jnareb@gmail.com>
Date: Sep 27, 2016, 19:33 UTC
Message-ID: <5e6a95a5-e744-1e9d-05c6-3700c595af36@gmail.com>
In-Reply-To: <20160927083939.par2ea5nqjrtkkjs@sigill.intra.peff.net>

W dniu 27.09.2016 o 10:39, Jeff King pisze:

Show 19 quoted lines

> On Mon, Sep 26, 2016 at 09:21:10PM +0200, Lars Schneider wrote:
> 
>> On 25 Sep 2016, at 13:26, Jakub Narębski <jnareb@gmail.com> wrote:
>>
>>> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>> ...
>>>>
>>>> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>>>
>>> I'm not sure what naming convention the rest of Git uses, but isn't
>>> it more like '*data' rather than '*buf' here?
>>
>> pkt-line seems to use 'buf' or 'buffer' for everything else.
> 
> I do not think we have definite rules, but I would generally expect to
> see "data" as an opaque thing (e.g., passing "void *data" to callbacks).
> "buf" or "buffer" makes sense here, but I don't think it really matters
> that much either way.

True.

Show 16 quoted lines

>>>> +	static char packet_write_buffer[LARGE_PACKET_MAX];
>>>
>>> I think there should be warning (as a comment before function
>>> declaration, or before function definition), that packet_write_gently()
>>> is not thread-safe (nor reentrant, but the latter does not matter here,
>>> I think).
>>>
>>> Thread-safe vs reentrant: http://stackoverflow.com/a/33445858/46058
>>>
>>> This is not something terribly important; I guess git code has tons
>>> of functions not marked as thread-unsafe...
>>
>> I agree that the function is not thread-safe. However, I can't find 
>> an example in the Git source that marks a function as not thread-safe.
>> Unless is it explicitly stated in the coding guidelines I would prefer
>> not to start way to mark functions.

There is *one* example: "fill_textconv is not remotely thread-safe;" comment in grep.c, but not in diff.{c,h} where it is declared/defined.

Also, it is static function; we should know if it is thread-safe or not.

I am thinking about supporting streaming in the future, and perhaps also running different filter drivers (for different files) in parallel. I guess that using "static __thread char packet_write_buffer[...]" is out of question (still not reentrant)?

> 
> I'd agree. A large number of functions in git are not reentrant, and I
> would not want to give the impression that those missing a warning are
> safe to use.

The fact tha git code is undercommented and underdocumented does not mean that we should not add comments and documentation.

Show 17 quoted lines

> 
>>>> +	if (size > sizeof(packet_write_buffer) - 4) {
>>>
>>> First, wouldn't the following be more readable:
>>>
>>>  +	if (size + 4 > LARGE_PACKET_MAX) {
>>
>> Peff suggested that here:
>> http://public-inbox.org/git/20160810132814.gqnipsdwyzjmuqjy@sigill.intra.peff.net/
> 
> There is a good reason to do size checks as a subtraction from a known
> quantity: you can be sure that you are not introducing an overflow
> (e.g., Jakub's suggestion does the wrong thing when "size" is within 4
> bytes of its maximum value). That's unlikely in this case, but then so
> is the size exceeding LARGE_PACKET_MAX in the first place (arguably this
> should be a die("BUG"), because it is the caller's responsibility to
> split their packets.

Right.  I should train myself to watch for overflows.

-- 
Jakub Narębski