[PATCH v2 0/4] extend smudge/clean filters with direct file access

[PATCH v2 0/4] extend smudge/clean filters with direct file access

[Joey Hess]

Reroll of this patch set with changes:

* Renamed the new filter drivers for consistency with other configs.
* Improved documentation with feedback from Junio and others.
* Eliminated %p and instead append the filename to the commands
  (separated by a space).
* Fixed an FD leak and a space leak.
* Only use smudgeToFile with regular files, not symlinks.
* After running the smudgeToFile command, double-check that the
  expected file is present, in case the command was buggy and deleted it.
* Added a warning message when the new filter commands are configured
  but the old ones are not, so that the user knows it's refusing to use
  their configuration.

There's been good and helpful documentation and interface review,
but some more code review would be good! Also, git-annex has a
improved-smudge-filters branch now that demonstrates this interface.

Joey Hess (4):
  add smudgeToFile and cleanFromFile filter configs
  use cleanFromFile in git add
  use smudgeToFile in git checkout etc
  warn on unusable smudgeToFile/cleanFromFile config

 Documentation/config.txt        |  18 +++++-
 Documentation/gitattributes.txt |  37 ++++++++++++
 convert.c                       | 126 +++++++++++++++++++++++++++++++++++-----
 convert.h                       |  10 ++++
 entry.c                         |  37 +++++++++---
 sha1_file.c                     |  42 ++++++++++++--
 t/t0021-conversion.sh           |  64 ++++++++++++++++++++
 7 files changed, 304 insertions(+), 30 deletions(-)

-- 
2.8.1

[PATCH v2 1/4] add smudgeToFile and cleanFromFile filter configs

[Joey Hess]

This adds new smudgeToFile and cleanFromFile filter commands,
which are similar to smudge and clean but allow direct access to files on
disk.

This interface can be much more efficient when operating on large files,
because the whole file content does not need to be streamed through the
filter. It even allows for things like cleanFromFile commands that avoid
reading the whole content of the file, and for smudgeToFile commands that
populate a work tree file using an efficient Copy On Write operation.

The new filter commands will not be used for all filtering. They are
efficient to use when git add is adding a file, or when the work tree is
being updated, but not a good fit when git is internally filtering blob
objects in memory for eg, a diff.

So, a user who wants to use smudgeToFile should also provide a smudge
command to be used in cases where smudgeToFile is not used. And ditto
with cleanFromFile and clean. To avoid foot-shooting configurations, the
new commands are not used unless the old commands are also configured.

That also ensures that a filter driver configuration that includes these
new commands will work, although less efficiently, when used with an older
version of git that does not support them.

Signed-off-by: Joey Hess <joeyh@joeyh.name>
---
 Documentation/config.txt        |  18 ++++++-
 Documentation/gitattributes.txt |  37 ++++++++++++++
 convert.c                       | 108 ++++++++++++++++++++++++++++++++++------
 convert.h                       |  10 ++++
 4 files changed, 157 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e1b2e4..38f54c1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1299,15 +1299,29 @@ format.useAutoBase::
 	format-patch by default.
 
 filter.<driver>.clean::
-	The command which is used to convert the content of a worktree
+	The command which is used as a filter to convert the content of a worktree
 	file to a blob upon checkin.  See linkgit:gitattributes[5] for
 	details.
 
 filter.<driver>.smudge::
-	The command which is used to convert the content of a blob
+	The command which is used as a filter to convert the content of a blob
 	object to a worktree file upon checkout.  See
 	linkgit:gitattributes[5] for details.
 
+filter.<driver>.cleanFromFile::
+	Similar to filter.<driver>.clean but the specified command 
+	directly accesses a worktree file on disk, rather than
+	receiving the file content from standard input. 
+	Only used when filter.<driver>.clean is also configured.
+	See linkgit:gitattributes[5] for details.
+
+filter.<driver>.smudgeToFile::
+	Similar to filter.<driver>.smudge but the specified command
+	writes the content of a blob directly to a worktree file,
+	rather than to standard output.
+	Only used when filter.<driver>.smudge is also configured.
+	See linkgit:gitattributes[5] for details.
+
 fsck.<msg-id>::
 	Allows overriding the message type (error, warn or ignore) of a
 	specific message ID such as `missingEmail`.
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 145dd10..5ae0783 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -380,6 +380,43 @@ not exist, or may have different contents. So, smudge and clean commands
 should not try to access the file on disk, but only act as filters on the
 content provided to them on standard input.
 
