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

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

[Joey Hess]

As discussed in this thread:
http://thread.gmane.org/gmane.comp.version-control.git/294425
This adds smudge-to-file and clean-from-file commands supplimenting the
smudge and clean filters.

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 clean-from-file commands that avoid
reading the whole content of the file, and for smudge-to-file commands that
populate a work tree file using an efficient Copy On Write operation.

Joey Hess (4):
  clarify %f documentation
  add smudge-to-file and clean-from-file filter configuration
  use clean-from-file in git add
  use smudge-to-file in git checkout etc

 Documentation/config.txt        |  27 +++++++---
 Documentation/gitattributes.txt |  44 ++++++++++++++++-
 convert.c                       | 107 ++++++++++++++++++++++++++++++++++------
 convert.h                       |  10 ++++
 entry.c                         |  34 ++++++++++---
 sha1_file.c                     |  42 +++++++++++++---
 t/t0021-conversion.sh           |  64 ++++++++++++++++++++++++
 7 files changed, 292 insertions(+), 36 deletions(-)

-- 
2.9.0.4.g2856e74.dirty

[PATCH 1/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>
---
 Documentation/gitattributes.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e3b1de8..e077cc9 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -365,8 +365,8 @@ you can declare that the filter is `required`, in the configuration:
 ------------------------
 
 Sequence "%f" on the filter command line is replaced with the name of
-the file the filter is working on.  A filter might use this in keyword
-substitution.  For example:
+the file in the git repository the filter is working on.
+A filter might use this in keyword substitution.  For example:
 
 ------------------------
 [filter "p4"]
@@ -374,6 +374,9 @@ substitution.  For example:
 	smudge = git-p4-filter --smudge %f
 ------------------------
 
+Note that the "%f" is the name of a file in the git repository; the
+corresponding file on disk may not exist, or may have unrelated contents to
+what git is filtering.
 
 Interaction between checkin/checkout attributes
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-- 
2.9.0.4.g2856e74.dirty

Re: [PATCH 1/4] clarify %f documentation

[Junio C Hamano]

Joey Hess <joeyh@joeyh.name> writes:

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

I agree that "the name of the file" can be interpreted in many ways,
and I agree that it would be a good idea to find a better phrase to
name the path that is being worked on, but I do not think "the file
in the git repository" is that phrase.  When I first read the
updated text without the above two lines in the log message, I
thought "hmph, so we may use a temporary file somewhere in $GIT_DIR/
and that would be pointed at by %f, not the actual path we are
working on???", i.e. the rephrasing had exactly the opposite effect
on me.

Given that this being part of gitattributes(5) that begins with

    A `gitattributes` file is a simple text file that gives
    `attributes` to pathnames.

    Each line in `gitattributes` file is of form:

            pattern	attr1 attr2 ...

    That is, a pattern followed by an attributes list,
    separated by whitespaces.  When the pattern matches the
    path in question, the attributes listed on the line are given to
    the path.

and that the readers already read something like this in the
paragraphs before the mention of '%f':

    For example, in .gitattributes, you would assign the `filter`
    attribute for paths.

    ------------------------
    *.c	filter=indent
    ------------------------

    Then you would define ...

I think using the word "path" somewhere in the updated description
is more likely to have the effect you desire.


> Signed-off-by: Joey Hess <joeyh@joeyh.name>
> ---
>  Documentation/gitattributes.txt | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index e3b1de8..e077cc9 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -365,8 +365,8 @@ you can declare that the filter is `required`, in the configuration:
>  ------------------------
>  
>  Sequence "%f" on the filter command line is replaced with the name of
> -the file the filter is working on.  A filter might use this in keyword
> -substitution.  For example:
> +the file in the git repository the filter is working on.
> +A filter might use this in keyword substitution.  For example:
>  
>  ------------------------
>  [filter "p4"]
> @@ -374,6 +374,9 @@ substitution.  For example:
>  	smudge = git-p4-filter --smudge %f
>  ------------------------
>  
> +Note that the "%f" is the name of a file in the git repository; the
> +corresponding file on disk may not exist, or may have unrelated contents to
> +what git is filtering.
>  
>  Interaction between checkin/checkout attributes
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Re: [PATCH 1/4] clarify %f documentation

[Joey Hess]

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

Junio C Hamano wrote:
> I agree that "the name of the file" can be interpreted in many ways,
> and I agree that it would be a good idea to find a better phrase to
> name the path that is being worked on, but I do not think "the file
> in the git repository" is that phrase.

> I think using the word "path" somewhere in the updated description
> is more likely to have the effect you desire.

"path" is also very ambiguous. I see that "tracked" is often used to
describe what %f is, so how about:
 
+ Note that "%f" is the name of a file tracked by Git. 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.

-- 
see shy jo

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

Re: [PATCH 1/4] clarify %f documentation

[Junio C Hamano]

Joey Hess <id@joeyh.name> writes:

> Junio C Hamano wrote:
>> I agree that "the name of the file" can be interpreted in many ways,
>> and I agree that it would be a good idea to find a better phrase to
>> name the path that is being worked on, but I do not think "the file
>> in the git repository" is that phrase.
>
>> I think using the word "path" somewhere in the updated description
>> is more likely to have the effect you desire.
>
> "path" is also very ambiguous. I see that "tracked" is often used to
> describe what %f is, so how about:
>  
> + Note that "%f" is the name of a file tracked by Git. 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.

I think that places stress on a wrong point.

I do have a preference between "file" or "path", merely because, as
I showed already (go back and read what you are responding to), the
preceding paragraphs all talk in terms of "paths".  But that is not
the important part.  

"tracked by Git" is not all that interesting, compared to the fact
that your filter needs to give contents relevant to that path
because that is what the command line argument Git gives you with
'%f' means.  It is not a random filename "tracked by Git".  Among 47
other files tracked by Git, the single one being given is the one
the code that drives the filter is WORKING ON, and I think that
needs to be written in the description, hence "the path that is
being worked on" was my suggestion.

Re: [PATCH 1/4] clarify %f documentation

[Joey Hess]

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

Junio C Hamano wrote:
> "tracked by Git" is not all that interesting, compared to the fact
> that your filter needs to give contents relevant to that path
> because that is what the command line argument Git gives you with
> '%f' means.  It is not a random filename "tracked by Git".  Among 47
> other files tracked by Git, the single one being given is the one
> the code that drives the filter is WORKING ON, and I think that
> needs to be written in the description, hence "the path that is
> being worked on" was my suggestion.

Ah, "being worked on" does clarify it well, I think:

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

-- 
see shy jo

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

Re: [PATCH 1/4] clarify %f documentation

[Junio C Hamano]

Joey Hess <id@joeyh.name> writes:

> Junio C Hamano wrote:
>> "tracked by Git" is not all that interesting, compared to the fact
>> that your filter needs to give contents relevant to that path
>> because that is what the command line argument Git gives you with
>> '%f' means.  It is not a random filename "tracked by Git".  Among 47
>> other files tracked by Git, the single one being given is the one
>> the code that drives the filter is WORKING ON, and I think that
>> needs to be written in the description, hence "the path that is
>> being worked on" was my suggestion.
>
> Ah, "being worked on" does clarify it well, I think:
>
> + 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.

Looks much better than the original text.  Thanks.

Another reason why you do not want to say "tracked" is for "git add"
that adds a brand-new file that is not yet tracked.  Technically,
"trackable file" may work but we have to add "trackable" to the
glossary, but so far we did without having to, and I'm reluctant to
invent yet another word, so...

[PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Joey Hess]

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

In smudge-to-file and clean-from-file, "%p" is expanded to the path to the
file that should be cleaned from, or smudged to.

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 clean-from-file commands that avoid
reading the whole content of the file, and for smudge-to-file 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 smudge-to-file should also provide a smudge
command to be used in cases where smudge-to-file is not used. And ditto
with clean-from-file 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        |  27 +++++++---
 Documentation/gitattributes.txt |  37 ++++++++++++++
 convert.c                       | 107 ++++++++++++++++++++++++++++++++++------
 convert.h                       |  10 ++++
 4 files changed, 160 insertions(+), 21 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e1b2e4..bbb9296 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1299,14 +1299,29 @@ format.useAutoBase::
 	format-patch by default.
 
 filter.<driver>.clean::
-	The command which is used to convert the content of a worktree
-	file to a blob upon checkin.  See linkgit:gitattributes[5] for
-	details.
+	The command which is used on checkin to convert the content of
+	a worktree file (provided on stdin) to a blob (written to stdout).
+	See linkgit:gitattributes[5] for details.
 
 filter.<driver>.smudge::
-	The command which is used to convert the content of a blob
-	object to a worktree file upon checkout.  See
-	linkgit:gitattributes[5] for details.
+	The command which is used on checkout to convert the content of a
+	blob object (provided on stdin) to a worktree file (written to
+	stdout).
+	See linkgit:gitattributes[5] for details.
+
+filter.<driver>.clean-from-file::
+	Optional command which is used on checkin to convert the content
+	of a worktree file, which can be read from disk, to a blob
+	(written to stdout).
+	Only used when filter.<driver>.clean is also configured.
+	See linkgit:gitattributes[5] for details.
+
+filter.<driver>.smudge-to-file::
+	Optional command which is used to convert the content of a blob
+	object (provided on stdin) to a worktree file, writing directly
+	to the file.
+	Only used when filter.<driver>.clean is also configured.
+	See linkgit:gitattributes[5] for details.
 
 fsck.<msg-id>::
 	Allows overriding the message type (error, warn or ignore) of a
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e077cc9..32621e7 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -378,6 +378,43 @@ Note that the "%f" is the name of a file in the git repository; the
 corresponding file on disk may not exist, or may have unrelated contents to
 what git is filtering.
 
+There are two extra commands "clean-from-file" and "smudge-to-file", 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 reading/writing files in the filesystem.
+This can be more efficient when using filters with large files that are not
+directly stored in the repository.
+
+Sequence "%p" on the "clean-from-file" and "smudge-to-file" command line
+is replaced with the path to a file that the filter can access.
+
+In the "clean-from-file" command, "%p" is the path to the file that
+it should clean. Like the "clean" command, it should output the cleaned
+version to standard output.
+
+In the "smudge-from-file" command, "%p" is the 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, "smudge-from-file"
+is fed the blob object from its standard input.
+
+Some git operations that need to apply filters cannot use "clean-from-file"
+and "smudge-to-file", since the files are not present to disk. So, to avoid
+inconsistent behavior, "clean-from-file" will only be used if "clean" is
+also configured, and "smudge-to-file" will only be used if "smudge" is also
+configured.
+
+An example large file storage filter driver using "*-from-file" follows:
+
+------------------------
+[filter "bigfiles"]
+	clean = store-bigfile --from-stdin
+	clean-from-file = store-bigfile %p
+	smudge = retrieve-bigfile --to-stdout
+	smudge-to-file = retrieve-bigfile %p
+	required
+
+------------------------
+
 Interaction between checkin/checkout attributes
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/convert.c b/convert.c
index b1614bf..bfb7578 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)
@@ -376,16 +377,22 @@ static int filter_buffer_or_fd(int in, int out, void *data)
 	/* apply % substitution to cmd */
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
+	struct strbuf fspath = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[] = {
 		{ "f", NULL, },
+		{ (params->fspath ? "p" : NULL), NULL, },
 		{ NULL, NULL, },
 	};
 
 	/* quote the path to preserve spaces, etc. */
 	sq_quote_buf(&path, params->path);
 	dict[0].value = path.buf;
