patch, 3 partsGit filter protocol

>  test_expect_success check '
>  
> -        cmp test.o test &&
> -        cmp test.o test.t &&
> +        test_cmp test.o test &&
> +        test_cmp test.o test.t &&
>  
>          # ident should be stripped in the repository
>          git diff --raw --exit-code :test :test.i &&
> @@ -47,10 +47,10 @@ test_expect_success check '
>          embedded=$(sed -ne "$script" test.i) &&
>          test "z$id" = "z$embedded" &&
>  
> -        git cat-file blob :test.t > test.r &&
> +        git cat-file blob :test.t >test.r &&
>  
> -        ./rot13.sh < test.o > test.t &&
> -        cmp test.r test.t
> +        ./rot13.sh < test.o >test.t &&

> Considering how close it is to your patch, you might also want to
> remove spaces after '<'.
>
> There is only one occurrence in this file and it's in a line you are
> already modifying.

> From: Lars Schneider <larsxschneider@gmail.com>
>
> Git's clean/smudge mechanism invokes an external filter process for every
> single blob that is affected by a filter. If Git filters a lot of blobs
> then the startup time of the external filter processes can become a
> significant part of the overall Git execution time.
>
> This patch adds the filter.<driver>.useProtocol option which, if enabled,
> keeps the external filter process running and processes all blobs with
> the following protocol over stdin/stdout.
>
> 1. Git starts the filter on first usage and expects a welcome message
> with protocol version number:
> 	Git <-- Filter: "git-filter-protocol\n"
> 	Git <-- Filter: "version 1"

>
> 2. Git sends the command (either "smudge" or "clean"), the filename, the
> content size in bytes, and the content separated by a newline character:
> 	Git --> Filter: "smudge\n"
> 	Git --> Filter: "testfile.dat\n"
> 	Git --> Filter: "7\n"
> 	Git --> Filter: "CONTENT"
>
> 3. The filter is expected to answer with the result content size in
> bytes and the result content separated by a newline character:
> 	Git <-- Filter: "15\n"
> 	Git <-- Filter: "SMUDGED_CONTENT"
>
> 4. The filter is expected to wait for the next file in step 2, again.
>
> Please note that the protocol filters do not support stream processing
> with this implemenatation because the filter needs to know the length of