+There are two extra commands "cleanFromFile" and "smudgeToFile", which
+can optionally be set in a filter driver. These are similar to the "clean"
+and "smudge" commands, but avoid needing to pipe the contents of files
+through the filters, and instead read/write files in the filesystem.
+This can be more efficient when using filters with large files that are not
+directly stored in the repository.
+
+Both "cleanFromFile" and "smudgeToFile" are provided a path as an
+added parameter after the configured command line.
+
+The "cleanFromFile" command is provided the path to the file that
+it should clean. Like the "clean" command, it should output the cleaned
+version to standard output.
+
+The "smudgeToFile" command is provided a path to the file that it
+should write to. (This file will already exist, as an empty file that can
+be written to or replaced.) Like the "smudge" command, "smudgeToFile"
+is fed the blob object from its standard input.
+
+Some git operations that need to apply filters cannot use "cleanFromFile"
+and "smudgeToFile", since the files are not present to disk. So, to avoid
+inconsistent behavior, "cleanFromFile" will only be used if "clean" is
+also configured, and "smudgeToFile" will only be used if "smudge" is also
+configured.
+
+An example large file storage filter driver using cleanFromFile and
+smudgeToFile follows:
+
+------------------------
+[filter "bigfiles"]
+	clean = store-bigfile --from-stdin
+	cleanFromFile = store-bigfile --from-file
+	smudge = retrieve-bigfile --to-stdout
+	smudgeToFile = retrieve-bigfile --to-file
+	required
+------------------------
+
 Interaction between checkin/checkout attributes
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/convert.c b/convert.c
index b1614bf..bf63ba0 100644
--- a/convert.c
+++ b/convert.c
@@ -360,7 +360,8 @@ struct filter_params {
 	unsigned long size;
 	int fd;
 	const char *cmd;
-	const char *path;
+	const char *path; /* Path within the git repository */
+	const char *fspath; /* Path to file on disk */
 };
 
 static int filter_buffer_or_fd(int in, int out, void *data)
@@ -389,6 +390,15 @@ static int filter_buffer_or_fd(int in, int out, void *data)
 	strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);
 	strbuf_release(&path);
 
+	/* append fspath to the command if it's set, separated with a space */
+	if (params->fspath) {
+		struct strbuf fspath = STRBUF_INIT;
+		sq_quote_buf(&fspath, params->fspath);
+		strbuf_addstr(&cmd, " ");
+		strbuf_addbuf(&cmd, &fspath);
+		strbuf_release(&fspath);
+	}
+
 	argv[0] = cmd.buf;
 
 	child_process.argv = argv;
@@ -427,7 +437,8 @@ static int filter_buffer_or_fd(int in, int out, void *data)
 	return (write_err || status);
 }
 