+	if (params->fspath) {
+		sq_quote_buf(&fspath, params->fspath);
+		dict[1].value = fspath.buf;
+	}
 
-	/* expand all %f with the quoted path */
+	/* expand all %f and %p with the quoted path and fspath */
 	strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);
 	strbuf_release(&path);
 
@@ -427,7 +434,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 +464,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 +495,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 +523,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>.smudge-to-file, filter.<name>.clean-from-file
+	 * specifies the command line:
 	 *
 	 *	command-line
 	 *
@@ -526,6 +538,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("smudge-to-file", key))
+		return git_config_string(&drv->smudge_to_file, var, value);
+
+	if (!strcmp("clean-from-file", 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 +841,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 clean-from-file 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 smudge-to-file 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 +912,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 +937,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: clean-from-file 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 +974,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 +988,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 +998,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 ? "smudge-to-file" : "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 smudge-to-file 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.9.0.4.g2856e74.dirty

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Junio C Hamano]

Joey Hess <joeyh@joeyh.name> writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e1b2e4..bbb9296 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1299,14 +1299,29 @@ format.useAutoBase::
>  	format-patch by default.
>  
>  filter.<driver>.clean::
> -	The command which is used to convert the content of a worktree
> -	file to a blob upon checkin.  See linkgit:gitattributes[5] for
> -	details.
> +	The command which is used on checkin to convert the content of
> +	a worktree file (provided on stdin) to a blob (written to stdout).
> +	See linkgit:gitattributes[5] for details.
>  
>  filter.<driver>.smudge::
> -	The command which is used to convert the content of a blob
> -	object to a worktree file upon checkout.  See
> -	linkgit:gitattributes[5] for details.
> +	The command which is used on checkout to convert the content of a
> +	blob object (provided on stdin) to a worktree file (written to
> +	stdout).
> +	See linkgit:gitattributes[5] for details.

A "filter" by definition reads from its standard input and writes
the result to its standard output, so I do not know if this change
is necessary.  If you are going to do this, in documentation
(i.e. not code), please spell standard input/output out, and avoid
stdin/stdout.

There is what we would want to fix, though.  "worktree file" should
be spelled "working tree file".  This used not to matter before "git
worktree" was invented (before that we used these two terms
interchangeably), but these days the distinction matters.

