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

> 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.)