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

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

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

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

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

[...]

quoted

quoted

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

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.
 

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 ;-)
 

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?

[...]

Best,
-- 
Jakub Narębski