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