> +filter.<driver>.clean-from-file::

Documentation/config.txt hopefully lists all the configuration, but
I do not see anything that uses 'words-joined-with-dash' format.
Please do not invent new out-of-convention names.

> +	Optional command which is used on checkin to convert the content
> +	of a worktree file, which can be read from disk, to a blob
> +	(written to stdout).

I am not sure why we want to say "which can be read from disk".

I think what a reader needs to be told (but this paragraph is not
telling her) in order to understand what you meant to say is:

	cleanFromFile is asked to produce the "cleaned" content to
	its standard output (to be stored in the object database),
	but unlike clean, it does not work as a filter and is not
	given a file descriptor to read the working tree contents
	from.  Instead, it is told the path for which the contents
	need to be generated as the first parameter on its command
	line.

(the above is deliberately made verbose and is not meant as a
suggestion to be literally used in your updated documentation).

With that understanding, the reader may be able to guess that "can
be read from disk" is a permission for her cleanFromFile filter
(i.e. it does not necessarily have to read from it and produced its
output by some other magic); otherwise it can be misread as if the
content of a working tree file is deposited on the disk to enable
the filter to read it, but the location of that on-disk file is
somewhere unspecified by this paragraph.

> +	Only used when filter.<driver>.clean is also configured.
> +	See linkgit:gitattributes[5] for details.
> +
> +filter.<driver>.smudge-to-file::
> +	Optional command which is used to convert the content of a blob
> +	object (provided on stdin) to a worktree file, writing directly
> +	to the file.

