Re: [PATCH v2 2/4] cat-file: introduce the --filters option
Johannes Schindelin writes:
>> > + 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?
>
> This is not a user-facing command, therefore we have to trust the caller
> that they know what they are doing.
The caller that knows what s/he is doing would rely on a documented
behaviour out of the command. That behaviour hopefully is an
intuitive and useful one for script writers.
You say
> Quite frankly, as cat-file is not an end-user-facing command, I think it
> is serious overkill to add more testing here.
and I think you would need a serious attitude adjustment here.
Scriptors are also an important class of end-users and cat-file
directly faces them.
Thinking about this one a bit more, as 'cat-file' especially with
the "--filters" option is the lowest-level way for scriptors to
externalize the data stored in Git object database to the
representation used in the outside world (in other words, it would
be a good ingredient if they want to implement what "checkout"
does), I would expect that an intuitive behaviour for
git cat-file --filters HEAD:Makefile >Makefile
git cat-file --filters HEAD:RelNotes >Relnotes
git cat-file --filters HEAD:t >t
to send the requested contents to the standard output stream, but
error out when the result of the command shown above would not
mimick what "checkout" would leave in the filesystem. IOW, the
first one should succeed, the second and the third ones should fail
the same way to signal what is requested cannot be performed to the
script that is calling these commands.
Re: [PATCH v2 2/4] cat-file: introduce the --filters option
Hi Junio,
On Wed, 24 Aug 2016, Junio C Hamano wrote:
> Johannes Schindelin 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?
This is not a user-facing command, therefore we have to trust the caller
that they know what they are doing.
And it is quite conceivable that a caller wants to simply apply filters
whenever a blob is specified, and simply not apply any filters when
something else was specified.
That would be in line with specifying the --filters options on a path for
which no filters are configured: the --filters option is basically a
no-op, then.
> In any case, silently succeeding without any output is probably what
> the end-user least expects.
Except that 1) the command is not for an end-user, and 2) when calling
`git cat-file --filters HEAD:directory/` the command does not silently
succeed:
error: blob expected for 'directory/'
> If we choose to fail the request, the necessary update to the test
> would look like this, I think.
Quite frankly, as cat-file is not an end-user-facing command, I think it
is serious overkill to add more testing here.
Ciao,
Dscho
Re: [PATCH v2 2/4] cat-file: introduce the --filters option
Junio C Hamano writes: >> +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 && Sorry, last-minute-edit-gone-bad-without-proofreading. It should have been something like: git update-index --add --cacheinfo 16,$EMPTY_BLOB,symlink && or sym=$(echo "hello" | git hash-objects --stdin -w) && git update-index --add --cacheinfo 16,$sym,symlink && -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] cat-file: introduce the --filters option
Johannes Schindelin 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 000..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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