> the result in advance. A protocol version 2 could address this in a
> future patch.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> Helped-by: Martin-Louis Bright <mlbright@gmail.com>
> ---
>  Documentation/gitattributes.txt |  41 +++++++-
>  convert.c                       | 210 ++++++++++++++++++++++++++++++++++++++--
>  t/t0021-conversion.sh           | 170 ++++++++++++++++++++++++++++++++
>  t/t0021/rot13.pl                |  80 +++++++++++++++
>  4 files changed, 494 insertions(+), 7 deletions(-)
>  create mode 100755 t/t0021/rot13.pl
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 8882a3e..7026d62 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -300,7 +300,10 @@ checkout, when the `smudge` command is specified, the command is
>  fed the blob object from its standard input, and its standard
>  output is used to update the worktree file.  Similarly, the
>  `clean` command is used to convert the contents of worktree file
> -upon checkin.
> +upon checkin. By default these commands process only a single
> +blob and terminate. If the setting filter.<driver>.useProtocol is
> +enabled then Git can process all blobs with a single filter command
> +invocation (see filter protocol below).
>
>  One use of the content filtering is to massage the content into a shape
>  that is more convenient for the platform, filesystem, and the user to use.
> @@ -375,6 +378,42 @@ substitution.  For example:
>  ------------------------
>
>
> +Filter Protocol
> +^^^^^^^^^^^^^^^
> +
> +If the setting filter.<driver>.useProtocol is enabled then Git
> +can process all blobs with a single filter command invocation
> +by talking with the following protocol over stdin/stdout.
> +
> +Git starts the filter on first usage and expects a welcome
> +message with protocol version number:
> +------------------------
> +Git < Filter: "git-filter-protocol\n"
> +Git < Filter: "version 1"
> +------------------------
> +
> +Afterwards Git sends a blob command (either "smudge" or "clean"),
> +the filename, the content size in bytes, and the content separated
> +by a newline character:
> +------------------------
> +Git > Filter: "smudge\n"
> +Git > Filter: "testfile.dat\n"
> +Git > Filter: "7\n"
> +Git > Filter: "CONTENT"
> +------------------------
> +
> +The filter is expected to answer with the result content size in
> +bytes and the result content separated by a newline character:
> +------------------------
> +Git < Filter: "15\n"
> +Git < Filter: "SMUDGED_CONTENT"
> +------------------------
> +
> +Afterwards the filter is expected to wait for the next blob process
> +command. A demo implementation can be found in `t/t0021/rot13.pl`
> +located in the Git core repository.
> +
> +
>  Interaction between checkin/checkout attributes
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> diff --git a/convert.c b/convert.c
> index 522e2c5..91ce86f 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -481,12 +481,188 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>  	return ret;
>  }
>
> +static int cmd_process_map_init = 0;
> +static struct hashmap cmd_process_map;
> +
> +struct cmd2process {
> +	struct hashmap_entry ent; /* must be the first member! */
> +	const char *cmd;
> +	long protocol;
> +	struct child_process process;
> +};
> +
> +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_protocol_filter_entry(const char *cmd)
> +{
> +	struct cmd2process k;
> +	hashmap_entry_init(&k, strhash(cmd));
> +	k.cmd = cmd;
> +	return hashmap_get(&cmd_process_map, &k, NULL);
> +}
> +
> +static void stop_protocol_filter(struct cmd2process *entry) {
> +	if (!entry)
> +		return;
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	close(entry->process.in);
> +	close(entry->process.out);
> +	sigchain_pop(SIGPIPE);
> +	finish_command(&entry->process);
> +	child_process_clear(&entry->process);
> +	hashmap_remove(&cmd_process_map, entry, NULL);
> +	free(entry);
> +}
> +
> +static struct cmd2process *start_protocol_filter(const char *cmd)
> +{
> +	int ret = 1;
> +	struct cmd2process *entry = NULL;
> +	struct child_process *process = NULL;
> +	struct strbuf nbuf = STRBUF_INIT;
> +	struct string_list split = STRING_LIST_INIT_NODUP;
> +	const char *argv[] = { NULL, NULL };
> +	const char *header = "git-filter-protocol\nversion";
> +
> +	entry = xmalloc(sizeof(*entry));
> +	hashmap_entry_init(entry, strhash(cmd));
> +	entry->cmd = cmd;
> +	process = &entry->process;
> +
> +	child_process_init(process);
> +	argv[0] = cmd;
> +	process->argv = argv;
> +	process->use_shell = 1;
> +	process->in = -1;
> +	process->out = -1;
> +
> +	if (start_command(process)) {
> +		error("cannot fork to run external persistent filter '%s'", cmd);
> +		return NULL;
> +	}
> +	strbuf_reset(&nbuf);
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
> +	sigchain_pop(SIGPIPE);
> +
> +	strbuf_stripspace(&nbuf, 0);
> +	string_list_split_in_place(&split, nbuf.buf, ' ', 2);
> +	ret &= split.nr > 1;
> +	ret &= strncmp(header, split.items[0].string, strlen(header)) == 0;
> +	if (ret) {
> +		entry->protocol = strtol(split.items[1].string, NULL, 10);
> +		switch (entry->protocol) {
> +			case 1:
> +				break;
> +			default:
> +				ret = 0;
> +				error("unsupported protocol version %s for external persistent filter '%s'",
> +					nbuf.buf, cmd);
> +		}
> +	}
> +	string_list_clear(&split, 0);
> +	strbuf_release(&nbuf);
> +
> +	if (!ret) {
> +		error("initialization for external persistent filter '%s' failed", cmd);
> +		return NULL;
> +	}
> +
> +	hashmap_add(&cmd_process_map, entry);
> +	return entry;
> +}
> +
> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
> +						int fd, struct strbuf *dst, const char *cmd,
> +						const char *filter_type)
> +{
> +	int ret = 1;
> +	struct cmd2process *entry = NULL;
> +	struct child_process *process = NULL;
> +	struct stat fileStat;
> +	struct strbuf nbuf = STRBUF_INIT;
> +	size_t nbuf_len;
> +	char *strtol_end;
> +	char c;
> +
> +	if (!cmd || !*cmd)
> +		return 0;
> +
> +	if (!dst)
> +		return 1;
> +
> +	if (!cmd_process_map_init) {
> +		cmd_process_map_init = 1;
> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
> +	} else {
> +		entry = find_protocol_filter_entry(cmd);
> +	}
> +
> +	if (!entry){
> +		entry = start_protocol_filter(cmd);
> +		if (!entry) {
> +			stop_protocol_filter(entry);
> +			return 0;
> +		}
> +	}
> +	process = &entry->process;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	switch (entry->protocol) {
> +		case 1:
> +			if (fd >= 0 && !src) {
> +				ret &= fstat(fd, &fileStat) != -1;
> +				len = fileStat.st_size;
> +			}
> +			strbuf_reset(&nbuf);
> +			strbuf_addf(&nbuf, "%s\n%s\n%zu\n", filter_type, path, len);
> +			ret &= write_str_in_full(process->in, nbuf.buf) > 1;
> +			if (len > 0) {
> +				if (src)
> +					ret &= write_in_full(process->in, src, len) == len;
> +				else if (fd >= 0)
> +					ret &= copy_fd(fd, process->in) == 0;
> +				else
> +					ret &= 0;
> +			}
> +
> +			strbuf_reset(&nbuf);
> +			while (xread(process->out, &c, 1) == 1 && c != '\n')
> +				strbuf_addchars(&nbuf, c, 1);
> +			nbuf_len = (size_t)strtol(nbuf.buf, &strtol_end, 10);
> +			ret &= (strtol_end != nbuf.buf && errno != ERANGE);
> +			strbuf_reset(&nbuf);
> +			if (nbuf_len > 0)
> +				ret &= strbuf_read_once(&nbuf, process->out, nbuf_len) == nbuf_len;
> +			break;
> +		default:
> +			ret = 0;
> +	}
> +	sigchain_pop(SIGPIPE);
> +
> +	if (ret) {
> +		strbuf_swap(dst, &nbuf);
> +	} else {
> +		// Something went wrong with the protocol filter. Force shutdown!
> +		stop_protocol_filter(entry);
> +	}
> +	strbuf_release(&nbuf);
> +	return ret;
> +}
> +
>  static struct convert_driver {
>  	const char *name;
>  	struct convert_driver *next;
>  	const char *smudge;
>  	const char *clean;
>  	int required;
> +	int use_protocol;
>  } *user_convert, **user_convert_tail;
>
>  static int read_convert_config(const char *var, const char *value, void *cb)
> @@ -526,6 +702,11 @@ 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("useprotocol", key)) {
> +		drv->use_protocol = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>  	if (!strcmp("required", key)) {
>  		drv->required = git_config_bool(var, value);
>  		return 0;
> @@ -823,7 +1004,10 @@ 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);
> +	if (ca.drv->use_protocol)
> +		return apply_protocol_filter(path, NULL, 0, -1, NULL, ca.drv->clean, "clean");
> +	else
> +		return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);
>  }
>
>  const char *get_convert_attr_ascii(const char *path)
> @@ -857,16 +1041,20 @@ int convert_to_git(const char *path, const char *src, size_t len,
>  {
>  	int ret = 0;
>  	const char *filter = NULL;
> -	int required = 0;
> +	int required = 0, use_protocol = 0;
>  	struct conv_attrs ca;
>
>  	convert_attrs(&ca, path);
>  	if (ca.drv) {
>  		filter = ca.drv->clean;
>  		required = ca.drv->required;
> +		use_protocol = ca.drv->use_protocol;
>  	}
>
> -	ret |= apply_filter(path, src, len, -1, dst, filter);
> +	if (use_protocol)
> +		ret |= apply_protocol_filter(path, src, len, -1, dst, filter, "clean");
> +	else
> +		ret |= apply_filter(path, src, len, -1, dst, filter);
>  	if (!ret && required)
>  		die("%s: clean filter '%s' failed", path, ca.drv->name);
>
> @@ -885,13 +1073,19 @@ int convert_to_git(const char *path, const char *src, size_t len,
>  void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
>  			      enum safe_crlf checksafe)
>  {
> +	int ret = 0;
>  	struct conv_attrs ca;
>  	convert_attrs(&ca, path);
>
>  	assert(ca.drv);
>  	assert(ca.drv->clean);
>
> -	if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
> +	if (ca.drv->use_protocol)
> +		ret = apply_protocol_filter(path, NULL, 0, fd, dst, ca.drv->clean, "clean");
> +	else
> +		ret = apply_filter(path, NULL, 0, fd, dst, ca.drv->clean);
> +
> +	if (!ret)
>  		die("%s: clean filter '%s' failed", path, ca.drv->name);
>
>  	crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
> @@ -904,13 +1098,14 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>  {
>  	int ret = 0, ret_filter = 0;
>  	const char *filter = NULL;
> -	int required = 0;
> +	int required = 0, use_protocol = 0;
>  	struct conv_attrs ca;
>
>  	convert_attrs(&ca, path);
>  	if (ca.drv) {
>  		filter = ca.drv->smudge;
>  		required = ca.drv->required;
> +		use_protocol = ca.drv->use_protocol;
>  	}
>
>  	ret |= ident_to_worktree(path, src, len, dst, ca.ident);
> @@ -930,7 +1125,10 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>  		}
>  	}
>
> -	ret_filter = apply_filter(path, src, len, -1, dst, filter);
> +	if (use_protocol)
> +		ret_filter = apply_protocol_filter(path, src, len, -1, dst, filter, "smudge");
> +	else
> +		ret_filter |= apply_filter(path, src, len, -1, dst, filter);
>  	if (!ret_filter && required)
>  		die("%s: smudge filter %s failed", path, ca.drv->name);
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index a05a8d2..d9077ea 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -268,4 +268,174 @@ test_expect_success 'disable filter with empty override' '
>  	test_must_be_empty err
>  '
>
> +test_expect_success 'required protocol filter should filter data' '
> +	test_config_global filter.protocol.smudge \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> +	test_config_global filter.protocol.clean \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> +	test_config_global filter.protocol.useprotocol true &&
> +	test_config_global filter.protocol.required true &&
> +	rm -rf repo &&
> +	mkdir repo &&
> +	(
> +		cd repo &&
> +		git init &&
> +
> +		echo "*.r filter=protocol" >.gitattributes &&
> +		git add . &&
> +		git commit . -m "test commit" &&
> +		git branch empty &&
> +
> +		cat ../test.o >test.r &&
> +		echo "test22" >test2.r &&
> +		echo "test333" >test3.r &&
> +
> +		rm -f output.log &&
> +		git add . &&
> +		sort output.log | uniq -c | sed "s/^[ ]*//" >uniq_output.log &&
> +		cat >expected_add.log <<-\EOF &&
> +			1 IN: clean test.r 57 [OK] -- OUT: 57 [OK]
> +			1 IN: clean test2.r 7 [OK] -- OUT: 7 [OK]
> +			1 IN: clean test3.r 8 [OK] -- OUT: 8 [OK]
> +			1 start
> +			1 wrote version
> +		EOF
> +		test_cmp expected_add.log uniq_output.log &&
> +
> +		printf "" >output.log &&
> +		git commit . -m "test commit" &&
> +		sort output.log | uniq -c | sed "s/^[ ]*//" |
> +			sed "s/^\([0-9]\) IN: clean/x IN: clean/" >uniq_output.log &&
> +		cat >expected_commit.log <<-\EOF &&
> +			x IN: clean test.r 57 [OK] -- OUT: 57 [OK]
> +			x IN: clean test2.r 7 [OK] -- OUT: 7 [OK]
> +			x IN: clean test3.r 8 [OK] -- OUT: 8 [OK]
> +			1 start
> +			1 wrote version
> +		EOF
> +		test_cmp expected_commit.log uniq_output.log &&
> +
> +		printf "" >output.log &&
> +		rm -f test?.r &&
> +		git checkout . &&
> +		cat output.log | grep -v "IN: clean" >smudge_output.log &&
> +		cat >expected_checkout.log <<-\EOF &&
> +			start
> +			wrote version
> +			IN: smudge test2.r 7 [OK] -- OUT: 7 [OK]
> +			IN: smudge test3.r 8 [OK] -- OUT: 8 [OK]
> +		EOF
> +		test_cmp expected_checkout.log smudge_output.log &&
> +
> +		git checkout empty &&
> +
> +		printf "" >output.log &&
> +		git checkout master &&
> +		cat output.log | grep -v "IN: clean" >smudge_output.log &&
> +		cat >expected_checkout_master.log <<-\EOF &&
> +			start
> +			wrote version
> +			IN: smudge test.r 57 [OK] -- OUT: 57 [OK]
> +			IN: smudge test2.r 7 [OK] -- OUT: 7 [OK]
> +			IN: smudge test3.r 8 [OK] -- OUT: 8 [OK]
> +		EOF
> +		test_cmp expected_checkout_master.log smudge_output.log
> +	)
> +'
> +
> +test_expect_success EXPENSIVE 'protocol filter large file' '
> +	test_config_global filter.protocol.clean \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> +	test_config_global filter.protocol.smudge \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> +	rm -rf repo &&
> +	mkdir repo &&
> +	(
> +		cd repo &&
> +		git init &&
> +
> +		echo "2GB filter=largefile" >.gitattributes &&
> +		for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&

> +		git add 2GB 2>err &&
> +		test_must_be_empty err &&
> +		rm -f 2GB &&
> +		git checkout -- 2GB 2>err &&
> +		test_must_be_empty err
> +	)
> +'
> +
> +test_expect_success 'required protocol filter should fail with clean' '
> +	test_config_global filter.protocol.clean \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> +	test_config_global filter.protocol.useprotocol true &&
> +	test_config_global filter.protocol.required true &&
> +	rm -rf repo &&
> +	mkdir repo &&
> +	(
> +		cd repo &&
> +		git init &&
> +
> +		echo "*.r filter=protocol" >.gitattributes &&
> +
> +		cat ../test.o >test.r &&
> +		echo "this is going to fail" >clean-write-fail.r &&
> +		echo "test333" >test3.r &&
> +
> +		# Note: There are three clean paths in convert.c we just test one here.
> +		test_must_fail git add .
> +	)
> +'
> +
> +test_expect_success 'protocol filter should restart after failure' '
> +	test_config_global filter.protocol.clean \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> +	test_config_global filter.protocol.smudge \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> +	test_config_global filter.protocol.useprotocol true &&
> +	rm -rf repo &&
> +	mkdir repo &&
> +	(
> +		cd repo &&
> +		git init &&
> +
> +		echo "*.r filter=protocol" >.gitattributes &&
> +
> +		cat ../test.o >test.r &&
> +		echo "1234567" >test2.o &&
> +		cat test2.o >test2.r &&
> +		echo "this is going to fail" >smudge-write-fail.o &&
> +		cat smudge-write-fail.o >smudge-write-fail.r &&
> +		git add . &&
> +		git commit . -m "test commit" &&
> +		rm -f *.r &&
> +
> +		printf "" >output.log &&

> +		git checkout . &&
> +		cat output.log | grep -v "IN: clean" >smudge_output.log &&
> +		cat >expected_checkout_master.log <<-\EOF &&
> +			start
> +			wrote version
> +			IN: smudge smudge-write-fail.r 22 [OK] -- OUT: 22 [FAIL]
> +			start
> +			wrote version
> +			IN: smudge test.r 57 [OK] -- OUT: 57 [OK]
> +			IN: smudge test2.r 8 [OK] -- OUT: 8 [OK]
> +		EOF
> +		test_cmp expected_checkout_master.log smudge_output.log &&
> +
> +		test_cmp ../test.o test.r &&
> +		./../rot13.sh <../test.o >expected &&
> +		git cat-file blob :test.r >actual &&
> +		test_cmp expected actual
> +
> +		test_cmp test2.o test2.r &&
> +		./../rot13.sh <test2.o >expected &&
> +		git cat-file blob :test2.r >actual &&
> +		test_cmp expected actual
> +
> +		test_cmp test2.o test2.r &&
> +		./../rot13.sh <test2.o >expected &&
> +		git cat-file blob :test2.r >actual &&
> +		test_cmp expected actual
> +
> +		! test_cmp smudge-write-fail.o smudge-write-fail.r && # Smudge failed!
> +		./../rot13.sh <smudge-write-fail.o >expected &&
> +		git cat-file blob :smudge-write-fail.r >actual &&
> +		test_cmp expected actual							  # Clean worked!
> +	)
> +'
> +
>  test_done
> diff --git a/t/t0021/rot13.pl b/t/t0021/rot13.pl
> new file mode 100755
> index 0000000..f2d7a03
> --- /dev/null
> +++ b/t/t0021/rot13.pl
> @@ -0,0 +1,80 @@
> +#!/usr/bin/env perl

> +#
> +# Example implementation for the Git filter protocol version 1
> +# See Documentation/gitattributes.txt, section "Filter Protocol"
> +#
> +
> +use strict;
> +use warnings;
> +use autodie;
> +
> +sub rot13 {
> +    my ($str) = @_;
> +    $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
> +    return $str;
> +}
> +
> +$| = 1; # autoflush STDOUT
> +
> +open my $debug, ">>", "output.log";
> +$debug->autoflush(1);
> +
> +print $debug "start\n";
> +
> +print STDOUT "git-filter-protocol\nversion 1";

> +print $debug "wrote version\n";
> +
> +while (1) {
> +    my $command = <STDIN>;
> +    unless (defined($command)) {
> +        exit();
> +    }
> +    chomp $command;
> +    print $debug "IN: $command";
> +    my $filename = <STDIN>;
> +    chomp $filename;
> +    print $debug " $filename";
> +    my $filelen  = <STDIN>;
> +    chomp $filelen;
> +    print $debug " $filelen";
> +
> +    $filelen = int($filelen);
> +    my $output;
> +
> +    if ( $filelen > 0 ) {
> +        my $input;
> +        {
> +            binmode(STDIN);
> +            my $bytes_read = 0;
> +            $bytes_read = read STDIN, $input, $filelen;
> +            if ( $bytes_read != $filelen ) {
> +                die "not enough to read";
> +            }
> +            print $debug " [OK] -- ";
> +        }
> +
> +        if ( $command eq "clean") {
> +            $output = rot13($input);
> +        }
> +        elsif ( $command eq "smudge" ) {
> +            $output = rot13($input);
> +        }
> +        else {
> +            die "bad command\n";
> +        }
> +    }
> +
> +    my $output_len = length($output);
> +    print STDOUT "$output_len\n";
> +    print $debug "OUT: $output_len";
> +    if ( $output_len > 0 ) {
> +        if ( ($command eq "clean" and $filename eq "clean-write-fail.r") or
> +             ($command eq "smudge" and $filename eq "smudge-write-fail.r") ) {
> +            print STDOUT "fail";
> +            print $debug " [FAIL]\n"
> +        } else {
> +            print STDOUT $output;
> +            print $debug " [OK]\n";
> +        }
> +    }
> +}
>

> On 07/22/2016 05:49 PM, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> [...]
>> 
>> 1. Git starts the filter on first usage and expects a welcome message
>> with protocol version number:
>> 	Git <-- Filter: "git-filter-protocol\n"
>> 	Git <-- Filter: "version 1"
> Is there no terminator here ?
> How long should the reading side wait without a '\n', or should it read
> "version 1\n" ?

>> [...]
>> 
>> Please note that the protocol filters do not support stream processing
>> with this implemenatation because the filter needs to know the length of
>            ^^^^^^^^^^^^^^^^typo

>> [...]
>> 
>> +
>> +test_expect_success EXPENSIVE 'protocol filter large file' '
>> +	test_config_global filter.protocol.clean \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
>> +	test_config_global filter.protocol.smudge \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
>> +	rm -rf repo &&
>> +	mkdir repo &&
>> +	(
>> +		cd repo &&
>> +		git init &&
>> +
>> +		echo "2GB filter=largefile" >.gitattributes &&
>> +		for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
> Side question:
> Is there a way to "re-use" the 2GB test file through t0021?
> It takes a long time to produce it, especially on my 32 Bit systems ;-)
> But this may be a different patch.

>> [...]
>> +
>> +		printf "" >output.log &&
> Is this the same as
> >output.log
> to produce an empty file ?

>> [...]
>> +++ b/t/t0021/rot13.pl
>> @@ -0,0 +1,80 @@
>> +#!/usr/bin/env perl
> Should this be
> "$PERL_PATH" ?

>> [...]
>> +
>> +print STDOUT "git-filter-protocol\nversion 1";
> Again, I don't like the missing terminator here.
> What if we step up to protocol "version 10" ?
> Could it work to use one and only one line,
> with one terminator, like this ?
> print STDOUT "git-filter-protocol version 1\1";

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Git's clean/smudge mechanism invokes an external filter process for every
> single blob that is affected by a filter. If Git filters a lot of blobs
> then the startup time of the external filter processes can become a
> significant part of the overall Git execution time.
> 
> This patch adds the filter.<driver>.useProtocol option which, if enabled,
> keeps the external filter process running and processes all blobs with
> the following protocol over stdin/stdout.
> 
> 1. Git starts the filter on first usage and expects a welcome message
> with protocol version number:
> 	Git <-- Filter: "git-filter-protocol\n"
> 	Git <-- Filter: "version 1"

> diff --git a/convert.c b/convert.c
> index 522e2c5..91ce86f 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -481,12 +481,188 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>  	return ret;
>  }
>  
> +static int cmd_process_map_init = 0;
> +static struct hashmap cmd_process_map;
> +
> +struct cmd2process {
> +	struct hashmap_entry ent; /* must be the first member! */
> +	const char *cmd;
> +	long protocol;
> +	struct child_process process;
> +};
> +
> +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_protocol_filter_entry(const char *cmd)
> +{
> +	struct cmd2process k;
> +	hashmap_entry_init(&k, strhash(cmd));
> +	k.cmd = cmd;
> +	return hashmap_get(&cmd_process_map, &k, NULL);
> +}
> +
> +static void stop_protocol_filter(struct cmd2process *entry) {
> +	if (!entry)
> +		return;
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	close(entry->process.in);
> +	close(entry->process.out);
> +	sigchain_pop(SIGPIPE);
> +	finish_command(&entry->process);
> +	child_process_clear(&entry->process);
> +	hashmap_remove(&cmd_process_map, entry, NULL);
> +	free(entry);
> +}
> +
> +static struct cmd2process *start_protocol_filter(const char *cmd)
> +{
> +	int ret = 1;
> +	struct cmd2process *entry = NULL;
> +	struct child_process *process = NULL;
> +	struct strbuf nbuf = STRBUF_INIT;
> +	struct string_list split = STRING_LIST_INIT_NODUP;
> +	const char *argv[] = { NULL, NULL };
> +	const char *header = "git-filter-protocol\nversion";
> +
> +	entry = xmalloc(sizeof(*entry));
> +	hashmap_entry_init(entry, strhash(cmd));
> +	entry->cmd = cmd;
> +	process = &entry->process;
> +
> +	child_process_init(process);
> +	argv[0] = cmd;
> +	process->argv = argv;
> +	process->use_shell = 1;
> +	process->in = -1;
> +	process->out = -1;
> +
> +	if (start_command(process)) {
> +		error("cannot fork to run external persistent filter '%s'", cmd);
> +		return NULL;
> +	}
> +	strbuf_reset(&nbuf);
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;

> +	sigchain_pop(SIGPIPE);
> +
> +	strbuf_stripspace(&nbuf, 0);
> +	string_list_split_in_place(&split, nbuf.buf, ' ', 2);
> +	ret &= split.nr > 1;
> +	ret &= strncmp(header, split.items[0].string, strlen(header)) == 0;
> +	if (ret) {
> +		entry->protocol = strtol(split.items[1].string, NULL, 10);
> +		switch (entry->protocol) {
> +			case 1:
> +				break;
> +			default:
> +				ret = 0;
> +				error("unsupported protocol version %s for external persistent filter '%s'",
> +					nbuf.buf, cmd);
> +		}
> +	}
> +	string_list_clear(&split, 0);
> +	strbuf_release(&nbuf);
> +
> +	if (!ret) {
> +		error("initialization for external persistent filter '%s' failed", cmd);
> +		return NULL;
> +	}
> +
> +	hashmap_add(&cmd_process_map, entry);
> +	return entry;
> +}
> +
> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
> +						int fd, struct strbuf *dst, const char *cmd,
> +						const char *filter_type)
> +{
> +	int ret = 1;
> +	struct cmd2process *entry = NULL;
> +	struct child_process *process = NULL;
> +	struct stat fileStat;
> +	struct strbuf nbuf = STRBUF_INIT;
> +	size_t nbuf_len;
> +	char *strtol_end;
> +	char c;
> +
> +	if (!cmd || !*cmd)
> +		return 0;
> +
> +	if (!dst)
> +		return 1;
> +
> +	if (!cmd_process_map_init) {
> +		cmd_process_map_init = 1;
> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
> +	} else {
> +		entry = find_protocol_filter_entry(cmd);
> +	}
> +
> +	if (!entry){
> +		entry = start_protocol_filter(cmd);
> +		if (!entry) {
> +			stop_protocol_filter(entry);
> +			return 0;
> +		}
> +	}
> +	process = &entry->process;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	switch (entry->protocol) {
> +		case 1:
> +			if (fd >= 0 && !src) {
> +				ret &= fstat(fd, &fileStat) != -1;
> +				len = fileStat.st_size;
> +			}
> +			strbuf_reset(&nbuf);
> +			strbuf_addf(&nbuf, "%s\n%s\n%zu\n", filter_type, path, len);
> +			ret &= write_str_in_full(process->in, nbuf.buf) > 1;

> +			if (len > 0) {
> +				if (src)
> +					ret &= write_in_full(process->in, src, len) == len;
> +				else if (fd >= 0)
> +					ret &= copy_fd(fd, process->in) == 0;
> +				else
> +					ret &= 0;
> +			}
> +
> +			strbuf_reset(&nbuf);
> +			while (xread(process->out, &c, 1) == 1 && c != '\n')
> +				strbuf_addchars(&nbuf, c, 1);
> +			nbuf_len = (size_t)strtol(nbuf.buf, &strtol_end, 10);
> +			ret &= (strtol_end != nbuf.buf && errno != ERANGE);
> +			strbuf_reset(&nbuf);
> +			if (nbuf_len > 0)
> +				ret &= strbuf_read_once(&nbuf, process->out, nbuf_len) == nbuf_len;

> 
> 
> On 22/07/16 16:49, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Git's clean/smudge mechanism invokes an external filter process for every
>> single blob that is affected by a filter. If Git filters a lot of blobs
>> then the startup time of the external filter processes can become a
>> significant part of the overall Git execution time.
>>
>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>> keeps the external filter process running and processes all blobs with
>> the following protocol over stdin/stdout.
>>
>> 1. Git starts the filter on first usage and expects a welcome message
>> with protocol version number:
>> 	Git <-- Filter: "git-filter-protocol\n"
>> 	Git <-- Filter: "version 1"
> 
> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
> interaction is fully defined, I guess it doesn't matter).
> 
> [If you wanted to check for a version, you could add a "version" command
> instead, just like "clean" and "smudge".]
> 
> [snip]
> 
>> diff --git a/convert.c b/convert.c
>> index 522e2c5..91ce86f 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -481,12 +481,188 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>>  	return ret;
>>  }
>>  
>> +static int cmd_process_map_init = 0;
>> +static struct hashmap cmd_process_map;
>> +
>> +struct cmd2process {
>> +	struct hashmap_entry ent; /* must be the first member! */
>> +	const char *cmd;
>> +	long protocol;
>> +	struct child_process process;
>> +};
>> +
>> +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_protocol_filter_entry(const char *cmd)
>> +{
>> +	struct cmd2process k;
>> +	hashmap_entry_init(&k, strhash(cmd));
>> +	k.cmd = cmd;
>> +	return hashmap_get(&cmd_process_map, &k, NULL);
>> +}
>> +
>> +static void stop_protocol_filter(struct cmd2process *entry) {
>> +	if (!entry)
>> +		return;
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	close(entry->process.in);
>> +	close(entry->process.out);
>> +	sigchain_pop(SIGPIPE);
>> +	finish_command(&entry->process);
>> +	child_process_clear(&entry->process);
>> +	hashmap_remove(&cmd_process_map, entry, NULL);
>> +	free(entry);
>> +}
>> +
>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry = NULL;
>> +	struct child_process *process = NULL;
>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	struct string_list split = STRING_LIST_INIT_NODUP;
>> +	const char *argv[] = { NULL, NULL };
>> +	const char *header = "git-filter-protocol\nversion";
>> +
>> +	entry = xmalloc(sizeof(*entry));
>> +	hashmap_entry_init(entry, strhash(cmd));
>> +	entry->cmd = cmd;
>> +	process = &entry->process;
>> +
>> +	child_process_init(process);
>> +	argv[0] = cmd;
>> +	process->argv = argv;
>> +	process->use_shell = 1;
>> +	process->in = -1;
>> +	process->out = -1;
>> +
>> +	if (start_command(process)) {
>> +		error("cannot fork to run external persistent filter '%s'", cmd);
>> +		return NULL;
>> +	}
>> +	strbuf_reset(&nbuf);
>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
> 
> Hmm, how much will be read into nbuf by this single call?
> Since strbuf_read_once() makes a single call to xread(), with
> a len argument that will probably be 8192, you can not really
> tell how much it will read, in general. (xread() does not
> guarantee how many bytes it will read.)
> 
> In particular, it could be less than strlen(header).

> On 22/07/16 16:49, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Git's clean/smudge mechanism invokes an external filter process for every
>> single blob that is affected by a filter. If Git filters a lot of blobs
>> then the startup time of the external filter processes can become a
>> significant part of the overall Git execution time.
>> 
>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>> keeps the external filter process running and processes all blobs with
>> the following protocol over stdin/stdout.
>> 
>> 1. Git starts the filter on first usage and expects a welcome message
>> with protocol version number:
>> 	Git <-- Filter: "git-filter-protocol\n"
>> 	Git <-- Filter: "version 1"
> 
> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
> interaction is fully defined, I guess it doesn't matter).
> 
> [If you wanted to check for a version, you could add a "version" command
> instead, just like "clean" and "smudge".]

> [...]
>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry = NULL;
>> +	struct child_process *process = NULL;
>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	struct string_list split = STRING_LIST_INIT_NODUP;
>> +	const char *argv[] = { NULL, NULL };
>> +	const char *header = "git-filter-protocol\nversion";
>> +
>> +	entry = xmalloc(sizeof(*entry));
>> +	hashmap_entry_init(entry, strhash(cmd));
>> +	entry->cmd = cmd;
>> +	process = &entry->process;
>> +
>> +	child_process_init(process);
>> +	argv[0] = cmd;
>> +	process->argv = argv;
>> +	process->use_shell = 1;
>> +	process->in = -1;
>> +	process->out = -1;
>> +
>> +	if (start_command(process)) {
>> +		error("cannot fork to run external persistent filter '%s'", cmd);
>> +		return NULL;
>> +	}
>> +	strbuf_reset(&nbuf);
>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
> 
> Hmm, how much will be read into nbuf by this single call?
> Since strbuf_read_once() makes a single call to xread(), with
> a len argument that will probably be 8192, you can not really
> tell how much it will read, in general. (xread() does not
> guarantee how many bytes it will read.)
> 
> In particular, it could be less than strlen(header).

>> [...]
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	switch (entry->protocol) {
>> +		case 1:
>> +			if (fd >= 0 && !src) {
>> +				ret &= fstat(fd, &fileStat) != -1;
>> +				len = fileStat.st_size;
>> +			}
>> +			strbuf_reset(&nbuf);
>> +			strbuf_addf(&nbuf, "%s\n%s\n%zu\n", filter_type, path, len);
>> +			ret &= write_str_in_full(process->in, nbuf.buf) > 1;
> 
> why not write_in_full(process->in, nbuf.buf, nbuf.len) ?

>> +			if (len > 0) {
>> +				if (src)
>> +					ret &= write_in_full(process->in, src, len) == len;
>> +				else if (fd >= 0)
>> +					ret &= copy_fd(fd, process->in) == 0;
>> +				else
>> +					ret &= 0;
>> +			}
>> +
>> +			strbuf_reset(&nbuf);
>> +			while (xread(process->out, &c, 1) == 1 && c != '\n')
>> +				strbuf_addchars(&nbuf, c, 1);
>> +			nbuf_len = (size_t)strtol(nbuf.buf, &strtol_end, 10);
>> +			ret &= (strtol_end != nbuf.buf && errno != ERANGE);
>> +			strbuf_reset(&nbuf);
>> +			if (nbuf_len > 0)
>> +				ret &= strbuf_read_once(&nbuf, process->out, nbuf_len) == nbuf_len;
> 
> Again, how many bytes will be read?
> Note, that in the default configuration, a _maximum_ of
> MAX_IO_SIZE (8MB or SSIZE_MAX, whichever is smaller) bytes
> will be read.

> 
> On 23 Jul 2016, at 01:19, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
>> On 22/07/16 16:49, larsxschneider@gmail.com wrote:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> Git's clean/smudge mechanism invokes an external filter process for every
>>> single blob that is affected by a filter. If Git filters a lot of blobs
>>> then the startup time of the external filter processes can become a
>>> significant part of the overall Git execution time.
>>>
>>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>>> keeps the external filter process running and processes all blobs with
>>> the following protocol over stdin/stdout.
>>>
>>> 1. Git starts the filter on first usage and expects a welcome message
>>> with protocol version number:
>>> 	Git <-- Filter: "git-filter-protocol\n"
>>> 	Git <-- Filter: "version 1"
>>
>> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
>> interaction is fully defined, I guess it doesn't matter).
>>
>> [If you wanted to check for a version, you could add a "version" command
>> instead, just like "clean" and "smudge".]
> 
> It was a conscious decision to have the `filter` talk first. My reasoning was:
> 
> (1) I want a reliable way to distinguish the existing filter protocol ("single-shot 
> invocation") from the new one ("long running"). I don't think there would be a
> situation where the existing protocol would talk first. Therefore the users would
> not accidentally mix them with a possibly half working, undetermined, outcome.

> 
> (2) In the future we could extend the pipe protocol (see $gmane/297994, it's very
> interesting). A filter could check Git's version and then pick the most appropriate
> filter protocol on startup.
> 
> 
>> [...]
>>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>>> +{
>>> +	int ret = 1;
>>> +	struct cmd2process *entry = NULL;
>>> +	struct child_process *process = NULL;
>>> +	struct strbuf nbuf = STRBUF_INIT;
>>> +	struct string_list split = STRING_LIST_INIT_NODUP;
>>> +	const char *argv[] = { NULL, NULL };
>>> +	const char *header = "git-filter-protocol\nversion";
>>> +
>>> +	entry = xmalloc(sizeof(*entry));
>>> +	hashmap_entry_init(entry, strhash(cmd));
>>> +	entry->cmd = cmd;
>>> +	process = &entry->process;
>>> +
>>> +	child_process_init(process);
>>> +	argv[0] = cmd;
>>> +	process->argv = argv;
>>> +	process->use_shell = 1;
>>> +	process->in = -1;
>>> +	process->out = -1;
>>> +
>>> +	if (start_command(process)) {
>>> +		error("cannot fork to run external persistent filter '%s'", cmd);
>>> +		return NULL;
>>> +	}
>>> +	strbuf_reset(&nbuf);
>>> +
>>> +	sigchain_push(SIGPIPE, SIG_IGN);
>>> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
>>
>> Hmm, how much will be read into nbuf by this single call?
>> Since strbuf_read_once() makes a single call to xread(), with
>> a len argument that will probably be 8192, you can not really
>> tell how much it will read, in general. (xread() does not
>> guarantee how many bytes it will read.)
>>
>> In particular, it could be less than strlen(header).
> 
> As mentioned to Torsten in $gmane/300156, I will add a newline
> and then read until I find the second newline. That should solve
> the problem, right?
> 
> (You wrote in $gmane/300119 that I should ignore your email but
> I think you have a valid point here ;-)

> 
> 
>>> [...]
>>> +	sigchain_push(SIGPIPE, SIG_IGN);
>>> +	switch (entry->protocol) {
>>> +		case 1:
>>> +			if (fd >= 0 && !src) {
>>> +				ret &= fstat(fd, &fileStat) != -1;
>>> +				len = fileStat.st_size;
>>> +			}
>>> +			strbuf_reset(&nbuf);
>>> +			strbuf_addf(&nbuf, "%s\n%s\n%zu\n", filter_type, path, len);
>>> +			ret &= write_str_in_full(process->in, nbuf.buf) > 1;
>>
>> why not write_in_full(process->in, nbuf.buf, nbuf.len) ?
> OK, this would save a "strlen" call. Do you think such a function could be of general
> use? If yes, then I would add:
> 
> static inline ssize_t write_strbuf_in_full(int fd, struct strbuf *str)
> {
> 	return write_in_full(fd, str->buf, str->len);
> }

> 
> 
>>> +			if (len > 0) {
>>> +				if (src)
>>> +					ret &= write_in_full(process->in, src, len) == len;
>>> +				else if (fd >= 0)
>>> +					ret &= copy_fd(fd, process->in) == 0;
>>> +				else
>>> +					ret &= 0;
>>> +			}
>>> +
>>> +			strbuf_reset(&nbuf);
>>> +			while (xread(process->out, &c, 1) == 1 && c != '\n')
>>> +				strbuf_addchars(&nbuf, c, 1);
>>> +			nbuf_len = (size_t)strtol(nbuf.buf, &strtol_end, 10);
>>> +			ret &= (strtol_end != nbuf.buf && errno != ERANGE);
>>> +			strbuf_reset(&nbuf);
>>> +			if (nbuf_len > 0)
>>> +				ret &= strbuf_read_once(&nbuf, process->out, nbuf_len) == nbuf_len;
>>
>> Again, how many bytes will be read?
>> Note, that in the default configuration, a _maximum_ of
>> MAX_IO_SIZE (8MB or SSIZE_MAX, whichever is smaller) bytes
>> will be read.

> Would something like this be more appropriate?
> 
> strbuf_reset(&nbuf);
> if (nbuf_len > 0) {
>     strbuf_grow(&nbuf, nbuf_len);
>     ret &= read_in_full(process->out, nbuf.buf, nbuf_len) == nbuf_len;
> }

>>>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>>>> keeps the external filter process running and processes all blobs with
>>>> the following protocol over stdin/stdout.
>>>>
>>>> 1. Git starts the filter on first usage and expects a welcome message
>>>> with protocol version number:
>>>> 	Git <-- Filter: "git-filter-protocol\n"
>>>> 	Git <-- Filter: "version 1"
>>>
>>> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
>>> interaction is fully defined, I guess it doesn't matter).
>>
>> It was a conscious decision to have the `filter` talk first. My reasoning was:
>>
>> (1) I want a reliable way to distinguish the existing filter protocol ("single-shot 
>> invocation") from the new one ("long running"). I don't think there would be a
>> situation where the existing protocol would talk first. Therefore the users would
>> not accidentally mix them with a possibly half working, undetermined, outcome.
> 
> If an 'single-shot' filter were incorrectly configured, instead of a new one, then
> the interaction could last a little while - since it would result in deadlock! ;-)
> 
> [If Git talks first instead, configuring a 'single-shot' filter _may_ still result
> in a deadlock - depending on pipe size, etc.]

> W dniu 2016-07-25 o 00:36, Ramsay Jones pisze:
>> On 24/07/16 18:16, Lars Schneider wrote:
>>> On 23 Jul 2016, at 01:19, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>>>> On 22/07/16 16:49, larsxschneider@gmail.com wrote:
> [...]
>>>>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>>>>> keeps the external filter process running and processes all blobs with
>>>>> the following protocol over stdin/stdout.
>>>>> 
>>>>> 1. Git starts the filter on first usage and expects a welcome message
>>>>> with protocol version number:
>>>>> 	Git <-- Filter: "git-filter-protocol\n"
>>>>> 	Git <-- Filter: "version 1"
>>>> 
>>>> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
>>>> interaction is fully defined, I guess it doesn't matter).
>>> 
>>> It was a conscious decision to have the `filter` talk first. My reasoning was:
>>> 
>>> (1) I want a reliable way to distinguish the existing filter protocol ("single-shot 
>>> invocation") from the new one ("long running"). I don't think there would be a
>>> situation where the existing protocol would talk first. Therefore the users would
>>> not accidentally mix them with a possibly half working, undetermined, outcome.
>> 
>> If an 'single-shot' filter were incorrectly configured, instead of a new one, then
>> the interaction could last a little while - since it would result in deadlock! ;-)
>> 
>> [If Git talks first instead, configuring a 'single-shot' filter _may_ still result
>> in a deadlock - depending on pipe size, etc.]
> 
> Would it be possible to do an equivalent of sending empty file to the filter?
> If it is misconfigured old-style script, it would exit after possibly empty
> output; if not, we would start new-style interaction.

> 
> This should be, if we agree that detecting misconfigured filters is a good
> thing, tested.
> 
>>> 
>>> (2) In the future we could extend the pipe protocol (see $gmane/297994, it's very
>>> interesting). A filter could check Git's version and then pick the most appropriate
>>> filter protocol on startup.

>>>> It was a conscious decision to have the `filter` talk first.
>>>> My reasoning was:
>>>> 
>>>> (1) I want a reliable way to distinguish the existing filter 
>>>> protocol ("single-shot invocation") from the new one ("long 
>>>> running"). I don't think there would be a situation where the 
>>>> existing protocol would talk first. Therefore the users would 
>>>> not accidentally mix them with a possibly half working, 
>>>> undetermined, outcome.
>>> 
>>> If an 'single-shot' filter were incorrectly configured, instead 
>>> of a new one, then the interaction could last a little while - 
>>> since it would result in deadlock! ;-)
>>> 
>>> [If Git talks first instead, configuring a 'single-shot' filter 
>>> _may_ still result in a deadlock - depending on pipe size, etc.]
>> 
>> Would it be possible to do an equivalent of sending empty file to 
>> the filter? If it is misconfigured old-style script, it would exit 
>> after possibly empty output; if not, we would start new-style 
>> interaction.
> 
> I think we would need to close the pipe to communicate "end" to the 
> filter, no? I would prefer to define the protocol explicitly as this 
> is clearly easier.

>> If an 'single-shot' filter were incorrectly configured, instead of
>> a new one, then the interaction could last a little while - since
>> it would result in deadlock! ;-)
>> 
>> [If Git talks first instead, configuring a 'single-shot' filter
>> _may_ still result in a deadlock - depending on pipe size, etc.]
> 
> Do you think this is an issue that needs to be addressed in the first
> version? If yes, I would probably look into "select" to specify a
> timeout for the filter.

> On 24/07/16 18:16, Lars Schneider wrote:
>> 
>> On 23 Jul 2016, at 01:19, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>> 
>>> On 22/07/16 16:49, larsxschneider@gmail.com wrote:
>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>> 
>>>> Git's clean/smudge mechanism invokes an external filter process for every
>>>> single blob that is affected by a filter. If Git filters a lot of blobs
>>>> then the startup time of the external filter processes can become a
>>>> significant part of the overall Git execution time.
>>>> 
>>>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>>>> keeps the external filter process running and processes all blobs with
>>>> the following protocol over stdin/stdout.
>>>> 
>>>> 1. Git starts the filter on first usage and expects a welcome message
>>>> with protocol version number:
>>>> 	Git <-- Filter: "git-filter-protocol\n"
>>>> 	Git <-- Filter: "version 1"
>>> 
>>> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
>>> interaction is fully defined, I guess it doesn't matter).
>>> 
>>> [If you wanted to check for a version, you could add a "version" command
>>> instead, just like "clean" and "smudge".]
>> 
>> It was a conscious decision to have the `filter` talk first. My reasoning was:
>> 
>> (1) I want a reliable way to distinguish the existing filter protocol ("single-shot 
>> invocation") from the new one ("long running"). I don't think there would be a
>> situation where the existing protocol would talk first. Therefore the users would
>> not accidentally mix them with a possibly half working, undetermined, outcome.
> 
> If an 'single-shot' filter were incorrectly configured, instead of a new one, then
> the interaction could last a little while - since it would result in deadlock! ;-)
> 
> [If Git talks first instead, configuring a 'single-shot' filter _may_ still result
> in a deadlock - depending on pipe size, etc.]

>> (2) In the future we could extend the pipe protocol (see $gmane/297994, it's very
>> interesting). A filter could check Git's version and then pick the most appropriate
>> filter protocol on startup.
>> 
>> 
>>> [...]
>>>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>>>> +{
>>>> +	int ret = 1;
>>>> +	struct cmd2process *entry = NULL;
>>>> +	struct child_process *process = NULL;
>>>> +	struct strbuf nbuf = STRBUF_INIT;
>>>> +	struct string_list split = STRING_LIST_INIT_NODUP;
>>>> +	const char *argv[] = { NULL, NULL };
>>>> +	const char *header = "git-filter-protocol\nversion";
>>>> +
>>>> +	entry = xmalloc(sizeof(*entry));
>>>> +	hashmap_entry_init(entry, strhash(cmd));
>>>> +	entry->cmd = cmd;
>>>> +	process = &entry->process;
>>>> +
>>>> +	child_process_init(process);
>>>> +	argv[0] = cmd;
>>>> +	process->argv = argv;
>>>> +	process->use_shell = 1;
>>>> +	process->in = -1;
>>>> +	process->out = -1;
>>>> +
>>>> +	if (start_command(process)) {
>>>> +		error("cannot fork to run external persistent filter '%s'", cmd);
>>>> +		return NULL;
>>>> +	}
>>>> +	strbuf_reset(&nbuf);
>>>> +
>>>> +	sigchain_push(SIGPIPE, SIG_IGN);
>>>> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
>>> 
>>> Hmm, how much will be read into nbuf by this single call?
>>> Since strbuf_read_once() makes a single call to xread(), with
>>> a len argument that will probably be 8192, you can not really
>>> tell how much it will read, in general. (xread() does not
>>> guarantee how many bytes it will read.)
>>> 
>>> In particular, it could be less than strlen(header).
>> 
>> As mentioned to Torsten in $gmane/300156, I will add a newline
>> and then read until I find the second newline. That should solve
>> the problem, right?
>> 
>> (You wrote in $gmane/300119 that I should ignore your email but
>> I think you have a valid point here ;-)
> 
> Heh, as I said, it was late and I was trying to do several things
> at once. (I am updating 3 installations of Linux Mint 17.3 to Linux
> Mint 18 - I decided to do a complete re-install, since I needed to
> change partition sizes anyway. I have only just got email back up ...)
> 
> I stopped commenting on the patch early but, after sending the first
> email, I decided to scan the rest of your patch before going to bed
> and noticed something which would invalidate my comments ...
> 
>> 
>> 
>>>> [...]
>>>> +	sigchain_push(SIGPIPE, SIG_IGN);
>>>> +	switch (entry->protocol) {
>>>> +		case 1:
>>>> +			if (fd >= 0 && !src) {
>>>> +				ret &= fstat(fd, &fileStat) != -1;
>>>> +				len = fileStat.st_size;
>>>> +			}
>>>> +			strbuf_reset(&nbuf);
>>>> +			strbuf_addf(&nbuf, "%s\n%s\n%zu\n", filter_type, path, len);
>>>> +			ret &= write_str_in_full(process->in, nbuf.buf) > 1;
>>> 
>>> why not write_in_full(process->in, nbuf.buf, nbuf.len) ?
>> OK, this would save a "strlen" call. Do you think such a function could be of general
>> use? If yes, then I would add:
>> 
>> static inline ssize_t write_strbuf_in_full(int fd, struct strbuf *str)
>> {
>> 	return write_in_full(fd, str->buf, str->len);
>> }
> 
> [I don't have strong feelings either way (but I suspect it's not worth it).]

>>>> +			if (len > 0) {
>>>> +				if (src)
>>>> +					ret &= write_in_full(process->in, src, len) == len;
>>>> +				else if (fd >= 0)
>>>> +					ret &= copy_fd(fd, process->in) == 0;
>>>> +				else
>>>> +					ret &= 0;
>>>> +			}
>>>> +
>>>> +			strbuf_reset(&nbuf);
>>>> +			while (xread(process->out, &c, 1) == 1 && c != '\n')
>>>> +				strbuf_addchars(&nbuf, c, 1);
>>>> +			nbuf_len = (size_t)strtol(nbuf.buf, &strtol_end, 10);
>>>> +			ret &= (strtol_end != nbuf.buf && errno != ERANGE);
>>>> +			strbuf_reset(&nbuf);
>>>> +			if (nbuf_len > 0)
>>>> +				ret &= strbuf_read_once(&nbuf, process->out, nbuf_len) == nbuf_len;
>>> 
>>> Again, how many bytes will be read?
>>> Note, that in the default configuration, a _maximum_ of
>>> MAX_IO_SIZE (8MB or SSIZE_MAX, whichever is smaller) bytes
>>> will be read.
> 
> ... In particular, your 2GB test case should not have worked, so
> I assumed that I had missed a loop somewhere ...

>> Would something like this be more appropriate?
>> 
>> strbuf_reset(&nbuf);
>> if (nbuf_len > 0) {
>>    strbuf_grow(&nbuf, nbuf_len);
>>    ret &= read_in_full(process->out, nbuf.buf, nbuf_len) == nbuf_len;
>> }
> 
> ... and this looks better. [Note: this comment would apply equally to the
> version message.]

> 
> Git's clean/smudge mechanism invokes an external filter process for every
> single blob that is affected by a filter. If Git filters a lot of blobs
> then the startup time of the external filter processes can become a
> significant part of the overall Git execution time.

> 
> 1. Git starts the filter on first usage and expects a welcome message
> with protocol version number:
> 	Git <-- Filter: "git-filter-protocol\n"
> 	Git <-- Filter: "version 1"

> 
> 3. The filter is expected to answer with the result content size in
> bytes and the result content separated by a newline character:
> 	Git <-- Filter: "15\n"
> 	Git <-- Filter: "SMUDGED_CONTENT"

> 
> 4. The filter is expected to wait for the next file in step 2, again.
> 
> Please note that the protocol filters do not support stream processing
> with this implemenatation because the filter needs to know the length of

> the result in advance. A protocol version 2 could address this in a
> future patch.
> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> Helped-by: Martin-Louis Bright <mlbright@gmail.com>
> ---
>  Documentation/gitattributes.txt |  41 +++++++-
>  convert.c                       | 210 ++++++++++++++++++++++++++++++++++++++--
>  t/t0021-conversion.sh           | 170 ++++++++++++++++++++++++++++++++

>  t/t0021/rot13.pl                |  80 +++++++++++++++
>  4 files changed, 494 insertions(+), 7 deletions(-)
>  create mode 100755 t/t0021/rot13.pl
> 
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 8882a3e..7026d62 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -300,7 +300,10 @@ checkout, when the `smudge` command is specified, the command is
>  fed the blob object from its standard input, and its standard
>  output is used to update the worktree file.  Similarly, the
>  `clean` command is used to convert the contents of worktree file
> -upon checkin.
> +upon checkin. By default these commands process only a single
> +blob and terminate. If the setting filter.<driver>.useProtocol is
> +enabled then Git can process all blobs with a single filter command
> +invocation (see filter protocol below).

>  
>  One use of the content filtering is to massage the content into a shape
>  that is more convenient for the platform, filesystem, and the user to use.
> @@ -375,6 +378,42 @@ substitution.  For example:
>  ------------------------
>  
>  
> +Filter Protocol
> +^^^^^^^^^^^^^^^
> +
> +If the setting filter.<driver>.useProtocol is enabled then Git

> diff --git a/convert.c b/convert.c
> index 522e2c5..91ce86f 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -481,12 +481,188 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>  	return ret;
>  }
>  
> +static int cmd_process_map_init = 0;
> +static struct hashmap cmd_process_map;
> +
> +struct cmd2process {
> +	struct hashmap_entry ent; /* must be the first member! */
> +	const char *cmd;
> +	long protocol;
> +	struct child_process process;
> +};

> +static struct cmd2process *find_protocol_filter_entry(const char *cmd)
> +{
> +	struct cmd2process k;
> +	hashmap_entry_init(&k, strhash(cmd));
> +	k.cmd = cmd;
> +	return hashmap_get(&cmd_process_map, &k, NULL);

> +static struct cmd2process *start_protocol_filter(const char *cmd)
> +{
> +	int ret = 1;
> +	struct cmd2process *entry = NULL;
> +	struct child_process *process = NULL;
> +	struct strbuf nbuf = STRBUF_INIT;
> +	struct string_list split = STRING_LIST_INIT_NODUP;
> +	const char *argv[] = { NULL, NULL };
> +	const char *header = "git-filter-protocol\nversion";
> +
> +	entry = xmalloc(sizeof(*entry));
> +	hashmap_entry_init(entry, strhash(cmd));
> +	entry->cmd = cmd;
> +	process = &entry->process;
> +
> +	child_process_init(process);
> +	argv[0] = cmd;
> +	process->argv = argv;
> +	process->use_shell = 1;
> +	process->in = -1;
> +	process->out = -1;
> +
> +	if (start_command(process)) {
> +		error("cannot fork to run external persistent filter '%s'", cmd);
> +		return NULL;
> +	}
> +	strbuf_reset(&nbuf);

> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
> +	sigchain_pop(SIGPIPE);
> +
> +	strbuf_stripspace(&nbuf, 0);
> +	string_list_split_in_place(&split, nbuf.buf, ' ', 2);
> +	ret &= split.nr > 1;
> +	ret &= strncmp(header, split.items[0].string, strlen(header)) == 0;
> +	if (ret) {
> +		entry->protocol = strtol(split.items[1].string, NULL, 10);

> +		switch (entry->protocol) {
> +			case 1:
> +				break;
> +			default:
> +				ret = 0;
> +				error("unsupported protocol version %s for external persistent filter '%s'",
> +					nbuf.buf, cmd);
> +		}
> +	}
> +	string_list_clear(&split, 0);
> +	strbuf_release(&nbuf);
> +
> +	if (!ret) {
> +		error("initialization for external persistent filter '%s' failed", cmd);
> +		return NULL;
> +	}

> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index a05a8d2..d9077ea 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -268,4 +268,174 @@ test_expect_success 'disable filter with empty override' '
>  	test_must_be_empty err
>  '
>  
> +test_expect_success 'required protocol filter should filter data' '
> +	test_config_global filter.protocol.smudge \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> +	test_config_global filter.protocol.clean \"$TEST_DIRECTORY/t0021/rot13.pl\" &&

> new file mode 100755
> index 0000000..f2d7a03
> --- /dev/null
> +++ b/t/t0021/rot13.pl
> @@ -0,0 +1,80 @@
> +#!/usr/bin/env perl

> +#
> +# Example implementation for the Git filter protocol version 1
> +# See Documentation/gitattributes.txt, section "Filter Protocol"
> +#
> +
> +use strict;
> +use warnings;
> +use autodie;

> +
> +sub rot13 {
> +    my ($str) = @_;
> +    $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
> +    return $str;
> +}
> +
> +$| = 1; # autoflush STDOUT

> +
> +open my $debug, ">>", "output.log";
> +$debug->autoflush(1);
> +
> +print $debug "start\n";
> +
> +print STDOUT "git-filter-protocol\nversion 1";
> +print $debug "wrote version\n";
> +
> +while (1) {
> +    my $command = <STDIN>;
> +    unless (defined($command)) {
> +        exit();
> +    }
> +    chomp $command;
> +    print $debug "IN: $command";
> +    my $filename = <STDIN>;
> +    chomp $filename;
> +    print $debug " $filename";
> +    my $filelen  = <STDIN>;
> +    chomp $filelen;
> +    print $debug " $filelen";
> +
> +    $filelen = int($filelen);
> +    my $output;
> +
> +    if ( $filelen > 0 ) {

> +        my $input;
> +        {
> +            binmode(STDIN);
> +            my $bytes_read = 0;
> +            $bytes_read = read STDIN, $input, $filelen;
> +            if ( $bytes_read != $filelen ) {
> +                die "not enough to read";

> +            }
> +            print $debug " [OK] -- ";
> +        }
> +
> +        if ( $command eq "clean") {
> +            $output = rot13($input);
> +        }
> +        elsif ( $command eq "smudge" ) {

> +
> +    my $output_len = length($output);
> +    print STDOUT "$output_len\n";
> +    print $debug "OUT: $output_len";
> +    if ( $output_len > 0 ) {
> +        if ( ($command eq "clean" and $filename eq "clean-write-fail.r") or
> +             ($command eq "smudge" and $filename eq "smudge-write-fail.r") ) {

> +            print $debug " [FAIL]\n"
> +        } else {
> +            print STDOUT $output;
> +            print $debug " [OK]\n";
> +        }
> +    }
> +}

> W dniu 2016-07-22 o 17:49, larsxschneider@gmail.com pisze:
> > +use strict;
> > +use warnings;
> > +use autodie;
> 
> autodie?

> Jakub Narębski <jnareb@gmail.com> wrote:
> > W dniu 2016-07-22 o 17:49, larsxschneider@gmail.com pisze:
> > > +use strict;
> > > +use warnings;
> > > +use autodie;
> > 
> > autodie?
> 
> "set -e" for Perl (man autodie)
> 
> It's been a part of Perl for ages, but I've never used it
> myself, either; I suppose it's fine for tests...

> W dniu 2016-07-22 o 17:49, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Nb. this line is only needed if you want author name and/or date
> different from the email sender, or if you have sender line misconfigured
> (e.g. lacking the human readable name).

>> Git's clean/smudge mechanism invokes an external filter process for every
>> single blob that is affected by a filter. If Git filters a lot of blobs
>> then the startup time of the external filter processes can become a
>> significant part of the overall Git execution time.
> 
> Do I understand it correctly (from the commit description) that with
> this new option you start one filter process for the whole life of
> the Git command (e.g. `git add .`), which may perform more than one
> cleanup, but do not create a daemon or daemon-like process which
> would live for several commands invocations?

>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>> keeps the external filter process running and processes all blobs with
>> the following protocol over stdin/stdout.
> 
> I agree with Junio that the name "useProtocol" is bad, and not quite
> right. Perhaps "persistent" would be better? Also, what is the value
> of `filter.<driver>.useProtocol`: boolean? or a script name?

> My proposal is to use `filter.<driver>.persistent` as an addition
> to 'clean' and 'smudge' variables, with the following possible
> values:
> 
>  * none (the default)
>  * clean
>  * smudge
>  * both

>> 1. Git starts the filter on first usage and expects a welcome message
>> with protocol version number:
>> 	Git <-- Filter: "git-filter-protocol\n"
>> 	Git <-- Filter: "version 1"
> 
> I was wondering how Git would know that filter executable was started,
> but then I realized it was once-per-command invocation, not a daemon.
> 
> I agree with Torsten that there should be a terminator after the
> version number.

> Also, for future extendability this should be probably followed by
> possibly empty list of script capabilities, that is:
> 
> 	Git <-- Filter: "git-filter-protocol\n"
> 	Git <-- Filter: "version 1.1\n"
> 	Git <-- Filter: "capabilities clean smudge\n"
> 
> Or we can add capabilities in later version...

>> 2. Git sends the command (either "smudge" or "clean"), the filename, the
>> content size in bytes, and the content separated by a newline character:
>> 	Git --> Filter: "smudge\n"
> 
> Would it help (for some cases) to pass the name of filter that
> is being invoked?

>> 	Git --> Filter: "testfile.dat\n"
> 
> Unfortunately, while sane filenames should not contain newlines[1],
> the unfortunate fact is that *filenames can include newlines*, and
> you need to be able to handle that[2].  Therefore you need either to
> choose a different separator (the only one that can be safely used
> is "\0", i.e. the NUL character - but it is not something easy to
> handle by shell scripts), or C-quote filenames as needed, or always
> C-quote filenames.  C-quoting at minimum should include quoting newline
> character, and the escape character itself.
> 
> BTW. is it the basename of a file, or a full pathname?
> 
> [1]: http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html
> [2]: http://www.dwheeler.com/essays/filenames-in-shell.html

>> 3. The filter is expected to answer with the result content size in
>> bytes and the result content separated by a newline character:
>> 	Git <-- Filter: "15\n"
>> 	Git <-- Filter: "SMUDGED_CONTENT"
> 
> I wonder how hard would be to write filters for this protocol...

>> 4. The filter is expected to wait for the next file in step 2, again.
>> 
>> Please note that the protocol filters do not support stream processing
>> with this implemenatation because the filter needs to know the length of
>            ^^^^^^^^~^^^^^^
> 
> implementation

>> the result in advance. A protocol version 2 could address this in a
>> future patch.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> Helped-by: Martin-Louis Bright <mlbright@gmail.com>
>> ---
>> Documentation/gitattributes.txt |  41 +++++++-
>> convert.c                       | 210 ++++++++++++++++++++++++++++++++++++++--
>> t/t0021-conversion.sh           | 170 ++++++++++++++++++++++++++++++++
> 
> Wouldn't it be better to name the test case something more
> descriptive, for example
> 
>   t/t0021-filter-driver-useProtocol.sh
> 
> The name of test should be adjusted to final name of the feature,
> of course.

>> t/t0021/rot13.pl                |  80 +++++++++++++++
>> 4 files changed, 494 insertions(+), 7 deletions(-)
>> create mode 100755 t/t0021/rot13.pl
>> 
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> index 8882a3e..7026d62 100644
>> --- a/Documentation/gitattributes.txt
>> +++ b/Documentation/gitattributes.txt
>> @@ -300,7 +300,10 @@ checkout, when the `smudge` command is specified, the command is
>> fed the blob object from its standard input, and its standard
>> output is used to update the worktree file.  Similarly, the
>> `clean` command is used to convert the contents of worktree file
>> -upon checkin.
>> +upon checkin. By default these commands process only a single
>> +blob and terminate. If the setting filter.<driver>.useProtocol is
>> +enabled then Git can process all blobs with a single filter command
>> +invocation (see filter protocol below).
> 
> This does not tell the precedence between `smudge`, `clean` and
> filter.<driver>.useProtocol, see above. Also, discrepancy in how
> config variables are referenced.

>> One use of the content filtering is to massage the content into a shape
>> that is more convenient for the platform, filesystem, and the user to use.
>> @@ -375,6 +378,42 @@ substitution.  For example:
>> ------------------------
>> 
>> 
>> +Filter Protocol
>> +^^^^^^^^^^^^^^^
>> +
>> +If the setting filter.<driver>.useProtocol is enabled then Git
> 
> This seems to tell that `useProtocol` is boolean-valued (?)

>> +can process all blobs with a single filter command invocation
>> +by talking with the following protocol over stdin/stdout.
> 
> Should we use stdin/stdout shortcut, or spell standard input
> and standard output in full?

>> diff --git a/convert.c b/convert.c
>> index 522e2c5..91ce86f 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -481,12 +481,188 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>> 	return ret;
>> }
>> 
>> +static int cmd_process_map_init = 0;
>> +static struct hashmap cmd_process_map;
>> +
>> +struct cmd2process {
>> +	struct hashmap_entry ent; /* must be the first member! */
>> +	const char *cmd;
>> +	long protocol;
>> +	struct child_process process;
>> +};
> [...]
>> +static struct cmd2process *find_protocol_filter_entry(const char *cmd)
>> +{
>> +	struct cmd2process k;
>> +	hashmap_entry_init(&k, strhash(cmd));
>> +	k.cmd = cmd;
>> +	return hashmap_get(&cmd_process_map, &k, NULL);
> 
> Should we use global variable cmd_process_map, or pass it as parameter?
> The same question apply for other procedures and functions.
> 
> Note that I am not saying that it is a bad thing to use global
> variable here.

> [...]
>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry = NULL;
>> +	struct child_process *process = NULL;
>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	struct string_list split = STRING_LIST_INIT_NODUP;
>> +	const char *argv[] = { NULL, NULL };
>> +	const char *header = "git-filter-protocol\nversion";
>> +
>> +	entry = xmalloc(sizeof(*entry));
>> +	hashmap_entry_init(entry, strhash(cmd));
>> +	entry->cmd = cmd;
>> +	process = &entry->process;
>> +
>> +	child_process_init(process);
>> +	argv[0] = cmd;
>> +	process->argv = argv;
>> +	process->use_shell = 1;
>> +	process->in = -1;
>> +	process->out = -1;
>> +
>> +	if (start_command(process)) {
>> +		error("cannot fork to run external persistent filter '%s'", cmd);
>> +		return NULL;
>> +	}
>> +	strbuf_reset(&nbuf);
> 
> Is strbuf_reset needed here? We have not used nbuf variable yet.

>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
>> +	sigchain_pop(SIGPIPE);
>> +
>> +	strbuf_stripspace(&nbuf, 0);
>> +	string_list_split_in_place(&split, nbuf.buf, ' ', 2);
>> +	ret &= split.nr > 1;
>> +	ret &= strncmp(header, split.items[0].string, strlen(header)) == 0;
>> +	if (ret) {
>> +		entry->protocol = strtol(split.items[1].string, NULL, 10);
> 
> This does not handle at least some errors in version number parsing,
> for example junk after version number. Don't we have some helper
> functions for this?

>> +		switch (entry->protocol) {
>> +			case 1:
>> +				break;
>> +			default:
>> +				ret = 0;
>> +				error("unsupported protocol version %s for external persistent filter '%s'",
>> +					nbuf.buf, cmd);
>> +		}
>> +	}
>> +	string_list_clear(&split, 0);
>> +	strbuf_release(&nbuf);
>> +
>> +	if (!ret) {
>> +		error("initialization for external persistent filter '%s' failed", cmd);
>> +		return NULL;
>> +	}
> 
> Do we handle persistent filter command being killed before it finishes?
> Or exiting with error? I don't know this Git API...

>> +
>> +	hashmap_add(&cmd_process_map, entry);
>> +	return entry;
>> +}
> [...]
> 
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index a05a8d2..d9077ea 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -268,4 +268,174 @@ test_expect_success 'disable filter with empty override' '
>> 	test_must_be_empty err
>> '
>> 
>> +test_expect_success 'required protocol filter should filter data' '
>> +	test_config_global filter.protocol.smudge \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
>> +	test_config_global filter.protocol.clean \"$TEST_DIRECTORY/t0021/rot13.pl\" &&
> 
> Perhaps align it?
> 
>  +	test_config_global filter.protocol.clean  \"$TEST_DIRECTORY/t0021/rot13.pl\" &&

>> new file mode 100755
>> index 0000000..f2d7a03
>> --- /dev/null
>> +++ b/t/t0021/rot13.pl
>> @@ -0,0 +1,80 @@
>> +#!/usr/bin/env perl
> 
> Don't we use other way to specify perl path for Git, and for its
> test suite?

>> +#
>> +# Example implementation for the Git filter protocol version 1
>> +# See Documentation/gitattributes.txt, section "Filter Protocol"
>> +#
>> +
>> +use strict;
>> +use warnings;
>> +use autodie;
> 
> autodie?

>> +
>> +sub rot13 {
>> +    my ($str) = @_;
>> +    $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
>> +    return $str;
>> +}
>> +
>> +$| = 1; # autoflush STDOUT
> 
> Perhaps *STDOUT->autoflush(1), if I remember my Perl correctly?
> Should this matter? Why it is needed?

>> +
>> +open my $debug, ">>", "output.log";
>> +$debug->autoflush(1);
>> +
>> +print $debug "start\n";
>> +
>> +print STDOUT "git-filter-protocol\nversion 1";
>> +print $debug "wrote version\n";
>> +
>> +while (1) {
>> +    my $command = <STDIN>;
>> +    unless (defined($command)) {
>> +        exit();
>> +    }
>> +    chomp $command;
>> +    print $debug "IN: $command";
>> +    my $filename = <STDIN>;
>> +    chomp $filename;
>> +    print $debug " $filename";
>> +    my $filelen  = <STDIN>;
>> +    chomp $filelen;
>> +    print $debug " $filelen";
>> +
>> +    $filelen = int($filelen);
>> +    my $output;
>> +
>> +    if ( $filelen > 0 ) {
> 
> Inconsistent style. You use
> 
>       unless (defined($command)) {
> 
> without extra whitespace after and before parentheses, but
> 
>       if ( $filelen > 0 ) {
> 
> instead of simply
> 
>       if ($filelen > 0) {

>> +        my $input;
>> +        {
>> +            binmode(STDIN);
>> +            my $bytes_read = 0;
>> +            $bytes_read = read STDIN, $input, $filelen;
>> +            if ( $bytes_read != $filelen ) {
>> +                die "not enough to read";
> 
> I know it's only a test script (well, a part of one), but we would probably
> want to have more information in the case of a real filter.

> 
>> +            }
>> +            print $debug " [OK] -- ";
>> +        }
>> +
>> +        if ( $command eq "clean") {
>> +            $output = rot13($input);
>> +        }
>> +        elsif ( $command eq "smudge" ) {
> 
> Style; I think we use
> 
>  +        } elsif ( $command eq "smudge" ) {

> 
>> +            $output = rot13($input);
>> +        }
>> +        else {
>> +            die "bad command\n";
> 
> Same here (both about style, and error message).
> 
>> +        }
>> +    }
> 
> What happens if $filelen is zero, or negative? Ah, I see that $output
> would be undef... which is bad, I think.

>> +
>> +    my $output_len = length($output);
>> +    print STDOUT "$output_len\n";
>> +    print $debug "OUT: $output_len";
>> +    if ( $output_len > 0 ) {
>> +        if ( ($command eq "clean" and $filename eq "clean-write-fail.r") or
>> +             ($command eq "smudge" and $filename eq "smudge-write-fail.r") ) {
> 
> Hardcoded filenames, without it being described in the file header?

> 
>> +            print STDOUT "fail";
> 
> This is not defined in the protocol description!  Unless anything that
> does not conform to the specification would work here, but at least it
> is a recommended practice to be described in the documentation, don't
> you think?
> 
> What would happen in $output_len is 4?

> On 23 Jul 2016, at 02:11, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 2016-07-22 o 17:49, larsxschneider@gmail.com pisze:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Nb. this line is only needed if you want author name and/or date
>> different from the email sender, or if you have sender line misconfigured
>> (e.g. lacking the human readable name).
> 
> I use "git format-patch" to generate these emails:
> 
> git format-patch --cover-letter --subject-prefix="PATCH ..." -M $BASE -o $OUTPUT
> 
> How would I disable this line? (I already checked the man page to no avail).

>>> Git's clean/smudge mechanism invokes an external filter process for every
>>> single blob that is affected by a filter. If Git filters a lot of blobs
>>> then the startup time of the external filter processes can become a
>>> significant part of the overall Git execution time.
>>
>> Do I understand it correctly (from the commit description) that with
>> this new option you start one filter process for the whole life of
>> the Git command (e.g. `git add .`), which may perform more than one
>> cleanup, but do not create a daemon or daemon-like process which
>> would live for several commands invocations?
> 
> Correct!

>>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>>> keeps the external filter process running and processes all blobs with
>>> the following protocol over stdin/stdout.
>>
>> I agree with Junio that the name "useProtocol" is bad, and not quite
>> right. Perhaps "persistent" would be better? Also, what is the value
>> of `filter.<driver>.useProtocol`: boolean? or a script name?

> I agree that the name is not ideal. "UseProtocol" as it is would be a boolean. 
> I thought about "persistent" but this name wouldn't convey the scope of the 
> persistency ("persistent for one Git operation" vs. "persistent for many Git 
> operations"). What do you think about the protocol as int version idea
> described in $gmane/300155 ?

>> I also agree that we might wat to be able to keep clean and smudge
>> filters separate, but be able to run a single program if they are
>> both the same. I think there is a special case for filter unset,
>> and/or filter being "cat" -- we would want to keep that.
> 
> Since 1a8630d there is a more efficient way to unset a filter ;-)
> Can you think of other cases where the separation would be useful?

>> My proposal is to use `filter.<driver>.persistent` as an addition
>> to 'clean' and 'smudge' variables, with the following possible
>> values:
>>
>>  * none (the default)
>>  * clean
>>  * smudge
>>  * both
> 
> That could work. However, I am not convinced, yet, that separate
> filters are an actual use case.

>> I assume that either Git would have to start multiple filter
>> commands for multi-threaded operation, or the protocol would have
>> to be extended to make persistent filter fork itself.
> 
> I think it would be better to have Git start multiple filter commands
> to keep the protocol as simple and error free as possible.

>> BTW. what would happen in your original proposal if the user had
>> *both* filter.<driver>.useProtocol and filter.<driver>.smudge
>> (and/or filter.<driver>.clean) set?
> 
> That wouldn't be an issue as "useProtocol" is just a boolean that
> tells Git how to talk to "filter.<driver>.smudge" and "filter.<driver>.clean".
> I need to make this more clear in the documentation.
> 
> 
>>> 1. Git starts the filter on first usage and expects a welcome message
>>> with protocol version number:
>>> 	Git <-- Filter: "git-filter-protocol\n"
>>> 	Git <-- Filter: "version 1"
>>
>> I was wondering how Git would know that filter executable was started,
>> but then I realized it was once-per-command invocation, not a daemon.
>>
>> I agree with Torsten that there should be a terminator after the
>> version number.
> 
> I agree, too :)

>> Also, for future extendability this should be probably followed by
>> possibly empty list of script capabilities, that is:
>>
>> 	Git <-- Filter: "git-filter-protocol\n"
>> 	Git <-- Filter: "version 1.1\n"

>> 	Git <-- Filter: "capabilities clean smudge\n"
>>
>> Or we can add capabilities in later version...
> 
> That is an interesting idea. My initial thought was to make the capabilities
> of a certain version fix. If we want to add new capabilities then we would 
> bump the version. I wonder what others think about your suggestion!

>> BTW. why not follow e.g. HTTP protocol example, and use
>>
>> 	Git <-- Filter: "git-filter-protocol/1\n"
> 
> I think my proposal is a bit more explicit as it states "version". If
> you feel strongly about it, I could be convinced otherwise.

>>> 2. Git sends the command (either "smudge" or "clean"), the filename, the
>>> content size in bytes, and the content separated by a newline character:
>>> 	Git --> Filter: "smudge\n"
>>
>> Would it help (for some cases) to pass the name of filter that
>> is being invoked?
> 
> Interesting thought! Can you imagine a case where this would be useful?

>>> 	Git --> Filter: "testfile.dat\n"
>>
>> Unfortunately, while sane filenames should not contain newlines[1],
>> the unfortunate fact is that *filenames can include newlines*, and
>> you need to be able to handle that[2].  Therefore you need either to
>> choose a different separator (the only one that can be safely used
>> is "\0", i.e. the NUL character - but it is not something easy to
>> handle by shell scripts), or C-quote filenames as needed, or always
>> C-quote filenames.  C-quoting at minimum should include quoting newline
>> character, and the escape character itself.
>>
>> BTW. is it the basename of a file, or a full pathname?
>>
>> [1]: http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html
>> [2]: http://www.dwheeler.com/essays/filenames-in-shell.html
> 
> Thanks for this explanation. A bash version of the protocol is not
> trivial (I tried it but ended up using Perl). Therefore I think "\0"
> would be a good choice?

>>> 	Git --> Filter: "7\n"
>>
>> That's the content size in bytes written as an ASCII number.
> 
> Correct.

>>> 	Git --> Filter: "CONTENT"
>>
>> Can filter ignore the content size, and just read all what it was
>> sent, that is until eof or something?
> 
> What would that something be? Since CONTENT is binary it can contain
> any character (even "\0")...

>>> 3. The filter is expected to answer with the result content size in
>>> bytes and the result content separated by a newline character:
>>> 	Git <-- Filter: "15\n"
>>> 	Git <-- Filter: "SMUDGED_CONTENT"
>>
>> I wonder how hard would be to write filters for this protocol...
> 
> Easy :-) Plus you can look at a Perl (see t/t0021) and a golang implementation
> already (https://github.com/github/git-lfs/pull/1382).

>>> ---
>>> Documentation/gitattributes.txt |  41 +++++++-
>>> convert.c                       | 210 ++++++++++++++++++++++++++++++++++++++--
>>> t/t0021-conversion.sh           | 170 ++++++++++++++++++++++++++++++++
>>
>> Wouldn't it be better to name the test case something more
>> descriptive, for example
>>
>>   t/t0021-filter-driver-useProtocol.sh
>>
>> The name of test should be adjusted to final name of the feature,
>> of course.
> 
> I think the prefix numbers should be unique, no? And t0022 is already taken.

>>> 4 files changed, 494 insertions(+), 7 deletions(-)
>>> create mode 100755 t/t0021/rot13.pl
>>>
>>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>>> index 8882a3e..7026d62 100644
>>> --- a/Documentation/gitattributes.txt
>>> +++ b/Documentation/gitattributes.txt
>>> @@ -300,7 +300,10 @@ checkout, when the `smudge` command is specified, the command is
>>> fed the blob object from its standard input, and its standard
>>> output is used to update the worktree file.  Similarly, the
>>> `clean` command is used to convert the contents of worktree file
>>> -upon checkin.
>>> +upon checkin. By default these commands process only a single
>>> +blob and terminate. If the setting filter.<driver>.useProtocol is
>>> +enabled then Git can process all blobs with a single filter command
>>> +invocation (see filter protocol below).
>>
>> This does not tell the precedence between `smudge`, `clean` and
>> filter.<driver>.useProtocol, see above. Also, discrepancy in how
>> config variables are referenced.
> 
> As mentioned above "useProtocol" is a boolean. Therefore precedence shouldn't
> be a problem.

>>> diff --git a/convert.c b/convert.c
>>> index 522e2c5..91ce86f 100644
>>> --- a/convert.c
>>> +++ b/convert.c
>>> @@ -481,12 +481,188 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>>> 	return ret;
>>> }
>>>
>>> +static int cmd_process_map_init = 0;
>>> +static struct hashmap cmd_process_map;
>>> +
>>> +struct cmd2process {
>>> +	struct hashmap_entry ent; /* must be the first member! */
>>> +	const char *cmd;
>>> +	long protocol;
>>> +	struct child_process process;
>>> +};
>> [...]
>>> +static struct cmd2process *find_protocol_filter_entry(const char *cmd)
>>> +{
>>> +	struct cmd2process k;
>>> +	hashmap_entry_init(&k, strhash(cmd));
>>> +	k.cmd = cmd;
>>> +	return hashmap_get(&cmd_process_map, &k, NULL);
>>
>> Should we use global variable cmd_process_map, or pass it as parameter?
>> The same question apply for other procedures and functions.
>>
>> Note that I am not saying that it is a bad thing to use global
>> variable here.
> 
> Passing it would be nicer as this would make at least a few functions "pure".
> I will change that!

>>> +
>>> +	sigchain_push(SIGPIPE, SIG_IGN);
>>> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
>>> +	sigchain_pop(SIGPIPE);
>>> +
>>> +	strbuf_stripspace(&nbuf, 0);
>>> +	string_list_split_in_place(&split, nbuf.buf, ' ', 2);
>>> +	ret &= split.nr > 1;
>>> +	ret &= strncmp(header, split.items[0].string, strlen(header)) == 0;
>>> +	if (ret) {
>>> +		entry->protocol = strtol(split.items[1].string, NULL, 10);
>>
>> This does not handle at least some errors in version number parsing,
>> for example junk after version number. Don't we have some helper
>> functions for this?
> 
> I am not sure. I haven't found one.

>>> +		switch (entry->protocol) {
>>> +			case 1:
>>> +				break;
>>> +			default:
>>> +				ret = 0;
>>> +				error("unsupported protocol version %s for external persistent filter '%s'",
>>> +					nbuf.buf, cmd);
>>> +		}
>>> +	}
>>> +	string_list_clear(&split, 0);
>>> +	strbuf_release(&nbuf);
>>> +
>>> +	if (!ret) {
>>> +		error("initialization for external persistent filter '%s' failed", cmd);
>>> +		return NULL;
>>> +	}
>>
>> Do we handle persistent filter command being killed before it finishes?
>> Or exiting with error? I don't know this Git API...
> 
> If the "apply_filter" function fails then Git would proceed and just not
> filter the content. If you define the "required" flag for the filter then
> Git would error in that case.

>>> diff --git a/t/t0021/rot13.pl b/t/t0021/rot13.pl
>>
>> That's bit more than rot13... but it might be O.K. for a filename here.
> 
> "rot13-$FEATURE_NAME.pl" ?

>>> +        my $input;
>>> +        {
>>> +            binmode(STDIN);
>>> +            my $bytes_read = 0;
>>> +            $bytes_read = read STDIN, $input, $filelen;
>>> +            if ( $bytes_read != $filelen ) {
>>> +                die "not enough to read";
>>
>> I know it's only a test script (well, a part of one), but we would probably
>> want to have more information in the case of a real filter.
> 
> True. Do you think there is anything to change in the script, though?

>>> +        }
>>> +    }
>>
>> What happens if $filelen is zero, or negative? Ah, I see that $output
>> would be undef... which is bad, I think.
> 
> Is this something we need to consider in the test script?

>>> +            print STDOUT "fail";
>>
>> This is not defined in the protocol description!  Unless anything that
>> does not conform to the specification would work here, but at least it
>> is a recommended practice to be described in the documentation, don't
>> you think?
>>
>> What would happen in $output_len is 4?
> 
> Then it would work :D
> I understand your point. However, this is not a reference implementation.
> It is a test script that is supposed to trigger bad behavior which we can test. 
> Therefore, I would argue that such a return value is OK. I will document it in 
> the header, though. 

>> I agree that the name is not ideal. "UseProtocol" as it is would be a boolean. 
>> I thought about "persistent" but this name wouldn't convey the scope of the 
>> persistency ("persistent for one Git operation" vs. "persistent for many Git 
>> operations"). What do you think about the protocol as int version idea
>> described in $gmane/300155 ?
>
> You mean the `protocol` as a config variable name (fully name being
> `filter.<driver>.protocol`), being integer-valued, isn't it? Wouldn't
> `protocolVersion` be a more explicit?

> W dniu 2016-07-24 o 22:14, Jakub Narębski pisze:
>> W dniu 2016-07-24 o 20:36, Lars Schneider pisze:
> 
>>> I agree that the name is not ideal. "UseProtocol" as it is would be a boolean. 
>>> I thought about "persistent" but this name wouldn't convey the scope of the 
>>> persistency ("persistent for one Git operation" vs. "persistent for many Git 
>>> operations"). What do you think about the protocol as int version idea
>>> described in $gmane/300155 ?
>> 
>> You mean the `protocol` as a config variable name (fully name being
>> `filter.<driver>.protocol`), being integer-valued, isn't it? Wouldn't
>> `protocolVersion` be a more explicit?
> 
> Just throwing out further ideas:
> 
> Perhaps make `persistent` string-valued variable, with the only value
> supported for now, namely "per-process" / "operation"?
> 
> Perhaps require for `pidfile` to be present for it to be daemon,
> that is persist for possibly many Git operations. Or allow "daemon"
> or "server" value for `persistent`, then?

> 
> On 24 Jul 2016, at 23:30, Jakub Narębski <jnareb@gmail.com> wrote:
> 
>> W dniu 2016-07-24 o 22:14, Jakub Narębski pisze:
>>> W dniu 2016-07-24 o 20:36, Lars Schneider pisze:
>>
>>>> I agree that the name is not ideal. "UseProtocol" as it is would be a boolean. 
>>>> I thought about "persistent" but this name wouldn't convey the scope of the 
>>>> persistency ("persistent for one Git operation" vs. "persistent for many Git 
>>>> operations"). What do you think about the protocol as int version idea
>>>> described in $gmane/300155 ?
>>>
>>> You mean the `protocol` as a config variable name (fully name being
>>> `filter.<driver>.protocol`), being integer-valued, isn't it? Wouldn't
>>> `protocolVersion` be a more explicit?
>>
>> Just throwing out further ideas:
>>
>> Perhaps make `persistent` string-valued variable, with the only value
>> supported for now, namely "per-process" / "operation"?
>>
>> Perhaps require for `pidfile` to be present for it to be daemon,
>> that is persist for possibly many Git operations. Or allow "daemon"
>> or "server" value for `persistent`, then?
> 
> I like the direction of this idea. What if we use a string-valued 
> "filter.<driver>.protocol" with the following options:
> 
> "simple" / "invocation-per-file" / << empty >> --> current clean/smudge behavior
> "invocation-per-process" --> new, proposed behavior
> 
> If necessary this could be enhanced in the future to support even a "daemon"
> mode (with a pidfile config).

> W dniu 2016-07-24 o 20:36, Lars Schneider pisze:
>> On 23 Jul 2016, at 02:11, Jakub Narębski <jnareb@gmail.com> wrote:
>>> W dniu 2016-07-22 o 17:49, larsxschneider@gmail.com pisze:
>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>> 
>>> Nb. this line is only needed if you want author name and/or date
>>> different from the email sender, or if you have sender line misconfigured
>>> (e.g. lacking the human readable name).
>> 
>> I use "git format-patch" to generate these emails:
>> 
>> git format-patch --cover-letter --subject-prefix="PATCH ..." -M $BASE -o $OUTPUT
>> 
>> How would I disable this line? (I already checked the man page to no avail).
> 
> If you are using `git send-email` or equivalent, I think it is
> stripped automatically if it is not needed (in you case it was,
> because Sender was lacking human readable name... at least I think
> it was because of what my email reader inserted as reply line).
> If you are using an ordinary email client, you need to remove it
> yourself, if needed.

>> Plus, what does "Nb" stand for? :-)
> 
> Nb. (or N.b.) stands for "nota bene", which I meant do denote
> as a note on a certain side aspect; I'll switch to "note", or
> "BTW" / "by the way".

>>>> Git's clean/smudge mechanism invokes an external filter process for every
>>>> single blob that is affected by a filter. If Git filters a lot of blobs
>>>> then the startup time of the external filter processes can become a
>>>> significant part of the overall Git execution time.
>>> 
>>> Do I understand it correctly (from the commit description) that with
>>> this new option you start one filter process for the whole life of
>>> the Git command (e.g. `git add .`), which may perform more than one
>>> cleanup, but do not create a daemon or daemon-like process which
>>> would live for several commands invocations?
>> 
>> Correct!
> 
> It would be nice to make it more obvious.

> 
>>>> This patch adds the filter.<driver>.useProtocol option which, if enabled,
>>>> keeps the external filter process running and processes all blobs with
>>>> the following protocol over stdin/stdout.
>>> 
>>> I agree with Junio that the name "useProtocol" is bad, and not quite
>>> right. Perhaps "persistent" would be better? Also, what is the value
>>> of `filter.<driver>.useProtocol`: boolean? or a script name?
> 
> As you see I was not sure if `useProtocol` was boolean or a script name,
> which means that it should be stated more explicitly.  Of course this
> would end to not matter if the way new protocol is used were changed.
> 
>> I agree that the name is not ideal. "UseProtocol" as it is would be a boolean. 
>> I thought about "persistent" but this name wouldn't convey the scope of the 
>> persistency ("persistent for one Git operation" vs. "persistent for many Git 
>> operations"). What do you think about the protocol as int version idea
>> described in $gmane/300155 ?
> 
> You mean the `protocol` as a config variable name (fully name being
> `filter.<driver>.protocol`), being integer-valued, isn't it? Wouldn't
> `protocolVersion` be a more explicit?

>>> I also agree that we might wat to be able to keep clean and smudge
>>> filters separate, but be able to run a single program if they are
>>> both the same. I think there is a special case for filter unset,
>>> and/or filter being "cat" -- we would want to keep that.
>> 
>> Since 1a8630d there is a more efficient way to unset a filter ;-)
>> Can you think of other cases where the separation would be useful?
> 
> I can't think of any, but it doesn't mean that it does not exist.
> It also does not mean that you need to consider situation that may
> not happen. Covering one-way filters, like "indent" filter for `clean`,
> should be enough... they do work with your proposal, don't they?

>>> My proposal is to use `filter.<driver>.persistent` as an addition
>>> to 'clean' and 'smudge' variables, with the following possible
>>> values:
>>> 
>>> * none (the default)
>>> * clean
>>> * smudge
>>> * both
>> 
>> That could work. However, I am not convinced, yet, that separate
>> filters are an actual use case.
> 
> YAGNI (You Ain't Gonna Need It), right.

>>> I assume that either Git would have to start multiple filter
>>> commands for multi-threaded operation, or the protocol would have
>>> to be extended to make persistent filter fork itself.
>> 
>> I think it would be better to have Git start multiple filter commands
>> to keep the protocol as simple and error free as possible.
> 
> Right. Also, I am not sure if exec+fork would be much faster than
> fork+exec (where fork is n-way fork, and n is number of threads
> that Git command invoking filter is using).
> 
>>> BTW. what would happen in your original proposal if the user had
>>> *both* filter.<driver>.useProtocol and filter.<driver>.smudge
>>> (and/or filter.<driver>.clean) set?
>> 
>> That wouldn't be an issue as "useProtocol" is just a boolean that
>> tells Git how to talk to "filter.<driver>.smudge" and "filter.<driver>.clean".
>> I need to make this more clear in the documentation.
>> 
>> 
>>>> 1. Git starts the filter on first usage and expects a welcome message
>>>> with protocol version number:
>>>> 	Git <-- Filter: "git-filter-protocol\n"
>>>> 	Git <-- Filter: "version 1"
>>> 
>>> I was wondering how Git would know that filter executable was started,
>>> but then I realized it was once-per-command invocation, not a daemon.
>>> 
>>> I agree with Torsten that there should be a terminator after the
>>> version number.
>> 
>> I agree, too :)
> 
> Note that if we agree about switch to `protocol` / `protocolVersion`
> as a way to specify this protocol, it would probably need to be "protocol 2"
> (assuming that "protocol 1" is the original implementation, with one fork
> per affected file).

>>> Also, for future extendability this should be probably followed by
>>> possibly empty list of script capabilities, that is:
>>> 
>>> 	Git <-- Filter: "git-filter-protocol\n"
>>> 	Git <-- Filter: "version 1.1\n"
> 
> Note that "version 1.1" would not work with current implementation;
> it accepts only integer version numbers. Which might be a good idea,
> anyway.

>>> 	Git <-- Filter: "capabilities clean smudge\n"
>>> 
>>> Or we can add capabilities in later version...
>> 
>> That is an interesting idea. My initial thought was to make the capabilities
>> of a certain version fix. If we want to add new capabilities then we would 
>> bump the version. I wonder what others think about your suggestion!
> 
> Using capabilities (like git-upload-pack / git-receive-pack, that is
> smart Git transfer protocols do) is probably slightly more difficult on
> the Git side (assuming no capabilities negotiation), but also much more
> flexible than pure version numbers.
> 
> One possible idea for a capability is support for passing input
> and output of a filter via filesystem, like cleanToFile and smudgeFromFile
> proposal in 'jh/clean-smudge-annex' (in 'pu').
> 
> For example:
> 
> 	Git <-- Filter: "capabilities clean smudge cleanToFile smudgeFromFile\n"

>>> BTW. why not follow e.g. HTTP protocol example, and use
>>> 
>>> 	Git <-- Filter: "git-filter-protocol/1\n"
>> 
>> I think my proposal is a bit more explicit as it states "version". If
>> you feel strongly about it, I could be convinced otherwise.
> 
> No, I don't feel strongly about this. I think SSH also uses a separate
> "version"-like line.
> 
>>>> 2. Git sends the command (either "smudge" or "clean"), the filename, the
>>>> content size in bytes, and the content separated by a newline character:
>>>> 	Git --> Filter: "smudge\n"
>>> 
>>> Would it help (for some cases) to pass the name of filter that
>>> is being invoked?
>> 
>> Interesting thought! Can you imagine a case where this would be useful?
> 
> Actually... no, I don't think so. I don't think there is a situation
> where we might want to use the same filter commands for different filters
> and have it behave differently depending on the filter name.

>>>> 	Git --> Filter: "testfile.dat\n"
>>> 
>>> Unfortunately, while sane filenames should not contain newlines[1],
>>> the unfortunate fact is that *filenames can include newlines*, and
>>> you need to be able to handle that[2].  Therefore you need either to
>>> choose a different separator (the only one that can be safely used
>>> is "\0", i.e. the NUL character - but it is not something easy to
>>> handle by shell scripts), or C-quote filenames as needed, or always
>>> C-quote filenames.  C-quoting at minimum should include quoting newline
>>> character, and the escape character itself.
>>> 
>>> BTW. is it the basename of a file, or a full pathname?
>>> 
>>> [1]: http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html
>>> [2]: http://www.dwheeler.com/essays/filenames-in-shell.html
>> 
>> Thanks for this explanation. A bash version of the protocol is not
>> trivial (I tried it but ended up using Perl). Therefore I think "\0"
>> would be a good choice?
> 
> That, or use git convention of surrounding C-quoted filenames in
> double quotes (which means that if it begins with quote, it is C-quoted).
> 
> For example:
> 
>  $ git commit -m 'Initial commit'
>  [master (root-commit) 266dab0] Initial commit
>   2 files changed, 2 insertions(+)
>   create mode 100644 foo
>   create mode 100644 "foo \" \\ ,"
>  $ ls -1
>  foo " \ ,
>  foo
> 
> I'm not sure which solution would be easier for filter writers,
> NUL termination, or C-quoting.

>>>> 	Git --> Filter: "7\n"
>>> 
>>> That's the content size in bytes written as an ASCII number.
>> 
>> Correct.
> 
> But not obvious from the description / documentation.

>>>> 	Git --> Filter: "CONTENT"
>>> 
>>> Can filter ignore the content size, and just read all what it was
>>> sent, that is until eof or something?
>> 
>> What would that something be? Since CONTENT is binary it can contain
>> any character (even "\0")...
> 
> Here by "or something" I meant some other way of detecting that there
> is nothing more to read. But providing the size upfront (or size of
> chunk / packet in the streaming interface, if/when it gets implemented)
> is probably a better idea. Git knows it anyway, cheaply.
> 
>>>> 3. The filter is expected to answer with the result content size in
>>>> bytes and the result content separated by a newline character:
>>>> 	Git <-- Filter: "15\n"
>>>> 	Git <-- Filter: "SMUDGED_CONTENT"
>>> 
>>> I wonder how hard would be to write filters for this protocol...
>> 
>> Easy :-) Plus you can look at a Perl (see t/t0021) and a golang implementation
>> already (https://github.com/github/git-lfs/pull/1382).
> 
> Right. Any programming language that has a way to specify "read N bytes"
> would work. I think even bash would work, with 'read -N $len -r'... I think.
> 
>>>> ---
>>>> Documentation/gitattributes.txt |  41 +++++++-
>>>> convert.c                       | 210 ++++++++++++++++++++++++++++++++++++++--
>>>> t/t0021-conversion.sh           | 170 ++++++++++++++++++++++++++++++++
>>> 
>>> Wouldn't it be better to name the test case something more
>>> descriptive, for example
>>> 
>>>  t/t0021-filter-driver-useProtocol.sh
>>> 
>>> The name of test should be adjusted to final name of the feature,
>>> of course.
>> 
>> I think the prefix numbers should be unique, no? And t0022 is already taken.
> 
> I meant here that the "conversion" part of "t/t0021-conversion.sh" test
> filename is not descriptive enough.

>>>> t/t0021/rot13.pl                |  80 +++++++++++++++
> 
> This is all right, because it is in t0021 context.
> 
>>>> 4 files changed, 494 insertions(+), 7 deletions(-)
>>>> create mode 100755 t/t0021/rot13.pl
>>>> 
>>>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>>>> index 8882a3e..7026d62 100644
>>>> --- a/Documentation/gitattributes.txt
>>>> +++ b/Documentation/gitattributes.txt
>>>> @@ -300,7 +300,10 @@ checkout, when the `smudge` command is specified, the command is
>>>> fed the blob object from its standard input, and its standard
>>>> output is used to update the worktree file.  Similarly, the
>>>> `clean` command is used to convert the contents of worktree file
>>>> -upon checkin.
>>>> +upon checkin. By default these commands process only a single
>>>> +blob and terminate. If the setting filter.<driver>.useProtocol is
>>>> +enabled then Git can process all blobs with a single filter command
>>>> +invocation (see filter protocol below).
>>> 
>>> This does not tell the precedence between `smudge`, `clean` and
>>> filter.<driver>.useProtocol, see above. Also, discrepancy in how
>>> config variables are referenced.
>> 
>> As mentioned above "useProtocol" is a boolean. Therefore precedence shouldn't
>> be a problem.
> 
> Which was not obvious (but might not matter in the end).
> 
>>             What do you mean by "discrepancy in how config variables are 
>> referenced"?
> 
> What I meant here that filter.<driver>.smudge and filter.<driver>.clean
> were referenced as "`smudge` command" and "`clean` command" in the paragraph
> you modified.
> 
> Perhaps filter.<driver>.useProtocol is all right (I have not looked further),
> but it should be formatted as `filter.<driver>.useProtocol` IMVHO.

> [...]
>>>> diff --git a/convert.c b/convert.c
>>>> index 522e2c5..91ce86f 100644
>>>> --- a/convert.c
>>>> +++ b/convert.c
>>>> @@ -481,12 +481,188 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>>>> 	return ret;
>>>> }
>>>> 
>>>> +static int cmd_process_map_init = 0;
>>>> +static struct hashmap cmd_process_map;
>>>> +
>>>> +struct cmd2process {
>>>> +	struct hashmap_entry ent; /* must be the first member! */
>>>> +	const char *cmd;
>>>> +	long protocol;
>>>> +	struct child_process process;
>>>> +};
>>> [...]
>>>> +static struct cmd2process *find_protocol_filter_entry(const char *cmd)
>>>> +{
>>>> +	struct cmd2process k;
>>>> +	hashmap_entry_init(&k, strhash(cmd));
>>>> +	k.cmd = cmd;
>>>> +	return hashmap_get(&cmd_process_map, &k, NULL);
>>> 
>>> Should we use global variable cmd_process_map, or pass it as parameter?
>>> The same question apply for other procedures and functions.
>>> 
>>> Note that I am not saying that it is a bad thing to use global
>>> variable here.
>> 
>> Passing it would be nicer as this would make at least a few functions "pure".
>> I will change that!
> 
> You can always provide convenience functions that use global variable.
> That's what Git code does with the_index, if I remember it correctly.
> 
> [...]
>>>> +
>>>> +	sigchain_push(SIGPIPE, SIG_IGN);
>>>> +	ret &= strbuf_read_once(&nbuf, process->out, 0) > 0;
>>>> +	sigchain_pop(SIGPIPE);
>>>> +
>>>> +	strbuf_stripspace(&nbuf, 0);
>>>> +	string_list_split_in_place(&split, nbuf.buf, ' ', 2);
>>>> +	ret &= split.nr > 1;
>>>> +	ret &= strncmp(header, split.items[0].string, strlen(header)) == 0;
>>>> +	if (ret) {
>>>> +		entry->protocol = strtol(split.items[1].string, NULL, 10);
>>> 
>>> This does not handle at least some errors in version number parsing,
>>> for example junk after version number. Don't we have some helper
>>> functions for this?
>> 
>> I am not sure. I haven't found one.
> 
> Hmmm... I remember there were some patches about this, but I don't know
> if they were accepted.  We have strtol_i() in git-compat-util.h. 
> 
> And you can always check where the parsing ended (by not passing NULL,
> of course).

> [...]
>>>> +		switch (entry->protocol) {
>>>> +			case 1:
>>>> +				break;
>>>> +			default:
>>>> +				ret = 0;
>>>> +				error("unsupported protocol version %s for external persistent filter '%s'",
>>>> +					nbuf.buf, cmd);
>>>> +		}
>>>> +	}
>>>> +	string_list_clear(&split, 0);
>>>> +	strbuf_release(&nbuf);
>>>> +
>>>> +	if (!ret) {
>>>> +		error("initialization for external persistent filter '%s' failed", cmd);
>>>> +		return NULL;
>>>> +	}
>>> 
>>> Do we handle persistent filter command being killed before it finishes?
>>> Or exiting with error? I don't know this Git API...
>> 
>> If the "apply_filter" function fails then Git would proceed and just not
>> filter the content. If you define the "required" flag for the filter then
>> Git would error in that case.
> 
> Ah, right. 
> 
> [...]
>>>> diff --git a/t/t0021/rot13.pl b/t/t0021/rot13.pl
>>> 
>>> That's bit more than rot13... but it might be O.K. for a filename here.
>> 
>> "rot13-$FEATURE_NAME.pl" ?
> 
> As I said, rot13.pl is all right; if change, then perhaps to rot13-filter.pl 

> [...]
>>>> +        my $input;
>>>> +        {
>>>> +            binmode(STDIN);
>>>> +            my $bytes_read = 0;
>>>> +            $bytes_read = read STDIN, $input, $filelen;
>>>> +            if ( $bytes_read != $filelen ) {
>>>> +                die "not enough to read";
>>> 
>>> I know it's only a test script (well, a part of one), but we would probably
>>> want to have more information in the case of a real filter.
>> 
>> True. Do you think there is anything to change in the script, though?
> 
> No, I don't think so. It is enough that the test script would crash
> if fed incorrect data from Git. Better error messages would be nice,
> but are not necessary.
> 
>    +                die "not enough to read: expected $filelen, got $bytes_read";
> 
> I have noticed that some 'die' have "\n" at the end, and some do
> not. If I remember correctly it is for supressing error message from
> Perl, with filename and line number, isn't it? Anyway, we probably
> want to be consistent.

>>>> +        }
>>>> +    }
>>> 
>>> What happens if $filelen is zero, or negative? Ah, I see that $output
>>> would be undef... which is bad, I think.
>> 
>> Is this something we need to consider in the test script?
> 
> I think the test script should die if it gets incorrect content length
> (e.g. negative), so that it catches bug on the Git side. It should
> work for zero-length files, even if we don't test it -- we can in the
> future.

> [...]
>>>> +            print STDOUT "fail";
>>> 
>>> This is not defined in the protocol description!  Unless anything that
>>> does not conform to the specification would work here, but at least it
>>> is a recommended practice to be described in the documentation, don't
>>> you think?
>>> 
>>> What would happen in $output_len is 4?
>> 
>> Then it would work :D
>> I understand your point. However, this is not a reference implementation.
>> It is a test script that is supposed to trigger bad behavior which we can test. 
>> Therefore, I would argue that such a return value is OK. I will document it in 
>> the header, though. 
> 
> Why print "fail", and not die?

> The problem is do the protocol need to have some way of communicating
> errors from the filter to Git?  Perhaps using stderr would be enough
> (but then Git would need to drain it, I think... unless it is not
> redirected), perhaps some command is needed?
> 
> For example, instead of:
> 
> 	Git <-- Filter: "15\n"
> 	Git <-- Filter: "SMUDGED_CONTENT"
> 
> perhaps filter should return
> 
> 	Git <-- Filter: "error\n"
> 	Git <-- Filter: "ONE_LINE_OF_ERROR_DESCRIPTION\n"
> 
> on error? Or if printing expected output length upfront is easier,
> use a signal (but that is supposedly not that reliable as message
> passing mechanism)?
> 
> It might be the case that some files return errors, but some do not.

> On 24 Jul 2016, at 22:14, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 2016-07-24 o 20:36, Lars Schneider pisze:
>>> On 23 Jul 2016, at 02:11, Jakub Narębski <jnareb@gmail.com> wrote:
>>>> W dniu 2016-07-22 o 17:49, larsxschneider@gmail.com pisze:
>>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>>
>>>> Nb. this line is only needed if you want author name and/or date
>>>> different from the email sender, or if you have sender line misconfigured
>>>> (e.g. lacking the human readable name).
>>>
>>> I use "git format-patch" to generate these emails:
>>>
>>> git format-patch --cover-letter --subject-prefix="PATCH ..." -M $BASE -o $OUTPUT
>>>
>>> How would I disable this line? (I already checked the man page to no avail).
>>
>> If you are using `git send-email` or equivalent, I think it is
>> stripped automatically if it is not needed (in you case it was,
>> because Sender was lacking human readable name... at least I think
>> it was because of what my email reader inserted as reply line).
>> If you are using an ordinary email client, you need to remove it
>> yourself, if needed.
> 
> Weird. I am sending the patches with this command:
> 
> git send-email mystuff/* --to=git@vger.kernel.org --cc=...
> 
> Maybe I need to try the "--suppress-from" switch?!

>>>> I also agree that we might want to be able to keep clean and smudge
>>>> filters separate, but be able to run a single program if they are
>>>> both the same. I think there is a special case for filter unset,
>>>> and/or filter being "cat" -- we would want to keep that.
>>>
>>> Since 1a8630d there is a more efficient way to unset a filter ;-)
>>> Can you think of other cases where the separation would be useful?
>>
>> I can't think of any, but it doesn't mean that it does not exist.
>> It also does not mean that you need to consider situation that may
>> not happen. Covering one-way filters, like "indent" filter for `clean`,
>> should be enough... they do work with your proposal, don't they?
> 
> This should work right now but it would be a bit inefficient (the filter
> would just pass the data unchanged through the smudge command). I plan to
> add a "capabilities" flag to the protocol. Then you can define only
> the "clean" capability and nothing or the current filter mechanism 
> would happen for smudge (I will make a test case to demonstrate that
> behavior in v2).

>>>> 	Git <-- Filter: "capabilities clean smudge\n"
>>>>
>>>> Or we can add capabilities in later version...
>>>
>>> That is an interesting idea. My initial thought was to make the capabilities
>>> of a certain version fix. If we want to add new capabilities then we would 
>>> bump the version. I wonder what others think about your suggestion!
>>
>> Using capabilities (like git-upload-pack / git-receive-pack, that is
>> smart Git transfer protocols do) is probably slightly more difficult on
>> the Git side (assuming no capabilities negotiation), but also much more
>> flexible than pure version numbers.
>>
>> One possible idea for a capability is support for passing input
>> and output of a filter via filesystem, like cleanToFile and smudgeFromFile
>> proposal in 'jh/clean-smudge-annex' (in 'pu').
>>
>> For example:
>>
>> 	Git <-- Filter: "capabilities clean smudge cleanToFile smudgeFromFile\n"
> 
> Yes, I like that very much. As stated above, I will add that in v2.

>>>>> 	Git --> Filter: "testfile.dat\n"
>>>>
>>>> Unfortunately, while sane filenames should not contain newlines[1],
>>>> the unfortunate fact is that *filenames can include newlines*, and
>>>> you need to be able to handle that[2].  Therefore you need either to
>>>> choose a different separator (the only one that can be safely used
>>>> is "\0", i.e. the NUL character - but it is not something easy to
>>>> handle by shell scripts), or C-quote filenames as needed, or always
>>>> C-quote filenames.  C-quoting at minimum should include quoting newline
>>>> character, and the escape character itself.
>>>>
>>>> BTW. is it the basename of a file, or a full pathname?
>>>>
>>>> [1]: http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html
>>>> [2]: http://www.dwheeler.com/essays/filenames-in-shell.html
>>>
>>> Thanks for this explanation. A bash version of the protocol is not
>>> trivial (I tried it but ended up using Perl). Therefore I think "\0"
>>> would be a good choice?
>>
>> That, or use git convention of surrounding C-quoted filenames in
>> double quotes (which means that if it begins with quote, it is C-quoted).
>>
>> For example:
>>
>>  $ git commit -m 'Initial commit'
>>  [master (root-commit) 266dab0] Initial commit
>>   2 files changed, 2 insertions(+)
>>   create mode 100644 foo
>>   create mode 100644 "foo \" \\ ,"
>>  $ ls -1
>>  foo " \ ,
>>  foo
>>
>> I'm not sure which solution would be easier for filter writers,
>> NUL termination, or C-quoting.
> 
> Unless someone has a convincing argument for one solution or the other
> I will go with the \0 termination as it seems easier.

>>>>> 	Git --> Filter: "7\n"
>>>>
>>>> That's the content size in bytes written as an ASCII number.
>>>
>>> Correct.
>>
>> But not obvious from the description / documentation.
> 
> I will improve that in v2. Should I add the info that it is base10 or
> would you consider that a given?

>>>>> ---
>>>>> Documentation/gitattributes.txt |  41 +++++++-
>>>>> convert.c                       | 210 ++++++++++++++++++++++++++++++++++++++--
>>>>> t/t0021-conversion.sh           | 170 ++++++++++++++++++++++++++++++++
>>>>
>>>> Wouldn't it be better to name the test case something more
>>>> descriptive, for example
>>>>
>>>>  t/t0021-filter-driver-useProtocol.sh
>>>>
>>>> The name of test should be adjusted to final name of the feature,
>>>> of course.
>>>
>>> I think the prefix numbers should be unique, no? And t0022 is already taken.
>>
>> I meant here that the "conversion" part of "t/t0021-conversion.sh" test
>> filename is not descriptive enough.
> 
> Ah, I see. You suggest to rename the test case? Would that be OK with the Git
> community?

>>>             What do you mean by "discrepancy in how config variables are 
>>> referenced"?
>>
>> What I meant here that filter.<driver>.smudge and filter.<driver>.clean
>> were referenced as "`smudge` command" and "`clean` command" in the paragraph
>> you modified.
>>
>> Perhaps filter.<driver>.useProtocol is all right (I have not looked further),
>> but it should be formatted as `filter.<driver>.useProtocol` IMVHO.
> 
> Initially I thought so, too. But "filter.<driver>.required", which is already
> mentioned in gitattributes.txt, does not use this style. Should I change that, too,
> or use the existing style?

>> The problem is do the protocol need to have some way of communicating
>> errors from the filter to Git?  Perhaps using stderr would be enough
>> (but then Git would need to drain it, I think... unless it is not
>> redirected), perhaps some command is needed?
>>
>> For example, instead of:
>>
>> 	Git <-- Filter: "15\n"
>> 	Git <-- Filter: "SMUDGED_CONTENT"
>>
>> perhaps filter should return
>>
>> 	Git <-- Filter: "error\n"
>> 	Git <-- Filter: "ONE_LINE_OF_ERROR_DESCRIPTION\n"
>>
>> on error? Or if printing expected output length upfront is easier,
>> use a signal (but that is supposedly not that reliable as message
>> passing mechanism)?
>>
>> It might be the case that some files return errors, but some do not.
> 
> I would prefer it if the filter just dies in case of trouble and that
> way communicates to Git that something went wrong. Everything else
> just complicates the protocol.

> +static void stop_protocol_filter(struct cmd2process *entry) {
> +	if (!entry)
> +		return;
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	close(entry->process.in);
> +	close(entry->process.out);
> +	sigchain_pop(SIGPIPE);
> +	finish_command(&entry->process);
> +	child_process_clear(&entry->process);
> +	hashmap_remove(&cmd_process_map, entry, NULL);
> +	free(entry);
> +}
> +
> +static struct cmd2process *start_protocol_filter(const char *cmd)
> +{
> +	int ret = 1;
> +	struct cmd2process *entry = NULL;
> +	struct child_process *process = NULL;

> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
> +						int fd, struct strbuf *dst, const char *cmd,
> +						const char *filter_type)
> +{
> +	int ret = 1;
> +	struct cmd2process *entry = NULL;
> +	struct child_process *process = NULL;

> +	struct stat fileStat;
> +	struct strbuf nbuf = STRBUF_INIT;
> +	size_t nbuf_len;
> +	char *strtol_end;
> +	char c;
> +
> +	if (!cmd || !*cmd)
> +		return 0;
> +
> +	if (!dst)
> +		return 1;
> +
> +	if (!cmd_process_map_init) {
> +		cmd_process_map_init = 1;
> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
> +	} else {
> +		entry = find_protocol_filter_entry(cmd);
> +	}
> +
> +	if (!entry){
> +		entry = start_protocol_filter(cmd);
> +		if (!entry) {
> +			stop_protocol_filter(entry);

> +			return 0;
> +		}
> +	}
> +	process = &entry->process;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	switch (entry->protocol) {
> +		case 1:
> +			if (fd >= 0 && !src) {
> +				ret &= fstat(fd, &fileStat) != -1;
> +				len = fileStat.st_size;

> +    my $filelen  = <STDIN>;
> +    chomp $filelen;
> +    print $debug " $filelen";
> +
> +    $filelen = int($filelen);

> larsxschneider@gmail.com wrote:
>> Please note that the protocol filters do not support stream processing
>> with this implemenatation because the filter needs to know the length of
>> the result in advance. A protocol version 2 could address this in a
>> future patch.
> 
> Would it be prudent to reuse pkt-line for this?

>> +static void stop_protocol_filter(struct cmd2process *entry) {
>> +	if (!entry)
>> +		return;
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	close(entry->process.in);
>> +	close(entry->process.out);
>> +	sigchain_pop(SIGPIPE);
>> +	finish_command(&entry->process);
>> +	child_process_clear(&entry->process);
>> +	hashmap_remove(&cmd_process_map, entry, NULL);
>> +	free(entry);
>> +}
>> +
>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry = NULL;
>> +	struct child_process *process = NULL;
> 
> These are unconditionally set below, so initializing to NULL
> may hide future bugs.

>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	struct string_list split = STRING_LIST_INIT_NODUP;
>> +	const char *argv[] = { NULL, NULL };
>> +	const char *header = "git-filter-protocol\nversion";
> 
> 	static const char header[] = "git-filter-protocol\nversion";
> 
> ...might be smaller by avoiding the extra pointer
> (but compilers ought to be able to optimize it)

>> +	entry = xmalloc(sizeof(*entry));
>> +	hashmap_entry_init(entry, strhash(cmd));
>> +	entry->cmd = cmd;
>> +	process = &entry->process;
> 
> <snip>
> 
>> +	ret &= strncmp(header, split.items[0].string, strlen(header)) == 0;
> 
> starts_with() is probably more readable, here.

>> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
>> +						int fd, struct strbuf *dst, const char *cmd,
>> +						const char *filter_type)
>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry = NULL;
>> +	struct child_process *process = NULL;
> 
> I would leave process initialized, here, since it should
> always be set below:

>> +	struct stat fileStat;
>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	size_t nbuf_len;
>> +	char *strtol_end;
>> +	char c;
>> +
>> +	if (!cmd || !*cmd)
>> +		return 0;
>> +
>> +	if (!dst)
>> +		return 1;
>> +
>> +	if (!cmd_process_map_init) {
>> +		cmd_process_map_init = 1;
>> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
>> +	} else {
>> +		entry = find_protocol_filter_entry(cmd);
>> +	}
>> +
>> +	if (!entry){
>> +		entry = start_protocol_filter(cmd);
>> +		if (!entry) {
>> +			stop_protocol_filter(entry);
> 
> stop_protocol_filter is a no-op, here, since entry is NULL

>> +			return 0;
>> +		}
>> +	}
>> +	process = &entry->process;
>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	switch (entry->protocol) {
>> +		case 1:
>> +			if (fd >= 0 && !src) {
>> +				ret &= fstat(fd, &fileStat) != -1;
>> +				len = fileStat.st_size;
> 
> There's a truncation bug when sizeof(size_t) < sizeof(off_t)

>> +    my $filelen  = <STDIN>;
>> +    chomp $filelen;
>> +    print $debug " $filelen";
>> +
>> +    $filelen = int($filelen);
> 
> Calling int() here is unnecessary and may hide bugs if you
> forget to check $debug.   Perhaps a regexp check is safer:
> 
> 	$filelen =~ /\A\d+\z/ or die "bad filelen: $filelen\n";

> On 23 Jul 2016, at 10:14, Eric Wong <e@80x24.org> wrote:
> > larsxschneider@gmail.com wrote:
> >> +static struct cmd2process *start_protocol_filter(const char *cmd)
> >> +{
> >> +	int ret = 1;
> >> +	struct cmd2process *entry = NULL;
> >> +	struct child_process *process = NULL;
> > 
> > These are unconditionally set below, so initializing to NULL
> > may hide future bugs.
> 
> OK. I thought it is generally a good thing to initialize a pointer with 
> NULL. Can you explain to me how this might hide future bugs?
> I will remove the initialization.

> >> +			if (fd >= 0 && !src) {
> >> +				ret &= fstat(fd, &fileStat) != -1;
> >> +				len = fileStat.st_size;
> > 
> > There's a truncation bug when sizeof(size_t) < sizeof(off_t)
> 
> OK. What would you suggest to do in that case? Should we just let the
> filter fail? Is there anything else we could do?

>
> On 23 Jul 2016, at 10:14, Eric Wong <e@80x24.org> wrote:
>
>> larsxschneider@gmail.com wrote:
>>> Please note that the protocol filters do not support stream processing
>>> with this implemenatation because the filter needs to know the length of
>>> the result in advance. A protocol version 2 could address this in a
>>> future patch.
>>
>> Would it be prudent to reuse pkt-line for this?
>
> Peff suggested that, too, in $gmane/299902.

> larsxschneider@gmail.com writes:
> 
>> The first two patches are cleanup patches which are not really necessary
>> for the feature.
> 
> These two looked trivially good.

> I think I can agree with what 3/3 wants to do in principle, but
> 
> * "protocol" is not quite the right word.  The current way to
>   interact with clean and smudge filters can be considered using a
>   different "protocol", that conveys the data and the options via
>   the command line and pipe.  The most distinguishing feature that
>   differentiates the old way and the new style this change allows
>   is that it allows you to have a single instance of the process
>   running that can be reused?

> * I am not sure what's the pros-and-cons in forcing people writing
>   a single program that can do both cleaning and smudging.  You
>   cannot have only "smudge" side that uses the long-running process
>   while "clean" side that runs single-shot invocation with this
>   design, which I'd imagine would be a downside.  If you are going
>   to use a long-running process interface for both sides, this
>   design allows you to do it with fewer number of processes, which
>   may be an upside.

> * The way the serialized access to these long-running processes
>   work in 3/3 would make it harder or impossible to later
>   parallelize conversion?  I am imagining a far future where we
>   would run "git checkout ." using (say) two threads, one
>   responsible for active_cache[0..active_nr/2] and the other
>   responsible for the remainder.

> > * The way the serialized access to these long-running processes
> >   work in 3/3 would make it harder or impossible to later
> >   parallelize conversion?  I am imagining a far future where we
> >   would run "git checkout ." using (say) two threads, one
> >   responsible for active_cache[0..active_nr/2] and the other
> >   responsible for the remainder.
> I hope this future is not too far away :-) 
> However, I don't think that would be a problem as we could start the
> long-running process once for each checkout thread, no?

> From: Lars Schneider <larsxschneider@gmail.com>
>
> `set_packet_header` converts an integer to a 4 byte hex string. Make
> this function publicly available so that other parts of Git can easily
> generate a pkt-line.

> On 27 Jul 2016, at 02:20, Junio C Hamano <gitster@pobox.com> wrote:
> 
> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> `set_packet_header` converts an integer to a 4 byte hex string. Make
>> this function publicly available so that other parts of Git can easily
>> generate a pkt-line.
> 
> I think that having to do this is a strong sign that the design of
> this series is going in a wrong direction.

> If you need a helper function that writes a pkt-line format that
> behaves differently from what is already available (for example,
> packet_write()), it would be much better to design that new function
> so that it would be generally useful and add that to pkt-line.[ch],
> instead of creating random helper functions that use write(2)
> directly, bypassing pkt-line API, to write stuff.
> 
> In other words, do not _mimick_ pkt-line; enhance pkt-line as
> necessary and use it.

>> On 27 Jul 2016, at 02:20, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> larsxschneider@gmail.com writes:
>> 
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>> 
>>> `set_packet_header` converts an integer to a 4 byte hex string. Make
>>> this function publicly available so that other parts of Git can easily
>>> generate a pkt-line.
>> 
>> I think that having to do this is a strong sign that the design of
>> this series is going in a wrong direction.
>
> Thanks for the feedback. Do you think using "pkt-line" is a move into
> the wrong direction in general or do you think only my usage of 
> "pkt-line" is not ideal?

> From: Lars Schneider <larsxschneider@gmail.com>
>
> Generate a more interesting large test file with random characters in
> between and reuse this test file in multiple tests. Run tests formerly
> marked as EXPENSIVE every time but with a smaller test file.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  t/t0021-conversion.sh | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 7b45136..b9911a4 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -4,6 +4,13 @@ test_description='blob conversion via gitattributes'
>
>  . ./test-lib.sh
>
> +if test_have_prereq EXPENSIVE
> +then
> +	T0021_LARGE_FILE_SIZE=2048
> +else
> +	T0021_LARGE_FILE_SIZE=30
> +fi
> +
>  cat <<EOF >rot13.sh
>  #!$SHELL_PATH
>  tr \
> @@ -31,7 +38,15 @@ test_expect_success setup '
>  	cat test >test.i &&
>  	git add test test.t test.i &&
>  	rm -f test test.t test.i &&
> -	git checkout -- test test.t test.i
> +	git checkout -- test test.t test.i &&
> +
> +	mkdir -p generated-test-data &&
> +	for i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)
> +	do
> +		# Generate 1MB of empty data and 100 bytes of random characters
> +		printf "%1048576d" 1
> +		printf "$(LC_ALL=C tr -dc "A-Za-z0-9" </dev/urandom | dd bs=$((RANDOM>>8)) count=1 2>/dev/null)"

> +	done >generated-test-data/large.file
>  '
>
>  script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
> @@ -199,9 +214,9 @@ test_expect_success 'required filter clean failure' '
>  test_expect_success 'filtering large input to small output should use little memory' '
>  	test_config filter.devnull.clean "cat >/dev/null" &&
>  	test_config filter.devnull.required true &&
> -	for i in $(test_seq 1 30); do printf "%1048576d" 1; done >30MB &&
> -	echo "30MB filter=devnull" >.gitattributes &&
> -	GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB
> +	cp generated-test-data/large.file large.file &&
> +	echo "large.file filter=devnull" >.gitattributes &&
> +	GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add large.file
>  '
>
>  test_expect_success 'filter that does not read is fine' '
> @@ -214,15 +229,15 @@ test_expect_success 'filter that does not read is fine' '
>  	test_cmp expect actual
>  '
>
> -test_expect_success EXPENSIVE 'filter large file' '
> +test_expect_success 'filter large file' '
>  	test_config filter.largefile.smudge cat &&
>  	test_config filter.largefile.clean cat &&
> -	for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
> -	echo "2GB filter=largefile" >.gitattributes &&
> -	git add 2GB 2>err &&
> +	echo "large.file filter=largefile" >.gitattributes &&
> +	cp generated-test-data/large.file large.file &&
> +	git add large.file 2>err &&
>  	test_must_be_empty err &&
> -	rm -f 2GB &&
> -	git checkout -- 2GB 2>err &&
> +	rm -f large.file &&
> +	git checkout -- large.file 2>err &&
>  	test_must_be_empty err
>  '
>
>

> > +	mkdir -p generated-test-data &&
> > +	for i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)
> > +	do
> > +		# Generate 1MB of empty data and 100 bytes of random characters
> > +		printf "%1048576d" 1
> > +		printf "$(LC_ALL=C tr -dc "A-Za-z0-9" </dev/urandom | dd bs=$((RANDOM>>8)) count=1 2>/dev/null)"
> I'm not sure how portable /dev/urandom is.
> The other thing, that "really random" numbers are an overkill, and
> it may be easier to use pre-defined numbers,

> On 27 Jul 2016, at 15:32, Jeff King <peff@peff.net> wrote:
> 
> On Wed, Jul 27, 2016 at 04:35:32AM +0200, Torsten Bögershausen wrote:
> 
>>> +	mkdir -p generated-test-data &&
>>> +	for i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)
>>> +	do
>>> +		# Generate 1MB of empty data and 100 bytes of random characters
>>> +		printf "%1048576d" 1
>>> +		printf "$(LC_ALL=C tr -dc "A-Za-z0-9" </dev/urandom | dd bs=$((RANDOM>>8)) count=1 2>/dev/null)"
>> I'm not sure how portable /dev/urandom is.
>> The other thing, that "really random" numbers are an overkill, and
>> it may be easier to use pre-defined numbers,
> 
> Right, there are a few reasons not to use /dev/urandom:
> 
>  - it's not portable
> 
>  - if we have to generate a lot of numbers, it drains the system's
>    entropy pool, which is an unfriendly thing to do (and may also be
>    slow)
> 
>  - it makes our tests random! This sounds like a good thing, but it
>    means that if some input happens to cause failure, you are unlikely
>    to be able to reproduce it.
> 
> Instead, use test-genrandom, which is an LCG that starts at a seed. So
> you get a large amount of random-ish quickly and portably, and you get
> the same data each time.

> +	do {
> +		bytes_read = packet_read(
> +			fd, NULL, NULL,
> +			sb->buf + total_bytes_read, sb->len - total_bytes_read - 1,
> +			PACKET_READ_GENTLE_ON_EOF
> +		);

> +static int multi_packet_write(const char *src, size_t len, const int in, const int out)
> +{
> +	int ret = 1;
> +	char header[4];
> +	char buffer[8192];
> +	off_t bytes_to_write;
> +	while (ret) {
> +		if (in >= 0) {
> +			bytes_to_write = xread(in, buffer, sizeof(buffer));

> On 27 Jul 2016, at 03:32, Jeff King <peff@peff.net> wrote:
> 
> On Wed, Jul 27, 2016 at 02:06:05AM +0200, larsxschneider@gmail.com wrote:
> 
>> +static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t size)
>> +{
>> +	off_t bytes_read;
>> +	off_t total_bytes_read = 0;
> 
> I haven't looked carefully at the whole patch yet, but there seems to be
> some type issues here. off_t is a good type for storing the whole size
> of a file (which may be larger than the amount of memory we can
> allocate). But size_t is the right size for an in-memory object.
> 
> This function takes a size_t size, which makes sense if it is meant to
> read everything into a strbuf.
> 
> So I think our total_bytes_read would probably want to be a size_t here,
> too, because it cannot possibly grow larger than that (and that is
> enforced by the loop below). Otherwise you get weirdness like "sb->buf +
> total_bytes_ref" possibly overflowing memory.

>> +	do {
>> +		bytes_read = packet_read(
>> +			fd, NULL, NULL,
>> +			sb->buf + total_bytes_read, sb->len - total_bytes_read - 1,
>> +			PACKET_READ_GENTLE_ON_EOF
>> +		);
> 
> packet_read() actually returns an int, and may return "-1" on EOF (and
> int is fine because we know that we are constrained to 16-bit values
> by the pkt-line definition). You read it into an "off_t". I _think_ that
> is OK, because I believe POSIX says off_t must be signed. But probably
> "int" is the more correct type here.

>> +	while (
>> +		bytes_read > 0 && 					// the last packet was no flush
>> +		sb->len - total_bytes_read - 1 > 0 	// we still have space left in the buffer
>> +	);
> 
> And I'm not sure if you need to distinguish between "0" and "-1" when
> checking byte_read here.

> 
>> +	strbuf_setlen(sb, total_bytes_read);
> 
> Passing an off_t to something expecting a size_t, which can involve
> truncation (though I think in practice you really are limited to
> size_t).

>> +static int multi_packet_write(const char *src, size_t len, const int in, const int out)
>> +{
>> +	int ret = 1;
>> +	char header[4];
>> +	char buffer[8192];
>> +	off_t bytes_to_write;
>> +	while (ret) {
>> +		if (in >= 0) {
>> +			bytes_to_write = xread(in, buffer, sizeof(buffer));
> 
> Likewise here, xread() is returning ssize_t. Again, OK if we can assume
> off_t is signed, but it probably makes sense to use the correct type (we
> also know it cannot be larger than 8K, of course).

>> +			if (bytes_to_write < 0)
>> +				ret &= 0;
> 
> I think "&= 0" is unusual for our codebase? Would just writing "= 0" be
> more clear?

>> +		set_packet_header(header, bytes_to_write + 4);
>> +		ret &= write_in_full(out, &header, sizeof(header)) == sizeof(header);
>> +		ret &= write_in_full(out, src, bytes_to_write) == bytes_to_write;
>> +	}
> 
> If you look at format_packet(), it pulls a slight trick: we have a
> buffer 4 bytes larger than we need, format into "buf + 4", and then
> write the final size at the beginning. That lets us write() it all in
> one go.
> 
> At first I thought this function was simply reinventing packet_write(),
> but I guess you are trying to avoid the extra copy of the data (once
> into the buffer from xread, and then again via format_packet just to add
> the extra bytes at the beginning).

> I agree with what Junio said elsewhere, that there may be a way to make
> the pkt-line code handle this zero-copy situation better. Perhaps
> something like:
> 
>  struct pktline {
> 	/* first 4 bytes are reserved for length header */
> 	char buf[LARGE_PACKET_MAX];
>  };
>  #define PKTLINE_DATA_START(pkt) ((pkt)->buf + 4)
>  #define PKTLINE_DATA_LEN (LARGE_PACKET_MAX - 4)
> 
>  ...
>  struct pktline pkt;
>  ssize_t len = xread(fd, PKTLINE_DATA_START(&pkt), PKTLINE_DATA_LEN);
>  packet_send(&pkt, len);
> 
> Then packet_send() knows that the first 4 bytes are reserved for it. I
> suspect that the strbuf used by format_packet() could get away with
> using such a "struct pktline" too, though in practice I doubt there's
> any real efficiency to be gained (we generally reuse the same strbuf
> over and over, so it will grow once to 64K and get reused).

>> +	ret &= write_in_full(out, "0000", 4) == 4;
> 
> packet_flush() ?
> 
> I know the packet functions are keen on write_or_die() versus
> write_in_full().  That is perhaps something that should be fixed.

> >> +	strbuf_grow(sb, size + 1);	// we need one extra byte for the packet flush
> > 
> > What happens if size is the maximum for size_t here (i.e., 4GB-1 on a
> > 32-bit system)?
> 
> Would that be an acceptable solution?
> 
> if (size + 1 > SIZE_MAX)
> 	return die("unrepresentable length for filter buffer");

> >> +	while (
> >> +		bytes_read > 0 && 					// the last packet was no flush
> >> +		sb->len - total_bytes_read - 1 > 0 	// we still have space left in the buffer
> >> +	);
> > 
> > And I'm not sure if you need to distinguish between "0" and "-1" when
> > checking byte_read here.
> 
> We want to finish reading in both cases, no?

> > Why 8K? The pkt-line format naturally restricts us to just under 64K, so
> > why not take advantage of that and minimize the framing overhead for
> > large data?
> 
> I took inspiration from here for 8K MAX_IO_SIZE:
> https://github.com/git/git/blob/master/copy.c#L6
> 
> Is this read limit correct? Should I read 8 times to fill a pkt-line?

> > We do sometimes do "ret |= something()" but that is in cases where
> > "ret" is zero for success, and non-zero (usually -1) otherwise. Perhaps
> > your function's error-reporting is inverted from our usual style?
> 
> I thought it makes the code easier to read and the filter doesn't care
> at what point the error happens anyways. The filter either succeeds
> or fails. What style would you suggest?

> On 27 Jul 2016, at 20:11, Jeff King <peff@peff.net> wrote:
> 
> On Wed, Jul 27, 2016 at 07:31:26PM +0200, Lars Schneider wrote:
> 
>>>> +	strbuf_grow(sb, size + 1);	// we need one extra byte for the packet flush
>>> 
>>> What happens if size is the maximum for size_t here (i.e., 4GB-1 on a
>>> 32-bit system)?
>> 
>> Would that be an acceptable solution?
>> 
>> if (size + 1 > SIZE_MAX)
>> 	return die("unrepresentable length for filter buffer");
> 
> No, because by definition "size" will wrap to 0. :)
> 
> You have to do:
> 
>  if (size > SIZE_MAX - 1)
> 	die("whoops");
> 
>> Can you point me to an example in the Git source how this kind of thing should
>> be handled?
> 
> The strbuf code itself checks for overflows. So you could do:
> 
>  strbuf_grow(sb, size);
>  ... fill up with size bytes ...
>  strbuf_addch(sb, ...); /* extra byte for whatever */
> 
> That does mean _possibly_ making a second allocation just to add the
> extra byte, but in practice it's not likely (unless the input exactly
> matches the strbuf's growth pattern).
> 
> If you want to do it yourself, I think:
> 
>  strbuf_grow(sb, st_add(size, 1));

> would work.
> 
>>>> +	while (
>>>> +		bytes_read > 0 && 					// the last packet was no flush
>>>> +		sb->len - total_bytes_read - 1 > 0 	// we still have space left in the buffer
>>>> +	);
>>> 
>>> And I'm not sure if you need to distinguish between "0" and "-1" when
>>> checking byte_read here.
>> 
>> We want to finish reading in both cases, no?
> 
> If we get "-1", that's from an unexpected EOF during the packet_read(),
> because you set GENTLE_ON_EOF. So there's nothing left to read, and we
> should break and return an error.

>>> Why 8K? The pkt-line format naturally restricts us to just under 64K, so
>>> why not take advantage of that and minimize the framing overhead for
>>> large data?
>> 
>> I took inspiration from here for 8K MAX_IO_SIZE:
>> https://github.com/git/git/blob/master/copy.c#L6
>> 
>> Is this read limit correct? Should I read 8 times to fill a pkt-line?
> 
> MAX_IO_SIZE is generally 8 _megabytes_, not 8K. The loop in copy.c just
> haad to pick an arbitrary size for doing its read/write proxying.  I
> think in practice you are not likely to get much benefit from going
> beyond 8K or so there, just because operating systems tend to do things
> in page-sizes anyway, which are usually 4K.
> 
> But since you are formatting the result into a form that has framing
> overhead, anything up to LARGE_PACKET_MAX will see benefits (though
> admittedly even 4 bytes per 8K is not much).
> 
> I don't think it's worth the complexity of reading 8 times, but just
> using a buffer size of LARGE_PACKET_MAX-4 would be the most efficient.
> 
> I doubt it matters _that much_ in practice, but any time I see a magic
> number I have to wonder at the "why". At least basing it on
> LARGE_PACKET_MAX has some basis, whereas 8K is largely just made-up. :)

> 
>>> We do sometimes do "ret |= something()" but that is in cases where
>>> "ret" is zero for success, and non-zero (usually -1) otherwise. Perhaps
>>> your function's error-reporting is inverted from our usual style?
>> 
>> I thought it makes the code easier to read and the filter doesn't care
>> at what point the error happens anyways. The filter either succeeds
>> or fails. What style would you suggest?
> 
> I think that's orthogonal. I just mean that using zero for success puts
> you in our usual style, and then accumulating errors can be done with
> "|=".

> I didn't look carefully at whether the accumulating style you're using
> makes sense or not. But something like:
> 
>>>> +		ret &= write_in_full(out, &header, sizeof(header)) == sizeof(header);
>>>> +		ret &= write_in_full(out, src, bytes_to_write) == bytes_to_write;
> 
> does mean that we call the second write() even if the first one failed.
> That's a waste of time (albeit a minor one), but it also means you could
> potentially cover up the value of "errno" from the first one (though in
> practice I'd expect the second one to fail for the same reason).

> > I think that's orthogonal. I just mean that using zero for success puts
> > you in our usual style, and then accumulating errors can be done with
> > "|=".
> 
> Ah. I guess I was misguided by the way errors are currently handled
> in `apply_filter` (success = 1; failure = 0):
> https://github.com/git/git/blob/8c6d1f9807c67532e7fb545a944b064faff0f70b/convert.c#L437-L479
> 
> I wouldn't like if the different filter protocols would use different
> error exit codes. Would it be OK to adjust the existing `apply_filter`
> function in a cleanup patch?

> +{
> +	int ret = 1;
> +	char header[4];
> +	char buffer[8192];
> +	off_t bytes_to_write;

> +	while (ret) {
> +		if (in >= 0) {
> +			bytes_to_write = xread(in, buffer, sizeof(buffer));
> +			if (bytes_to_write < 0)
> +				ret &= 0;
> +			src = buffer;
> +		} else {
> +			bytes_to_write = len > LARGE_PACKET_MAX - 4 ? LARGE_PACKET_MAX - 4 : len;
> +			len -= bytes_to_write;
> +		}
> +		if (!bytes_to_write)
> +			break;

> +		set_packet_header(header, bytes_to_write + 4);
> +		ret &= write_in_full(out, &header, sizeof(header)) == sizeof(header);
> +		ret &= write_in_full(out, src, bytes_to_write) == bytes_to_write;
> +	}
> +	ret &= write_in_full(out, "0000", 4) == 4;
> +	return ret;
> +}
> +

> +	}
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	packet_write(process->in, "%s\n", filter_type);
> +	packet_write(process->in, "%s\n", path);
> +	packet_write(process->in, "%zu\n", len);

> On 27 Jul 2016, at 11:41, Eric Wong <e@80x24.org> wrote:
> 
> larsxschneider@gmail.com wrote:
>> +static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t size)
> 
> I'm no expert in C, but this might be const-correctness taken
> too far.  I think basing this on the read(2) prototype is less
> surprising:
> 
>   static ssize_t multi_packet_read(int fd, struct strbuf *sb, size_t size)

> Also what Jeff said about off_t vs size_t, but my previous
> emails may have confused you w.r.t. off_t usage...
> 
>> +static int multi_packet_write(const char *src, size_t len, const int in, const int out)
> 
> Same comment about over const ints above.
> len can probably be off_t based on what is below; but you need
> to process the loop in ssize_t-friendly chunks.

> 
>> +{
>> +	int ret = 1;
>> +	char header[4];
>> +	char buffer[8192];
>> +	off_t bytes_to_write;
> 
> What Jeff said, this should be ssize_t to match read(2) and xread

> 
>> +	while (ret) {
>> +		if (in >= 0) {
>> +			bytes_to_write = xread(in, buffer, sizeof(buffer));
>> +			if (bytes_to_write < 0)
>> +				ret &= 0;
>> +			src = buffer;
>> +		} else {
>> +			bytes_to_write = len > LARGE_PACKET_MAX - 4 ? LARGE_PACKET_MAX - 4 : len;
>> +			len -= bytes_to_write;
>> +		}
>> +		if (!bytes_to_write)
>> +			break;
> 
> The whole ret &= .. style error handling is hard-to-follow and
> here, a source of bugs.  I think the expected convention on
> hitting errors is:
> 
> 	1) stop whatever you're doing
> 	2) cleanup
> 	3) propagate the error to callers
> 
> "goto" is an acceptable way of accomplishing this.
> 
> For example, byte_to_write may still be negative at this point
> (and interpreted as a really big number when cast to unsigned
> size_t) and src/buffer could be stack garbage.

>> +		set_packet_header(header, bytes_to_write + 4);
>> +		ret &= write_in_full(out, &header, sizeof(header)) == sizeof(header);
>> +		ret &= write_in_full(out, src, bytes_to_write) == bytes_to_write;
>> +	}
>> +	ret &= write_in_full(out, "0000", 4) == 4;
>> +	return ret;
>> +}
>> +
> 
>> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
>> +						int fd, struct strbuf *dst, const char *cmd,
>> +						const char *filter_type)
>> +{
> 
> <snip>
> 
>> +	if (fd >= 0 && !src) {
>> +		ret &= fstat(fd, &file_stat) != -1;
>> +		len = file_stat.st_size;
> 
> Same truncation bug I noticed earlier; what I originally meant
> is the `len' arg probably ought to be off_t, here, not size_t.
> 32-bit x86 Linux systems have 32-bit size_t (unsigned), but
> large file support means off_t is 64-bits (signed).

>> +	}
>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +
>> +	packet_write(process->in, "%s\n", filter_type);
>> +	packet_write(process->in, "%s\n", path);
>> +	packet_write(process->in, "%zu\n", len);
> 
> I'm not sure if "%zu" is portable since we don't do C99 (yet?)
> For 64-bit signed off_t, you can probably do:
> 
> 	packet_write(process->in, "%"PRIuMAX"\n", (uintmax_t)len);
> 
> Since we don't have PRIiMAX or intmax_t, here, and a negative
> len would be a bug (probably from failed fstat) anyways.

>>> +static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t size)
>> 
>> I'm no expert in C, but this might be const-correctness taken
>> too far.  I think basing this on the read(2) prototype is less
>> surprising:
>> 
>>   static ssize_t multi_packet_read(int fd, struct strbuf *sb, size_t size)
>
> Hm... ok. I like `const` because I think it is usually easier to read/understand
> functions that do not change their input variables. This way I can communicate
> my intention to future people modifying this function!

> I agree with your reordering of the parameters, though!
> 
> Speaking of coding style... convert.c is already big and gets only bigger 
> with this patch (1720 lines). Would it make sense to add a new file 
> "convert-pipe-protocol.c"
> or something for my additions?

> On 29 Jul 2016, at 13:24, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 2016-07-29 o 12:38, Lars Schneider pisze:
>> On 27 Jul 2016, at 11:41, Eric Wong <e@80x24.org> wrote:
>>> larsxschneider@gmail.com wrote:
> 
>>>> +static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t size)
>>> 
>>> I'm no expert in C, but this might be const-correctness taken
>>> too far.  I think basing this on the read(2) prototype is less
>>> surprising:
>>> 
>>>  static ssize_t multi_packet_read(int fd, struct strbuf *sb, size_t size)
>> 
>> Hm... ok. I like `const` because I think it is usually easier to read/understand
>> functions that do not change their input variables. This way I can communicate
>> my intention to future people modifying this function!
> 
> Well, scalar types like `size_t` are always passed by value, so here `const`
> doesn't matter, and it makes line longer.  I think library functions do not
> use `const` for `size_t` parameters.
> 
> You are reading from the file descriptor `fd`, so it state would change.
> Using `const` feels a bit like lying.  Also, it is scalar type.

> 
> [...] 
>> I agree with your reordering of the parameters, though!
>> 
>> Speaking of coding style... convert.c is already big and gets only bigger 
>> with this patch (1720 lines). Would it make sense to add a new file 
>> "convert-pipe-protocol.c"
>> or something for my additions?
> 
> I wonder if it would be possible to enhance existing functions, instead
> of redoing them (at least in part) for per-command filter driver protocol.

> > On 27 Jul 2016, at 11:41, Eric Wong <e@80x24.org> wrote:
> > larsxschneider@gmail.com wrote:
> >> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
> >> +						int fd, struct strbuf *dst, const char *cmd,
> >> +						const char *filter_type)
> >> +{
> > 
> > <snip>
> > 
> >> +	if (fd >= 0 && !src) {
> >> +		ret &= fstat(fd, &file_stat) != -1;
> >> +		len = file_stat.st_size;
> > 
> > Same truncation bug I noticed earlier; what I originally meant
> > is the `len' arg probably ought to be off_t, here, not size_t.
> > 32-bit x86 Linux systems have 32-bit size_t (unsigned), but
> > large file support means off_t is 64-bits (signed).
> 
> OK. Would it be OK to keep size_t for this patch series?

> On 05 Aug 2016, at 20:55, Eric Wong <e@80x24.org> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> wrote:
>>> On 27 Jul 2016, at 11:41, Eric Wong <e@80x24.org> wrote:
>>> larsxschneider@gmail.com wrote:
>>>> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
>>>> +						int fd, struct strbuf *dst, const char *cmd,
>>>> +						const char *filter_type)
>>>> +{
>>> 
>>> <snip>
>>> 
>>>> +	if (fd >= 0 && !src) {
>>>> +		ret &= fstat(fd, &file_stat) != -1;
>>>> +		len = file_stat.st_size;
>>> 
>>> Same truncation bug I noticed earlier; what I originally meant
>>> is the `len' arg probably ought to be off_t, here, not size_t.
>>> 32-bit x86 Linux systems have 32-bit size_t (unsigned), but
>>> large file support means off_t is 64-bits (signed).
>> 
>> OK. Would it be OK to keep size_t for this patch series?
> 
> I think there should at least be a truncation warning (or die)
> for larger-than-4GB files on 32-bit.  I don't know how common
> they are for git-lfs users.
> 
> Perhaps using xsize_t in git-compat-util.h works for now:
> 
> 	len = xsize_t(file_stat.st_size);

> > On 05 Aug 2016, at 20:55, Eric Wong <e@80x24.org> wrote:
> > Perhaps using xsize_t in git-compat-util.h works for now:
> > 
> > 	len = xsize_t(file_stat.st_size);
> 
> Thanks for the hint! Should I add the same check to sha1_file's use
> of fstat in line 1002 or is it not needed there?
> 
> https://github.com/git/git/blob/c6b0597e9ac7277e148e2fd4d7615ac6e0bfb661/sha1_file.c#L1002

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Git's clean/smudge mechanism invokes an external filter process for every
> single blob that is affected by a filter. If Git filters a lot of blobs
> then the startup time of the external filter processes can become a
> significant part of the overall Git execution time.

> keeps the external filter process running and processes all blobs with
> the following packet format (pkt-line) based protocol over standard input
> and standard output.
> 
> Git starts the filter on first usage and expects a welcome
> message, protocol version number, and filter capabilities
> seperated by spaces:

> ------------------------
> Supported filter capabilities are "clean" and "smudge".
> 
> Afterwards Git sends a command (e.g. "smudge" or "clean" - based
> on the supported capabilities), the filename, the content size as
> ASCII number in bytes, and the content in packet format with a
> flush packet at the end:
> ------------------------
> packet:          git> smudge
> packet:          git> testfile.dat

> 
> The filter is expected to respond with the result content size as
> ASCII number in bytes and the result content in packet format with
> a flush packet at the end:
> ------------------------
> packet:          git< 57

> packet:          git< SMUDGED_CONTENT
> packet:          git< 0000
> ------------------------
> Please note: In a future version of Git the capability "stream"
> might be supported. In that case the content size must not be
> part of the filter response.
> 
> Afterwards the filter is expected to wait for the next command.

> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> Helped-by: Martin-Louis Bright <mlbright@gmail.com>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  Documentation/gitattributes.txt |  54 +++++++-
>  convert.c                       | 269 ++++++++++++++++++++++++++++++++++++++--
>  t/t0021-conversion.sh           | 175 ++++++++++++++++++++++++++
>  t/t0021/rot13-filter.pl         | 146 ++++++++++++++++++++++
>  4 files changed, 631 insertions(+), 13 deletions(-)
>  create mode 100755 t/t0021/rot13-filter.pl
> 
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 8882a3e..8fb40d2 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -300,7 +300,11 @@ checkout, when the `smudge` command is specified, the command is
>  fed the blob object from its standard input, and its standard
>  output is used to update the worktree file.  Similarly, the
>  `clean` command is used to convert the contents of worktree file
> -upon checkin.
> +upon checkin. By default these commands process only a single
> +blob and terminate. If a long running filter process (see section
> +below) is used then Git can process all blobs with a single filter
> +invocation for the entire life of a single Git command (e.g.
> +`git add .`).

>  
>  One use of the content filtering is to massage the content into a shape
>  that is more convenient for the platform, filesystem, and the user to use.
> @@ -375,6 +379,54 @@ substitution.  For example:
>  ------------------------
>  
>  
> +Long Running Filter Process
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +If the filter command (string value) is defined via
> +filter.<driver>.process then Git can process all blobs with a
> +single filter invocation for the entire life of a single Git
> +command by talking with the following packet format (pkt-line)
> +based protocol over standard input and standard output.

> +
> +Git starts the filter on first usage and expects a welcome
> +message, protocol version number, and filter capabilities
> +seperated by spaces:
> +------------------------
> +packet:          git< git-filter-protocol
> +packet:          git< version 2
> +packet:          git< clean smudge
> +------------------------

> diff --git a/convert.c b/convert.c
> index 522e2c5..5ff200b 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.
> @@ -481,11 +482,232 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>  	return ret;
>  }
>  
> +static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t size)

> +	do {
> +		bytes_read = packet_read(
> +			fd, NULL, NULL,
> +			sb->buf + total_bytes_read, sb->len - total_bytes_read - 1,
> +			PACKET_READ_GENTLE_ON_EOF
> +		);
> +		total_bytes_read += bytes_read;
> +	}
> +	while (
> +		bytes_read > 0 && 					// the last packet was no flush
> +		sb->len - total_bytes_read - 1 > 0 	// we still have space left in the buffer
> +	);
> +	strbuf_setlen(sb, total_bytes_read);
> +	return total_bytes_read;
> +}
> +
> +static int multi_packet_write(const char *src, size_t len, const int in, const int out)

> +	off_t bytes_to_write;
> +	while (ret) {
> +		if (in >= 0) {
> +			bytes_to_write = xread(in, buffer, sizeof(buffer));
> +			if (bytes_to_write < 0)
> +				ret &= 0;
> +			src = buffer;
> +		} else {
> +			bytes_to_write = len > LARGE_PACKET_MAX - 4 ? LARGE_PACKET_MAX - 4 : len;
> +			len -= bytes_to_write;
> +		}
> +		if (!bytes_to_write)
> +			break;
> +		set_packet_header(header, bytes_to_write + 4);
> +		ret &= write_in_full(out, &header, sizeof(header)) == sizeof(header);
> +		ret &= write_in_full(out, src, bytes_to_write) == bytes_to_write;

> +	return ret;
> +}
> +
> +struct cmd2process {
> +	struct hashmap_entry ent; /* must be the first member! */
> +	const char *cmd;
> +	int clean;
> +	int smudge;

> +{
> +	struct cmd2process k;
> +	hashmap_entry_init(&k, strhash(cmd));
> +	k.cmd = cmd;
> +	return hashmap_get(hashmap, &k, NULL);
> +}

> +static struct cmd2process *start_protocol_filter(struct hashmap *hashmap, const char *cmd)
> +{
> +	int ret = 1;
> +	struct cmd2process *entry;
> +	struct child_process *process;
> +	const char *argv[] = { NULL, NULL };

> +	struct string_list capabilities = STRING_LIST_INIT_NODUP;
> +	char *capabilities_buffer;
> +	int i;
> +
> +	entry = xmalloc(sizeof(*entry));
> +	hashmap_entry_init(entry, strhash(cmd));
> +	entry->cmd = cmd;
> +	entry->clean = 0;
> +	entry->smudge = 0;

> +	process = &entry->process;
> +
> +	child_process_init(process);
> +	argv[0] = cmd;
> +	process->argv = argv;
> +	process->use_shell = 1;
> +	process->in = -1;

> +		stop_protocol_filter(hashmap, entry);
> +		return NULL;
> +	}
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	ret &= strcmp(packet_read_line(process->out, NULL), "git-filter-protocol") == 0;
> +	ret &= strcmp(packet_read_line(process->out, NULL), "version 2") == 0;

> +	for (i = 0; i < capabilities.nr; i++) {
> +		const char *requested = capabilities.items[i].string;
> +		if (!strcmp(requested, "clean")) {
> +			entry->clean = 1;
> +		} else if (!strcmp(requested, "smudge")) {
> +			entry->smudge = 1;
> +		} else {
> +			warning(
> +				"filter process '%s' requested unsupported filter capability '%s'",
> +				cmd, requested
> +			);

> +		}
> +	}
> +	string_list_clear(&capabilities, 0);
> +
> +	if (!ret) {
> +		error("initialization for external persistent filter '%s' failed", cmd);

> +		stop_protocol_filter(hashmap, entry);
> +		return NULL;
> +	}
> +
> +	hashmap_add(hashmap, entry);
> +	return entry;
> +}
> +
> +static int cmd_process_map_init = 0;
> +static struct hashmap cmd_process_map;
> +
> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
> +						int fd, struct strbuf *dst, const char *cmd,
> +						const char *filter_type)

> +{
> +	int ret = 1;
> +	struct cmd2process *entry;
> +	struct child_process *process;
> +	struct stat file_stat;
> +	struct strbuf nbuf = STRBUF_INIT;
> +	off_t expected_bytes;
> +	char *strtol_end;
> +	char *strbuf;
> +
> +	if (!cmd || !*cmd)
> +		return 0;
> +
> +	if (!dst)
> +		return 1;
> +
> +	if (!cmd_process_map_init) {
> +		cmd_process_map_init = 1;
> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
> +		entry = NULL;

> +	} else {
> +		entry = find_protocol_filter_entry(&cmd_process_map, cmd);
> +	}
> +
> +	if (!entry) {
> +		entry = start_protocol_filter(&cmd_process_map, cmd);

> +		if (!entry) {
> +			return 0;
> +		}
> +	}
> +	process = &entry->process;
> +
> +	if (!(!strcmp(filter_type, "clean") && entry->clean) &&
> +		!(!strcmp(filter_type, "smudge") && entry->smudge)) {

> +		return 0;
> +	}
> +
> +	if (fd >= 0 && !src) {
> +		ret &= fstat(fd, &file_stat) != -1;
> +		len = file_stat.st_size;
> +	}

> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	packet_write(process->in, "%s\n", filter_type);
> +	packet_write(process->in, "%s\n", path);
> +	packet_write(process->in, "%zu\n", len);

> +
> +	strbuf = packet_read_line(process->out, NULL);
> +	expected_bytes = (off_t)strtol(strbuf, &strtol_end, 10);
> +	ret &= (strtol_end != strbuf && errno != ERANGE);
> +
> +	if (expected_bytes > 0) {
> +		ret &= multi_packet_read(&nbuf, process->out, expected_bytes) == expected_bytes;
> +	}
> +
> +	sigchain_pop(SIGPIPE);
> +
> +	if (ret) {
> +		strbuf_swap(dst, &nbuf);
> +	} else {
> +		// Something went wrong with the protocol filter. Force shutdown!
> +		stop_protocol_filter(&cmd_process_map, entry);

> +	}
> +	strbuf_release(&nbuf);
> +	return ret;
> +}
> +

> @@ -823,7 +1049,10 @@ 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);
> +	if (!ca.drv->clean && ca.drv->process)
> +		return apply_protocol_filter(path, NULL, 0, -1, NULL, ca.drv->process, "clean");
> +	else
> +		return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);

>  }
>  
>  const char *get_convert_attr_ascii(const char *path)
> @@ -856,17 +1085,22 @@ 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;
> +	const char *clean_filter = NULL;
> +	const char *process_filter = NULL;
>  	int required = 0;
>  	struct conv_attrs ca;
>  
>  	convert_attrs(&ca, path);
>  	if (ca.drv) {
> -		filter = ca.drv->clean;
> +		clean_filter = ca.drv->clean;
> +		process_filter = ca.drv->process;
>  		required = ca.drv->required;
>  	}
>  
> -	ret |= apply_filter(path, src, len, -1, dst, filter);
> +	if (!clean_filter && process_filter)
> +		ret |= apply_protocol_filter(path, src, len, -1, dst, process_filter, "clean");
> +	else
> +		ret |= apply_filter(path, src, len, -1, dst, clean_filter);

> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index b9911a4..c4793ed 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -4,6 +4,11 @@ test_description='blob conversion via gitattributes'
>  
>  . ./test-lib.sh
>  
> +if ! test_have_prereq PERL; then
> +	skip_all='skipping perl interface tests, perl not available'
> +	test_done
> +fi

> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> new file mode 100755
> index 0000000..7176836
> --- /dev/null
> +++ b/t/t0021/rot13-filter.pl
> @@ -0,0 +1,146 @@
> +#!/usr/bin/perl
> +#
> +# Example implementation for the Git filter protocol version 2
> +# See Documentation/gitattributes.txt, section "Filter Protocol"
> +#
> +# This implementation supports two special test cases:
> +# (1) If data with the filename "clean-write-fail.r" is processed with
> +#     a "clean" operation then the write operation will die.
> +# (2) If data with the filename "smudge-write-fail.r" is processed with
> +#     a "smudge" operation then the write operation will die.

> +#
> +
> +use strict;
> +use warnings;
> +
> +my $MAX_PACKET_CONTENT_SIZE = 65516;
> +
> +sub rot13 {
> +    my ($str) = @_;
> +    $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
> +    return $str;
> +}
> +
> +sub packet_read {
> +    my $buffer;
> +    my $bytes_read = read STDIN, $buffer, 4;
> +    if ( $bytes_read == 0 ) {
> +        return;
> +    }
> +    elsif ( $bytes_read != 4 ) {

> +        die "invalid packet size '$bytes_read' field";
> +    }
> +    my $pkt_size = hex($buffer);
> +    if ( $pkt_size == 0 ) {
> +        return ( 1, "" );
> +    }
> +    elsif ( $pkt_size > 4 ) {
> +        my $content_size = $pkt_size - 4;
> +        $bytes_read = read STDIN, $buffer, $content_size;
> +        if ( $bytes_read != $content_size ) {
> +            die "invalid packet";
> +        }
> +        return ( 0, $buffer );
> +    }
> +    else {
> +        die "invalid packet size";
> +    }
> +}

> +
> +sub packet_write {
> +    my ($packet) = @_;
> +    print STDOUT sprintf( "%04x", length($packet) + 4 );
> +    print STDOUT $packet;
> +    STDOUT->flush();
> +}

> +
> +sub packet_flush {
> +    print STDOUT sprintf( "%04x", 0 );
> +    STDOUT->flush();
> +}
> +
> +open my $debug, ">>", "output.log";
> +print $debug "start\n";
> +$debug->flush();
> +
> +packet_write("git-filter-protocol\n");
> +packet_write("version 2\n");
> +packet_write("clean smudge\n");
> +print $debug "wrote filter header\n";
> +$debug->flush();

> +
> +while (1) {
> +    my $command = packet_read();
> +    unless ( defined($command) ) {
> +        exit();
> +    }
> +    chomp $command;
> +    print $debug "IN: $command";
> +    $debug->flush();
> +    my $filename = packet_read();
> +    chomp $filename;
> +    print $debug " $filename";
> +    $debug->flush();
> +    my $filelen = packet_read();
> +    chomp $filelen;
> +    print $debug " $filelen";
> +    $debug->flush();
> +
> +    $filelen =~ /\A\d+\z/ or die "bad filelen: $filelen";
> +    my $output;
> +
> +    if ( $filelen > 0 ) {
> +        my $input = "";
> +        {
> +            binmode(STDIN);
> +            my $buffer;
> +            my $done = 0;
> +            while ( !$done ) {
> +                ( $done, $buffer ) = packet_read();
> +                $input .= $buffer;
> +            }
> +            print $debug " [OK] -- ";
> +            $debug->flush();
> +        }
> +
> +        if ( $command eq "clean" ) {
> +            $output = rot13($input);
> +        }
> +        elsif ( $command eq "smudge" ) {
> +            $output = rot13($input);
> +        }
> +        else {
> +            die "bad command";

> +        }
> +    }
> +
> +    my $output_len = length($output);
> +    packet_write("$output_len\n");
> +    print $debug "OUT: $output_len ";
> +    $debug->flush();
> +    if ( $output_len > 0 ) {
> +        if (   ( $command eq "clean" and $filename eq "clean-write-fail.r" )

> +            or
> +            ( $command eq "smudge" and $filename eq "smudge-write-fail.r" ) )
> +        {
> +            print $debug " [FAIL]\n";
> +            $debug->flush();
> +            die "write error";
> +        }
> +        else {
> +            while ( length($output) > 0 ) {
> +                my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );
> +                packet_write($packet);
> +                if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {
> +                    $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );
> +                }
> +                else {
> +                    $output = "";
> +                }
> +            }
> +            packet_flush();
> +            print $debug "[OK]\n";
> +            $debug->flush();
> +        }
> +    }
> +}
> 

> On 28 Jul 2016, at 01:31, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 2016-07-27 o 02:06, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Git's clean/smudge mechanism invokes an external filter process for every
>> single blob that is affected by a filter. If Git filters a lot of blobs
>> then the startup time of the external filter processes can become a
>> significant part of the overall Git execution time.
> 
> It is not strictly necessary... but do we have any benchmarks for this,
> or is it just the feeling?  That is, in what situations Git may filter
> a large number of files (initial checkout? initial add?, switching
> to unrelated branch? getting large files from LFS solution?, and when
> startup time might become significant part of execution time (MS Windows?
> fast filters?)?

>> This patch adds the filter.<driver>.process string option which, if used,
> 
> String option... what are possible values?  What happens if you use
> value that is not recognized by Git (it is "if used", isn't it)?  That's
> not obvious from the commit message (though it might be from the docs).

>> keeps the external filter process running and processes all blobs with
>> the following packet format (pkt-line) based protocol over standard input
>> and standard output.
>> 
>> Git starts the filter on first usage and expects a welcome
>> message, protocol version number, and filter capabilities
>> seperated by spaces:
> 
> s/seperated/separated/

>> ------------------------
>> packet:          git< git-filter-protocol
>> packet:          git< version 2
>> packet:          git< clean smudge
> 
> Wouldn't "capabilities clean smudge" be better?  Or is it the
> "clean smudge" proposal easier to handle?

>> ------------------------
>> Supported filter capabilities are "clean" and "smudge".
>> 
>> Afterwards Git sends a command (e.g. "smudge" or "clean" - based
>> on the supported capabilities), the filename, the content size as
>> ASCII number in bytes, and the content in packet format with a
>> flush packet at the end:
>> ------------------------
>> packet:          git> smudge
>> packet:          git> testfile.dat
> 
> And here we don't have any problems with files containing embedded
> newlines etc.  Also Git should not be sending invalid file names.
> The question remains: is it absolute file path, or basename?

> 
>> packet:          git> 7
>> packet:          git> CONTENT
> 
> Can Git send file contents using more than one packet?  I think
> it should be stated upfront.

>> packet:          git> 0000
>> ------------------------
> 
> Why we need to send content size upfront?  Well, it is not a problem
> for Git, but (as I wrote in reply to the cover letter for this
> series) might be a problem for filter scripts.

>> The filter is expected to respond with the result content size as
>> ASCII number in bytes and the result content in packet format with
>> a flush packet at the end:
>> ------------------------
>> packet:          git< 57
> 
> This is not neccessary (and might be hard for scripts to do) if
> pkt-line protocol is used.
> 
> In short: I think pkt-line is not worth the complication on
> the Git side and on the filter size, unless it is used for
> streaming, or at least filter not having to calculate output
> size upfront.

> 
>> packet:          git< SMUDGED_CONTENT
>> packet:          git< 0000
>> ------------------------
>> Please note: In a future version of Git the capability "stream"
>> might be supported. In that case the content size must not be
>> part of the filter response.
>> 
>> Afterwards the filter is expected to wait for the next command.
> 
> When filter is supposed to exit, then?

>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> Helped-by: Martin-Louis Bright <mlbright@gmail.com>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> Documentation/gitattributes.txt |  54 +++++++-
>> convert.c                       | 269 ++++++++++++++++++++++++++++++++++++++--
>> t/t0021-conversion.sh           | 175 ++++++++++++++++++++++++++
>> t/t0021/rot13-filter.pl         | 146 ++++++++++++++++++++++
>> 4 files changed, 631 insertions(+), 13 deletions(-)
>> create mode 100755 t/t0021/rot13-filter.pl
>> 
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> index 8882a3e..8fb40d2 100644
>> --- a/Documentation/gitattributes.txt
>> +++ b/Documentation/gitattributes.txt
>> @@ -300,7 +300,11 @@ checkout, when the `smudge` command is specified, the command is
>> fed the blob object from its standard input, and its standard
>> output is used to update the worktree file.  Similarly, the
>> `clean` command is used to convert the contents of worktree file
>> -upon checkin.
>> +upon checkin. By default these commands process only a single
>> +blob and terminate. If a long running filter process (see section
>> +below) is used then Git can process all blobs with a single filter
>> +invocation for the entire life of a single Git command (e.g.
>> +`git add .`).
> 
> Ah, all right, here we give an example.
> 
> But, is "blob" term used in this document, or do we use "file"
> and "file contents" only?

>> 
>> One use of the content filtering is to massage the content into a shape
>> that is more convenient for the platform, filesystem, and the user to use.
>> @@ -375,6 +379,54 @@ substitution.  For example:
>> ------------------------
>> 
>> 
>> +Long Running Filter Process
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +If the filter command (string value) is defined via
>> +filter.<driver>.process then Git can process all blobs with a
>> +single filter invocation for the entire life of a single Git
>> +command by talking with the following packet format (pkt-line)
>> +based protocol over standard input and standard output.
> 
> Ah, so now it is the name of command,

> and I assume it is
> exclusive with `clean` / `smudge`, or does it only takes
> precedence based on capabilities of filter (that is if
> for example "`process`" does not include 'clean' capability,
> then `clean` filter is used, using per-file "protocol").
> Or do something different happens (like preference for
> old-style `clean` and `smudge` filters, and `process`
> used if any unset)?

>> +
>> +Git starts the filter on first usage and expects a welcome
>> +message, protocol version number, and filter capabilities
>> +seperated by spaces:
>> +------------------------
>> +packet:          git< git-filter-protocol
>> +packet:          git< version 2
>> +packet:          git< clean smudge
>> +------------------------
> 
> Neither of those is terminated by end of line character,
> that is, "\n", isn't it?

> 
>> diff --git a/convert.c b/convert.c
>> index 522e2c5..5ff200b 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.
>> @@ -481,11 +482,232 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>> 	return ret;
>> }
>> 
>> +static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t size)
> 
> What's the purpose of this function?  Is it to gather read whole
> contents of file into strbuf?  Or is it to read at most 'size'
> bytes of file / of pkt-line stream into strbuf?

>> +{
>> +	off_t bytes_read;
>> +	off_t total_bytes_read = 0;
>> +	strbuf_grow(sb, size + 1);	// we need one extra byte for the packet flush
> 
> Why we put packet flush into strbuf?  Or is it only temporarily,
> and we adjust that at the end... I see that it is.

>> +	do {
>> +		bytes_read = packet_read(
>> +			fd, NULL, NULL,
>> +			sb->buf + total_bytes_read, sb->len - total_bytes_read - 1,
>> +			PACKET_READ_GENTLE_ON_EOF
>> +		);
>> +		total_bytes_read += bytes_read;
>> +	}
>> +	while (
>> +		bytes_read > 0 && 					// the last packet was no flush
>> +		sb->len - total_bytes_read - 1 > 0 	// we still have space left in the buffer
>> +	);
>> +	strbuf_setlen(sb, total_bytes_read);
>> +	return total_bytes_read;
>> +}
>> +
>> +static int multi_packet_write(const char *src, size_t len, const int in, const int out)
> 
> What's the purpose of this function?  What are those 'in' and 'out'
> parameters?  Those names do not describe them well.  If they are
> file descriptors, add fd_* prefix (or whatever Git code uses).
> Edit: I see that's what existing code uses.

>> +{
>> +	int ret = 1;
>> +	char header[4];
>> +	char buffer[8192];
> 
> Could those two be in one variable?  Also, 'header' or 'pkt_header'?
> 
> Why 8192, and not LARGE_PACKET_MAX - 4?

> 
>> +	off_t bytes_to_write;
>> +	while (ret) {
>> +		if (in >= 0) {
>> +			bytes_to_write = xread(in, buffer, sizeof(buffer));
>> +			if (bytes_to_write < 0)
>> +				ret &= 0;
>> +			src = buffer;
>> +		} else {
>> +			bytes_to_write = len > LARGE_PACKET_MAX - 4 ? LARGE_PACKET_MAX - 4 : len;
>> +			len -= bytes_to_write;
>> +		}
>> +		if (!bytes_to_write)
>> +			break;
>> +		set_packet_header(header, bytes_to_write + 4);
>> +		ret &= write_in_full(out, &header, sizeof(header)) == sizeof(header);
>> +		ret &= write_in_full(out, src, bytes_to_write) == bytes_to_write;
> 
> These three lines are equivalent to write_packet(), or however
> it is named, isn't it?

>> +	}
>> +	ret &= write_in_full(out, "0000", 4) == 4;
> 
> This is equivalent to packet_flush(), or however it is named,
> isn't it?

>> +	return ret;
>> +}
>> +
>> +struct cmd2process {
>> +	struct hashmap_entry ent; /* must be the first member! */
>> +	const char *cmd;
>> +	int clean;
>> +	int smudge;
> 
> These two are 'int' used as 'bool', isn't it?

>> +	struct child_process process;
>> +};
> [...]
>> +static struct cmd2process *find_protocol_filter_entry(struct hashmap *hashmap, const char *cmd)
> 
> Wouldn't it be more descriptive to name the first parameter
> to this function 'cmd_hashmap', or something like that, rather
> than plain 'hashmap' (it might be the same that is used / was
> used for a global variable)?
> 
> Edit: or 'cmd_process_map'.

>> +{
>> +	struct cmd2process k;
>> +	hashmap_entry_init(&k, strhash(cmd));
>> +	k.cmd = cmd;
>> +	return hashmap_get(hashmap, &k, NULL);
>> +}
> 
> [...]
>> +static struct cmd2process *start_protocol_filter(struct hashmap *hashmap, const char *cmd)
>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry;
>> +	struct child_process *process;
>> +	const char *argv[] = { NULL, NULL };
> 
> Could we initialize it with  { cmd, NULL };?
> 
> Edit: Ah, I see that you follow filter_buffer_or_fd() example from
> convert.c, isn't it?

>> +	struct string_list capabilities = STRING_LIST_INIT_NODUP;
>> +	char *capabilities_buffer;
>> +	int i;
>> +
>> +	entry = xmalloc(sizeof(*entry));
>> +	hashmap_entry_init(entry, strhash(cmd));
>> +	entry->cmd = cmd;
>> +	entry->clean = 0;
>> +	entry->smudge = 0;
> 
> Wouldn't
> 
>   	entry->clean = entry->smudge = 0;
> 
> be more readable?

>> +	process = &entry->process;
>> +
>> +	child_process_init(process);
>> +	argv[0] = cmd;
>> +	process->argv = argv;
>> +	process->use_shell = 1;
>> +	process->in = -1;
> 
> Maybe
> 
>  +	process->in  = -1;
> 
> to align, but perhaps it is not worth it.

>> +	process->out = -1;
>> +
>> +	if (start_command(process)) {
>> +		error("cannot fork to run external persistent filter '%s'", cmd);
> 
> Just a question: is "cannot fork" the only reason why start_command()
> might have failed there?
> 
> Edit: Ah, I see that you follow filter_buffer_or_fd() example from
> convert.c, again.

>> +		stop_protocol_filter(hashmap, entry);
>> +		return NULL;
>> +	}
>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	ret &= strcmp(packet_read_line(process->out, NULL), "git-filter-protocol") == 0;
>> +	ret &= strcmp(packet_read_line(process->out, NULL), "version 2") == 0;
> 
> So that's why you need packet_read_line() to return string...
> 
>> +	capabilities_buffer = packet_read_line(process->out, NULL);
>> +	sigchain_pop(SIGPIPE);
>> +
>> +	string_list_split_in_place(&capabilities, capabilities_buffer, ' ', -1);
> 
> This does not modify capabilities_buffer, does it?

>> +	for (i = 0; i < capabilities.nr; i++) {
>> +		const char *requested = capabilities.items[i].string;
>> +		if (!strcmp(requested, "clean")) {
>> +			entry->clean = 1;
>> +		} else if (!strcmp(requested, "smudge")) {
>> +			entry->smudge = 1;
>> +		} else {
>> +			warning(
>> +				"filter process '%s' requested unsupported filter capability '%s'",
>> +				cmd, requested
>> +			);
> 
> Nice.  This makes it (somewhat) forward- and backward-compatibile.

>> +		}
>> +	}
>> +	string_list_clear(&capabilities, 0);
>> +
>> +	if (!ret) {
>> +		error("initialization for external persistent filter '%s' failed", cmd);
> 
> Do we need more detailed information about the source of error?

>> +		stop_protocol_filter(hashmap, entry);
>> +		return NULL;
>> +	}
>> +
>> +	hashmap_add(hashmap, entry);
>> +	return entry;
>> +}
>> +
>> +static int cmd_process_map_init = 0;
>> +static struct hashmap cmd_process_map;
>> +
>> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
>> +						int fd, struct strbuf *dst, const char *cmd,
>> +						const char *filter_type)
> 
> That is... quite a lot of parameters.  But I guess there is precedens.

>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry;
>> +	struct child_process *process;
>> +	struct stat file_stat;
>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	off_t expected_bytes;
>> +	char *strtol_end;
>> +	char *strbuf;
>> +
>> +	if (!cmd || !*cmd)
>> +		return 0;
>> +
>> +	if (!dst)
>> +		return 1;
>> +
>> +	if (!cmd_process_map_init) {
>> +		cmd_process_map_init = 1;
>> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
>> +		entry = NULL;
> 
> Is it better than having entry NULL-initialized?

>> +	} else {
>> +		entry = find_protocol_filter_entry(&cmd_process_map, cmd);
>> +	}
>> +
>> +	if (!entry) {
>> +		entry = start_protocol_filter(&cmd_process_map, cmd);
> 
> Hmmm... apply_filter() uses start_async() for some reason.  Why
> it does not apply for this new kind of filter?

>> +		if (!entry) {
>> +			return 0;
>> +		}
>> +	}
>> +	process = &entry->process;
>> +
>> +	if (!(!strcmp(filter_type, "clean") && entry->clean) &&
>> +		!(!strcmp(filter_type, "smudge") && entry->smudge)) {
> 
> Would it be more readable as !(A || B) rather than (!A && !B)?

>> +		return 0;
>> +	}
>> +
>> +	if (fd >= 0 && !src) {
>> +		ret &= fstat(fd, &file_stat) != -1;
>> +		len = file_stat.st_size;
>> +	}
> 
> All right, so we either use src+len,  or if we use fd we get
> file size.

>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +
>> +	packet_write(process->in, "%s\n", filter_type);
>> +	packet_write(process->in, "%s\n", path);
>> +	packet_write(process->in, "%zu\n", len);
> 
> So "\n" is included in protocol?

>> +
>> +	strbuf = packet_read_line(process->out, NULL);
>> +	expected_bytes = (off_t)strtol(strbuf, &strtol_end, 10);
>> +	ret &= (strtol_end != strbuf && errno != ERANGE);
>> +
>> +	if (expected_bytes > 0) {
>> +		ret &= multi_packet_read(&nbuf, process->out, expected_bytes) == expected_bytes;
>> +	}
>> +
>> +	sigchain_pop(SIGPIPE);
>> +
>> +	if (ret) {
>> +		strbuf_swap(dst, &nbuf);
>> +	} else {
>> +		// Something went wrong with the protocol filter. Force shutdown!
>> +		stop_protocol_filter(&cmd_process_map, entry);
> 
> Some error message would be nice... or do we print in down in stack?

>> +	}
>> +	strbuf_release(&nbuf);
>> +	return ret;
>> +}
>> +
> 
> 
> [...]
>> @@ -823,7 +1049,10 @@ 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);
>> +	if (!ca.drv->clean && ca.drv->process)
>> +		return apply_protocol_filter(path, NULL, 0, -1, NULL, ca.drv->process, "clean");
>> +	else
>> +		return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);
> 
> So the rule is: if `clean` is not set, and `process` is, try to use
> process for cleaning.  It was not clear for me from the documentation.

>> }
>> 
>> const char *get_convert_attr_ascii(const char *path)
>> @@ -856,17 +1085,22 @@ 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;
>> +	const char *clean_filter = NULL;
>> +	const char *process_filter = NULL;
>> 	int required = 0;
>> 	struct conv_attrs ca;
>> 
>> 	convert_attrs(&ca, path);
>> 	if (ca.drv) {
>> -		filter = ca.drv->clean;
>> +		clean_filter = ca.drv->clean;
>> +		process_filter = ca.drv->process;
>> 		required = ca.drv->required;
>> 	}
>> 
>> -	ret |= apply_filter(path, src, len, -1, dst, filter);
>> +	if (!clean_filter && process_filter)
>> +		ret |= apply_protocol_filter(path, src, len, -1, dst, process_filter, "clean");
>> +	else
>> +		ret |= apply_filter(path, src, len, -1, dst, clean_filter);
> 
> And the rule is the same here, as it should.
> 
>> 	if (!ret && required)
>> 		die("%s: clean filter '%s' failed", path, ca.drv->name);
> 
> Is it a correct error message for `process`?  I guess it is, as it prints
> the name of driver, and not attempted command.  Well, we might be using
> "process" filter in 'clean' mode,... but that is sophistry.

> [...]
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index b9911a4..c4793ed 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -4,6 +4,11 @@ test_description='blob conversion via gitattributes'
>> 
>> . ./test-lib.sh
>> 
>> +if ! test_have_prereq PERL; then
>> +	skip_all='skipping perl interface tests, perl not available'
>> +	test_done
>> +fi
> 
> Do all tests require Perl?

>> +test_expect_success 'required protocol filter should filter data' '
> [...]
>> +test_expect_success 'protocol filter large file' '
> [...]
>> +test_expect_success 'required protocol filter should fail with clean' '
> [...]
>> +test_expect_success 'protocol filter should restart after failure' '
> [...]
> 
>> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
>> new file mode 100755
>> index 0000000..7176836
>> --- /dev/null
>> +++ b/t/t0021/rot13-filter.pl
>> @@ -0,0 +1,146 @@
>> +#!/usr/bin/perl
>> +#
>> +# Example implementation for the Git filter protocol version 2
>> +# See Documentation/gitattributes.txt, section "Filter Protocol"
>> +#
>> +# This implementation supports two special test cases:
>> +# (1) If data with the filename "clean-write-fail.r" is processed with
>> +#     a "clean" operation then the write operation will die.
>> +# (2) If data with the filename "smudge-write-fail.r" is processed with
>> +#     a "smudge" operation then the write operation will die.
> 
> Nice.
> 
>> +#
>> +
>> +use strict;
>> +use warnings;
>> +
>> +my $MAX_PACKET_CONTENT_SIZE = 65516;
>> +
>> +sub rot13 {
>> +    my ($str) = @_;
>> +    $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
>> +    return $str;
>> +}
>> +
>> +sub packet_read {
>> +    my $buffer;
>> +    my $bytes_read = read STDIN, $buffer, 4;
>> +    if ( $bytes_read == 0 ) {
>> +        return;
>> +    }
>> +    elsif ( $bytes_read != 4 ) {
> 
> This is a bit untypical bracket style...

> 
>> +        die "invalid packet size '$bytes_read' field";
>> +    }
>> +    my $pkt_size = hex($buffer);
>> +    if ( $pkt_size == 0 ) {
>> +        return ( 1, "" );
>> +    }
>> +    elsif ( $pkt_size > 4 ) {
>> +        my $content_size = $pkt_size - 4;
>> +        $bytes_read = read STDIN, $buffer, $content_size;
>> +        if ( $bytes_read != $content_size ) {
>> +            die "invalid packet";
>> +        }
>> +        return ( 0, $buffer );
>> +    }
>> +    else {
>> +        die "invalid packet size";
>> +    }
>> +}
> 
> So packet reading is not that difficult...
> 
>> +
>> +sub packet_write {
>> +    my ($packet) = @_;
>> +    print STDOUT sprintf( "%04x", length($packet) + 4 );
>> +    print STDOUT $packet;
>> +    STDOUT->flush();
>> +}
> 
> ...and packet write is easy.
> 
>> +
>> +sub packet_flush {
>> +    print STDOUT sprintf( "%04x", 0 );
>> +    STDOUT->flush();
>> +}
>> +
>> +open my $debug, ">>", "output.log";
>> +print $debug "start\n";
>> +$debug->flush();
>> +
>> +packet_write("git-filter-protocol\n");
>> +packet_write("version 2\n");
>> +packet_write("clean smudge\n");
>> +print $debug "wrote filter header\n";
>> +$debug->flush();
> 
> Isn't $debug flushed automatically?

>> +
>> +while (1) {
>> +    my $command = packet_read();
>> +    unless ( defined($command) ) {
>> +        exit();
>> +    }
>> +    chomp $command;
>> +    print $debug "IN: $command";
>> +    $debug->flush();
>> +    my $filename = packet_read();
>> +    chomp $filename;
>> +    print $debug " $filename";
>> +    $debug->flush();
>> +    my $filelen = packet_read();
>> +    chomp $filelen;
>> +    print $debug " $filelen";
>> +    $debug->flush();
>> +
>> +    $filelen =~ /\A\d+\z/ or die "bad filelen: $filelen";
>> +    my $output;
>> +
>> +    if ( $filelen > 0 ) {
>> +        my $input = "";
>> +        {
>> +            binmode(STDIN);
>> +            my $buffer;
>> +            my $done = 0;
>> +            while ( !$done ) {
>> +                ( $done, $buffer ) = packet_read();
>> +                $input .= $buffer;
>> +            }
>> +            print $debug " [OK] -- ";
>> +            $debug->flush();
>> +        }
>> +
>> +        if ( $command eq "clean" ) {
>> +            $output = rot13($input);
>> +        }
>> +        elsif ( $command eq "smudge" ) {
>> +            $output = rot13($input);
>> +        }
>> +        else {
>> +            die "bad command";
> 
> Perhaps
> 
>               die "bad command $command";

>> +        }
>> +    }
>> +
>> +    my $output_len = length($output);
>> +    packet_write("$output_len\n");
>> +    print $debug "OUT: $output_len ";
>> +    $debug->flush();
>> +    if ( $output_len > 0 ) {
>> +        if (   ( $command eq "clean" and $filename eq "clean-write-fail.r" )
> 
> What happened here with whitespace around parentheses?

> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>> I think sending it upfront is nice for buffer allocations of big files
>> and it doesn't cost us anything to do it.
> 
> While I do NOT think "total size upfront" MUST BE avoided at all costs,
> I do not think the above statement to justify it makes ANY sense.
> 
> Big files are by definition something you cannot afford to hold its
> entirety in core, so you do not want to be told that you'd be fed 40GB
> and ask xmalloc to allocate that much.

> 
> It allows the reader to be lazy for buffer allocations as long as
> you know the file fits in-core, at the cost of forcing the writer to
> somehow come up with the total number of bytes even before sending a
> single byte (in other words, if the writer cannot produce and hold
> the data in-core, it may even have to spool the data in a temporary
> file only to count, and then play it back after showing the total
> size).

> 
> It is good that you allow both mode of operations and the size of
> the data can either be given upfront (which allows a single fixed
> allocation upfront without realloc, as long as the data fits in
> core), or be left "(atend)".

> I just don't want to see it oversold as a "feature" that the size
> has to come before data.  That is a limitation, not a feature.
> 
> Thanks.
> 

> On 30 Jul 2016, at 01:11, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 2016-07-29 o 19:35, Junio C Hamano pisze:
>> Lars Schneider <larsxschneider@gmail.com> writes:
>> 
>>> I think sending it upfront is nice for buffer allocations of big files
>>> and it doesn't cost us anything to do it.
>> 
>> While I do NOT think "total size upfront" MUST BE avoided at all costs,
>> I do not think the above statement to justify it makes ANY sense.
>> 
>> Big files are by definition something you cannot afford to hold its
>> entirety in core, so you do not want to be told that you'd be fed 40GB
>> and ask xmalloc to allocate that much.
> 
> I don't know much how filter driver work internally, but in some cases
> Git reads or writes from file (file descriptor), in other cases it reads
> or writes from str+len pair (it probably predates strbuf) - I think in
> those cases file needs to fit in memory (in size_t).  So in some cases
> Git reads file into memory.  Whether it uses xmalloc or mmap, I don't
> know.
> 
>> 
>> It allows the reader to be lazy for buffer allocations as long as
>> you know the file fits in-core, at the cost of forcing the writer to
>> somehow come up with the total number of bytes even before sending a
>> single byte (in other words, if the writer cannot produce and hold
>> the data in-core, it may even have to spool the data in a temporary
>> file only to count, and then play it back after showing the total
>> size).
> 
> For some types of filters you can know the size upfront:
> - for filters such as rot13, with 1-to-1 transformation, you know
>   that the output size is the same as the input size
> - for block encodings, and for constant-width to constant-width
>   encoding conversion, filter can calculate output size from the
>   input size (e.g. <output size> = 2*<input size>)
> - filter may have get size from somewhere, for example LFS filter
>   stub is constant size, and files are stored in artifactory with
>   their length 
> 
>> 
>> It is good that you allow both mode of operations and the size of
>> the data can either be given upfront (which allows a single fixed
>> allocation upfront without realloc, as long as the data fits in
>> core), or be left "(atend)".
> 
> I think the protocol should be either: <size> + <contents>, or
> <size unknown> + <contents> + <flush>, that is do not use flush
> packet if size is known upfront -- it would be a second point
> of truth (SPOT principle).

> 
>> I just don't want to see it oversold as a "feature" that the size
>> has to come before data.  That is a limitation, not a feature.
>> 
>> Thanks.
>> 
> 

>> I think the protocol should be either: <size> + <contents>, or
>> <size unknown> + <contents> + <flush>, that is do not use flush
>> packet if size is known upfront -- it would be a second point
>> of truth (SPOT principle).
>
> As I mentioned elsewhere a <flush> packet is always send right now.
> I have no strong opinion if this is good or bad. The implementation
> was a little bit simpler and that's why I did it. I will implement 
> whatever option the majority prefers :-)

> On 27 Jul 2016, at 22:01, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 2016-07-27 o 02:06, larsxschneider@gmail.com pisze:
> 
>> Git filter with spaces (e.g. `filter.sh foo`) are hard to read in
>> error messages. Quote them to improve the readability.
> 
> This is not something very important, but the above commit message
> feels a bit clunky to me.  The change is easy to understand, though,
> so the commit message style is not that important.
> 
> Perhaps "Git filter driver command with spaces"?
> 
> Well, nevermind; if you have a better idea, good.  If not, it is
> good enough, IMHO.

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Hi,
> 
> thanks a lot for the extensive reviews. I tried to address all mentioned
> concerns and summarized them below. The most prominent changes since v1 are
> the following:
> * Git offers a number of filter capabilities that a filter can request
>   (right now only "smudge" and "clean" - in the future maybe "cleanFromFile",
>   "smudgeToFile", and/or "stream")
> * pipe communication uses a packet format (pkt-line) based protocol

> ## Torsten:
> * add "\n" line terminator after version in init sequence
> * prepare big file for EXPENSIVE tests once
> * set "#!/usr/bin/perl" as shebang for rot13.pl to mimic other Perl test scripts
> * add test_have_prereq PERL to t0021
> 
> ## Ramsay:
> * use write_in_full(process->in, nbuf.buf, nbuf.len) to avoid unneccesary strlen call
> * use read_in_full to read data that exceeds MAX_IO_SIZE properly
> * fix test case to check for large file filtering
> 
> ## Jakub:
> * use standard input/standard output instead of stdin/stdout [in description/documentation]
> * replace global variable "cmd_process_map" with a function parameter where possible
> * avoid "strbuf_reset" after STRBUF_INIT
> * align test_config_global
> * rename rot13.pl to rot13-filter.pl
> * make Perl style consistent
> * describe hard coded filenames in test filter header
> * improve docs
> * add filter capabilities field (enables cleanToFile, smudgeFromFile, and/or stream later)
> * explain that content size in bytes is encoded in ASCII
> * consistent line ending for die call in Perl (without "\n")
> * make rot13 test filter die in case of failure (instead of returning "fail")
> 
> ## Eric:
> * flush explicitly in Perl test filter
> * do not initialize variables to NULL if they are set unconditionally
> * fix no-op stop_protocol_filter
> * use off_t instead of size_t
> * improve test filter int parsing ($filelen =~ /\A\d+\z/ or die "bad filelen: $filelen")
> 
> ## Peff:
> * use pkt-line protocol
> * do not use Perl autodie
> 
> ## Remi:
> * remove spaces after '<'

> On 27 Jul 2016, at 21:08, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 2016-07-27 o 02:06, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Hi,
>> 
>> thanks a lot for the extensive reviews. I tried to address all mentioned
>> concerns and summarized them below. The most prominent changes since v1 are
>> the following:
>> * Git offers a number of filter capabilities that a filter can request
>>  (right now only "smudge" and "clean" - in the future maybe "cleanFromFile",
>>  "smudgeToFile", and/or "stream")
>> * pipe communication uses a packet format (pkt-line) based protocol
> 
> I wonder if it would make sense to support both whole-file pipe communication,
> and packet format (pkt-line) pipe communication.
> 
> The problem with whole-file pipe communication (original proposal for
> new filter protocol is that it needs file size upfront.  For some types
> of filters it is not a problem:
> - if a filtered file has the same size as original, like for rot13
>   example in the test for the feature
> - if you can calculate the resulting file size from original size,
>   like for most if not all encryption formats (that includes GPG,
>   uudecode, base64, quoted-printable, hex, etc.); same for decryption,
>   and from converting between fixed-width encodings
> - if resulting file size is saved somewhere that is easy to get, like
>   for LFS solutions (I think).
> 
> For other filters it is serious problem.  For example indent, keyword
> expansion, rezipping with zero compression (well, maybe not this one,
> but at least the reverse of it), converting between encodings where
> at least one is variable width (like UTF-8),...
> 
> IMHO writing whole-file persistent filters is easier than using pkt-line.
> On the other hand using pkt-line allow for more detailed progress report.

>> On 27 Jul 2016, at 21:08, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 2016-07-27 o 02:06, larsxschneider@gmail.com pisze:
>>>
>>> thanks a lot for the extensive reviews. I tried to address all mentioned
>>> concerns and summarized them below. The most prominent changes since v1 are
>>> the following:
>>> * Git offers a number of filter capabilities that a filter can request
>>>  (right now only "smudge" and "clean" - in the future maybe "cleanFromFile",
>>>  "smudgeToFile", and/or "stream")
>>> * pipe communication uses a packet format (pkt-line) based protocol
>>
>> I wonder if it would make sense to support both whole-file pipe communication,
>> and packet format (pkt-line) pipe communication.
>>
>> The problem with whole-file pipe communication (original proposal for
>> new filter protocol is that it needs file size upfront.  For some types
>> of filters it is not a problem:
>> - if a filtered file has the same size as original, like for rot13
>>   example in the test for the feature
>> - if you can calculate the resulting file size from original size,
>>   like for most if not all encryption formats (that includes GPG,
>>   uudecode, base64, quoted-printable, hex, etc.); same for decryption,
>>   and from converting between fixed-width encodings
>> - if resulting file size is saved somewhere that is easy to get, like
>>   for LFS solutions (I think).
>>
>> For other filters it is serious problem.  For example indent, keyword
>> expansion, rezipping with zero compression (well, maybe not this one,
>> but at least the reverse of it), converting between encodings where
>> at least one is variable width (like UTF-8),...
>>
>> IMHO writing whole-file persistent filters is easier than using pkt-line.
>> On the other hand using pkt-line allow for more detailed progress report.
> 
> I initially wanted to support only "while-file" pipe, too.
> 
> But Peff ($gmane/299902), Duy, and Eric, seemed to prefer the pkt-line
> solution (gmane is down - otherwise I would have given you the links).

> 
> After I have looked at it I think the pkt-line solution is indeed nicer
> for the following reasons:
> 
> (1) A stream optimized version (read/write in separate threads) of the filter
>     protocol can be implemented in the future without changing the protocol

> 
> 
>>> * a long running filter application is defined with "filter.<driver>.process"
>>
>> I hope that won't confuse Git users into trying to use single-shot
>> filters with a new protocol...
> 
> Yes, that was my intention for this new config.

> On Thu, Jul 28, 2016 at 09:16:18AM +0200, Lars Schneider wrote:
> 
>> But Peff ($gmane/299902), Duy, and Eric, seemed to prefer the pkt-line
>> solution (gmane is down - otherwise I would have given you the links).
> 
> FWIW, I think there are arguments for transmitting size + content
> (namely, that it is simpler); the downside is that it doesn't allow
> streaming.

> 
> So I think there are two viable alternatives:
> 
>   1. Total size of data in ASCII decimal, newline, then that many bytes
>      of content.
> 
>   2. No size header, then a series of pkt-lines followed by a flush
>      packet.

> And you should choose between the two based on whether it's more
> important to allow streaming, or more important to make the filter
> implementations simple[1].
> 
> Any solution that is in between those (like sending a size header and
> then using pktlines anyway) is sacrificing simplicity but not getting
> the streaming benefits.
> 
> -Peff
> 
> [1] I haven't thought hard enough about it to have a real opinion. My
>     gut says to go with the streaming, just because we've had to
>     retrofit streaming in other areas when dealing with blobs, so I
>     think we'll end up there eventually. So choosing a simpler protocol
>     like (1) would probably mean eventually implementing a next-version
>     protocol that does (2), and having to support both.
> 
> PS Jakub asked for links, but gmane is down. Here are the relevant threads:
> 
>    http://public-inbox.org/git/20160720134916.GB19359@sigill.intra.peff.net
> 
>    http://public-inbox.org/git/20160722154900.19477-1-larsxschneider%40gmail.com/t/#u
> 

> On 29 Jul 2016, at 09:40, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 2016-07-28 o 15:29, Jeff King pisze:
>> On Thu, Jul 28, 2016 at 09:16:18AM +0200, Lars Schneider wrote:
>> 
>>> But Peff ($gmane/299902), Duy, and Eric, seemed to prefer the pkt-line
>>> solution (gmane is down - otherwise I would have given you the links).
>> 
>> FWIW, I think there are arguments for transmitting size + content
>> (namely, that it is simpler); the downside is that it doesn't allow
>> streaming.
> 
> And that it requires for the filter to know the size of its output
> upfront (which, as I wrote, might be easy to do based on size of input
> and data stored elsewhere, or might need generating whole output to
> know).
> 
> I don't know how parallel Git is, but if it is parallel enough,
> and other limits do not apply (limited amount of CPU cores, I/O limits),
> without streaming new filter protocol might be slower, unless startup
> time dominates (MS Windows?):
> 
> Current parallel:
> 
>   |   startup   | processing 1 |
>    |  startup    | processing 2  |
>   | startup |  processing 3 |
>     |  startup  |  processing 4  |
> 
> Protocol v2:
> 
>   |  startup  | processing 1 | processing 2 | processing 3 | processing 4 |

>> So I think there are two viable alternatives:
>> 
>>  1. Total size of data in ASCII decimal, newline, then that many bytes
>>     of content.
>> 
>>  2. No size header, then a series of pkt-lines followed by a flush
>>     packet.
> 
>    3. Optional size header[2][3], then a series of pkt-lines followed
>       by a flush packet[4].
> 
> [2] Git should always provide size, because it is easy to do, and
>    I think quite cheap (stored with blob, stored in index, or stat()
>    on file away).  Filter can provide size if it is easy to calculate,
>    or approximation of size / size hint[5] - it helps to avoid
>    reallocation.

> [4] On one hand you need to catch cases where real size is larger than
>    size sent upfront, or smaller than size sent upfront; on the
>    other hand it might be a place where to send warnings and errors...
>    unless we utilize stderr of a process (but then there is a problem
>    of deadlocking, I think).
> [5] I suggest
> 
>        <size as ascii decimal>
>        "approx" SPC <size as ascii decimal>
>        "unknown"
>        "fail"

> On 29 Jul 2016, at 17:57, Jeff King <peff@peff.net> wrote:
> 
> On Fri, Jul 29, 2016 at 10:14:17AM +0200, Lars Schneider wrote:
> 
>> My current implementation supports only two cases. Either the filter
>> knows the size and sends it back. Or the filter doesn't know the size
>> and Git reads until the flush packet (your "unknown" case). "Approx" is 
>> probably hard to do and fail shouldn't be part of the size, no?
> 
> Ah, OK, I missed that you could handle both cases. I think that is a
> reasonable approach. It means the filter has to bother with pkt-lines,
> but beyond that, it can choose the simple or streaming approach as
> appropriate.

>> That being said a "fail" response is a very good idea! This allows
>> the filter to communicate to git that a non required filter process
>> failed. I will add that to the protocol. Thanks :) 
> 
> Maybe just send "ok <size>", "ok -1" (for streaming), or "fail <reason>"
> followed by the content? That is similar to other Git protocols, though
> I am not sure they are good models for sanity or extensibility. :)
> 
> I don't know if you would want to leave room for other "headers" in the
> response, but you could also do something more HTTP-like, with a status
> code, and arbitrary headers. And presumably git would just ignore
> headers it doesn't know about. I think that's what Jakub's example was
> leaning towards. I'm just not sure what other headers are really useful,
> but it does leave room for extensibility.

> >> That being said a "fail" response is a very good idea! This allows
> >> the filter to communicate to git that a non required filter process
> >> failed. I will add that to the protocol. Thanks :) 
> > 
> > Maybe just send "ok <size>", "ok -1" (for streaming), or "fail <reason>"
> > followed by the content? That is similar to other Git protocols, though
> > I am not sure they are good models for sanity or extensibility. :)
> > 
> > I don't know if you would want to leave room for other "headers" in the
> > response, but you could also do something more HTTP-like, with a status
> > code, and arbitrary headers. And presumably git would just ignore
> > headers it doesn't know about. I think that's what Jakub's example was
> > leaning towards. I'm just not sure what other headers are really useful,
> > but it does leave room for extensibility.
> 
> Well, "ok <size>" wouldn't make much sense as we already transmitted
> the size upfront I think. Right now I have implemented the following options:

> "success\n" --> everything was alright
> "reject\n" --> the filter rejected the operation but this is no error 
>                if "filter.<driver>.required = false"
> <anything else> --> failure that stops/restarts the filter process
> 
> I don't think sending any failure reason makes sense because if a failure
> happens then we are likely in a bad state already (that's why I restart the
> filter process. I think the filter can report trouble on its own via stdout,
> no? I think this is what Git-LFS already does.

> On 29 Jul 2016, at 18:50, Jeff King <peff@peff.net> wrote:
> 
> On Fri, Jul 29, 2016 at 06:20:51PM +0200, Lars Schneider wrote:
> 
>>>> That being said a "fail" response is a very good idea! This allows
>>>> the filter to communicate to git that a non required filter process
>>>> failed. I will add that to the protocol. Thanks :) 
>>> 
>>> Maybe just send "ok <size>", "ok -1" (for streaming), or "fail <reason>"
>>> followed by the content? That is similar to other Git protocols, though
>>> I am not sure they are good models for sanity or extensibility. :)
>>> 
>>> I don't know if you would want to leave room for other "headers" in the
>>> response, but you could also do something more HTTP-like, with a status
>>> code, and arbitrary headers. And presumably git would just ignore
>>> headers it doesn't know about. I think that's what Jakub's example was
>>> leaning towards. I'm just not sure what other headers are really useful,
>>> but it does leave room for extensibility.
>> 
>> Well, "ok <size>" wouldn't make much sense as we already transmitted
>> the size upfront I think. Right now I have implemented the following options:
> 
> Maybe I'm confused about where in the protocol we are. I was imagining:
> 
>  git> smudge
>  git> <filename>
>  git> <size>
>  git> ...pkt-lines...
>  git> pktline-flush
> 
>  git< ok <size>
>  git< ...pkt-lines...
>  git< flush
> 
> That is, we should say "I have something for you" or "I do not" before
> sending a size, because in the "I do not" case we have no size to send.

> A more extensible protocol might look like:
> 
>  git> smudge
>  git> filename=<filename>
>  git> size=<size>
>  git> pktline-flush
>  git> ...pkt-lines of data...
>  git> pktline-flush
> 
>  git< ok (or success, or whatever status code you like)
>  git< size=<size>
>  git< pkt-line-flush
>  git< ...pkt-lines of data...
>  git< pktline-flush
> 
> That leaves room for new "keys" to be added before the first pkt-flush,
> without having to change the parsing at all.

>> "success\n" --> everything was alright
>> "reject\n" --> the filter rejected the operation but this is no error 
>>               if "filter.<driver>.required = false"
>> <anything else> --> failure that stops/restarts the filter process
>> 
>> I don't think sending any failure reason makes sense because if a failure
>> happens then we are likely in a bad state already (that's why I restart the
>> filter process. I think the filter can report trouble on its own via stdout,
>> no? I think this is what Git-LFS already does.
> 
> Git-LFS sends to stderr because there's no other option. I wonder if it
> would be nicer to make it Git's responsibility to talk to the user,
> because then it could respect things like "--quiet". I guess error
> messages are generally printed regardless of verbosity, though, so
> printing them unconditionally is OK.

> Here is the reject case (non-streaming):
> 
> git> smudge
> git> <filename>
> git> <size>
> git> ...pkt-lines...
> git> pktline-flush
> 
> git< 0
> git< reject
> 
> 
> Do you see a problem with this approach?