W dniu 27.09.2016 o 10:39, Jeff King pisze: > On Mon, Sep 26, 2016 at 09:21:10PM +0200, Lars Schneider wrote: > >> On 25 Sep 2016, at 13:26, Jakub Narębski <jnareb@xxxxxxxxx> wrote: >> >>> W dniu 20.09.2016 o 21:02, larsxschneider@xxxxxxxxx pisze: >>>> From: Lars Schneider <larsxschneider@xxxxxxxxx> >>>> ... >>>> >>>> +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. >>>> + 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. > >>>> + 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@xxxxxxxxxxxxxxxxxxxxx/ > > 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