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

On 04 Oct 2016, at 23:00, Jakub Narębski [off-list ref] wrote:

[Some of answers and comments may got invalidated by v9]

W dniu 30.09.2016 o 21:38, Lars Schneider pisze:

quoted

quoted

On 27 Sep 2016, at 17:37, Jakub Narębski [off-list ref] wrote:

Part second of the review of 11/11.

[...]

quoted

quoted

quoted

+
+	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.

quoted

quoted

quoted

+		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

quoted

quoted

quoted

+	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.

quoted

quoted

quoted

+	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