{"thread":{"id":"41252","subject":"[PATCH] convert: legitimately disable clean/smudge filter with an empty override","startedAt":"2016-01-24T12:22:50Z","lastAt":"2016-01-28T23:51:44Z","messageCount":11,"participants":["larsxschneider@gmail.com","Torsten Bögershausen","Eric Sunshine","Jeff King","Junio C Hamano","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"276626","messageId":"1453638170-52041-1-git-send-email-larsxschneider@gmail.com","threadId":"41252","inReplyTo":null,"subject":"[PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-24T12:22:50Z","receivedAt":"2016-01-24T12:22:50Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nA clean/smudge filter can be disabled if set to an empty string. However,\nGit will try to run the empty string as command which results in a error\nmessage per processed file.\n\nTeach Git to consider an empty clean/smudge filter as legitimately disabled\nand do not print an error message.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n convert.c             |  4 ++--\n t/t0021-conversion.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 814e814..58af965 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \tstruct conv_attrs ca;\n\n \tconvert_attrs(&ca, path);\n-\tif (ca.drv) {\n+\tif (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {\n \t\tfilter = ca.drv->clean;\n \t\trequired = ca.drv->required;\n \t}\n@@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \tstruct conv_attrs ca;\n\n \tconvert_attrs(&ca, path);\n-\tif (ca.drv) {\n+\tif (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {\n \t\tfilter = ca.drv->smudge;\n \t\trequired = ca.drv->required;\n \t}\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 718efa0..56e385c 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -252,4 +252,18 @@ test_expect_success \"filter: smudge empty file\" '\n \ttest_cmp expected filtered-empty-in-repo\n '\n\n+test_expect_success 'disable filter with empty override' '\n+\tgit config filter.disable.smudge false &&\n+\tgit config filter.disable.clean false &&\n+\n+\techo \"*.disable filter=disable\" >.gitattributes &&\n+\n+\techo test >test.disable &&\n+\tgit -c filter.disable.clean= add test.disable 2>err &&\n+\t! test -s err &&\n+\trm -f test.disable &&\n+\tgit -c filter.disable.smudge= checkout -- test.disable 2>err &&\n+\t! test -s err\n+'\n+\n test_done\n--\n2.5.1\n"},{"id":"276630","messageId":"56A4E86B.1010909@web.de","threadId":"41252","inReplyTo":"1453638170-52041-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-01-24T15:06:19Z","receivedAt":"2016-01-24T15:06:19Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 24.01.16 13:22, larsxschneider@gmail.com wrote:\n> From: Lars Schneider <larsxschneider@gmail.com>\nSome minor nits inside: \n> \n> A clean/smudge filter can be disabled if set to an empty string. \n\"set to an empty string\" refers to \"git config\" (in opposite to the\nfilter as such, which is specified in .gitattributes.\nDoes it make sense to write \n\"git config filter.XXX.smudge ''\" or so ?\n\n> However,\n> Git will try to run the empty string as command which results in a error\n> message per processed file.\n> \n> Teach Git to consider an empty clean/smudge filter as legitimately disabled\n> and do not print an error message.\n> \n> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n> ---\n>  convert.c             |  4 ++--\n>  t/t0021-conversion.sh | 14 ++++++++++++++\n>  2 files changed, 16 insertions(+), 2 deletions(-)\n> \n> diff --git a/convert.c b/convert.c\n> index 814e814..58af965 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n>  \tstruct conv_attrs ca;\n> \n>  \tconvert_attrs(&ca, path);\n> -\tif (ca.drv) {\n> +\tif (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {\n>  \t\tfilter = ca.drv->clean;\n>  \t\trequired = ca.drv->required;\n>  \t}\n> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n>  \tstruct conv_attrs ca;\n> \n>  \tconvert_attrs(&ca, path);\n> -\tif (ca.drv) {\n> +\tif (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {\n>  \t\tfilter = ca.drv->smudge;\n>  \t\trequired = ca.drv->required;\n>  \t}\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 718efa0..56e385c 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -252,4 +252,18 @@ test_expect_success \"filter: smudge empty file\" '\n>  \ttest_cmp expected filtered-empty-in-repo\n>  '\n> \n> +test_expect_success 'disable filter with empty override' '\n> +\tgit config filter.disable.smudge false &&\n> +\tgit config filter.disable.clean false &&\n> +\n> +\techo \"*.disable filter=disable\" >.gitattributes &&\n> +\n> +\techo test >test.disable &&\n> +\tgit -c filter.disable.clean= add test.disable 2>err &&\n> +\t! test -s err &&\nHow about \ntest_cmp /dev/null err\nto make debugging easier ?\n> +\trm -f test.disable &&\n> +\tgit -c filter.disable.smudge= checkout -- test.disable 2>err &&\n> +\t! test -s err\n> +'\n> +\n>  test_done\n> --\n> 2.5.1\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"276674","messageId":"CAPig+cSo5vHXgBSzWQ6HtL=4XCigw6VdoAifmov7mhLiRMSkHQ@mail.gmail.com","threadId":"41252","inReplyTo":"56A4E86B.1010909@web.de","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-24T21:35:13Z","receivedAt":"2016-01-24T21:35:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jan 24, 2016 at 10:06 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n> On 24.01.16 13:22, larsxschneider@gmail.com wrote:\n>> From: Lars Schneider <larsxschneider@gmail.com>\n>> diff --git a/convert.c b/convert.c\n>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n>>       struct conv_attrs ca;\n>>\n>>       convert_attrs(&ca, path);\n>> -     if (ca.drv) {\n>> +     if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {\n\nMore idiomatic:\n\n    if (ca.drv && ca.drv->clean && *ca.drv->clean) {\n\n>>               filter = ca.drv->clean;\n>>               required = ca.drv->required;\n>>       }\n>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n>>       struct conv_attrs ca;\n>>\n>>       convert_attrs(&ca, path);\n>> -     if (ca.drv) {\n>> +     if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {\n\nDitto.\n\n>>               filter = ca.drv->smudge;\n>>               required = ca.drv->required;\n>>       }\n>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n>> @@ -252,4 +252,18 @@ test_expect_success \"filter: smudge empty file\" '\n>> +test_expect_success 'disable filter with empty override' '\n>> +     git config filter.disable.smudge false &&\n>> +     git config filter.disable.clean false &&\n\ntest_config perhaps?\n\n>> +     echo \"*.disable filter=disable\" >.gitattributes &&\n>> +\n>> +     echo test >test.disable &&\n>> +     git -c filter.disable.clean= add test.disable 2>err &&\n>> +     ! test -s err &&\n> How about\n> test_cmp /dev/null err\n> to make debugging easier ?\n\nEven better:\n\n    test_must_be_empty err &&\n\n>> +     rm -f test.disable &&\n>> +     git -c filter.disable.smudge= checkout -- test.disable 2>err &&\n>> +     ! test -s err\n>> +'\n"},{"id":"276675","messageId":"20160124214459.GA31091@sigill.intra.peff.net","threadId":"41252","inReplyTo":"1453638170-52041-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-24T21:45:00Z","receivedAt":"2016-01-24T21:45:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 24, 2016 at 01:22:50PM +0100, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider <larsxschneider@gmail.com>\n> \n> A clean/smudge filter can be disabled if set to an empty string. However,\n> Git will try to run the empty string as command which results in a error\n> message per processed file.\n> \n> Teach Git to consider an empty clean/smudge filter as legitimately disabled\n> and do not print an error message.\n\nThat makes sense to me, as I do not think the empty filter name can\npossibly do anything useful. You omitted the real motivation here, but I\nknow what it is from past discussions: you want to be able to\ntemporarily disable a filter with \"git -c filter.foo.clean= ...\". Which\nI think makes it more immediately obvious that this is a useful thing to\nhave, and not just user error.\n\n> diff --git a/convert.c b/convert.c\n> index 814e814..58af965 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n>  \tstruct conv_attrs ca;\n> \n>  \tconvert_attrs(&ca, path);\n> -\tif (ca.drv) {\n> +\tif (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {\n>  \t\tfilter = ca.drv->clean;\n>  \t\trequired = ca.drv->required;\n>  \t}\n> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n>  \tstruct conv_attrs ca;\n> \n>  \tconvert_attrs(&ca, path);\n> -\tif (ca.drv) {\n> +\tif (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {\n>  \t\tfilter = ca.drv->smudge;\n>  \t\trequired = ca.drv->required;\n>  \t}\n\nThis catches two calls, but I think there are others. What about\nwould_convert_to_git_filter_fd and convert_to_git_filter_fd?\n\nWould it make more sense for apply_filter() to treat the empty string as\na noop, just as it does for NULL?\n\nI.e.:\n\n\ndiff --git a/convert.c b/convert.c\nindex 814e814..02d5f1e 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -395,7 +395,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n \tstruct async async;\n \tstruct filter_params params;\n \n-\tif (!cmd)\n+\tif (!cmd || !*cmd)\n \t\treturn 0;\n \n \tif (!dst)\n\nwhich I think would cover all callers?\n\n-Peff\n"},{"id":"276685","messageId":"xmqqr3h6bgbe.fsf@gitster.mtv.corp.google.com","threadId":"41252","inReplyTo":"1453638170-52041-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-25T01:25:57Z","receivedAt":"2016-01-25T01:25:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider <larsxschneider@gmail.com>\n>\n> A clean/smudge filter can be disabled if set to an empty string. However,\n> Git will try to run the empty string as command which results in a error\n> message per processed file.\n\nThe above two sentences do not make any sense to me.  You observe\nthat the command refuses to work when the variable is set to an\nempty string--why then can you claim \"filter can be disabled if set\nto an empty string\"?  I'd consider that the system is not working\nwith such a configuration, i.e. \"filter cannot be disabled by\nsetting it to empty; such a request will result in an error\".\n\n> Teach Git to consider an empty clean/smudge filter as legitimately disabled\n> and do not print an error message.\n\nOn the other hand, this does make sense to me, as I do not think of\na good way to say \"earlier configuration entry said we should use\nthis filter, but we actually do not want to use that one (or any)\"\nbecause a configuration, unlike attributes entry, cannot be reset.\nThe closest you can do is to set it to empty, so it may be a good\nnew feature to do this.\n"},{"id":"276880","messageId":"C850C103-E400-43C3-84A4-8399FA1CD5FB@gmail.com","threadId":"41252","inReplyTo":"56A4E86B.1010909@web.de","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-27T09:42:37Z","receivedAt":"2016-01-27T09:42:37Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 24 Jan 2016, at 16:06, Torsten Bögershausen <tboegi@web.de> wrote:\n\n> On 24.01.16 13:22, larsxschneider@gmail.com wrote:\n>> From: Lars Schneider <larsxschneider@gmail.com>\n> Some minor nits inside: \n>> \n>> A clean/smudge filter can be disabled if set to an empty string. \n> \"set to an empty string\" refers to \"git config\" (in opposite to the\n> filter as such, which is specified in .gitattributes.\n> Does it make sense to write \n> \"git config filter.XXX.smudge ''\" or so ?\n\nI am not sure I get what you want here. Would this work for you?\n\nThe clean/smudge filter commands (filter.XYZ.smudge and filter.XYZ.clean)\ncan be disabled if set to an empty string. However, Git will still consider the \nempty string as command which results in a error message per processed \nfile.\n\n\n> \n>> However,\n>> Git will try to run the empty string as command which results in a error\n>> message per processed file.\n>> \n>> Teach Git to consider an empty clean/smudge filter as legitimately disabled\n>> and do not print an error message.\n>> \n>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n>> ---\n>> convert.c             |  4 ++--\n>> t/t0021-conversion.sh | 14 ++++++++++++++\n>> 2 files changed, 16 insertions(+), 2 deletions(-)\n>> \n>> diff --git a/convert.c b/convert.c\n>> index 814e814..58af965 100644\n>> --- a/convert.c\n>> +++ b/convert.c\n>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n>> \tstruct conv_attrs ca;\n>> \n>> \tconvert_attrs(&ca, path);\n>> -\tif (ca.drv) {\n>> +\tif (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {\n>> \t\tfilter = ca.drv->clean;\n>> \t\trequired = ca.drv->required;\n>> \t}\n>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n>> \tstruct conv_attrs ca;\n>> \n>> \tconvert_attrs(&ca, path);\n>> -\tif (ca.drv) {\n>> +\tif (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {\n>> \t\tfilter = ca.drv->smudge;\n>> \t\trequired = ca.drv->required;\n>> \t}\n>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n>> index 718efa0..56e385c 100755\n>> --- a/t/t0021-conversion.sh\n>> +++ b/t/t0021-conversion.sh\n>> @@ -252,4 +252,18 @@ test_expect_success \"filter: smudge empty file\" '\n>> \ttest_cmp expected filtered-empty-in-repo\n>> '\n>> \n>> +test_expect_success 'disable filter with empty override' '\n>> +\tgit config filter.disable.smudge false &&\n>> +\tgit config filter.disable.clean false &&\n>> +\n>> +\techo \"*.disable filter=disable\" >.gitattributes &&\n>> +\n>> +\techo test >test.disable &&\n>> +\tgit -c filter.disable.clean= add test.disable 2>err &&\n>> +\t! test -s err &&\n> How about \n> test_cmp /dev/null err\n> to make debugging easier ?\nRight. Although I would probably go with Eric's suggestion and use \"test_must_be_empty\".\n\nThat being said, I copied \"! test -s\" from the \"filter large file\" test in this file (added with 0b6806b).\nHow does the list handle these cases? Should I add another commit to replace \"! test -s\" there, \ntoo?\n\nThanks,\nLars"},{"id":"276881","messageId":"897531AD-96CE-458B-8F9D-9C7937732132@gmail.com","threadId":"41252","inReplyTo":"CAPig+cSo5vHXgBSzWQ6HtL=4XCigw6VdoAifmov7mhLiRMSkHQ@mail.gmail.com","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-27T09:49:30Z","receivedAt":"2016-01-27T09:49:30Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 24 Jan 2016, at 22:35, Eric Sunshine <sunshine@sunshineco.com> wrote:\n\n> On Sun, Jan 24, 2016 at 10:06 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n>> On 24.01.16 13:22, larsxschneider@gmail.com wrote:\n>>> From: Lars Schneider <larsxschneider@gmail.com>\n>>> diff --git a/convert.c b/convert.c\n>>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n>>>      struct conv_attrs ca;\n>>> \n>>>      convert_attrs(&ca, path);\n>>> -     if (ca.drv) {\n>>> +     if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {\n> \n> More idiomatic:\n> \n>    if (ca.drv && ca.drv->clean && *ca.drv->clean) {\nAgreed! Although I will go with Jeff's suggestion to implement that\nlogic in apply_filter.\n\n\n> \n>>>              filter = ca.drv->clean;\n>>>              required = ca.drv->required;\n>>>      }\n>>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n>>>      struct conv_attrs ca;\n>>> \n>>>      convert_attrs(&ca, path);\n>>> -     if (ca.drv) {\n>>> +     if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {\n> \n> Ditto.\n> \n>>>              filter = ca.drv->smudge;\n>>>              required = ca.drv->required;\n>>>      }\n>>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n>>> @@ -252,4 +252,18 @@ test_expect_success \"filter: smudge empty file\" '\n>>> +test_expect_success 'disable filter with empty override' '\n>>> +     git config filter.disable.smudge false &&\n>>> +     git config filter.disable.clean false &&\n> \n> test_config perhaps?\n\nI was wondering about that, too. Especially because I could also use test_config_global.\nWhy does no test in t0021 use these functions? Should that be fixed?\n\n\n> \n>>> +     echo \"*.disable filter=disable\" >.gitattributes &&\n>>> +\n>>> +     echo test >test.disable &&\n>>> +     git -c filter.disable.clean= add test.disable 2>err &&\n>>> +     ! test -s err &&\n>> How about\n>> test_cmp /dev/null err\n>> to make debugging easier ?\n> \n> Even better:\n> \n>    test_must_be_empty err &&\n\nTrue! I will use that.\n\nThanks,\nLars\n"},{"id":"276882","messageId":"31B39C4A-0344-402B-9E0E-60D1B1FD75D9@gmail.com","threadId":"41252","inReplyTo":"20160124214459.GA31091@sigill.intra.peff.net","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-27T09:50:26Z","receivedAt":"2016-01-27T09:50:26Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 24 Jan 2016, at 22:45, Jeff King <peff@peff.net> wrote:\n\n> On Sun, Jan 24, 2016 at 01:22:50PM +0100, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider <larsxschneider@gmail.com>\n>> \n>> A clean/smudge filter can be disabled if set to an empty string. However,\n>> Git will try to run the empty string as command which results in a error\n>> message per processed file.\n>> \n>> Teach Git to consider an empty clean/smudge filter as legitimately disabled\n>> and do not print an error message.\n> \n> That makes sense to me, as I do not think the empty filter name can\n> possibly do anything useful. You omitted the real motivation here, but I\n> know what it is from past discussions: you want to be able to\n> temporarily disable a filter with \"git -c filter.foo.clean= ...\". Which\n> I think makes it more immediately obvious that this is a useful thing to\n> have, and not just user error.\n> \n>> diff --git a/convert.c b/convert.c\n>> index 814e814..58af965 100644\n>> --- a/convert.c\n>> +++ b/convert.c\n>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n>> \tstruct conv_attrs ca;\n>> \n>> \tconvert_attrs(&ca, path);\n>> -\tif (ca.drv) {\n>> +\tif (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {\n>> \t\tfilter = ca.drv->clean;\n>> \t\trequired = ca.drv->required;\n>> \t}\n>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n>> \tstruct conv_attrs ca;\n>> \n>> \tconvert_attrs(&ca, path);\n>> -\tif (ca.drv) {\n>> +\tif (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {\n>> \t\tfilter = ca.drv->smudge;\n>> \t\trequired = ca.drv->required;\n>> \t}\n> \n> This catches two calls, but I think there are others. What about\n> would_convert_to_git_filter_fd and convert_to_git_filter_fd?\n> \n> Would it make more sense for apply_filter() to treat the empty string as\n> a noop, just as it does for NULL?\n\nYes :-)\n\nThanks,\nLars\n\n> \n> I.e.:\n> \n> \n> diff --git a/convert.c b/convert.c\n> index 814e814..02d5f1e 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -395,7 +395,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,\n> \tstruct async async;\n> \tstruct filter_params params;\n> \n> -\tif (!cmd)\n> +\tif (!cmd || !*cmd)\n> \t\treturn 0;\n> \n> \tif (!dst)\n> \n> which I think would cover all callers?\n> \n> -Peff\n"},{"id":"276980","messageId":"AB5433EB-2C86-4A71-9E9F-057AC3CC150A@gmail.com","threadId":"41252","inReplyTo":"xmqqr3h6bgbe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-28T09:27:20Z","receivedAt":"2016-01-28T09:27:20Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 25 Jan 2016, at 02:25, Junio C Hamano <gitster@pobox.com> wrote:\n\n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider <larsxschneider@gmail.com>\n>> \n>> A clean/smudge filter can be disabled if set to an empty string. However,\n>> Git will try to run the empty string as command which results in a error\n>> message per processed file.\n> \n> The above two sentences do not make any sense to me.  You observe\n> that the command refuses to work when the variable is set to an\n> empty string--why then can you claim \"filter can be disabled if set\n> to an empty string\"?  I'd consider that the system is not working\n> with such a configuration, i.e. \"filter cannot be disabled by\n> setting it to empty; such a request will result in an error\".\n\nIf I am not mistaken then Git exits with 0 (success) and an error message\nif the filter command is empty and the filter is not required. If the filter\ncommand is empty and the filter is required then Git will exit with 1 (error).\n\nHow about this?\n\nIf the clean/smudge command of a Git filter driver (filter.<driver>.smudge and\nfilter.<driver>.clean) is set to an empty string (\"\") and the filter driver is\nnot required (filter.<driver>.required=false) then Git will run successfully.\nHowever, Git will print an error for every file that is affected by the filter.\n\nTeach Git to consider an empty clean/smudge filter as legitimately disabled\nand do not print an error message if the filter is not required.\n\nThanks,\nLars\n\n\n> \n>> Teach Git to consider an empty clean/smudge filter as legitimately disabled\n>> and do not print an error message.\n> \n> On the other hand, this does make sense to me, as I do not think of\n> a good way to say \"earlier configuration entry said we should use\n> this filter, but we actually do not want to use that one (or any)\"\n> because a configuration, unlike attributes entry, cannot be reset.\n> The closest you can do is to set it to empty, so it may be a good\n> new feature to do this.\n> \n> \n"},{"id":"277014","messageId":"xmqqvb6dl5oe.fsf@gitster.mtv.corp.google.com","threadId":"41252","inReplyTo":"AB5433EB-2C86-4A71-9E9F-057AC3CC150A@gmail.com","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-28T22:06:57Z","receivedAt":"2016-01-28T22:06:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n> If the clean/smudge command of a Git filter driver (filter.<driver>.smudge and\n> filter.<driver>.clean) is set to an empty string (\"\") and the filter driver is\n> not required (filter.<driver>.required=false) then Git will run successfully.\n> However, Git will print an error for every file that is affected by the filter.\n>\n> Teach Git to consider an empty clean/smudge filter as legitimately disabled\n> and do not print an error message if the filter is not required.\n\nThat makes more sense to me.\n"},{"id":"277023","messageId":"xmqqa8npl0tr.fsf@gitster.mtv.corp.google.com","threadId":"41252","inReplyTo":"1453638170-52041-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH] convert: legitimately disable clean/smudge filter with an empty override","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-28T23:51:44Z","receivedAt":"2016-01-28T23:51:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> -\tif (ca.drv) {\n> +\tif (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {\n\nYou are not interested in its length, but if it is an empty string\nor not, so I'd tweak this like so:\n\n> +\tif (ca.drv && ca.drv->smudge && *ca.drv->smudge) {\n"}]}