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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2016, 17:46 UTC
Message-ID: <xmqqmvk2qcv8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <320f5e862e8df12bbd1d918f0b347aeacbd25d17.1472041389.git.johannes.schindelin@gmx.de>

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

Show 23 quoted lines

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

Show 15 quoted lines

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

Show 23 quoted lines

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