Stefan Beller <sbel...@google.com> writes: > On Thu, Jul 13, 2017 at 8:01 AM, Jeff King <p...@peff.net> wrote: > >> builtin/branch.c | 14 +++++++------- >> builtin/for-each-ref.c | 22 ++++++++++++---------- >> builtin/tag.c | 30 ++++++++++++++++-------------- >> builtin/verify-tag.c | 12 ++++++------ >> ref-filter.c | 22 ++++++++++++---------- >> ref-filter.h | 22 +++++++++++++++++----- >> 6 files changed, 70 insertions(+), 52 deletions(-) > > The patch looks good to me. So some off-topic comments: > I reviewed this patch from bottom up, i.e. I started looking at > ref-filter.h, then ref-filter.c and then the rest. If only you had formatted > the patches with an orderfile. ;)
As a reviewer, for this particular patchq, I actually appreciated that ref-filter.[ch] came at the end. That forced me to think. When I see something that used to be 0 is lost from the parameter list of show_ref_array_item() at a callsite, I was forced to guess what it is by looking at what is moved into the new structure nearby, and keep doing that "figure out what is going on" game until the "author's answer" is revealed at the end of the patch. By the time I reached ref-filter.[ch], I had a pretty good idea what was done by only looking at the callers and being able to see if my understanding matched the "author's answer" at the end of the patch turned out to be a very good way to double-check if the patch was sane. If I were given the author's answer upfront, especially an answer by somebody as competent as Peff, I'm sure I would have been swayed into believing that whatever is written in the patch must be correct without thinking the changes needed in the patch myself. I do want to present from Doc to header to code when I am showing my patch to others, so this is probably a good example that illustrates that the preferred presentation order is not just personal preference, but is different on occasion even for the same person.
