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

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

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

A clean/smudge filter can be disabled if set to an empty string. However,
Git will try to run the empty string as command which results in a error
message per processed file.

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

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

diff --git a/convert.c b/convert.c
index 814e814..58af965 100644
--- a/convert.c
+++ b/convert.c
@@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
 	struct conv_attrs ca;

 	convert_attrs(&ca, path);
-	if (ca.drv) {
+	if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
 		filter = ca.drv->clean;
 		required = ca.drv->required;
 	}
@@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
 	struct conv_attrs ca;

 	convert_attrs(&ca, path);
-	if (ca.drv) {
+	if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
 		filter = ca.drv->smudge;
 		required = ca.drv->required;
 	}
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 718efa0..56e385c 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -252,4 +252,18 @@ test_expect_success "filter: smudge empty file" '
 	test_cmp expected filtered-empty-in-repo
 '

+test_expect_success 'disable filter with empty override' '
+	git config filter.disable.smudge false &&
+	git 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 -s err &&
+	rm -f test.disable &&
+	git -c filter.disable.smudge= checkout -- test.disable 2>err &&
+	! test -s err
+'
+
 test_done
--
2.5.1

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

[Torsten Bögershausen]

On 24.01.16 13:22, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
Some minor nits inside: 
> 
> A clean/smudge filter can be disabled if set to an empty string. 
"set to an empty string" refers to "git config" (in opposite to the
filter as such, which is specified in .gitattributes.
Does it make sense to write 
"git config filter.XXX.smudge ''" or so ?

> However,
> Git will try to run the empty string as command which results in a error
> message per processed file.
> 
> Teach Git to consider an empty clean/smudge filter as legitimately disabled
> and do not print an error message.
> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  convert.c             |  4 ++--
>  t/t0021-conversion.sh | 14 ++++++++++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index 814e814..58af965 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
>  	struct conv_attrs ca;
> 
>  	convert_attrs(&ca, path);
> -	if (ca.drv) {
> +	if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
>  		filter = ca.drv->clean;
>  		required = ca.drv->required;
>  	}
> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>  	struct conv_attrs ca;
> 
>  	convert_attrs(&ca, path);
> -	if (ca.drv) {
> +	if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
>  		filter = ca.drv->smudge;
>  		required = ca.drv->required;
>  	}
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 718efa0..56e385c 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -252,4 +252,18 @@ test_expect_success "filter: smudge empty file" '
>  	test_cmp expected filtered-empty-in-repo
>  '
> 
> +test_expect_success 'disable filter with empty override' '
> +	git config filter.disable.smudge false &&
> +	git 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 -s err &&
How about 
test_cmp /dev/null err
to make debugging easier ?
> +	rm -f test.disable &&
> +	git -c filter.disable.smudge= checkout -- test.disable 2>err &&
> +	! test -s err
> +'
> +
>  test_done
> --
> 2.5.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Eric Sunshine]

