Re: [PATCH] convert filter: supply path to external driver

Pete Wyckoff [off-list ref] writes:

Filtering to support keyword expansion may need the name of
the file being filtered.  In particular, to support p4 keywords
like

    $File: //depot/product/dir/script.sh $

the smudge filter needs to know the name of the file it is
smudging.

Add a "%s" conversion specifier to the gitattribute for filter.
It will be expanded with the path name to the file when invoking
the external filter command.

Signed-off-by: Pete Wyckoff <redacted>
---

pw@padd.com wrote on Sat, 18 Dec 2010 17:38 -0500:

quoted

I'm using git-p4 to import and work with upstream p4
repositories.  Some of the files are ktext, meaning they expect
expansion of $Id$ and similar identifiers.

Using the filter driver for this file, I can do the "clean" part
easily, but to calculate the "smudge" correctly, I need to know
the filename inside the filter driver.

This works fine for me.  It is backward compatible, and leaves
open the possibility of adding other % modifiers if we find
a need later.

This is not backward compatible for people who wanted to use '%' literal
on their filter command line for whatever reason, so please do not
advertise as such.  A fair argument you could make is "Even though this is
not strictly backward compatible, it is very unlikely that people passed a
literal % to their filter command line, and the benefit of being able to
give the pathname information would outweigh the downside of not being
compatible", and people can agree or disagree.

I am personally moderately negative about $any expansion$ (I don't use it
myself, and I don't think sane people use it either).  As far as I can
tell, this should has no impact on the correctness and very little impact
on the performance for people who do not use $any expansion$, so I am Ok
with the patch.

Modulo one worry.  Don't we have, or don't we at least plant to allow us
to have, a facility to cache expensive blob conversion result, similar to
the textconv caching?  How would this change interact with two blobs that
live in different paths?

On Sun, Dec 19, 2010 at 01:59:50PM -0800, Junio C Hamano wrote:

Modulo one worry.  Don't we have, or don't we at least plant to allow us
to have, a facility to cache expensive blob conversion result, similar to
the textconv caching?  How would this change interact with two blobs that
live in different paths?

Yeah, it has been talked about, but I don't think anyone is working on
it (I don't personally use clean/smudge at all, so it is not something I
have thought _that_ much about).

This does definitely complicate matters, as the filtering is no longer a
pure mapping of sha1->sha1. However, I think in practice we could do
just fine by using a multi-level lookup. I.e., mapping a sha1 to be
filtered into a tree. Each tree entry would represent the remaining
cache parameters. In this case, the only other parameter we have is the
path given to the filter (but it could easily be extended to include
other parameters, if they existed, in this or other caching cases).

We only get a high-performance lookup for the first part of the
multi-level (i.e., the sha1), but that's OK if we assume the number of
second-level items is going to be small. Which I think is the case here
(a sha1 will tend to be found only under one or a few names).

An alternative would be to combine all parts of the filter under a
single lookup key. E.g., calculate and store under sha1(sha1(blob) +
filename)). But that means the notes keys are not actual object sha1s,
which throws off pruning.

Anyway, that's just my quick thinking on the subject. I don't see any
reason to restrict a feature just because we might want to cache it in
the future. At the very worst, we could always cache filters which do
not use %s, and make only %s users pay the penalty.

-Peff

On Sun, 19 Dec 2010, Jeff King wrote:

On Sun, Dec 19, 2010 at 01:59:50PM -0800, Junio C Hamano wrote:

quoted

Modulo one worry.  Don't we have, or don't we at least plant to allow us
to have, a facility to cache expensive blob conversion result, similar to
the textconv caching?  How would this change interact with two blobs that
live in different paths?

Yeah, it has been talked about, but I don't think anyone is working on
it (I don't personally use clean/smudge at all, so it is not something I
have thought _that_ much about).

This does definitely complicate matters, as the filtering is no longer a
pure mapping of sha1->sha1. However, I think in practice we could do
just fine by using a multi-level lookup. I.e., mapping a sha1 to be
filtered into a tree. Each tree entry would represent the remaining
cache parameters. In this case, the only other parameter we have is the
path given to the filter (but it could easily be extended to include
other parameters, if they existed, in this or other caching cases).

We only get a high-performance lookup for the first part of the
multi-level (i.e., the sha1), but that's OK if we assume the number of
second-level items is going to be small. Which I think is the case here
(a sha1 will tend to be found only under one or a few names).

An alternative would be to combine all parts of the filter under a
single lookup key. E.g., calculate and store under sha1(sha1(blob) +
filename)). But that means the notes keys are not actual object sha1s,
which throws off pruning.

Anyway, that's just my quick thinking on the subject. I don't see any
reason to restrict a feature just because we might want to cache it in
the future. At the very worst, we could always cache filters which do
not use %s, and make only %s users pay the penalty.

or you could cache the stats of the filtered file, including any changes 
that are made.

David Lang

Filtering to support keyword expansion may need the name of
the file being filtered.  In particular, to support p4 keywords
like

    $File: //depot/product/dir/script.sh $

the smudge filter needs to know the name of the file it is
smudging.

Add a "%s" conversion specifier to the gitattribute for filter.
It will be expanded with the path name to the file when invoking
the external filter command.  The path name is quoted and
special characters are escaped to prevent the shell from splitting
incorrectly.

Signed-off-by: Pete Wyckoff <redacted>
---
gitster@pobox.com wrote on Sun, 19 Dec 2010 13:59 -0800:

This is not backward compatible for people who wanted to use '%' literal
on their filter command line for whatever reason, so please do not
advertise as such.  A fair argument you could make is "Even though this is
not strictly backward compatible, it is very unlikely that people passed a
literal % to their filter command line, and the benefit of being able to
give the pathname information would outweigh the downside of not being
compatible", and people can agree or disagree.

I overlooked that, but agree it is unlikely anyone was using % in
filter definitions.  Putting the path in an environment variable
is the other option I considered.

I am personally moderately negative about $any expansion$ (I don't use it
myself, and I don't think sane people use it either).  As far as I can
tell, this should has no impact on the correctness and very little impact
on the performance for people who do not use $any expansion$, so I am Ok
with the patch.

Keyword expansion is indeed nasty.  My only motivation is to
support working with an upstream that relies on it.

This version of the patch handles quoting of shell
meta-characters, as pointed out by Hannes.  I decided to invoke
sq_quote_buf directly on the path before expanding %s, rather
than writing a dict entry to understand %'s.  There is no
requirement for users to use single-quotes around %s in their
config files, this way, either.

Also added a test case to make sure %s and quoting works as
advertised.

		-- Pete

 Documentation/gitattributes.txt |   12 ++++++++++
 convert.c                       |   22 +++++++++++++++++-
 t/t0021-conversion.sh           |   47 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+), 1 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 564586b..9ac2138 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt

@@ -317,6 +317,18 @@ command is "cat").
 	smudge = cat
 ------------------------
 
+If your filter needs the path of the file it is working on,
+you can use the "%s" conversion specification.  It will be
+replaced with the relative path to the file.  This is important
+for keyword substitution that depends on the name of the
+file.  Like this:
+
+------------------------
+[filter "p4"]
+	clean = git-p4-filter --clean %s
+	smudge = git-p4-filter --smudge %s
+------------------------
+
 
 Interaction between checkin/checkout attributes
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

diff --git a/convert.c b/convert.c
index e41a31e..1ef83a0 100644
--- a/convert.c
+++ b/convert.c

@@ -317,6 +317,7 @@ struct filter_params {
 	const char *src;
 	unsigned long size;
 	const char *cmd;
+	const char *path;
 };
 
 static int filter_buffer(int in, int out, void *data)

@@ -329,7 +330,23 @@ static int filter_buffer(int in, int out, void *data)
 	int write_err, status;
 	const char *argv[] = { NULL, NULL };
 
-	argv[0] = params->cmd;
+	/* replace optional %s with path */
+	struct strbuf cmd = STRBUF_INIT;
+	struct strbuf path = STRBUF_INIT;
+	struct strbuf_expand_dict_entry dict[] = {
+	    "s", NULL,
+	    NULL, NULL,
+	};
+
+	/* quote the path to preserve spaces, etc. */
+	sq_quote_buf(&path, params->path);
+	dict[0].value = path.buf;
+
+	/* expand all %s with the quoted path */
+	strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);
+	strbuf_release(&path);
+
+	argv[0] = cmd.buf;
 
 	memset(&child_process, 0, sizeof(child_process));
 	child_process.argv = argv;

@@ -349,6 +366,8 @@ static int filter_buffer(int in, int out, void *data)
 	status = finish_command(&child_process);
 	if (status)
 		error("external filter %s failed %d", params->cmd, status);
+
+	strbuf_release(&cmd);
 	return (write_err || status);
 }
 

@@ -376,6 +395,7 @@ static int apply_filter(const char *path, const char *src, size_t len,
 	params.src = src;
 	params.size = len;
 	params.cmd = cmd;
+	params.path = path;
 
 	fflush(NULL);
 	if (start_async(&async))

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 828e35b..c5c394d 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -93,4 +93,51 @@ test_expect_success expanded_in_repo '
 	cmp expanded-keywords expected-output
 '
 
+cat <<EOF >argc.sh
+#!$SHELL_PATH
+echo argc: \$# "\$@"
+echo argc running >&2
+EOF
+chmod +x argc.sh
+
+#
+# The use of %s in a filter definition is expanded to the path to
+# the filename being smudged or cleaned.  It must be shell escaped.
+#
+test_expect_success 'shell-escaped filenames' '
+    norm=name-no-magic &&
+    spec=$(echo name:sgl\"dbl\ spc!bang | tr : \\047) &&
+    echo some test text > test
+    cat test > $norm &&
+    cat test > "$spec" &&
+    git add $norm &&
+    git add "$spec" &&
+    git commit -m "add files" &&
+
+    echo "name* filter=argc" > .gitattributes &&
+
+    # delete the files and check them out again, using the smudge filter
+    git config filter.argc.smudge "./argc.sh %s" &&
+    rm $norm "$spec" &&
+    git checkout -- $norm "$spec" &&
+
+    # make sure argc.sh counted the right number of args
+    echo "argc: 1 $norm" > res &&
+    cmp res $norm &&
+    echo "argc: 1 $spec" > res &&
+    cmp res "$spec" &&
+
+    # %s with other args
+    git config filter.argc.smudge "./argc.sh %s --myword" &&
+    rm $norm "$spec" &&
+    git checkout -- $norm "$spec" &&
+
+    # make sure argc.sh counted the right number of args
+    echo "argc: 2 $norm --myword" > res &&
+    cmp res $norm &&
+    echo "argc: 2 $spec --myword" > res &&
+    cmp res "$spec" &&
+    :
+'
+
 test_done

-- 
1.7.2.3