A similar comment applies.  With ", writing directly to the file."
replaced with something like ". The command is expected to write to
the working tree file at path given as the first parameter on the
command line." it would be sufficient (i.e. it is clear enough that
it does not give the smudged contents via its standard output, and
no need to say "unlike smudge filter, ..." like we needed to for the
"clean" side).

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Joey Hess]

Junio C Hamano wrote:
> There is what we would want to fix, though.  "worktree file" should
> be spelled "working tree file". This used not to matter before "git
> worktree" was invented (before that we used these two terms
> interchangeably), but these days the distinction matters.

The existing documentation that I am patching uses the term "worktree
file" which is why I continued to use that wording.

(Unless this is a documentation transition that you want to happen
peicemeal as documentation is updated for other reasons?)

> > +filter.<driver>.clean-from-file::
> 
> Documentation/config.txt hopefully lists all the configuration, but
> I do not see anything that uses 'words-joined-with-dash' format.
> Please do not invent new out-of-convention names.

Point taken; I'll use cleanFromFile and smudgeToFile.

Here's a revised version of the documentation that I think takes the other
suggestions onboard. I emphasise that clean and smudge operate as filters,
to contrast better with cleanFromFile and smudgeToFile not operating as
regular stdio filters.

 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. 
+       In the command, "%p" is replaced with the name of the file. 
+       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.
+       In the command, "%p" is replaced with the name of the file.
+       Only used when filter.<driver>.smudge is also configured.
+       See linkgit:gitattributes[5] for details.
+

This could be extended more, but I think this should describe the config
settings concisely and point to the more involved discussion of filter drivers
in gitattributes.

