patchgrep: Add option --max-line-len

This tells git grep to skip files longer than a specified length, which is often the result of generators and not actual source files.

Signed-off-by: Marc-Antoine Ruel <maruel@chromium.org>
---
 Documentation/git-grep.txt | 5 +++++
 builtin/grep.c             | 2 ++
 grep.c                     | 4 ++++
 grep.h                     | 1 +
 t/t7810-grep.sh            | 5 +++++
 5 files changed, 17 insertions(+)

On Thu, Nov 23, 2017 at 10:41 AM, Marc-Antoine Ruel <maruel@chromium.org> wrote:

s/files/lines/

This documentation doesn't explain what it means by a line's length. This implementation seems to take into consideration only the line's byte count, however, given that displayed lines are normally tab-expanded, people might intuitively expect that expansion to count toward the length. A similar question arises for Unicode characters.

Should this option take tab-expansion and Unicode into account? Regardless of the answer to that question, the documentation should make it clear what "line length" means.

On this project, it is typical to have only the long form of an option name when the option is first implemented so as not to squat on one of the relatively limited number of short option names. Only after it is seen that an option is popular, does it get a short name. Whether this actually matters in this case, I don't know, but is something to take into consideration.

Why the choice of 'M'? Is there precedence in other Git or Unix commands for that name over, say, 'N' or <choose-your-favorite-letter>? Is 'M' is for "max"? If so, what would a short name for --max-depth be (if it ever got one)? (These are somewhat rhetorical questions, but I'm interested in the answers if you have any.)

If the user specifies "-M0", should that error out or at least warn the user that the value is non-sensical? What about -1, etc.? (These are UX-related questions; the implementation obviously doesn't care one way or the other.)

Meh, what is this actually testing? The output of "git grep -M18 -W include" is no different that the output of "git grep -W include" in the test just above this one. And, why is -W in this test at all? Its presence confuses the reader into thinking that it is somehow significant. I would have expected this test to look like this:

    cat >expected <<-\EOF
    hello.c:#include <stdio.h>
    EOF

    test_expect_success 'grep -M skips long lines' '
        git grep -M18 include >actual &&
        test_cmp expected actual
    '

Marc-Antoine Ruel <maruel@chromium.org> writes:

All the excellent review comments from Eric I agree with.

With the name of the option and the above end-user facing description, it is very clear that the only thing this feature does is to declare that an overlong line does _not_ match when trying to check against any pattern.

That is a much clearer definition and description than random new features people propose here (and kicked back by reviewers, telling them to make the specification clearer), and I'd commend you for that.

But it still leaves at least one thing unclear. How should it interact with "-v"? If we consider an overlong line never matches, would "git grep -v <pattern>" should include the line in its output?

Speaking of the output, it also makes me wonder if the feature really wants to include an overlong line as a context line when showing a near-by line that matches the pattern when -A/-B/-C/-W options are in use. Even though it is clear that it does from the above description, is it really the best thing the feature can do to help the end users?

Which leads me to suspect that this "feature" might not be the ideal you wanted to achive, but is an approximate substitution that you found is "good enough" to simulate what the real thing you wanted to do, especially when I go back and read the justfication in the proposed log message that talks about "result of generators".

Isn't it a property of the entire file, not individual lines, if you find it uninteresting to see reported by "git grep"? I cannot shake the suspicion that this feature happened to have ended up in this shape, instead of "ignore a file with a line this long", only because your starting point was to use "has overlong lines" as the heuristic for "not interesting", and because "git grep" code is not structured to first scan the entire file to decide if it is worth working on it, and it is extra work to restructure the codeflow to make it so (which you avoided).

If your real motivation was either

 (1) whether the file has or does not have the pattern for certain
     class of files are uninteresting; do not even run "grep"
     processing for them; or

 (2) hits or no-hits may be intereseting but output of overlong
     lines from certain class of files I do not wish to see;

then I can think of two alternatives.

For (1), can't we tell "result of generators" and other files with pathspec? If so, perhaps a negative pathspec can rescue. e.g.

    git grep <pattern> -- '*.cc' ':!*-autogen.cc'

For (2), can't we model this after how users can tell "git diff" that certain paths are not worth computing and showing textual patches for, which is to Unset the 'diff' attribute? When you have

    *-autogen.cc	-diff

in your .gitattributes, "git diff" would say "Binary files A and B differ" instead of explaining line-by-line differences in the patch form. Perhaps we can also have a 'grep' attribute and squelch the output if it is Unset?

It is debatable but one could propose extending the use of existing 'diff' attribute to cover 'grep' too, with an argument that anything not worth showing patch (i.e. 'diff' attribute is Unset) is not worth showing grep hits from.

[second try, now with text format]

Thanks a lot for the reviews. Replying to both.

If I send a follow up, I'll fix the commit description and the help string, remove the shorthand -M, write a more sensible test.

2017-11-23 14:24 GMT-05:00 Eric Sunshine <sunshine@sunshineco.com>:

Excellent question. I don't have an immediate answer.

Precedent with -A is to ignore the negative value. I don't have a strong opinion.

2017-11-23 20:44 GMT-05:00 Junio C Hamano <gitster@pobox.com>:

Ah! No idea. :/

Thanks for the thoughtful analysis. My main motivation was (1), thus filtering with a pathspec is much better than trying to work around the issue. The issues raised in the review are significant enough that committing this patch could cause significant issues; I don't know how to resolve handling with -v and how to handle tabs.

After further thinking, what I'd like is a smarter version of the git-gs shortcut wrapper that limits the search space on well known extensions but I'd like it to also limit itself to "source-like" files, similar in some ways to the -I flag. So in some ways this could be better served as a git config but I'm not even sure what kind of heuristic would be generic enough to be valuable to a large number of users.

As such I'll drop the patch as I don't see a clear path forward with the current one.

Thanks,

M-A

Marc-Antoine Ruel <maruel@chromium.org> writes:

OK.  Thanks for thinking it through.