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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Jan 27, 2016, 09:49 UTC
Message-ID: <897531AD-96CE-458B-8F9D-9C7937732132@gmail.com>
In-Reply-To: <CAPig+cSo5vHXgBSzWQ6HtL=4XCigw6VdoAifmov7mhLiRMSkHQ@mail.gmail.com>

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

Show 14 quoted lines

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

Show 23 quoted lines

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

Show 13 quoted lines

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