-static int apply_filter(const char *path, const char *src, size_t len, int fd,
+static int apply_filter(const char *path, const char *fspath,
+			const char *src, size_t len, int fd,
                         struct strbuf *dst, const char *cmd)
 {
 	/*
@@ -456,6 +467,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
 	params.fd = fd;
 	params.cmd = cmd;
 	params.path = path;
+	params.fspath = fspath;
 
 	fflush(NULL);
 	if (start_async(&async))
@@ -486,6 +498,8 @@ static struct convert_driver {
 	struct convert_driver *next;
 	const char *smudge;
 	const char *clean;
+	const char *smudge_to_file;
+	const char *clean_from_file;
 	int required;
 } *user_convert, **user_convert_tail;
 
@@ -512,8 +526,9 @@ static int read_convert_config(const char *var, const char *value, void *cb)
 	}
 
 	/*
-	 * filter.<name>.smudge and filter.<name>.clean specifies
-	 * the command line:
+	 * filter.<name>.smudge, filter.<name>.clean,
+	 * filter.<name>.smudgeToFile, filter.<name>.cleanFromFile
+	 * specifies the command line:
 	 *
 	 *	command-line
 	 *
@@ -526,6 +541,12 @@ 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("smudgetofile", key))
+		return git_config_string(&drv->smudge_to_file, var, value);
+
+	if (!strcmp("cleanfromfile", key))
+		return git_config_string(&drv->clean_from_file, var, value);
+
 	if (!strcmp("required", key)) {
 		drv->required = git_config_bool(var, value);
 		return 0;
@@ -823,7 +844,35 @@ int would_convert_to_git_filter_fd(const char *path)
 	if (!ca.drv->required)
 		return 0;
 
-	return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);
+	return apply_filter(path, NULL, NULL, 0, -1, NULL, ca.drv->clean);
+}
+
+int can_clean_from_file(const char *path)
+{
+	struct conv_attrs ca;
+
+	convert_attrs(&ca, path);
+	if (!ca.drv)
+		return 0;
+
+	/* Only use the cleanFromFile filter when the clean filter is also
+	 * configured.
+	 */
+	return (ca.drv->clean_from_file && ca.drv->clean);
+}
+
+int can_smudge_to_file(const char *path)
+{
+	struct conv_attrs ca;
+
+	convert_attrs(&ca, path);
+	if (!ca.drv)
+		return 0;
+
+	/* Only use the smudgeToFile filter when the smudge filter is also
+	 * configured.
+	 */
+	return (ca.drv->smudge_to_file && ca.drv->smudge);
 }
 
 const char *get_convert_attr_ascii(const char *path)
@@ -866,7 +915,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
 		required = ca.drv->required;
 	}
 
-	ret |= apply_filter(path, src, len, -1, dst, filter);
+	ret |= apply_filter(path, NULL, src, len, -1, dst, filter);
 	if (!ret && required)
 		die("%s: clean filter '%s' failed", path, ca.drv->name);
 
@@ -891,14 +940,33 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
 	assert(ca.drv);
 	assert(ca.drv->clean);
 
-	if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
+	if (!apply_filter(path, NULL, NULL, 0, fd, dst, ca.drv->clean))
 		die("%s: clean filter '%s' failed", path, ca.drv->name);
 
 	crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
 	ident_to_git(path, dst->buf, dst->len, dst, ca.ident);
 }
 
-static int convert_to_working_tree_internal(const char *path, const char *src,
+void convert_to_git_filter_from_file(const char *path, struct strbuf *dst,
+				   enum safe_crlf checksafe)
+{
+	struct conv_attrs ca;
+	convert_attrs(&ca, path);
+
+	assert(ca.drv);
+	assert(ca.drv->clean);
+	assert(ca.drv->clean_from_file);
+
+	if (!apply_filter(path, path, "", 0, -1, dst, ca.drv->clean_from_file))
+		die("%s: cleanFromFile filter '%s' failed", path, ca.drv->name);
+
+	crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
+	ident_to_git(path, dst->buf, dst->len, dst, ca.ident);
+}
+
+static int convert_to_working_tree_internal(const char *path,
+					    const char *destpath,
+					    const char *src,
 					    size_t len, struct strbuf *dst,
 					    int normalizing)
 {
@@ -909,7 +977,10 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
 
 	convert_attrs(&ca, path);
 	if (ca.drv) {
-		filter = ca.drv->smudge;
+		if (destpath)
+			filter = ca.drv->smudge_to_file;
+		else
+			filter = ca.drv->smudge;
 		required = ca.drv->required;
 	}
 
@@ -920,7 +991,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
 	}
 	/*
 	 * CRLF conversion can be skipped if normalizing, unless there
-	 * is a smudge filter.  The filter might expect CRLFs.
+	 * is a filter.  The filter might expect CRLFs.
 	 */
 	if (filter || !normalizing) {
 		ret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);
@@ -930,21 +1001,30 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
 		}
 	}
 
-	ret_filter = apply_filter(path, src, len, -1, dst, filter);
+	ret_filter = apply_filter(path, destpath, src, len, -1, dst, filter);
 	if (!ret_filter && required)
