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

On Sat, Jan 23, 2021 at 04:38:31PM +0100, Ævar Arnfjörð Bjarmason wrote:

On Tue, Dec 22 2020, Emily Shaffer wrote:

quoted

    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