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

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

Part second of the review of 11/11.

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:

quoted

+
+	if (!drv->process && (CAP_CLEAN & wanted_capability) && drv->clean)

This is just a very minor nitpicking, but wouldn't it be easier
to read with those checks reordered?

 +	if ((wanted_capability & CAP_CLEAN) && !drv->process && drv->clean)

OK

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.

quoted

+
+	sigchain_push(SIGPIPE, SIG_IGN);

I guess that this is here to handle errors writing to filter
by ourself, isn't it?

Yes.

quoted

+		error("external filter '%s' does not support long running filter protocol", cmd);

We could have described the error here better.

 +		error("external filter '%s' does not support filter protocol version 2", cmd);

OK

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

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

quoted

+		}

Shouldn't we free 'struct strbuf **pair', maybe allocated by the
strbuf_split_str() function, and reset to NULL?

True. strbuf_list_free() should be enough.

quoted

+	fflush(NULL);

Why this fflush(NULL) is needed here?

This flushes all open output streams. The single filter does the same.

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.

quoted

+
+	err = strlen(filter_type) > PKTLINE_DATA_MAXLEN;
+	if (err)
+		goto done;

Errr... this should never happen.  We control which capabilities
we pass, it can be only "clean" or "smudge", nothing else. Those
would always be shorter than PKTLINE_DATA_MAXLEN.

Never mind that that is "command=smudge\n" etc. that needs to
be shorter that PKTLINE_DATA_MAXLEN!

So, IMHO it should be at most assert, and needs to be corrected
anyway.

OK!

This should never happen, PATH_MAX everywhere is much shorter
than PKTLINE_DATA_MAXLEN / LARGE_PACKET_MAX.  Or is it?

Anyway, we should probably explain or warn

  		error("path name too long: '%s'", path);

OK

quoted

+			/*
+			 * Something went wrong with the protocol filter.
+			 * Force shutdown and restart if another blob requires filtering!

Is this exclamation mark '!' here necessary?

No.


Thanks,
Lars