On Sun, Jan 24, 2016 at 10:06 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 24.01.16 13:22, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> diff --git a/convert.c b/convert.c
>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
>>       struct conv_attrs ca;
>>
>>       convert_attrs(&ca, path);
>> -     if (ca.drv) {
>> +     if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {

More idiomatic:

    if (ca.drv && ca.drv->clean && *ca.drv->clean) {

>>               filter = ca.drv->clean;
>>               required = ca.drv->required;
>>       }
>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>>       struct conv_attrs ca;
>>
>>       convert_attrs(&ca, path);
>> -     if (ca.drv) {
>> +     if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {

Ditto.

>>               filter = ca.drv->smudge;
>>               required = ca.drv->required;
>>       }
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> @@ -252,4 +252,18 @@ test_expect_success "filter: smudge empty file" '
>> +test_expect_success 'disable filter with empty override' '
>> +     git config filter.disable.smudge false &&
>> +     git config filter.disable.clean false &&

test_config perhaps?

>> +     echo "*.disable filter=disable" >.gitattributes &&
>> +
>> +     echo test >test.disable &&
>> +     git -c filter.disable.clean= add test.disable 2>err &&
>> +     ! test -s err &&
> How about
> test_cmp /dev/null err
> to make debugging easier ?

Even better:

    test_must_be_empty err &&

>> +     rm -f test.disable &&
>> +     git -c filter.disable.smudge= checkout -- test.disable 2>err &&
>> +     ! test -s err
>> +'

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

[Lars Schneider]

On 24 Jan 2016, at 22:35, Eric Sunshine <sunshine@sunshineco.com> wrote:

> On Sun, Jan 24, 2016 at 10:06 AM, Torsten Bögershausen <tboegi@web.de> wrote:
>> On 24.01.16 13:22, larsxschneider@gmail.com wrote:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>> diff --git a/convert.c b/convert.c
>>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
>>>      struct conv_attrs ca;
>>> 
>>>      convert_attrs(&ca, path);
>>> -     if (ca.drv) {
>>> +     if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
> 
> More idiomatic:
> 
>    if (ca.drv && ca.drv->clean && *ca.drv->clean) {
Agreed! Although I will go with Jeff's suggestion to implement that
logic in apply_filter.


> 
>>>              filter = ca.drv->clean;
>>>              required = ca.drv->required;
>>>      }
>>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>>>      struct conv_attrs ca;
>>> 
>>>      convert_attrs(&ca, path);
>>> -     if (ca.drv) {
>>> +     if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
> 
> Ditto.
> 
>>>              filter = ca.drv->smudge;
>>>              required = ca.drv->required;
>>>      }
>>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>>> @@ -252,4 +252,18 @@ test_expect_success "filter: smudge empty file" '
>>> +test_expect_success 'disable filter with empty override' '
>>> +     git config filter.disable.smudge false &&
>>> +     git config filter.disable.clean false &&
> 
> test_config perhaps?

I was wondering about that, too. Especially because I could also use test_config_global.
Why does no test in t0021 use these functions? Should that be fixed?


> 
>>> +     echo "*.disable filter=disable" >.gitattributes &&
>>> +
>>> +     echo test >test.disable &&
>>> +     git -c filter.disable.clean= add test.disable 2>err &&
>>> +     ! test -s err &&
>> How about
>> test_cmp /dev/null err
>> to make debugging easier ?
> 
> Even better:
> 
>    test_must_be_empty err &&

True! I will use that.

Thanks,
Lars

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

[Lars Schneider]

On 24 Jan 2016, at 16:06, Torsten Bögershausen <tboegi@web.de> wrote:

> On 24.01.16 13:22, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
> Some minor nits inside: 
>> 
>> A clean/smudge filter can be disabled if set to an empty string. 
> "set to an empty string" refers to "git config" (in opposite to the
> filter as such, which is specified in .gitattributes.
> Does it make sense to write 
> "git config filter.XXX.smudge ''" or so ?

I am not sure I get what you want here. Would this work for you?

The clean/smudge filter commands (filter.XYZ.smudge and filter.XYZ.clean)
can be disabled if set to an empty string. However, Git will still consider the 
empty string as command which results in a error message per processed 
file.


> 
>> However,
>> Git will try to run the empty string as command which results in a error
>> message per processed file.
>> 
>> Teach Git to consider an empty clean/smudge filter as legitimately disabled
>> and do not print an error message.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> convert.c             |  4 ++--
>> t/t0021-conversion.sh | 14 ++++++++++++++
>> 2 files changed, 16 insertions(+), 2 deletions(-)
>> 
>> diff --git a/convert.c b/convert.c
>> index 814e814..58af965 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
>> 	struct conv_attrs ca;
>> 
>> 	convert_attrs(&ca, path);
>> -	if (ca.drv) {
>> +	if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
>> 		filter = ca.drv->clean;
>> 		required = ca.drv->required;
>> 	}
>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>> 	struct conv_attrs ca;
>> 
>> 	convert_attrs(&ca, path);
>> -	if (ca.drv) {
>> +	if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
>> 		filter = ca.drv->smudge;
>> 		required = ca.drv->required;
>> 	}
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index 718efa0..56e385c 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -252,4 +252,18 @@ test_expect_success "filter: smudge empty file" '
>> 	test_cmp expected filtered-empty-in-repo
>> '
>> 
>> +test_expect_success 'disable filter with empty override' '
>> +	git config filter.disable.smudge false &&
>> +	git 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 -s err &&
> How about 
> test_cmp /dev/null err
> to make debugging easier ?
Right. Although I would probably go with Eric's suggestion and use "test_must_be_empty".

That being said, I copied "! test -s" from the "filter large file" test in this file (added with 0b6806b).
How does the list handle these cases? Should I add another commit to replace "! test -s" there, 
too?

Thanks,
Lars

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

[Jeff King]

On Sun, Jan 24, 2016 at 01:22:50PM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> A clean/smudge filter can be disabled if set to an empty string. However,
> Git will try to run the empty string as command which results in a error
> message per processed file.
> 
> Teach Git to consider an empty clean/smudge filter as legitimately disabled
> and do not print an error message.

That makes sense to me, as I do not think the empty filter name can
possibly do anything useful. You omitted the real motivation here, but I
know what it is from past discussions: you want to be able to
temporarily disable a filter with "git -c filter.foo.clean= ...". Which
I think makes it more immediately obvious that this is a useful thing to
have, and not just user error.

> diff --git a/convert.c b/convert.c
> index 814e814..58af965 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
>  	struct conv_attrs ca;
> 
>  	convert_attrs(&ca, path);
> -	if (ca.drv) {
> +	if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
>  		filter = ca.drv->clean;
>  		required = ca.drv->required;
>  	}
> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>  	struct conv_attrs ca;
> 
>  	convert_attrs(&ca, path);
> -	if (ca.drv) {
> +	if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
>  		filter = ca.drv->smudge;
>  		required = ca.drv->required;
>  	}

This catches two calls, but I think there are others. What about
would_convert_to_git_filter_fd and convert_to_git_filter_fd?

Would it make more sense for apply_filter() to treat the empty string as
a noop, just as it does for NULL?

I.e.:


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)

which I think would cover all callers?

-Peff

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

[Lars Schneider]

On 24 Jan 2016, at 22:45, Jeff King <peff@peff.net> wrote:

> On Sun, Jan 24, 2016 at 01:22:50PM +0100, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> A clean/smudge filter can be disabled if set to an empty string. However,
>> Git will try to run the empty string as command which results in a error
>> message per processed file.
>> 
>> Teach Git to consider an empty clean/smudge filter as legitimately disabled
>> and do not print an error message.
> 
> That makes sense to me, as I do not think the empty filter name can
> possibly do anything useful. You omitted the real motivation here, but I
> know what it is from past discussions: you want to be able to
> temporarily disable a filter with "git -c filter.foo.clean= ...". Which
> I think makes it more immediately obvious that this is a useful thing to
> have, and not just user error.
> 
>> diff --git a/convert.c b/convert.c
>> index 814e814..58af965 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
>> 	struct conv_attrs ca;
>> 
>> 	convert_attrs(&ca, path);
>> -	if (ca.drv) {
>> +	if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
>> 		filter = ca.drv->clean;
>> 		required = ca.drv->required;
>> 	}
>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
>> 	struct conv_attrs ca;
>> 
>> 	convert_attrs(&ca, path);
>> -	if (ca.drv) {
>> +	if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
>> 		filter = ca.drv->smudge;
>> 		required = ca.drv->required;
>> 	}
> 
> This catches two calls, but I think there are others. What about
> would_convert_to_git_filter_fd and convert_to_git_filter_fd?
> 
> Would it make more sense for apply_filter() to treat the empty string as
> a noop, just as it does for NULL?

Yes :-)

Thanks,
Lars

> 
> I.e.:
> 
> 
> 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)
> 
> which I think would cover all callers?
> 
> -Peff

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

[Junio C Hamano]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> A clean/smudge filter can be disabled if set to an empty string. However,
> Git will try to run the empty string as command which results in a error
> message per processed file.

The above two sentences do not make any sense to me.  You observe
that the command refuses to work when the variable is set to an
empty string--why then can you claim "filter can be disabled if set
to an empty string"?  I'd consider that the system is not working
with such a configuration, i.e. "filter cannot be disabled by
setting it to empty; such a request will result in an error".

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

On the other hand, this does make sense to me, as I do not think of
a good way to say "earlier configuration entry said we should use
this filter, but we actually do not want to use that one (or any)"
because a configuration, unlike attributes entry, cannot be reset.
The closest you can do is to set it to empty, so it may be a good
new feature to do this.

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

[Lars Schneider]

On 25 Jan 2016, at 02:25, Junio C Hamano <gitster@pobox.com> wrote:

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> A clean/smudge filter can be disabled if set to an empty string. However,
>> Git will try to run the empty string as command which results in a error
>> message per processed file.
> 
> The above two sentences do not make any sense to me.  You observe
> that the command refuses to work when the variable is set to an
> empty string--why then can you claim "filter can be disabled if set
> to an empty string"?  I'd consider that the system is not working
> with such a configuration, i.e. "filter cannot be disabled by
> setting it to empty; such a request will result in an error".

If I am not mistaken then Git exits with 0 (success) and an error message
if the filter command is empty and the filter is not required. If the filter
command is empty and the filter is required then Git will exit with 1 (error).

How about this?

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.

Thanks,
Lars


> 
>> Teach Git to consider an empty clean/smudge filter as legitimately disabled
>> and do not print an error message.
> 
> On the other hand, this does make sense to me, as I do not think of
> a good way to say "earlier configuration entry said we should use
> this filter, but we actually do not want to use that one (or any)"
> because a configuration, unlike attributes entry, cannot be reset.
> The closest you can do is to set it to empty, so it may be a good
> new feature to do this.
> 
> 

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

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

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

That makes more sense to me.

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

[Junio C Hamano]

larsxschneider@gmail.com writes:

> -	if (ca.drv) {
> +	if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {

You are not interested in its length, but if it is an empty string
or not, so I'd tweak this like so:

> +	if (ca.drv && ca.drv->smudge && *ca.drv->smudge) {