Re: [PATCH v2 2/4] cat-file: introduce the --filters option

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> +static int filter_object(const char *path, unsigned mode,
> +			 const unsigned char *sha1,
> +			 char **buf, unsigned long *size)
> +{
> +	enum object_type type;
> +
> +	*buf = read_sha1_file(sha1, &type, size);
> +	if (!*buf)
> +		return error(_("cannot read object %s '%s'"),
> +			sha1_to_hex(sha1), path);
> +	if (type != OBJ_BLOB) {
> +		free(*buf);
> +		return error(_("blob expected for %s '%s'"),
> +			sha1_to_hex(sha1), path);
> +	}
> +	if (S_ISREG(mode)) {
> +		struct strbuf strbuf = STRBUF_INIT;
> +		if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> +			free(*buf);
> +			*size = strbuf.len;
> +			*buf = strbuf_detach(&strbuf, NULL);
> +		}
> +	}

When we see a blob that is not ISREG, what is expected to happen?
Is it an error?

We can argue both ways.

Currently the ONLY kind of blob that is not ISREG is a symbolic
link, and it might be OK to output it as-is without any conversion
[*1*], in which case we can simply lose the S_ISREG(mode) check
altogether (and "mode" parameter to this function).

On the other hand, because "cat-file --filters" is meant to be a
smaller-granularity operation that could be used as a building block
to emulate what "git checkout" does, i.e. "imagine that we had the
blob in the index and checking it out from the path, and hand the
caller what would have been written out to the filesystem, so that
the convert_to_working_tree() logic does not have to be emulated in
the userspace", erroring out when we see a symbolic link may be also
a valid way to handle it.  We know the usual CRLF and other conversions
do not apply to them.

In any case, silently succeeding without any output is probably what
the end-user least expects.

If we choose to fail the request, the necessary update to the test
would look like this, I think.

> diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
> new file mode 100755
> index 0000000..e466634
> --- /dev/null
> +++ b/t/t8010-cat-file-filters.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='git cat-file filters support'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup ' '
> +	echo "*.txt eol=crlf diff=txt" >.gitattributes &&
> +	echo "hello" | append_cr >world.txt &&
> +	git add .gitattributes world.txt &&

	git update-index --cacheinfo :world.txt,$EMPTY_BLOB,symlink &&

> +	test_tick &&
> +	git commit -m "Initial commit"
> +'
> +
> +has_cr () {
> +	tr '\015' Q <"$1" | grep Q >/dev/null
> +}
> +
> +test_expect_success 'no filters with `git show`' '
> +	git show HEAD:world.txt >actual &&
> +	! has_cr actual
> +
> +'
> +
> +test_expect_success 'no filters with cat-file' '
> +	git cat-file blob HEAD:world.txt >actual &&
> +	! has_cr actual
> +'
> +
> +test_expect_success 'cat-file --filters converts to worktree version' '
> +	git cat-file --filters HEAD:world.txt >actual &&
> +	has_cr actual
> +'

test_expect_success 'cat-file --filters rejects a non-blob' '
	test_must_fail git cat-file --filters HEAD:
'

test_expect_success 'cat-file --filters rejects a non-regular blob' '
	test_must_fail git cat-file --filters HEAD:symlink
'

And if we choose to accept as long as it is a blob, then the last
step can lose test_must_fail (and perhaps the result needs to be
checked against "hello" without CR).


[Footnote]

*1* But of course other kinds of non-ISREG blob we would add later
    may not be something you would want to write out as-is.