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

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

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