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

[Jeff King <peff@peff.net>]

On Thu, Jan 26, 2012 at 05:45:16PM +0100, Michael Haggerty wrote:

> I think decisions such as whether to include an imported module in "git
> diff" output is a personal preference and should not be decided at the
> level of the git project.

You're right. I thought of it as an annotation that the project could
mark via .gitattributes, or the user could mark via .git/info/attributes.
But that is not following the right split of responsibility for
attributes and config. The attributes should annotate "this isn't really
part of the regular git code base" or "this is really part of the
nedmalloc codebase". And then the _config_ should say "when I am
grepping, I am not interested in nedmalloc". I.e.:

  # mark a set of paths with an attribute
  echo "compat/nedmalloc external" >>.gitattributes

  # and then ignore that attribute for this grep
  git grep --exclude-attr=external 

  # or for all greps
  git config --add grep.exclude external

and git doesn't even have to care about what the attribute is called.
It's between the project and the user how they want to annotate their
files, and how they want to feed them to grep.

Or any other program, for that matter. I wonder if this could also be a
more powerful way of grouping files to be included or excluded from diff
pathspecs. Something like (and I'm just talking off the top of my head,
so there may be some syntactic conflicts here):

  # annotate some files
  cat >>.gitattributes <<-\EOF
  t/t????-*.sh test-script
  t/lib-*.sh test-script
  t/test-lib.sh test-script
  EOF

  # and then consider the tagged files to be a group, and look only at
  # that group
  git log :attr:test-script

  # ditto, but imagine we had the negative pathspecs Duy has proposed
  git log :~attr:test-script

That seems kind of cool to me. But maybe it is getting into crazy
over-engineering. I like the idea that we don't need a new option to
grep or diff; rather it is simply a new syntax for mentioning paths.

> The in-tree .gitattributes files should, by and large, just *describe*
> the files and leave it to users to associate policies with the tags
> (or at least make it possible for users to override the policies) via
> .git/info/attributes.  For example, the repository could set an
> "external=nedmalloc" attribute on all files under compat/nedmalloc,
> and users could themselves configure a macro "[attr]external -diff
> -grep" (or maybe something like "[attr]external=nedmalloc -diff
> -grep") if that is their preference.

So obviously I took what you were saying here and ran with it above. But
I do disagree with one thing here: the attributes should be giving some
tag to the paths, but the actual decision about whether to grep should
be part of the _config_. That's the usual split we have for all of the
other attributes, and I think it makes sense and has worked well.

> Is it really common to want to use the same argument on multiple macros
> without also wanting to set other things specifically?  If not, then
> there is not much reason to complicate macros with argument support.

I dunno. I admit my attribute usage tends to just match by extension,
and I generally only have one or two such lines.

> For example, I do something like
> 
>     [attr]type-python type=python text diff=python check-ws
>     *.py type-python
> 
>     [attr]type-makefile type=makefile text diff check-ws -check-tab
>     Makefile.* type-makefile
> 
> for the main file types in my repository, and it is not very cumbersome.

I think it's not a big deal if you are making your own macros. I was
more concerned that people would want to use the "binary" macro to get
the "-grep" automagic, but could not do so because they don't want
"-diff", but rather "diff=foo".

Anyway, after reading your response and thinking on it more, I think
"-grep" is totally the wrong way to go.  If the files are marked binary,
then grep should be respecting "-diff" or the "diff.*.binary" config. If
we want to do more advanced exclusion, then the right place for that is
the config file (or the weird :attr pathspec thing I mentioned above).

> "type-python" and "type=python" seem redundant but they are not.
> "type-python" is needed so that it can be used as a macro.
> "type=python" makes it easier to inquire about the type of a file using
> something like "git check-attr type -- PATH" rather than having to
> inquire about each possible type-* attribute.  It might be nice to
> support a slightly extended macro definition syntax like
> 
>     [attr]type=python text diff=python check-ws
>     *.py type=python
> 
>     [attr]type=makefile text diff check-ws -check-tab
>     Makefile.* type=makefile
> 
> (i.e., macros that are only triggered for particular values of an
> attribute).

I don't think there's any semantic reason why that is not workable. It's
simply not syntactically allowed at this point.

-Peff