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

From: Jakub Narębski <jnareb@gmail.com>
Date: Sep 27, 2016, 15:37 UTC
Message-ID: <2a604438-b6cd-876d-0ec2-90027dea99b9@gmail.com>
In-Reply-To: <20160920190247.82189-12-larsxschneider@gmail.com>

Part second of the review of 11/11.

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

Show 5 quoted lines

> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
> new file mode 100755
> index 0000000..c13a631
> --- /dev/null
> +++ b/contrib/long-running-filter/example.pl

[...]

> +( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
> +( packet_txt_read() eq ( 0, "version=2" ) )         || die "bad version";
> +( packet_bin_read() eq ( 1, "" ) )                  || die "bad version end";

What I would like to see here is some kind of packet_read_list() or packet_txt_read_list() that reads until flush packet or EOF, and returns list of chomp-ed lines (without LF terminator).

Then you can examine those lines:

   my @lines = packet_read_list();
   $lines[0] eq "git-filter-client"      or die "bad initialization: '$lines[0]'";
   grep { $_ eq "version=2" } @lines     or die "bad version: version=2 not found";

Note: I have not checked that I got operator precedence right.

> +
> +packet_txt_write("git-filter-server");
> +packet_txt_write("version=2");

Here using packet_write_list() would help us to not forget to send the flush packet:

  packet_write_list(
  	"git-filter-server",
  	"version=2"
  );

[...]

Show 34 quoted lines

> diff --git a/convert.c b/convert.c
> index 597f561..bd66257 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -3,6 +3,7 @@
>  #include "run-command.h"
>  #include "quote.h"
>  #include "sigchain.h"
> +#include "pkt-line.h"
>  
>  /*
>   * convert.c - convert a file when checking it out and checking it in.
> @@ -442,7 +443,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
>  	return (write_err || status);
>  }
>  
> -static int apply_filter(const char *path, const char *src, size_t len, int fd,
> +static int apply_single_file_filter(const char *path, const char *src, size_t len, int fd,
>                          struct strbuf *dst, const char *cmd)
>  {
>  	/*
> @@ -456,12 +457,6 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>  	struct async async;
>  	struct filter_params params;
>  
> -	if (!cmd || !*cmd)
> -		return 0;
> -
> -	if (!dst)
> -		return 1;
> -
>  	memset(&async, 0, sizeof(async));
>  	async.proc = filter_buffer_or_fd;
>  	async.data = &params;

I have reordered a few chunks of this patch to make it easier to see what happens here, and to review this part of patch.

[moved here from further in the patch]

Show 11 quoted lines

> +static int apply_filter(const char *path, const char *src, size_t len,
> +                        int fd, struct strbuf *dst, struct convert_driver *drv,
> +                        const unsigned int wanted_capability)
> +{
> +	const char *cmd = NULL;
> +
> +	if (!drv)
> +		return 0;
> +
> +	if (!dst)
> +		return 1;

To reduce the size of this patch (which is not yet of the size that would make vger reject the email :-/), perhaps this split into apply_single_file_filter() and apply_filter(), without yet adding apply_multi_file_filter(). The apply_filter() would be semi-simple wrapper, with the same signature as above.

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

That is: if we want `clean` capability, and `process` is not set, and there is `clean` filter.

Though the following would also work...

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

Show 5 quoted lines

> +		cmd = drv->clean;
> +	else if (!drv->process && (CAP_SMUDGE & wanted_capability) && drv->smudge)
> +		cmd = drv->smudge;
> +
> +	if (cmd && *cmd)

... thanks to this check (the 'cmd' part, which we need to check anyway).

'cmd = drv->clean', then 'if (cmd)' is the same as 'if (drv->clean)', then 'cmd = drv->clean', then 'if (cmd)', isn't it.

Not sure if it would be more readable, or less readable.

CAP_CLEAN and CAP_SMUDGE are, in theory, mutually exclusive. Note that the above order prefers `smudge` to `clean` if both given, while in other places we prefer `clean` to `smudge` if both given.

Show 6 quoted lines

> +		return apply_single_file_filter(path, src, len, fd, dst, cmd);
> +	else if (drv->process && *drv->process)
> +		return apply_multi_file_filter(path, src, len, fd, dst, drv->process, wanted_capability);
> +
> +	return 0;
> +}

Nice and clean wrapper.

> +

[moved here from further in the patch]

Show 9 quoted lines

> @@ -839,7 +1140,7 @@ int would_convert_to_git_filter_fd(const char *path)
>  	if (!ca.drv->required)
>  		return 0;
>  
> -	return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);
> +	return apply_filter(path, NULL, 0, -1, NULL, ca.drv, CAP_CLEAN);
>  }
>  
>  const char *get_convert_attr_ascii(const char *path)

This would also be a part of move adding apply_single_file_filter() and converting apply_filter() to wrapper around single / multi file filters.

Show 6 quoted lines

> @@ -493,14 +488,317 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>  	return !err;
>  }
>  
> +#define CAP_CLEAN    (1u<<0)
> +#define CAP_SMUDGE   (1u<<1)

It's a pity that ANSI C does not include binary constants, like e.g. modern Perl, that is '0b0001u' and '0b0010u'; we could use hexadecimal constants '0x01u' and '0x02u', but perhaps the above is more readable, and as performant.

Show 25 quoted lines

> +
> +struct cmd2process {
> +	struct hashmap_entry ent; /* must be the first member! */
> +	unsigned int supported_capabilities;
> +	const char *cmd;
> +	struct child_process process;
> +};
> +
> +static int cmd_process_map_initialized;
> +static struct hashmap cmd_process_map;
> +
> +static int cmd2process_cmp(const struct cmd2process *e1,
> +                           const struct cmd2process *e2,
> +                           const void *unused)
> +{
> +	return strcmp(e1->cmd, e2->cmd);
> +}
> +
> +static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
> +{
> +	struct cmd2process key;
> +	hashmap_entry_init(&key, strhash(cmd));
> +	key.cmd = cmd;
> +	return hashmap_get(hashmap, &key, NULL);
> +}

