Re: [PATCH v2 02/13] reftable: define the public API

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Oct 01, 2020 at 08:58:51PM -0700, Jonathan Nieder wrote:
> 
> Hi,
> 
> Han-Wen Nienhuys wrote:
> 
> >  reftable/reftable.h | 585 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 585 insertions(+)
> >  create mode 100644 reftable/reftable.h
> 
> Adding a header in a separate patch from the implementation doesn't
> match the usual practice.  Can we add declarations in the same patch
> as the functions being declared instead?
> 
> We could still introduce the header early in its own patch if we want,
> but it would be a skeleton of a header that gets filled out by later
> patches.

To poke a little deeper into what I think Jonathan is trying to say:

When we reviewed this for review club, the general feeling was that A)
this commit structure is still pretty difficult to review, and B) we
want to make sure you aren't spending a ton of time reorganizing the
code only to find that people still don't like the structure (as may
have happened with the current iteration).

One point I think Jonathan is making is that each commit should stand
alone: compile, be tested, and do something succinct. Another point (or
maybe the same point from a different angle) is that a series of commits
should tell a progressive story. That is, something like

  add infrastructure to support reftable library
  reftable: read/write blocks
  reftable: write files
  reftable: read files
  reftable: generate binary tree from file
  ...

is much more compelling (and easier to review) than

  reftable: LICENSE
  reftable: headers
  reftable: tests
  ...

Towards the latter end of your series it seems like you started to take
that approach; but some commits, like this one (reftable: define the
public API) are not quite so. That is, this commit is hard to review
without the context of the rest of the series: I read it, I say, "well
WHY do we need these functions?", and then I cannot continue my review
of this patch until I've completed my review of the rest of the series.

Of course, taking a completed project - like your initial reftable
submission - and then chopping it up into a cute story of commits is a
pain in the ***. Doing it twice - or more - is just aggravating. So I
wonder whether we can bikeshed what story would look nice before you
even pick up your 'git rebase -i'? Doing that bikeshedding here on list
means that we also have a chance for someone to interrupt and say, "no,
that organization doesn't make sense" - or even for someone to say
"Emily, there is no need to reorganize these commits, go sit down" ;)

> All in all, I like this API.  Thanks for putting it together
> thoughtfully.
> 
> The API builds up in layers (blocks, reftables, merged reftables, etc),
> suggesting a natural division for the patch series.  I think the rest of
> the series follows that --- let's see.

Or maybe what I said runs contrary to what Jonathan was actually saying.
Please, correct me :)

 - Emily