[PATCH v2] convert: legitimately disable clean/smudge filter with an empty override

[PATCH v2] convert: legitimately disable clean/smudge filter with an empty override

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v1:
* improve commit message (Thanks Junio & Torsten)
* check for empty filter strings in apply_filter to catch all use cases (Thanks Peff)
* use more idiomatic style to check for an empty string (Thanks Eric)
* use test_config/test_config_global to set configs (Thanks Eric)
* use test_must_be_empty to check for empty err file (Thanks Eric)

Cheers,
Lars


Lars Schneider (1):
  convert: legitimately disable clean/smudge filter with an empty
    override

 convert.c             |  2 +-
 t/t0021-conversion.sh | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

--
2.5.1

[PATCH v2] convert: legitimately disable clean/smudge filter with an empty override

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

If the clean/smudge command of a Git filter driver (filter.<driver>.smudge and
filter.<driver>.clean) is set to an empty string ("") and the filter driver is
not required (filter.<driver>.required=false) then Git will run successfully.
However, Git will print an error for every file that is affected by the filter.

Teach Git to consider an empty clean/smudge filter as legitimately disabled
and do not print an error message if the filter is not required.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 convert.c             |  2 +-
 t/t0021-conversion.sh | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index 814e814..02d5f1e 100644
--- a/convert.c
+++ b/convert.c
@@ -395,7 +395,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
 	struct async async;
 	struct filter_params params;
 
-	if (!cmd)
+	if (!cmd || !*cmd)
 		return 0;
 
 	if (!dst)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 718efa0..7bac2bc 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -252,4 +252,20 @@ test_expect_success "filter: smudge empty file" '
 	test_cmp expected filtered-empty-in-repo
 '
 
+test_expect_success 'disable filter with empty override' '
+	test_config_global filter.disable.smudge false &&
+	test_config_global filter.disable.clean false &&
+	test_config filter.disable.smudge false &&
+	test_config filter.disable.clean false &&
+
+	echo "*.disable filter=disable" >.gitattributes &&
+
+	echo test >test.disable &&
+	git -c filter.disable.clean= add test.disable 2>err &&
+	test_must_be_empty err &&
+	rm -f test.disable &&
+	git -c filter.disable.smudge= checkout -- test.disable 2>err &&
+	test_must_be_empty err
+'
+
 test_done
-- 
2.5.1

Re: [PATCH v2] convert: legitimately disable clean/smudge filter with an empty override

[Junio C Hamano]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> If the clean/smudge command of a Git filter driver (filter.<driver>.smudge and
> filter.<driver>.clean) is set to an empty string ("") and the filter driver is
> not required (filter.<driver>.required=false) then Git will run successfully.
> However, Git will print an error for every file that is affected by the filter.
>
> Teach Git to consider an empty clean/smudge filter as legitimately disabled
> and do not print an error message if the filter is not required.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  convert.c             |  2 +-
>  t/t0021-conversion.sh | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/convert.c b/convert.c
> index 814e814..02d5f1e 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -395,7 +395,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>  	struct async async;
>  	struct filter_params params;
>  
> -	if (!cmd)
> +	if (!cmd || !*cmd)
>  		return 0;

This is certainly simpler than v1.  I was initially worried about
the fact that slightly changes the semantics around the "required"
variable relative to v1, which said:

	if (ca.drv && ca.drv->clean && *ca.drv->clean) {
        	filter = ca.drv->clean;
                required = ca.drv->required;
	}
	ret |= apply_filter(path, src, len, -1, dst, filter);
        if (!ret && required)
        	die;

but in v2, this part of the code is just as before, i.e.

	if (ca.drv) {
        	filter = ca.drv->clean;
                required = ca.drv->required;
	}
	ret |= apply_filter(path, src, len, -1, dst, filter);
        if (!ret && required)
        	die;

So unlike v1, 'required' is set to true in the function, which is a
good thing, but because in v2, apply_filter knows that an extrernal
filter command that is an empty string is a no-op success, the above
codepath behaves identically to v1 when observed from outside, i.e.
"an empty string given as clean/smudge filter is a no-op success".

Good.

By the way, I find it somewhat annoying to see "legitimately" twice
in the log message.  It makes it sound like there are legitimate way
and not-so-kosher way to disable the filters.  Perhaps something
like this instead?

-- >8 --
convert: treat an empty string for clean/smudge filters as "cat"

Once a lower-priority configuration file defines a clean or smudge
filter, there is no convenient way to override it.  Even though the
configuration mechanism implements "the last one wins" semantics,
you cannot set them to an empty string and expect them to work, as
apply_filter() would try to run the empty string as an external
command and fail.  The conversion is not done, but the function
would still report a failure to convert.

Even though resetting the variable to "cat" (i.e. pass the data back
as-is and report success) is an obvious and a viable way to solve
this, it is wasteful to spawn an external process just as a
workaround.

Instead, teach apply_filter() to treat an empty string given as a
filter means the input must be returned as-is without conversion,
and the operation must always succeed.
-- >8 --

>  
>  	if (!dst)
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 718efa0..7bac2bc 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -252,4 +252,20 @@ test_expect_success "filter: smudge empty file" '
>  	test_cmp expected filtered-empty-in-repo
>  '
>  
> +test_expect_success 'disable filter with empty override' '
> +	test_config_global filter.disable.smudge false &&
> +	test_config_global filter.disable.clean false &&
> +	test_config filter.disable.smudge false &&
> +	test_config filter.disable.clean false &&
> +
> +	echo "*.disable filter=disable" >.gitattributes &&
> +
> +	echo test >test.disable &&
> +	git -c filter.disable.clean= add test.disable 2>err &&
> +	test_must_be_empty err &&
> +	rm -f test.disable &&
> +	git -c filter.disable.smudge= checkout -- test.disable 2>err &&
> +	test_must_be_empty err
> +'
> +
>  test_done

