Re: [PATCH v4] 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 "%f" 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.

You do not specify the filter in attributes file, and this is not just
about splitting incorrectly (think $var substitutions).  I rephrased the
last paragraph when I tentatively queued v3 (and then I had to discard it
after seeing this round) like this:

    Allow "%f" in the custom filter command line specified in the
    configuration.  This will be substituted by the filename inside a
    single-quote pair to be passed to the shell.

I couldn't quite bring myself to delete the nice spaces in
redirections like "> test".  Rest of the usage in t/ seems
to be about 1/3 for space, 2/3 against.

While existing mistakes by others do not make a good excuse to throw
unnecessary code-churn patches at me, it is not an excuse to introduce
more mistakes of the same kind in new code.

Maybe "important" to you, but not really.  Just let the reader decide the
importance.  I rephrased this when I tentatively queued v3 (and then I had
to discard it after seeing this round) like this:

    Sequence "%f" on the filter command line is replaced with the name of
    the file the filter is working on (a filter can use this in keyword
    substitution, for example):

I think this is going overboard.  The correctness of sq_quote_buf() is not
what we are really testing with this test; we are primarily interested in
testing that '%f' is substituted here.  You can and should drop at least
dq from the funny pathname, so that this can be run on Windows as well.
Perhaps (note two SPs between "name" and "with"):

	special="name  with '\''sq'\'' and \$x" &&

Do not over-abbreviate variable names unnecessarily; it will only confuse
the readers.  "spec" typically stands for "specification", but you don't
mean that here.

Also if you define the filter as "sh ./argc.sh %f", you should be able to
stop worrying about the "chmod +x" failing, no?

+	echo some test text > test &&
+	cat test > $norm &&
+	cat test > "$spec" &&

Quote both for consistency.

	cat test >"$norm"
        cat test >"$spec"

+	git add $norm &&
+	git add "$spec" &&

Why add them separately?

+	echo "name* filter=argc" > .gitattributes &&
+
+	# delete the files and check them out again, using a smudge filter
+	# that will count the args and echo the command-line back to us
+	git config filter.argc.smudge "./argc.sh %f" &&
+	rm $norm "$spec" &&
+	git checkout -- $norm "$spec" &&
+
+	# make sure argc.sh counted the right number of args
+	echo "argc: 1 $norm" > res &&

Perhaps "res" stands for "result", but both are misleading as it is
unclear if that is an expected result or an actual result.

	echo "argc: 1 $norm" >expect &&

Thanks.

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.

Allow "%f" in the custom filter command line specified in the
configuration.  This will be substituted by the filename
inside a single-quote pair to be passed to the shell.

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

gitster@pobox.com wrote on Tue, 21 Dec 2010 13:24 -0800:

[detailed review]

Changes from v4:
- Updated commit message, docs per Junio mods
- Removed space after shell redirection ">"
- Simplified test case per Junio recommendations

Hopefully this is the last round of review and it is
safe to stage now.  Thanks,

		-- Pete

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

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 564586b..8c2fdd1 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt

@@ -317,6 +317,16 @@ command is "cat").
 	smudge = cat
 ------------------------
 
+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:
+
+------------------------
+[filter "p4"]
+	clean = git-p4-filter --clean %f
+	smudge = git-p4-filter --smudge %f
+------------------------
+
 
 Interaction between checkin/checkout attributes
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

diff --git a/convert.c b/convert.c
index e41a31e..8f020bc 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;
+	/* apply % substitution to cmd */
+	struct strbuf cmd = STRBUF_INIT;
+	struct strbuf path = STRBUF_INIT;
+	struct strbuf_expand_dict_entry dict[] = {
+	    "f", NULL,
+	    NULL, NULL,
+	};
+
+	/* quote the path to preserve spaces, etc. */
+	sq_quote_buf(&path, params->path);
+	dict[0].value = path.buf;
+
+	/* expand all %f 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..aacfd00 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -93,4 +93,46 @@ test_expect_success expanded_in_repo '
 	cmp expanded-keywords expected-output
 '
 
+# The use of %f in a filter definition is expanded to the path to
+# the filename being smudged or cleaned.  It must be shell escaped.
+# First, set up some interesting file names and pet them in
+# .gitattributes.
+test_expect_success 'filter shell-escaped filenames' '
+	cat >argc.sh <<-EOF &&
+	#!$SHELL_PATH
+	echo argc: \$# "\$@"
+	EOF
+	normal=name-no-magic &&
+	special="name  with '\''sq'\'' and \$x" &&
+	echo some test text >"$normal" &&
+	echo some test text >"$special" &&
+	git add "$normal" "$special" &&
+	git commit -q -m "add files" &&
+	echo "name* filter=argc" >.gitattributes &&
+
+	# delete the files and check them out again, using a smudge filter
+	# that will count the args and echo the command-line back to us
+	git config filter.argc.smudge "sh ./argc.sh %f" &&
+	rm "$normal" "$special" &&
+	git checkout -- "$normal" "$special" &&
+
+	# make sure argc.sh counted the right number of args
+	echo "argc: 1 $normal" >expect &&
+	test_cmp expect "$normal" &&
+	echo "argc: 1 $special" >expect &&
+	test_cmp expect "$special" &&
+
+	# do the same thing, but with more args in the filter expression
+	git config filter.argc.smudge "sh ./argc.sh %f --my-extra-arg" &&
+	rm "$normal" "$special" &&
+	git checkout -- "$normal" "$special" &&
+
+	# make sure argc.sh counted the right number of args
+	echo "argc: 2 $normal --my-extra-arg" >expect &&
+	test_cmp expect "$normal" &&
+	echo "argc: 2 $special --my-extra-arg" >expect &&
+	test_cmp expect "$special" &&
+	:
+'
+
 test_done

-- 
1.7.2.3