All right, basic hashmap for the list of command processes, here so that we can find correct driver for current file, and reuse it if it was started already.

I see that git code does not use /* ............ */ etc to separate subsections / subparts of a file, so I won't ask for it ;-(

Show 10 quoted lines

> +
> +static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *entry)
> +{
> +	if (!entry)
> +		return;
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	/*
> +	 * We kill the filter most likely because an error happened already.
> +	 * That's why we are not interested in any error code here.
> +	 */

Good explanation.

Show 7 quoted lines

> +	close(entry->process.in);
> +	close(entry->process.out);
> +	sigchain_pop(SIGPIPE);
> +	finish_command(&entry->process);
> +	hashmap_remove(hashmap, entry, NULL);
> +	free(entry);
> +}

That's more 'kill_and_remove_...', but that would make too long function name.

Small and readable.  Nice.

Show 10 quoted lines

> +
> +static int packet_write_list(int fd, const char *line, ...)
> +{
> +	va_list args;
> +	int err;
> +	va_start(args, line);
> +	for (;;) {
> +		if (!line)
> +			break;
> +		if (strlen(line) > PKTLINE_DATA_MAXLEN)

Here we see that having PKTLINE_DATA_MAXLEN (or LARGE_PACKET_DATA_MAX) constant is quite useful.

> +			return -1;
> +		err = packet_write_fmt_gently(fd, "%s\n", line);

I wonder if adding the fact that we are writing text packets to function name would be worth it. Nah. Also, it is file-local (static) function.

Show 7 quoted lines

> +		if (err)
> +			return err;
> +		line = va_arg(args, const char*);
> +	}
> +	va_end(args);
> +	return packet_flush_gently(fd);
> +}

