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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 9, 2016, 17:16 UTC
Message-ID: <xmqqvay5c7t3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqbmzxdpjp.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 26 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > +	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);
>>> > +		}
>>> > +	}
>>> 
>>> This needs to error out when mode is not ISREG just like it errors
>>> out when type is not BLOB.
>>
>> Are you sure that this is desirable in batch mode?
>
> I do not quite see a reason why we should not diagnose a bad input
> that does not produce a filtered result.  In batch mode or not, it
> diagnoses when the user feeds it a non-blob, and I think it should
> do so for non-regular, too. Both are "you asked me to filter, but
> you shouldn't have".

Stepping back a bit, I can also see a use case that would be helped if this filter_object() function by default gives the contents of the requested object as-is, unless the object is a regular blob with a path for which filtering is defined. Driving such a mechanism via the batch interface will allow you to first ask about the top-level tree object (given back to you as-is), and you can iterate over its entries recursively and get the blobs to be placed in a new working directory (i.e. "git archive" piped to "tar xf" but regular files are passed thru convert_to_working_tree()). In such an application, after you learn the mode from the containing tree object and know that RelNotes is a symbolic link blob, you still would want the contents out of the pipe going to the same batch interface process that is not filtered.

So I would not mind if we define the semantics of "--filters" as such (as long as it is clearly documented, of course). AFAICS, the batch interface does not call filter_object() for non-blobs, and by returning successfully without doing anything special for a symbolic link from filter_object() automatically gives us the "by default return as-is, but give processed output only for regular file blobs" semantics to the batch mode.

But for a non-batch mode, it feels somewhat funny to be giving the as-is output without saying anything to symbolic links; we can argue that it is being consistent with what we do in the batch mode, though.

Thanks.