[Some of answers and comments may got invalidated by v9]
W dniu 30.09.2016 o 21:38, Lars Schneider pisze:
>> On 27 Sep 2016, at 17:37, Jakub Narębski <jnareb@xxxxxxxxx> wrote:
>>
>> Part second of the review of 11/11.
[...]
>>> +
>>> + if (start_command(process)) {
>>> + error("cannot fork to run external filter '%s'", cmd);
>>> + kill_multi_file_filter(hashmap, entry);
>>> + return NULL;
>>> + }
>>
>> I guess there is a reason why we init hashmap entry, try to start
>> external process, then kill entry of unable to start, instead of
>> trying to start external process, and adding hashmap entry when
>> we succeed?
>
> Yes. This way I can reuse the kill_multi_file_filter() function.
I don't quite understand. If you didn't fill the entry before
using start_command(process), you would not need kill_multi_file_filter(),
which in that case IIUC just removes the just created entry from hashmap.
Couldn't you add entry to hashmap in the 'else' part? Or would it
be racy?
[...]
>>> +static void read_multi_file_filter_values(int fd, struct strbuf *status) {
>>
>> This is more
>>
>> +static void read_multi_file_filter_status(int fd, struct strbuf *status) {
>>
>> It doesn't read arbitrary values, it examines 'metadata' from
>> filter for "status=<foo>" lines.
>
> True!
>
>>> + if (pair[0] && pair[0]->len && pair[1]) {
>>> + if (!strcmp(pair[0]->buf, "status=")) {
>>> + strbuf_reset(status);
>>> + strbuf_addbuf(status, pair[1]);
>>> + }
>>
>> So it is last status=<foo> line wins behavior?
>
> Correct.
Perhaps this should be described in code comment.
>>>
>>> + fflush(NULL);
>>
>> Why this fflush(NULL) is needed here?
>
> This flushes all open output streams. The single filter does the same.
I know what it does, but I don't know why. But "single filter does it"
is good enough for me. Still would want to know why, though ;-)
>>>
>>> + if (fd >= 0 && !src) {
>>> + if (fstat(fd, &file_stat) == -1)
>>> + return 0;
>>> + len = xsize_t(file_stat.st_size);
>>> + }
>>
>> Errr... is it necessary? The protocol no longer provides size=<n>
>> hint, and neither uses such hint if provided.
>
> We require the size in write_packetized_from_buf() later.
Don't we use write_packetized_from_fd() in the case of fd >= 0?
[...]
Best,
--
Jakub Narębski