Nice abstraction.

Show 28 quoted lines

> +
> +static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)
> +{
> +	int err;
> +	struct cmd2process *entry;
> +	struct child_process *process;
> +	const char *argv[] = { cmd, NULL };
> +	struct string_list cap_list = STRING_LIST_INIT_NODUP;
> +	char *cap_buf;
> +	const char *cap_name;
> +
> +	entry = xmalloc(sizeof(*entry));
> +	hashmap_entry_init(entry, strhash(cmd));
> +	entry->cmd = cmd;
> +	entry->supported_capabilities = 0;
> +	process = &entry->process;
> +
> +	child_process_init(process);
> +	process->argv = argv;
> +	process->use_shell = 1;
> +	process->in = -1;
> +	process->out = -1;
> +
> +	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?

> +
> +	sigchain_push(SIGPIPE, SIG_IGN);

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

> +
> +	err = packet_write_list(process->in, "git-filter-client", "version=2", NULL);
> +	if (err)
> +		goto done;

Ugh, error / exception handling in C.

Show 8 quoted lines

> +
> +	err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
> +	if (err) {
> +		error("external filter '%s' does not support long running filter protocol", cmd);
> +		goto done;
> +	}
> +	err = strcmp(packet_read_line(process->out, NULL), "version=2");
> +	if (err)

We could have described the error here better.

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

But this is probably not necessary; it should be rare to find filter process that support the protocol halfway right.

> +		goto done;

I guess this would get more complicated if/when there is need for new version of the protocol.

Shouldn't we read flush packet here? Ah, sorry, we know that we should get only two lines from the `process` filter driver, and not variable number of lines, so there is no need to flush here.

Disregard my comments about lack of flush packet in the example of long running filter script. Well, unless the protocol itself would get adjusted to always use flush packet to terminate set of metadata lines, even if number of lines is fixed.

> +
> +	err = packet_write_list(process->in, "clean=true", "smudge=true", NULL);

So I see that Git sends all capabilities it supports, not only those that given git command needs (which might be hard to find out).

If it were possible at this point of code for Git to know, for example, that it would only do `clean` operation, shouldn't it write "clean=true", "smudge=false"? ;-PPP

Note that this "=true" is totally spurious. Maybe "capability=clean", or just "clean" would make a better protocol?

Show 24 quoted lines

> +
> +	for (;;) {
> +		cap_buf = packet_read_line(process->out, NULL);
> +		if (!cap_buf)
> +			break;
> +		string_list_split_in_place(&cap_list, cap_buf, '=', 1);
> +
> +		if (cap_list.nr != 2 || strcmp(cap_list.items[1].string, "true"))
> +			continue;
> +
> +		cap_name = cap_list.items[0].string;
> +		if (!strcmp(cap_name, "clean")) {
> +			entry->supported_capabilities |= CAP_CLEAN;
> +		} else if (!strcmp(cap_name, "smudge")) {
> +			entry->supported_capabilities |= CAP_SMUDGE;
> +		} else {
> +			warning(
> +				"external filter '%s' requested unsupported filter capability '%s'",
> +				cmd, cap_name
> +			);
> +		}
> +
> +		string_list_clear(&cap_list, 0);
> +	}

I guess there is a reason why it was not extracted into helper function?

Well, both because handling of variable-length response, where multiple lines must be analyzed, happens only once, and also because returning list of variable-length strings in C is hard (alloca? string_list?).

Show 9 quoted lines

> +
> +done:
> +	sigchain_pop(SIGPIPE);
> +
> +	if (err || errno == EPIPE) {
> +		error("initialization for external filter '%s' failed", cmd);
> +		kill_multi_file_filter(hashmap, entry);
> +		return NULL;
> +	}

Good.

Show 6 quoted lines

> +
> +	hashmap_add(hashmap, entry);
> +	return entry;
> +}
> +
> +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.

> +	struct strbuf **pair;

