Re: [PATCH] Don't search files with an unset "grep" attribute

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2012, 08:01 UTC
Message-ID: <7vhazb3rtm.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120125214625.GA4666@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 32 quoted lines

> On Mon, Jan 23, 2012 at 10:59:45PM -0800, Junio C Hamano wrote:
>
>> Conrad Irwin <conrad.irwin@gmail.com> writes:
>> > I used to use this approach, hooking into the "diff" attribute directly to mark
>> > a file as binary, however that was clearly a hack.
>> 
>> After thinking about this a bit more, I have to say I disagree that it is
>> a hack.
>
> I kind of agree.
>
> The biggest problem is that the name is wrong.  The "diff.*.command"
> option really is about generating a diff between two blobs of a certain
> type. But "diff.*.textconv" and "diff.*.binary" are really just
> attributes of the file, and may or may not have to do with generating a
> diff. Ditto for diff.*.funcname, I think.
>
> You argue, and I agree, that if we are talking about attributes of the
> files and not diff-specific things, then other parts of git can and
> should make use of that information.
>
> So if this was all spelled:
>
>   $ cat .gitattributes
>   *.pdf filetype=pdf
>   $ cat .git/config
>   [filetype "pdf"]
>           binary = true
>           textconv = pdf2txt
>
> I think it would be a no-brainer that those type attributes should apply
> to "git grep".

I think this discussion has, instead of forking into two equally interesting subthreads, veered to a more intellectually stimulating tangent and we ended up losing focus.

Regardless of what to do with "I do not want to grep in these types of files" and "I want textconv applied when grepping in these types", which would be new attributes to implement two new features, I would like to see us first concentrate on fixing the "binary" issue. When somebody tells us "Your autodetection may screw it up, but this file is binary; just show 'Binary files differ.' when comparing." with "-diff" (or "binary"), we should honor that when "git grep" decides if it should take the 'Binary file matches' codepath. We currently do not, and it clearly is a bug.

This is especially made somewhat urgent because I do not want a half-baked "two pathspecs" approach that only "git grep" knows about when we add the support for "git grep --exclude-path=...".

We should have to teach the underlying machinery that matches pathspec about negative pathspec entries only once. After we have done so, all the callers, not just "git grep", should be able to take advantage of the change by just learning to place negative pathspec entries in the "struct pathspec" they pass to the machinery. Doing anything else will lead to madness of adding ad-hoc "here we should further filter with the other negative 'struct pathspec'" in each and every application.

But I suspect that it would not materialize anytime soon. And I also suspect that the correct handling of 'Binary file matches', which is a pure bugfix, should solve the original issue started these threads 90% in practice.