-		die("%s: smudge filter %s failed", path, ca.drv->name);
+		die("%s: %s filter %s failed", path, destpath ? "smudgeToFile" : "smudge", ca.drv->name);
 
 	return ret | ret_filter;
 }
 
 int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst)
 {
-	return convert_to_working_tree_internal(path, src, len, dst, 0);
+	return convert_to_working_tree_internal(path, NULL, src, len, dst, 0);
+}
+
+int convert_to_working_tree_filter_to_file(const char *path, const char *destpath, const char *src, size_t len)
+{
+	struct strbuf output = STRBUF_INIT;
+	int ret = convert_to_working_tree_internal(path, destpath, src, len, &output, 0);
+	/* The smudgeToFile filter stdout is not used. */
+	strbuf_release(&output);
+	return ret;
 }
 
 int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst)
 {
-	int ret = convert_to_working_tree_internal(path, src, len, dst, 1);
+	int ret = convert_to_working_tree_internal(path, NULL, src, len, dst, 1);
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;
diff --git a/convert.h b/convert.h
index ccf436b..53e1474 100644
--- a/convert.h
+++ b/convert.h
@@ -41,6 +41,10 @@ extern int convert_to_git(const char *path, const char *src, size_t len,
 			  struct strbuf *dst, enum safe_crlf checksafe);
 extern int convert_to_working_tree(const char *path, const char *src,
 				   size_t len, struct strbuf *dst);
+extern int convert_to_working_tree_filter_to_file(const char *path,
+						  const char *destpath,
+						  const char *src,
+						  size_t len);
 extern int renormalize_buffer(const char *path, const char *src, size_t len,
 			      struct strbuf *dst);
 static inline int would_convert_to_git(const char *path)
@@ -52,6 +56,12 @@ extern void convert_to_git_filter_fd(const char *path, int fd,
 				     struct strbuf *dst,
 				     enum safe_crlf checksafe);
 extern int would_convert_to_git_filter_fd(const char *path);
+/* Precondition: can_clean_from_file(path) == true */
+extern void convert_to_git_filter_from_file(const char *path,
+					    struct strbuf *dst,
+					    enum safe_crlf checksafe);
+extern int can_clean_from_file(const char *path);
+extern int can_smudge_to_file(const char *path);
 
 /*****************************************************************
  *
-- 
2.8.1

[PATCH v2 2/4] use cleanFromFile in git add

[Joey Hess]

Includes test cases.

Signed-off-by: Joey Hess <joeyh@joeyh.name>
---
 sha1_file.c           | 42 ++++++++++++++++++++++++++++++++++++------
 t/t0021-conversion.sh | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index d5e1121..8df86a0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3329,6 +3329,29 @@ static int index_stream_convert_blob(unsigned char *sha1, int fd,
 	return ret;
 }
 
+static int index_from_file_convert_blob(unsigned char *sha1,
+				      const char *path, unsigned flags)
+{
+	int ret;
+	const int write_object = flags & HASH_WRITE_OBJECT;
+	struct strbuf sbuf = STRBUF_INIT;
+
+	assert(path);
+	assert(can_clean_from_file(path));
+
+	convert_to_git_filter_from_file(path, &sbuf,
+				 write_object ? safe_crlf : SAFE_CRLF_FALSE);
+
+	if (write_object)
+		ret = write_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),
+				      sha1);
+	else
+		ret = hash_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),
+				     sha1);
+	strbuf_release(&sbuf);
+	return ret;
+}
+
 static int index_pipe(unsigned char *sha1, int fd, enum object_type type,
 		      const char *path, unsigned flags)
 {
@@ -3421,12 +3444,19 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, unsigned
 
 	switch (st->st_mode & S_IFMT) {
 	case S_IFREG:
-		fd = open(path, O_RDONLY);
-		if (fd < 0)
-			return error_errno("open(\"%s\")", path);
-		if (index_fd(sha1, fd, st, OBJ_BLOB, path, flags) < 0)
-			return error("%s: failed to insert into database",
-				     path);
+		if (can_clean_from_file(path)) {
+			if (index_from_file_convert_blob(sha1, path, flags) < 0)
+				return error("%s: failed to insert into database",
+					     path);
+		}
+		else {
+			fd = open(path, O_RDONLY);
+			if (fd < 0)
+				return error_errno("open(\"%s\")", path);
+			if (index_fd(sha1, fd, st, OBJ_BLOB, path, flags) < 0)
+				return error("%s: failed to insert into database",
+					     path);
+		}
 		break;
 	case S_IFLNK:
 		if (strbuf_readlink(&sb, path, st->st_size))
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 7bac2bc..399f92b 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -12,6 +12,14 @@ tr \
 EOF
 chmod +x rot13.sh
 
+cat <<EOF >rot13-from-file.sh
+#!$SHELL_PATH
+fsfile="\$1"
+touch rot13-from-file.ran
+cat "\$fsfile" | ./rot13.sh
+EOF
+chmod +x rot13-from-file.sh
+
 test_expect_success setup '
 	git config filter.rot13.smudge ./rot13.sh &&
 	git config filter.rot13.clean ./rot13.sh &&
@@ -268,4 +276,32 @@ test_expect_success 'disable filter with empty override' '
 	test_must_be_empty err
 '
 
+test_expect_success 'cleanFromFile filter is used when adding a file' '
+	test_config filter.rot13.cleanFromFile ./rot13-from-file.sh &&
+
+	echo "*.t filter=rot13" >.gitattributes &&
+
+	cat test.t >fstest.t &&
+	git add fstest.t &&
+	test -e rot13-from-file.ran &&
+	rm -f rot13-from-file.ran &&
+
+	rm -f fstest.t &&
+	git checkout -- fstest.t &&
+	cmp test.t fstest.t
+'
+
+test_expect_success 'cleanFromFile filter is not used when clean filter is not configured' '
+	test_config filter.noclean.smudge ./rot13.sh &&
+	test_config filter.noclean.cleanFromFile ./rot13-from-file.sh &&
+
+	echo "*.no filter=noclean" >.gitattributes &&
+
+	cat test.t >test.no &&
+	git add test.no &&
+	test ! -e rot13-from-file.ran &&
+	git cat-file blob :test.no >actual &&
+	cmp test.t actual
+'
+
 test_done
-- 
2.8.1

[PATCH v2 3/4] use smudgeToFile in git checkout etc

[Joey Hess]

This makes git checkout, git reset, etc use smudgeToFile.

Includes test cases.

(There's a call to convert_to_working_tree in merge-recursive.c
that could also be made to use smudgeToFile as well.)

Signed-off-by: Joey Hess <joeyh@joeyh.name>
---
 entry.c               | 37 +++++++++++++++++++++++++++++--------
 t/t0021-conversion.sh | 38 +++++++++++++++++++++++++++++++++-----
 2 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/entry.c b/entry.c
index 519e042..97975e5 100644
--- a/entry.c
+++ b/entry.c
@@ -175,8 +175,13 @@ static int write_entry(struct cache_entry *ce,
 
 		/*
 		 * Convert from git internal format to working tree format
+		 * unless the smudgeToFile filter can write to the
+		 * file directly.
 		 */
-		if (ce_mode_s_ifmt == S_IFREG &&
+		int regular_file = ce_mode_s_ifmt == S_IFREG;
+		int smudge_to_file = regular_file
+			&& can_smudge_to_file(ce->name);
+		if (regular_file && ! smudge_to_file &&
 		    convert_to_working_tree(ce->name, new, size, &buf)) {
 			free(new);
 			new = strbuf_detach(&buf, &newsize);
@@ -189,13 +194,29 @@ static int write_entry(struct cache_entry *ce,
 			return error_errno("unable to create file %s", path);
 		}
 
-		wrote = write_in_full(fd, new, size);
-		if (!to_tempfile)
-			fstat_done = fstat_output(fd, state, &st);
-		close(fd);
-		free(new);
-		if (wrote != size)
-			return error("unable to write file %s", path);
+		if (! smudge_to_file) {
+			wrote = write_in_full(fd, new, size);
+			if (!to_tempfile)
+				fstat_done = fstat_output(fd, state, &st);
+			close(fd);
+			free(new);
+			if (wrote != size)
+				return error("unable to write file %s", path);
+		}
+		else {
+			close(fd);
+			convert_to_working_tree_filter_to_file(ce->name, path, new, size);
+			free(new);
+			/* The smudgeToFile filter may have replaced the
+			 * file; open it to make sure that the file
+			 * exists. */
+			fd = open(path, O_RDONLY);
+			if (fd < 0)
+				return error_errno("unable to create file %s", path);
+			if (!to_tempfile)
+				fstat_done = fstat_output(fd, state, &st);
+			close(fd);
+		}
 		break;
 	case S_IFGITLINK:
 		if (to_tempfile)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 399f92b..a8042d1 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -14,12 +14,20 @@ chmod +x rot13.sh
 
 cat <<EOF >rot13-from-file.sh
 #!$SHELL_PATH
-fsfile="\$1"
+srcfile="\$1"
 touch rot13-from-file.ran
-cat "\$fsfile" | ./rot13.sh
+cat "\$srcfile" | ./rot13.sh
 EOF
 chmod +x rot13-from-file.sh
 
+cat <<EOF >rot13-to-file.sh
+#!$SHELL_PATH
+destfile="\$1"
+touch rot13-to-file.ran
+./rot13.sh > "\$destfile"
+EOF
+chmod +x rot13-to-file.sh
+
 test_expect_success setup '
 	git config filter.rot13.smudge ./rot13.sh &&
 	git config filter.rot13.clean ./rot13.sh &&
@@ -291,6 +299,17 @@ test_expect_success 'cleanFromFile filter is used when adding a file' '
 	cmp test.t fstest.t
 '
 
+test_expect_success 'smudgeToFile filter is used when checking out a file' '
+	test_config filter.rot13.smudgeToFile ./rot13-to-file.sh &&
+
+	rm -f fstest.t &&
+	git checkout -- fstest.t &&
+	cmp test.t fstest.t &&
+
+	test -e rot13-to-file.ran &&
+	rm -f rot13-to-file.ran
+'
+
 test_expect_success 'cleanFromFile filter is not used when clean filter is not configured' '
 	test_config filter.noclean.smudge ./rot13.sh &&
 	test_config filter.noclean.cleanFromFile ./rot13-from-file.sh &&
@@ -299,9 +318,18 @@ test_expect_success 'cleanFromFile filter is not used when clean filter is not c
 
 	cat test.t >test.no &&
 	git add test.no &&
-	test ! -e rot13-from-file.ran &&
-	git cat-file blob :test.no >actual &&
-	cmp test.t actual
+	test ! -e rot13-from-file.ran
+'
+
+test_expect_success 'smudgeToFile filter is not used when smudge filter is not configured' '
+	test_config filter.nosmudge.clean ./rot13.sh &&
+	test_config filter.nosmudge.smudgeToFile ./rot13-to-file.sh &&
+
+	echo "*.no filter=nosmudge" >.gitattributes &&
+
+	rm -f fstest.t &&
+	git checkout -- fstest.t &&
+	test ! -e rot13-to-file.ran
 '
 
 test_done
-- 
2.8.1

[PATCH v2 4/4] warn on unusable smudgeToFile/cleanFromFile config

[Joey Hess]

Let the user know when they have a smudgeToFile/cleanFromFile config
that cannot be used because the corresponding smudge/clean config
is missing.

The warning is only displayed a maximum of once per git invocation,
and only when doing an operation that would use the filter.

Signed-off-by: Joey Hess <joeyh@joeyh.name>
---
 convert.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/convert.c b/convert.c
index bf63ba0..84f6bc5 100644
--- a/convert.c
+++ b/convert.c
@@ -847,32 +847,50 @@ int would_convert_to_git_filter_fd(const char *path)
 	return apply_filter(path, NULL, NULL, 0, -1, NULL, ca.drv->clean);
 }
 
+static int can_filter_file(const char *filefilter, const char *filefiltername,
+			   const char *stdiofilter, const char *stdiofiltername,
+			   const struct conv_attrs *ca,
+			   int *warncount)
+{
+	if (! filefilter)
+		return 0;
+
+	if (stdiofilter)
+		return 1;
+
+	if (*warncount == 0)
+		warning("Not running your configured filter.%s.%s command, because filter.%s.%s is not configured",
+			ca->drv->name, filefiltername,
+			ca->drv->name, stdiofiltername);
+		*warncount=*warncount+1;
+	
+	return 0;
+}
+
 int can_clean_from_file(const char *path)
 {
 	struct conv_attrs ca;
+	static int warncount = 0;
 
 	convert_attrs(&ca, path);
 	if (!ca.drv)
 		return 0;
 
-	/* Only use the cleanFromFile filter when the clean filter is also
-	 * configured.
-	 */
-	return (ca.drv->clean_from_file && ca.drv->clean);
+	return can_filter_file(ca.drv->clean_from_file, "cleanFromFile",
+			       ca.drv->clean, "clean", &ca, &warncount);
 }
 
 int can_smudge_to_file(const char *path)
 {
 	struct conv_attrs ca;
+	static int warncount = 0;
 
 	convert_attrs(&ca, path);
 	if (!ca.drv)
 		return 0;
 
-	/* Only use the smudgeToFile filter when the smudge filter is also
-	 * configured.
-	 */
-	return (ca.drv->smudge_to_file && ca.drv->smudge);
+	return can_filter_file(ca.drv->smudge_to_file, "smudgeToFile",
+			       ca.drv->smudge, "smudge", &ca, &warncount);
 }
 
 const char *get_convert_attr_ascii(const char *path)
-- 
2.8.1

Re: [PATCH v2 0/4] extend smudge/clean filters with direct file access

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

Doing a quick benchmark of this new interface and git-annex's use of it, git
checkout of a 1 gigabyte file with git-annex providing the smudge filter took:

    19 seconds using the smudge interface
    11 seconds using smudgeToFile
    0.1 seconds with smudgeToFile and annex.thin set
         (while also saving 1 gb of disk space!)

So around 2x speed improvement due to not needing to pipe the file content
through the filter, even without git-annex's annex.thin tricks.

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH v2 0/4] extend smudge/clean filters with direct file access

[Junio C Hamano]

Joey Hess <joeyh@joeyh.name> writes:

> Reroll of this patch set with changes:

... where is this 4-patch series designed to apply?  The first one
already fails...

Applying: add smudgeToFile and cleanFromFile filter configs
.git/rebase-apply/patch:28: trailing whitespace.
	Similar to filter.<driver>.clean but the specified command 
.git/rebase-apply/patch:30: trailing whitespace.
	receiving the file content from standard input. 
.git/rebase-apply/patch:129: indent with spaces.
                        struct strbuf *dst, const char *cmd)
