> On 29 Jan 2016, at 19:20, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> larsxschneider@xxxxxxxxx writes:
>
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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 --
That reads perfect. I am sorry that I caused so much work for you with this patch.
I really appreciate your editing as this helps me to improve my commit message writing skills!
Thanks,
Lars
>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html