-- 
see shy jo

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Junio C Hamano]

Joey Hess <id@joeyh.name> writes:

> Here's a revised version of the documentation that I think takes the other
> suggestions onboard. I emphasise that clean and smudge operate as filters,
> to contrast better with cleanFromFile and smudgeToFile not operating as
> regular stdio filters.
> ...
> This could be extended more, but I think this should describe the config
> settings concisely and point to the more involved discussion of filter drivers
> in gitattributes.

Yup.  This version I can agree with.

Thanks.

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Eric Sunshine]

On Thu, Jun 16, 2016 at 4:32 PM, Joey Hess <joeyh@joeyh.name> wrote:
> This adds new smudge-to-file and clean-from-file filter commands,
> which are similar to smudge and clean but allow direct access to files on
> disk.
> [...]
> Signed-off-by: Joey Hess <joeyh@joeyh.name>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1299,14 +1299,29 @@ format.useAutoBase::
> +filter.<driver>.clean-from-file::
> +       Optional command which is used on checkin to convert the content
> +       of a worktree file, which can be read from disk, to a blob
> +       (written to stdout).
> +       Only used when filter.<driver>.clean is also configured.
> +       See linkgit:gitattributes[5] for details.
> +
> +filter.<driver>.smudge-to-file::
> +       Optional command which is used to convert the content of a blob
> +       object (provided on stdin) to a worktree file, writing directly
> +       to the file.
> +       Only used when filter.<driver>.clean is also configured.

s/clean/smudge/

> +       See linkgit:gitattributes[5] for details.

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Michael J Gruber]

Joey Hess venit, vidit, dixit 16.06.2016 22:32:
> This adds new smudge-to-file and clean-from-file filter commands,
> which are similar to smudge and clean but allow direct access to files on
> disk.
> 
> In smudge-to-file and clean-from-file, "%p" is expanded to the path to the
> file that should be cleaned from, or smudged to.
> 
> 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 clean-from-file commands that avoid
> reading the whole content of the file, and for smudge-to-file 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 smudge-to-file should also provide a smudge
> command to be used in cases where smudge-to-file is not used. And ditto
> with clean-from-file and clean. To avoid foot-shooting configurations, the
> new commands are not used unless the old commands are also configured.

I'm not sure this will save all feet. I foresee "why is smudge-to-file
not doing anything" reports...

In addition, it opens the way to doing completely different things in
smudge and smudge-to-file - which partly is intentional, of course.

Do you make any promises that %p is a seekable file?