Shouldn't it be initialized to NULL, like in strbuf_split_buf() code?

Show 6 quoted lines

> +	char *line;
> +	for (;;) {
> +		line = packet_read_line(fd, NULL);
> +		if (!line)
> +			break;
> +		pair = strbuf_split_str(line, '=', 2);

Why, oh why, there is no Documentation/technical/api-strbuf.txt? Well, strbuf.h is really well commented... but perhaps not enough.

Show 5 quoted lines

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

> +		}

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

Show 12 quoted lines

> +	}
> +}
> +
> +static int apply_multi_file_filter(const char *path, const char *src, size_t len,
> +                                   int fd, struct strbuf *dst, const char *cmd,
> +                                   const unsigned int wanted_capability)
> +{
> +	int err;
> +	struct cmd2process *entry;
> +	struct child_process *process;
> +	struct stat file_stat;
> +	struct strbuf nbuf = STRBUF_INIT;

This name doesn't tell us much, but I guess there is precedence?

Show 12 quoted lines

> +	struct strbuf filter_status = STRBUF_INIT;
> +	char *filter_type;
> +
> +	if (!cmd_process_map_initialized) {
> +		cmd_process_map_initialized = 1;
> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
> +		entry = NULL;
> +	} else {
> +		entry = find_multi_file_filter_entry(&cmd_process_map, cmd);
> +	}
> +
> +	fflush(NULL);

Why this fflush(NULL) is needed here?

Show 7 quoted lines

> +
> +	if (!entry) {
> +		entry = start_multi_file_filter(&cmd_process_map, cmd);
> +		if (!entry)
> +			return 0;
> +	}
> +	process = &entry->process;

All right, we start process filter, or get existing instance.

> +
> +	if (!(wanted_capability & entry->supported_capabilities))
> +		return 0;

If filter doesn't support wanted capability, then Git just wouldn't filter. Looks good to me.

Show 7 quoted lines

> +
> +	if (CAP_CLEAN & wanted_capability)
> +		filter_type = "clean";
> +	else if (CAP_SMUDGE & wanted_capability)
> +		filter_type = "smudge";
> +	else
> +		die("unexpected filter type");

This should never happen; we should always request one of those capabilities, and only one.

Show 6 quoted lines

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

> +
> +	sigchain_push(SIGPIPE, SIG_IGN);

Right, we want to handle errors ourself.

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

Show 6 quoted lines

> +
> +	err = packet_write_fmt_gently(process->in, "command=%s\n", filter_type);
> +	if (err)
> +		goto done;
> +
> +	err = strlen(path) > PKTLINE_DATA_MAXLEN;

Actually

  +	err = strlen(path) > PKTLINE_DATA_MAXLEN - strlen("pathname=\n");

This version was chosen in the very unlikely case if strlen(path) + strlen("pathname=\n") would overflow.

  +	err = strlen("pathname=") + strlen(path) + strlen("\n") > PKTLINE_DATA_MAXLEN;

;-)

> +	if (err)
> +		goto done;

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

Though if length of pathname is of the order of 2^16, I don't think printing it would help :-)

Show 8 quoted lines

> +
> +	err = packet_write_fmt_gently(process->in, "pathname=%s\n", path);
> +	if (err)
> +		goto done;
> +
> +	err = packet_flush_gently(process->in);
> +	if (err)
> +		goto done;

All right, this list of values, currently composed of "command=<sth>" and "pathname=<sth>" - both of which are required, may be variable length, so we need flush packet.

Show 7 quoted lines

> +
> +	if (fd >= 0)
> +		err = write_packetized_from_fd(fd, process->in);
> +	else
> +		err = write_packetized_from_buf(src, len, process->in);
> +	if (err)
> +		goto done;

Looks good, and I think it is better if the caller decided rather that write_packetized(fd, src, len, process->in) deciding.

