patchconvert: legitimately disable clean/smudge filter with an empty override

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

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

>>               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
>> @@ -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 &&

>> +     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 ?

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

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

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

> 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;
>  	}

> 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

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

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

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