> 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        |  27 +++++++---
>  Documentation/gitattributes.txt |  37 ++++++++++++++
>  convert.c                       | 107 ++++++++++++++++++++++++++++++++++------
>  convert.h                       |  10 ++++
>  4 files changed, 160 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e1b2e4..bbb9296 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1299,14 +1299,29 @@ format.useAutoBase::
>  	format-patch by default.
>  
>  filter.<driver>.clean::
> -	The command which is used to convert the content of a worktree
> -	file to a blob upon checkin.  See linkgit:gitattributes[5] for
> -	details.
> +	The command which is used on checkin to convert the content of
> +	a worktree file (provided on stdin) to a blob (written to stdout).
> +	See linkgit:gitattributes[5] for details.
>  
>  filter.<driver>.smudge::
> -	The command which is used to convert the content of a blob
> -	object to a worktree file upon checkout.  See
> -	linkgit:gitattributes[5] for details.
> +	The command which is used on checkout to convert the content of a
> +	blob object (provided on stdin) to a worktree file (written to
> +	stdout).
> +	See linkgit:gitattributes[5] for details.
> +
> +filter.<driver>.clean-from-file::
> +	Optional command which is used on checkin to convert the content
> +	of a worktree file, which can be read from disk, to a blob
> +	(written to stdout).
> +	Only used when filter.<driver>.clean is also configured.
> +	See linkgit:gitattributes[5] for details.
> +
> +filter.<driver>.smudge-to-file::
> +	Optional command which is used to convert the content of a blob
> +	object (provided on stdin) to a worktree file, writing directly
> +	to the file.
> +	Only used when filter.<driver>.clean is also configured.
> +	See linkgit:gitattributes[5] for details.
>  
>  fsck.<msg-id>::
>  	Allows overriding the message type (error, warn or ignore) of a
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index e077cc9..32621e7 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -378,6 +378,43 @@ Note that the "%f" is the name of a file in the git repository; the
>  corresponding file on disk may not exist, or may have unrelated contents to
>  what git is filtering.
>  
> +There are two extra commands "clean-from-file" and "smudge-to-file", 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 reading/writing files in the filesystem.
> +This can be more efficient when using filters with large files that are not
> +directly stored in the repository.
> +
> +Sequence "%p" on the "clean-from-file" and "smudge-to-file" command line
> +is replaced with the path to a file that the filter can access.
> +
> +In the "clean-from-file" command, "%p" is the path to the file that
> +it should clean. Like the "clean" command, it should output the cleaned
> +version to standard output.
> +
> +In the "smudge-from-file" command, "%p" is the 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, "smudge-from-file"
> +is fed the blob object from its standard input.
> +
> +Some git operations that need to apply filters cannot use "clean-from-file"
> +and "smudge-to-file", since the files are not present to disk. So, to avoid
> +inconsistent behavior, "clean-from-file" will only be used if "clean" is
> +also configured, and "smudge-to-file" will only be used if "smudge" is also
> +configured.
> +
> +An example large file storage filter driver using "*-from-file" follows:
> +
> +------------------------
> +[filter "bigfiles"]
> +	clean = store-bigfile --from-stdin
> +	clean-from-file = store-bigfile %p
> +	smudge = retrieve-bigfile --to-stdout
> +	smudge-to-file = retrieve-bigfile %p
> +	required
> +
> +------------------------
> +
>  Interaction between checkin/checkout attributes
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  
> diff --git a/convert.c b/convert.c
> index b1614bf..bfb7578 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)
> @@ -376,16 +377,22 @@ static int filter_buffer_or_fd(int in, int out, void *data)
>  	/* apply % substitution to cmd */
>  	struct strbuf cmd = STRBUF_INIT;
>  	struct strbuf path = STRBUF_INIT;
> +	struct strbuf fspath = STRBUF_INIT;
>  	struct strbuf_expand_dict_entry dict[] = {
>  		{ "f", NULL, },
> +		{ (params->fspath ? "p" : NULL), NULL, },
>  		{ NULL, NULL, },
>  	};
>  
>  	/* quote the path to preserve spaces, etc. */
>  	sq_quote_buf(&path, params->path);
>  	dict[0].value = path.buf;
> +	if (params->fspath) {
> +		sq_quote_buf(&fspath, params->fspath);
> +		dict[1].value = fspath.buf;
> +	}
>  
> -	/* expand all %f with the quoted path */
> +	/* expand all %f and %p with the quoted path and fspath */
>  	strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);
>  	strbuf_release(&path);
>  
> @@ -427,7 +434,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 +464,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 +495,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 +523,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>.smudge-to-file, filter.<name>.clean-from-file
> +	 * specifies the command line:
>  	 *
>  	 *	command-line
>  	 *
> @@ -526,6 +538,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("smudge-to-file", key))
> +		return git_config_string(&drv->smudge_to_file, var, value);
> +
> +	if (!strcmp("clean-from-file", 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 +841,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 clean-from-file 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 smudge-to-file 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 +912,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 +937,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: clean-from-file 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 +974,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 +988,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 +998,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 ? "smudge-to-file" : "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 smudge-to-file 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);
>  
>  /*****************************************************************
>   *
> 

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Joey Hess]

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

Michael J Gruber wrote:
> I'm not sure this will save all feet. I foresee "why is smudge-to-file
> not doing anything" reports...

It could display a warning message if smudge-to-file is set and smudge is
not.

> In addition, it opens the way to doing completely different things in
> smudge and smudge-to-file - which partly is intentional, of course.

They can be implemented very differently, but need to provide the same
file content. Otherwise git checkout and git diff would show different
content for the same file, for example. This is up to the implementor of
a filter driver to get right.

> Do you make any promises that %p is a seekable file?

Yes, %p is a regular file and so is seekable, statable, etc.

-- 
see shy jo

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

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Junio C Hamano]

Joey Hess <id@joeyh.name> writes:

>> Do you make any promises that %p is a seekable file?
>
> Yes, %p is a regular file and so is seekable, statable, etc.

I wonder if we prefer not to make this customizable (i.e. not having
to use'%p').  Unlike '%f' that is optional, smudgeTo and cleanFrom
"filters" are not filters and MUST read from the path given and not
from any other paths.

A misconfigured smudgeTo filter that uses %f would overwrite a wrong
file when used with checkout-index --prefix=<there>, right?

The only thing '%p' buys is that a "filter" could be written in such
a way that takes the pathname at arbitrary place on the command
line.  A command line that does not have '%p' anywhere is invalid,
which is quite different from how '%f' behaves.

Would an interface that always appends the pathname at the end of
the command line string work?  E.g.

	[filter "foo"] smugeToFile = cmd --from-file

would run "cmd --from-file PATH" when Git wants it to read from
PATH, and

	[filter "bar"] smugeToFile = sh -c 'cmd --from-file="$0"'

would run

	sh -c 'cmd --from-file="$0"' PATH

which in turn becomes 'cmd --from-file=PATH'.

Or something like that.

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I wonder if we prefer not to make this customizable (i.e. not having
> to use'%p').  Unlike '%f' that is optional, smudgeTo and cleanFrom
> "filters" are not filters and MUST read from the path given and not
> from any other paths.
> ...
> Would an interface that always appends the pathname at the end of
> the command line string work?

Just to avoid misunderstanding, when I say "I wonder if we want to
go this route instead", I am open to a response that says "No, we do
not; going that route is worse because ...".  Please do not take it
as a suggestion or request to "do it this way instead".

I may not even favor that other "route" I mention in my message with
"I wonder"; I am merely saying that I cannot come up with that
"because ..."  part to reject it myself, and asking for help from
others.

In this case, I would be happy to hear "Yes, '%p' may merely be a
way to allow taking the pathname at anywhere on the command line,
but that flexibility is important because...".

Thanks.

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Joey Hess]

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

Junio C Hamano wrote:
> Would an interface that always appends the pathname at the end of
> the command line string work?

I'm ok with this, and like getting rid of %p as it's not distinguishable
from %f without reading the documentation.

The sh -c trick can of course be used if some other ordering of
parameters is needed. Probably anything using this interface is gonna be
implemented with the interface in mind from the beginning and won't need
such a trick.

-- 
see shy jo

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

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Joey Hess]

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

Junio C Hamano wrote:
> Would an interface that always appends the pathname at the end of
> the command line string work?

One problem with this is that "appends" is subtly unclear in this case.

With the example of smugeToFile = cmd --to-file
it seems that a space should be added by git before the filename.

On the other handle, consider smugeToFile = cmd --to-file=
here a space is not wanted before the filename.

So, either a space is automatically included before the filename
and the second example breaks, or no space is included, and
to make the first example work would need careful inclusion of the
trailing space with quoting to prevent it being elided
eg, smugeToFile = "cmd --to-file "

%p does avoid this ambiguity. But as Junio noted, %p is mandatory in the
command for it to possibly work. Git could refuse to use smugeToFile = cmd
as not containing a %p and so not possibly being able to work.

Or we could pick one of the two methods of appending the file
(I prefer not including a space before it as more flexible), and
anything using this interface would need to design its command line
parsing with this interface in mind, and would probably choose to use
--to-file=foo rather than --to-file foo.

-- 
see shy jo

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

Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

[Junio C Hamano]

Joey Hess <id@joeyh.name> writes:

> Or we could pick one of the two methods of appending the file
> (I prefer not including a space before it as more flexible), and
> anything using this interface would need to design its command line
> parsing with this interface in mind, and would probably choose to use
> --to-file=foo rather than --to-file foo.

When I gave the example in the message you are responding to, I
chose to assume that we will always have a space there, and this was
to to avoid a problem that will become common if we did otherwise,
namely:

    [filter "foo"] cleanFromFile = command

which must be spelled as

    [filter "foo"] cleanFromFile = "command "

to separate the command name with it argument if we didn't add the
space there.

I tend to agree with what you said in the other response, i.e.
"Probably anything using this interface is gonna be implemented with
the interface in mind from the beginning", so they will not have an
issue if the interface always added a space there, and there is the
"sh -c" thing if they really want to do something unusual.

[PATCH 3/4] use clean-from-file 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..1043ea5 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 'clean-from-file filter is used when adding a file' '
+	test_config filter.rot13.clean-from-file "./rot13-from-file.sh %p" &&
+
+	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 'clean-from-file filter is not used when clean filter is not configured' '
+	test_config filter.no.smudge ./rot13.sh &&
+	test_config filter.no.clean-from-file "./rot13-from-file.sh %p" &&
+
+	echo "*.no filter=no" >.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.9.0.4.g2856e74.dirty

[PATCH 4/4] use smudge-to-file in git checkout etc

[Joey Hess]

This makes git checkout, git reset, etc use smudge-to-file.

Includes test cases.

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

Signed-off-by: Joey Hess <joeyh@joeyh.name>
---
 entry.c               | 34 +++++++++++++++++++++++++++-------
 t/t0021-conversion.sh | 44 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/entry.c b/entry.c
index 519e042..6a23159 100644
--- a/entry.c
+++ b/entry.c
@@ -175,8 +175,12 @@ static int write_entry(struct cache_entry *ce,
 
 		/*
 		 * Convert from git internal format to working tree format
+		 * unless the smudge-to-file filter can write to the
+		 * file directly.
 		 */
+		int smudge_to_file = can_smudge_to_file(ce->name);
 		if (ce_mode_s_ifmt == S_IFREG &&
+		    ! smudge_to_file &&
 		    convert_to_working_tree(ce->name, new, size, &buf)) {
 			free(new);
 			new = strbuf_detach(&buf, &newsize);
@@ -189,13 +193,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 (! can_smudge_to_file(ce->name)) {
+			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);
+			if (!to_tempfile) {
+				/* Re-open the file to stat it; the
+				 * smudge-to-file filter may have replaced
+				 * the file. */
+				fd = open(path, O_RDONLY);
+				if (fd < 0) {
+					return error_errno("unable to create file %s", path);
+				}
+			}
+		}
 		break;
 	case S_IFGITLINK:
 		if (to_tempfile)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 1043ea5..b0e2e5e 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,17 +299,37 @@ test_expect_success 'clean-from-file filter is used when adding a file' '
 	cmp test.t fstest.t
 '
 
+test_expect_success 'smudge-to-file filter is used when checking out a file' '
+	test_config filter.rot13.smudge-to-file "./rot13-to-file.sh %p" &&
+
+	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 'clean-from-file filter is not used when clean filter is not configured' '
-	test_config filter.no.smudge ./rot13.sh &&
-	test_config filter.no.clean-from-file "./rot13-from-file.sh %p" &&
+	test_config filter.noclean.smudge ./rot13.sh &&
+	test_config filter.noclean.clean-from-file "./rot13-from-file.sh %p" &&
 
-	echo "*.no filter=no" >.gitattributes &&
+	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 ! -e rot13-from-file.ran
+'
+
+test_expect_success 'smudge-to-file filter is not used when smudge filter is not configured' '
+	test_config filter.nosmudge.clean ./rot13.sh &&
+	test_config filter.nosmudge.smudge-to-file "./rot13-to-file.sh %p" &&
+
+	echo "*.no filter=nosmudge" >.gitattributes &&
+
+	rm -f fstest.t &&
+	git checkout -- fstest.t &&
+	test ! -e rot13-to-file.ran
 '
 
 test_done
-- 
2.9.0.4.g2856e74.dirty

Re: [PATCH 4/4] use smudge-to-file in git checkout etc

[Joey Hess]

Joey Hess wrote:
> +		int smudge_to_file = can_smudge_to_file(ce->name);
>  		if (ce_mode_s_ifmt == S_IFREG &&
> +		    ! smudge_to_file &&
>  		    convert_to_working_tree(ce->name, new, size, &buf)) {
>  			free(new);
>  			new = strbuf_detach(&buf, &newsize);
> @@ -189,13 +193,29 @@ static int write_entry(struct cache_entry *ce,

> +		if (! can_smudge_to_file(ce->name)) {
> +		}
> +		else {
> +			close(fd);
> +			convert_to_working_tree_filter_to_file(ce->name, path, new, size);

Oops, I had meant to avoid using smudge-to-file when
e_mode_s_ifmt != S_IFREG, and forgot it in this patch, so it does the
wrong thing for symlinks.

I'll send an updated patch set fixing this after any other review.

-- 
see shy jo