Re: [PATCH v7 01/17] doc: propose hooks managed by the config

[Emily Shaffer <emilyshaffer@google.com>]

On Sat, Jan 23, 2021 at 04:38:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Tue, Dec 22 2020, Emily Shaffer wrote:
> 
> >     Since v4, addressed comments from Jonathan Tan about wording. However, I have
> >     not addressed AEvar's comments or done a full re-review of this document.
> >     I wanted to get the rest of the series out for initial review first.
> 
> As you note here and in a couple of other patch notes a lot of the
> current state was based on my v4 commentary. I see we have parallel hook
> execution by default now, woohoo!
> 
> I've been keeping an eye on this series, but have been kicking the can
> down the road on reviewing it.
> 
> Skimming it now I think the state of it looks mostly good now when
> viewing the end result, I think it's mainly got one big problem, but the
> good news is that it's relatively easy to solve :)
> 
> Which is that I think it's really hard to follow along with it because
> 01/17 starts with a big design doc that's partially outdated, and
> partially saying things that aren't in or should be in either a
> user-facing doc or commit message.

Sure.

> 
> And then individual patches (e.g. 12/17) either don't have tests
> associated with them to test the feature they add, don't update/add
> docs, or the docs are at the very beginning.

Thanks, sure.

> 
> I think we should aim to mostly or entirely get rid of
> Documentation/technical/config-based-hooks.txt, it was more of a "what
> about this design?" document in the beginning.

I'm not 100% sure that I agree - there are a couple other design docs in
Documentation/technical which I still refer to from time to time, e.g.
for sparse checkout. But I *do* agree that there's a lot of info there
that needs to be in end-user docs.

> 
> In a series we'd apply most or all of it should really be in end-user
> doc (and stuff like "Future work" can just be noted in commit messages
> as we go along).
> 
> So long story short, I started trying to review this, but found myself
> trying to reply to one patch and then grabbing docs from 01/17, or
> (e.g. for the parallel stuff) not having tests and starting to come up
> with them myself.

Yeah. A related issue I could imagine, although not what you mentioned
here, is needing to do the same thing between part I and part II of the
series, as I often added some functionality late in part I and then used
it first late in part II. I don't think this is worth reordering for,
but probably better notes would be handy.

> 
> So I thought I'd send this E-Mail instead as prodding to maybe convince
> you to re-roll it again to make it easier to follow along in a piecemeal
> fashion.
> 
> As noted before I'm happy to help with this series if needed. I just
> thought I'd send this first given that it's been a month since the last
> submission, perhaps you've got some more local WIP changes by now...

The biggest help for me would be review focused on nits, code style,
missed free(), etc. - I still haven't gotten that kind of review yet,
and I feel the series as it is now is pretty much "feature complete".

In fact, and I'll mention this in reply to the cover letter in a moment,
we picked this series up as-is from Junio's branch in 'gitster/git' and
have been using it at Google for a couple of weeks now - primarily with
users running their legacy hooks living in hookdir, but also with a
subset of users encouraged to try out the config functionality.

Thanks for the bump.
 - Emily