Note for implementers: write in full, read in full, no streaming support (Git doesn't start to read filter output until it writes to filter in full). This is opposed to what `clean` and `smudge` filters support.

Show 12 quoted lines

> +
> +	read_multi_file_filter_values(process->out, &filter_status);
> +	err = strcmp(filter_status.buf, "success");
> +	if (err)
> +		goto done;
> +
> +	err = read_packetized_to_buf(process->out, &nbuf) < 0;
> +	if (err)
> +		goto done;
> +
> +	read_multi_file_filter_values(process->out, &filter_status);
> +	err = strcmp(filter_status.buf, "success");

Looks good to me (LGTM).

Show 18 quoted lines

> +
> +done:
> +	sigchain_pop(SIGPIPE);
> +
> +	if (err || errno == EPIPE) {
> +		if (!strcmp(filter_status.buf, "error")) {
> +			/* The filter signaled a problem with the file. */
> +		} else if (!strcmp(filter_status.buf, "abort")) {
> +			/*
> +			 * The filter signaled a permanent problem. Don't try to filter
> +			 * files with the same command for the lifetime of the current
> +			 * Git process.
> +			 */
> +			 entry->supported_capabilities &= ~wanted_capability;
> +		} else {
> +			/*
> +			 * Something went wrong with the protocol filter.
> +			 * Force shutdown and restart if another blob requires filtering!

Is this exclamation mark '!' here necessary?

> +			 */
> +			error("external filter '%s' failed", cmd);
> +			kill_multi_file_filter(&cmd_process_map, entry);
> +		}

Looks good. Three error conditions: resumable error from filter, failure of filter (kill, would restart if necessary), and abort.

Show 5 quoted lines

> +	} else {
> +		strbuf_swap(dst, &nbuf);
> +	}
> +	strbuf_release(&nbuf);
> +	return !err;

I guess this is for `filter.<driver>.required` to handle correctly filter error-ing out, or filter failing, while not aborting if filter simply doesn't support `clean` or `smudge` capability.

Show 8 quoted lines

> +}
> +
>  static struct convert_driver {
>  	const char *name;
>  	struct convert_driver *next;
>  	const char *smudge;
>  	const char *clean;
> +	const char *process;

LGTM.

>  	int required;
>  } *user_convert, **user_convert_tail;
>

[a section of chunk moved up]

Show 10 quoted lines

