On Sun, Apr 22, 2018 at 9:17 PM, Taylor Blau <me@xxxxxxxxxxxx> wrote: > On Sun, Apr 22, 2018 at 08:16:12PM -0400, Eric Sunshine wrote: >> 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 > > 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 Thanks. Perhaps not surprisingly, I find the restructured patch series easier to follow and review. Each patch has a very specific purpose, thus demands lower review-time cognitive load than with the old structure. One important issue I noticed is that patch 3/7 neglects to update grep.c:init_grep_defaults() to initialize opt.color_columnno. Looking at the tests again, are you gaining anything by placing them inside that for-loop? (Genuine question.)