fatal: sha1 information is lacking or useless (Documentation/gitattributes.txt).
error: could not build fake ancestor
Patch failed at 0001 add smudgeToFile and cleanFromFile filter configs
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


>
> * Renamed the new filter drivers for consistency with other configs.
> * Improved documentation with feedback from Junio and others.
> * Eliminated %p and instead append the filename to the commands
>   (separated by a space).
> * Fixed an FD leak and a space leak.
> * Only use smudgeToFile with regular files, not symlinks.
> * After running the smudgeToFile command, double-check that the
>   expected file is present, in case the command was buggy and deleted it.
> * Added a warning message when the new filter commands are configured
>   but the old ones are not, so that the user knows it's refusing to use
>   their configuration.
>
> There's been good and helpful documentation and interface review,
> but some more code review would be good! Also, git-annex has a
> improved-smudge-filters branch now that demonstrates this interface.
>
> Joey Hess (4):
>   add smudgeToFile and cleanFromFile filter configs
>   use cleanFromFile in git add
>   use smudgeToFile in git checkout etc
>   warn on unusable smudgeToFile/cleanFromFile config
>
>  Documentation/config.txt        |  18 +++++-
>  Documentation/gitattributes.txt |  37 ++++++++++++
>  convert.c                       | 126 +++++++++++++++++++++++++++++++++++-----
>  convert.h                       |  10 ++++
>  entry.c                         |  37 +++++++++---
>  sha1_file.c                     |  42 ++++++++++++--
>  t/t0021-conversion.sh           |  64 ++++++++++++++++++++
>  7 files changed, 304 insertions(+), 30 deletions(-)

[PATCH v2 0/4] clarify %f documentation

[Joey Hess]

It's natural to expect %f to be an actual file on disk; help avoid that
mistake.

Signed-off-by: Joey Hess <joeyh@joeyh.name>
---

This patch series was meant to contain 5 patches; here's the missing
one. This patch will apply cleanly on top of v2.9.0.

 Documentation/gitattributes.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e3b1de8..145dd10 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -374,6 +374,11 @@ substitution.  For example:
 	smudge = git-p4-filter --smudge %f
 ------------------------
 
+Note that "%f" is the name of the path that is being worked on. Depending
+on the version that is being filtered, the corresponding file on disk may
+not exist, or may have different contents. So, smudge and clean commands
+should not try to access the file on disk, but only act as filters on the
+content provided to them on standard input.
 
 Interaction between checkin/checkout attributes
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-- 
2.8.1