On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau <me@xxxxxxxxxxxx> 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.) > 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. > Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
