> On 04 Oct 2016, at 23:00, Jakub Narębski <jnareb@xxxxxxxxx> wrote:
>
> [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?
You are right. I'll fix that.
>>
>>>> + 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.
OK
>>>>
>>>> + 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 ;-)
TBH I am not 100% sure why, too. I think this ensures that we don't have
any outdated/unrelated/previous data in the stream buffers.
>>>>
>>>> + 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?
Of course! Ah too many refactorings :-)
I'll remove that.
Thank you,
Lars