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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Pete Wyckoff <pw@xxxxxxxx> 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.

>  Documentation/gitattributes.txt |   12 +++++++++
>  convert.c                       |   22 +++++++++++++++++-
>  t/t0021-conversion.sh           |   48 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 564586b..1afcf01 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 "%f" 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:

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):

> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 828e35b..69c22a6 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -93,4 +93,52 @@ test_expect_success expanded_in_repo '
>  	cmp expanded-keywords expected-output
>  '
>  
> +test_expect_success 'filter shell-escaped filenames setup' '
> +	cat >argc.sh <<-EOF &&
> +	#!$SHELL_PATH
> +	echo argc: \$# "\$@"
> +	EOF
> +	chmod +x argc.sh
> +'
>
> +# 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 test' '
> +	norm=name-no-magic &&
> +	spec=$(echo name:sgl\"dbl\ spc!bang | tr : \\047) &&

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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]