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
