Re: [PATCH v8 11/11] convert: add filter.<driver>.process option

["Jakub Narębski" <jnareb@gmail.com>]

[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@gmail.com> 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