Re: [PATCH v2] convert: legitimately disable clean/smudge filter with an empty override

[Junio C Hamano]

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

> Instead, teach apply_filter() to treat an empty string given as a
> filter means the input must be returned as-is without conversion,
> and the operation must always succeed.

Ugh, that was a non-sentence.  

  Instead, teach apply_filter() to treat an empty string as a no-op
  filter that always returns successfully its input as-is without
  conversion.

was what I meant to say.

> -- >8 --
>
>>  
>>  	if (!dst)
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index 718efa0..7bac2bc 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -252,4 +252,20 @@ test_expect_success "filter: smudge empty file" '
>>  	test_cmp expected filtered-empty-in-repo
>>  '
>>  
>> +test_expect_success 'disable filter with empty override' '
>> +	test_config_global filter.disable.smudge false &&
>> +	test_config_global filter.disable.clean false &&
>> +	test_config filter.disable.smudge false &&
>> +	test_config filter.disable.clean false &&
>> +
>> +	echo "*.disable filter=disable" >.gitattributes &&
>> +
>> +	echo test >test.disable &&
>> +	git -c filter.disable.clean= add test.disable 2>err &&
>> +	test_must_be_empty err &&
>> +	rm -f test.disable &&
>> +	git -c filter.disable.smudge= checkout -- test.disable 2>err &&
>> +	test_must_be_empty err
>> +'
>> +
>>  test_done

Re: [PATCH v2] convert: legitimately disable clean/smudge filter with an empty override

[Lars Schneider]

> On 29 Jan 2016, at 19:20, Junio C Hamano <gitster@pobox.com> wrote:
> 
> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> If the clean/smudge command of a Git filter driver (filter.<driver>.smudge and
>> filter.<driver>.clean) is set to an empty string ("") and the filter driver is
>> not required (filter.<driver>.required=false) then Git will run successfully.
>> However, Git will print an error for every file that is affected by the filter.
>> 
>> Teach Git to consider an empty clean/smudge filter as legitimately disabled
>> and do not print an error message if the filter is not required.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> convert.c             |  2 +-
>> t/t0021-conversion.sh | 16 ++++++++++++++++
>> 2 files changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/convert.c b/convert.c
>> index 814e814..02d5f1e 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -395,7 +395,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>>    struct async async;
>>    struct filter_params params;
>> 
>> -    if (!cmd)
>> +    if (!cmd || !*cmd)
>>        return 0;
> 
> This is certainly simpler than v1.  I was initially worried about
> the fact that slightly changes the semantics around the "required"
> variable relative to v1, which said:
> 
>    if (ca.drv && ca.drv->clean && *ca.drv->clean) {
>            filter = ca.drv->clean;
>                required = ca.drv->required;
>    }
>    ret |= apply_filter(path, src, len, -1, dst, filter);
>        if (!ret && required)
>            die;
> 
> but in v2, this part of the code is just as before, i.e.
> 
>    if (ca.drv) {
>            filter = ca.drv->clean;
>                required = ca.drv->required;
>    }
>    ret |= apply_filter(path, src, len, -1, dst, filter);
>        if (!ret && required)
>            die;
> 
> So unlike v1, 'required' is set to true in the function, which is a
> good thing, but because in v2, apply_filter knows that an extrernal
> filter command that is an empty string is a no-op success, the above
> codepath behaves identically to v1 when observed from outside, i.e.
> "an empty string given as clean/smudge filter is a no-op success".
> 
> Good.
> 
> By the way, I find it somewhat annoying to see "legitimately" twice
> in the log message.  It makes it sound like there are legitimate way
> and not-so-kosher way to disable the filters.  Perhaps something
> like this instead?
> 
> -- >8 --
> convert: treat an empty string for clean/smudge filters as "cat"
> 
> Once a lower-priority configuration file defines a clean or smudge
> filter, there is no convenient way to override it.  Even though the
> configuration mechanism implements "the last one wins" semantics,
> you cannot set them to an empty string and expect them to work, as
> apply_filter() would try to run the empty string as an external
> command and fail.  The conversion is not done, but the function
> would still report a failure to convert.
> 
> Even though resetting the variable to "cat" (i.e. pass the data back
> as-is and report success) is an obvious and a viable way to solve
> this, it is wasteful to spawn an external process just as a
> workaround.
> 
> Instead, teach apply_filter() to treat an empty string given as a
> filter means the input must be returned as-is without conversion,
> and the operation must always succeed.
> -- >8 --

That reads perfect. I am sorry that I caused so much work for you with this patch. 

I really appreciate your editing as this helps me to improve my commit message writing skills!

Thanks,
Lars

> 
>> 
>>    if (!dst)
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index 718efa0..7bac2bc 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -252,4 +252,20 @@ test_expect_success "filter: smudge empty file" '
>>    test_cmp expected filtered-empty-in-repo
>> '
>> 
>> +test_expect_success 'disable filter with empty override' '
>> +    test_config_global filter.disable.smudge false &&
>> +    test_config_global filter.disable.clean false &&
>> +    test_config filter.disable.smudge false &&
>> +    test_config filter.disable.clean false &&
>> +
>> +    echo "*.disable filter=disable" >.gitattributes &&
>> +
>> +    echo test >test.disable &&
>> +    git -c filter.disable.clean= add test.disable 2>err &&
>> +    test_must_be_empty err &&
>> +    rm -f test.disable &&
>> +    git -c filter.disable.smudge= checkout -- test.disable 2>err &&
>> +    test_must_be_empty err
>> +'
>> +
>> test_done