>  static int read_convert_config(const char *var, const char *value, void *cb)
>  {
>  	const char *key, *name;
> @@ -538,6 +836,9 @@ static int read_convert_config(const char *var, const char *value, void *cb)
>  	if (!strcmp("clean", key))
>  		return git_config_string(&drv->clean, var, value);
>  
> +	if (!strcmp("process", key))
> +		return git_config_string(&drv->process, var, value);
> +

LGTM.

>  	if (!strcmp("required", key)) {
>  		drv->required = git_config_bool(var, value);
>  		return 0;

[a chunk of diff moved up]

Show 5 quoted lines

> @@ -872,18 +1173,12 @@ int convert_to_git(const char *path, const char *src, size_t len,
>                     struct strbuf *dst, enum safe_crlf checksafe)
>  {
>  	int ret = 0;
> -	const char *filter = NULL;

All right, this was (I think) moved into apply_filter()...

> -	int required = 0;

...but this looks like just a removal of a temporary variable, which could have been done in a separate preparatory patch.

Show 13 quoted lines

>  	struct conv_attrs ca;
>  
>  	convert_attrs(&ca, path);
> -	if (ca.drv) {
> -		filter = ca.drv->clean;
> -		required = ca.drv->required;
> -	}
>  
> -	ret |= apply_filter(path, src, len, -1, dst, filter);
> -	if (!ret && required)
> +	ret |= apply_filter(path, src, len, -1, dst, ca.drv, CAP_CLEAN);
> +	if (!ret && ca.drv && ca.drv->required)
>  		die("%s: clean filter '%s' failed", path, ca.drv->name);

Looks good. (And could be a part of patch adding apply_filter() as wrapper.)

Show 8 quoted lines

>  
>  	if (ret && dst) {
> @@ -905,9 +1200,9 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
>  	convert_attrs(&ca, path);
>  
>  	assert(ca.drv);
> -	assert(ca.drv->clean);
> +	assert(ca.drv->clean || ca.drv->process);

Hmmm... asserts.  Well, they were here.

Show 5 quoted lines

>  
> -	if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
> +	if (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN))
>  		die("%s: clean filter '%s' failed", path, ca.drv->name);
>

Looks good. (And could be a part of patch adding apply_filter() as wrapper.)

Show 14 quoted lines

>  	crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
> @@ -919,15 +1214,9 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>  					    int normalizing)
>  {
>  	int ret = 0, ret_filter = 0;
> -	const char *filter = NULL;
> -	int required = 0;
>  	struct conv_attrs ca;
>  
>  	convert_attrs(&ca, path);
> -	if (ca.drv) {
> -		filter = ca.drv->smudge;
> -		required = ca.drv->required;
> -	}

Well, this is the same change as a bit eaelier.

Show 27 quoted lines

>  
>  	ret |= ident_to_worktree(path, src, len, dst, ca.ident);
>  	if (ret) {
> @@ -936,9 +1225,10 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>  	}
>  	/*
>  	 * CRLF conversion can be skipped if normalizing, unless there
> -	 * is a smudge filter.  The filter might expect CRLFs.
> +	 * is a smudge or process filter (even if the process filter doesn't
> +	 * support smudge).  The filters might expect CRLFs.
>  	 */
> -	if (filter || !normalizing) {
> +	if ((ca.drv && (ca.drv->smudge || ca.drv->process)) || !normalizing) {
>  		ret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);
>  		if (ret) {
>  			src = dst->buf;
> @@ -946,8 +1236,8 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>  		}
>  	}
>  
> -	ret_filter = apply_filter(path, src, len, -1, dst, filter);
> -	if (!ret_filter && required)
> +	ret_filter = apply_filter(path, src, len, -1, dst, ca.drv, CAP_SMUDGE);
> +	if (!ret_filter && ca.drv && ca.drv->required)
>  		die("%s: smudge filter %s failed", path, ca.drv->name);
>  
>  	return ret | ret_filter;

Looks good to me. I understand ca.drv is checked so that ca.drv->required and/or ca.drv->smudge / ca.drv->clean / ca.drv->process can be safely checked, isn't it.

Show 6 quoted lines

> @@ -1399,7 +1689,7 @@ struct stream_filter *get_stream_filter(const char *path, const unsigned char *s
>  	struct stream_filter *filter = NULL;
>  
>  	convert_attrs(&ca, path);
> -	if (ca.drv && (ca.drv->smudge || ca.drv->clean))
> +	if (ca.drv && (ca.drv->process || ca.drv->smudge || ca.drv->clean))

LGTM, understandable change.

Show 12 quoted lines

>  		return NULL;
>  
>  	if (ca.crlf_action == CRLF_AUTO || ca.crlf_action == CRLF_AUTO_CRLF)
> diff --git a/pkt-line.h b/pkt-line.h
> index 6df8449..3d873f3 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -86,6 +86,7 @@ ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out);
>  
>  #define DEFAULT_PACKET_MAX 1000
>  #define LARGE_PACKET_MAX 65520
> +#define PKTLINE_DATA_MAXLEN (LARGE_PACKET_MAX - 4)

What the... didn't you use PKTLINE_DATA_MAXLEN in one of earlier patches in this series? How this even...?

Show 7 quoted lines

>  extern char packet_buffer[LARGE_PACKET_MAX];
>  
>  #endif
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index dc50938..210c4f6 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh

I'll stop here, and I'll finish the review later.

To be continued,

-- 
Jakub Narębski