Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau <me@ttaylorr.com>]

On Sun, Apr 22, 2018 at 08:16:12PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > show_line() currently receives the line number within the
> > 'grep_opt->buf' in order to determine which line number to display. In
> > order to display information about the matching column number--if
> > requested--we must additionally take in that information.
> >
> > To do so, we extend the signature of show_line() to take in an
> > additional unsigned "cno". "cno" is either:
> >
> >   * A 1-indexed column number of the first match on the given line, or
> >   * 0, if the column number is irrelevant (when displaying a function
> >     name, context lines, etc).
>
> This information about how 'cno' is interpreted seems important enough
> to have as an in-code comment somewhere. Unfortunately, this patch
> never actually uses 'cno', so it's hard to add such a comment to
> non-existent code. In fact, the granularity of this patch feels wrong;
> it seems to exist for some purpose but, at the same time, is a
> do-nothing patch.
>
> This issue illustrates a larger problem with how this patch series is
> structured overall. In his review, Ævar suggested collapsing several
> patches into one, but the problems go deeper than that when you have
> patches which implement some bit of functionality but don't document
> that functionality until some later step which exposes some other bit
> of functionality, and so forth. As a reviewer, I expect a patch series
> to hold my hand and lead me on a straightforward journey from building
> blocks to final product, but this series tends to jump around without
> apparent logic.
>
> One way to achieve a more coherent patch series would be to build the
> machinery first and then expose it to the user in various ways. Also,
> each patch which implements some user-facing functionality should also
> document that functionality. For instance, a more understandable
> series might be structured something like this:
>
>   1. grep: match_line: expose matched column
>   2. grep: extend grep_opt to allow showing matched column
>   3. grep: display column number of first match
>   4. builtin/grep: add --column-number option
>   5. grep: add configuration variables to show matched column
>   6. contrib/git-jump: jump to match column in addition to line
>
> There may be fewer or more patches than shown here (I believe Ævar
> suggested a cleanup patch), but this should give the general idea.
> Patches 4 and 5 might also be swapped if that seems more logical.
>
> (Sorry if any of the above sounds harsh; it's not meant to be, but is
> intended to be constructive.)

I appreciate the suggestion, and did not take it as harsh. I think that
the existing structure is OK, but I think that I am biased since I'm the
author of the series. I have given your proposed structure a shot and I
think that it should improve the readability of this series for others
on the list as well.

To avoid sending more email than is necessary, I have pushed a draft of
this series to my copy of Git, and would greatly appreciate your
feedback on whether the structure of these patches is sensible. If they
all seem OK to you, I'll happily push v3.

For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno

> > We additionally modify all calls to show_line() in order to pass the new
> > required argument.
>
> Nit: No need to state the obvious; this final sentence could easily be dropped.

Fair; I have removed this from the patch.


Thanks,
Taylor