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