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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 31, 2016, 20:31 UTC
Message-ID: <alpine.DEB.2.20.1608312153030.129229@virtualbox>
In-Reply-To: <xmqqmvk2qcv8.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Wed, 24 Aug 2016, Junio C Hamano wrote:

Show 28 quoted lines

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

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 <sha-1> '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