| Message ID | 20201222000220.1491091-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | propose config-based hooks (part I) | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > Since v6: > > - Converted 'enum hookdir_opt' to UPPER_SNAKE > - Coccinelle fix in the hook destructor > - Fixed a bug where builtin/hook.c wasn't running the default git config setup > and therefore missed hooks in core.hooksPath when it was set. (These hooks > would still run except when invoked by 'git hook run' as the config was > called by the processes which invoked the hook library.) Thanks. Queued both series (it probably is easier to think of these as a single 34-patch series, as long as they both are in flight at the same time).
On Mon, Dec 21, 2020 at 06:11:05PM -0800, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > Since v6: > > > > - Converted 'enum hookdir_opt' to UPPER_SNAKE > > - Coccinelle fix in the hook destructor > > - Fixed a bug where builtin/hook.c wasn't running the default git config setup > > and therefore missed hooks in core.hooksPath when it was set. (These hooks > > would still run except when invoked by 'git hook run' as the config was > > called by the processes which invoked the hook library.) > > Thanks. Queued both series (it probably is easier to think of these > as a single 34-patch series, as long as they both are in flight at > the same time). > Do you want me to send them as a single thread for next version?
On Mon, Dec 28, 2020 at 02:37:16PM -0800, Emily Shaffer wrote: Argh. I am having awful Monday brain and this should have been in-reply-to the other thread. I guess that's a point in opposition of splitting big topics into multiple threads. :| I'll resend it on the other topic. I'm sorry. - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > On Mon, Dec 21, 2020 at 06:11:05PM -0800, Junio C Hamano wrote: >> >> Emily Shaffer <emilyshaffer@google.com> writes: >> >> > Since v6: >> > >> > - Converted 'enum hookdir_opt' to UPPER_SNAKE >> > - Coccinelle fix in the hook destructor >> > - Fixed a bug where builtin/hook.c wasn't running the default git config setup >> > and therefore missed hooks in core.hooksPath when it was set. (These hooks >> > would still run except when invoked by 'git hook run' as the config was >> > called by the processes which invoked the hook library.) >> >> Thanks. Queued both series (it probably is easier to think of these >> as a single 34-patch series, as long as they both are in flight at >> the same time). >> > > Do you want me to send them as a single thread for next version? Unless we deliberately focus on stabilizing the early 17 patches into a shape that they won't need updating while working on the later part of the series, I'd guess that your next resend would contain updated versions of these 17 patches, so the only effect that it has to pretend that the patches belong to two separate series is to invite mistakes while queuing on my part. So either (1) a single thread of all patches, or (2) just the early part to really make sure everybody is happy with them, so that we can graduate it early even while the remainder may be going through revisions, would be more preferrable than the way they have been structured so far. Thanks.
On Mon, Dec 21, 2020 at 04:02:03PM -0800, Emily Shaffer wrote: > > Since v6: > > - Converted 'enum hookdir_opt' to UPPER_SNAKE > - Coccinelle fix in the hook destructor > - Fixed a bug where builtin/hook.c wasn't running the default git config setup > and therefore missed hooks in core.hooksPath when it was set. (These hooks > would still run except when invoked by 'git hook run' as the config was > called by the processes which invoked the hook library.) > > CI run: https://github.com/nasamuffin/git/actions/runs/436864964 Some updates on this series... Since Jan 21 we've been running this series as picked from gitster/git:es/config-hooks on Googler machines, with a subset of users asked to try out putting their hooks into config instead of hookdir. So far we haven't heard any crashes or bugs like that, although I did hear a couple places where the user documentation is lacking. I feel encouraged by that, and I'm hoping to improve the documentation in the next week or so, pending $DAYJOB concerns. We also addressed some of this series in our every-other-week review club (me, Jonathan Tan, Jonathan Nieder, and Josh Steadmon; although in this case I tried to be quiet :) ) and so I hope there will be some comments from my three teammates coming to list sometime next week. Since I feel pretty comfortable that it doesn't seem to explode anywhere, I'm really keen to hear nitpicky reviews and try to push to get this into 'next'; maybe I can barter my eyes on someone else's neglected review? That sounds pretty mercenary but I think Junio is the one who suggested it a few weeks ago... ;) - Emily
On 2020.12.21 16:02, Emily Shaffer wrote: > Since v6: > > - Converted 'enum hookdir_opt' to UPPER_SNAKE > - Coccinelle fix in the hook destructor > - Fixed a bug where builtin/hook.c wasn't running the default git config setup > and therefore missed hooks in core.hooksPath when it was set. (These hooks > would still run except when invoked by 'git hook run' as the config was > called by the processes which invoked the hook library.) > > CI run: https://github.com/nasamuffin/git/actions/runs/436864964 > > Thanks! > - Emily > > Emily Shaffer (17): > doc: propose hooks managed by the config > hook: scaffolding for git-hook subcommand > hook: add list command > hook: include hookdir hook in list > hook: respect hook.runHookDir > hook: implement hookcmd.<name>.skip > parse-options: parse into strvec > hook: add 'run' subcommand > hook: replace find_hook() with hook_exists() > hook: support passing stdin to hooks > run-command: allow stdin for run_processes_parallel > hook: allow parallel hook execution > hook: allow specifying working directory for hooks > run-command: add stdin callback for parallelization > hook: provide stdin by string_list or callback > run-command: allow capturing of collated output > hooks: allow callers to capture output > > .gitignore | 1 + > Documentation/Makefile | 1 + > Documentation/config/hook.txt | 19 + > Documentation/git-hook.txt | 117 +++++ > Documentation/technical/api-parse-options.txt | 5 + > .../technical/config-based-hooks.txt | 355 +++++++++++++++ > Makefile | 2 + > builtin.h | 1 + > builtin/bugreport.c | 4 +- > builtin/fetch.c | 1 + > builtin/hook.c | 176 ++++++++ > builtin/submodule--helper.c | 2 +- > command-list.txt | 1 + > git.c | 1 + > hook.c | 416 ++++++++++++++++++ > hook.h | 154 +++++++ > parse-options-cb.c | 16 + > parse-options.h | 4 + > run-command.c | 85 +++- > run-command.h | 31 ++ > submodule.c | 1 + > t/helper/test-run-command.c | 46 +- > t/t0061-run-command.sh | 37 ++ > t/t1360-config-based-hooks.sh | 256 +++++++++++ > 24 files changed, 1717 insertions(+), 15 deletions(-) > create mode 100644 Documentation/config/hook.txt > create mode 100644 Documentation/git-hook.txt > create mode 100644 Documentation/technical/config-based-hooks.txt > create mode 100644 builtin/hook.c > create mode 100644 hook.c > create mode 100644 hook.h > create mode 100755 t/t1360-config-based-hooks.sh > > -- > 2.28.0.rc0.142.g3c755180ce-goog > Sorry for the delayed reply. I am happy with this series as-is. Thanks for all your work on it! Reviewed-by: Josh Steadmon <steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes: >> Emily Shaffer (17): >> doc: propose hooks managed by the config >> hook: scaffolding for git-hook subcommand >> hook: add list command >> hook: include hookdir hook in list >> hook: respect hook.runHookDir >> hook: implement hookcmd.<name>.skip >> parse-options: parse into strvec >> hook: add 'run' subcommand >> hook: replace find_hook() with hook_exists() >> hook: support passing stdin to hooks >> run-command: allow stdin for run_processes_parallel >> hook: allow parallel hook execution >> hook: allow specifying working directory for hooks >> run-command: add stdin callback for parallelization >> hook: provide stdin by string_list or callback >> run-command: allow capturing of collated output >> hooks: allow callers to capture output >> > Sorry for the delayed reply. I am happy with this series as-is. Thanks > for all your work on it! > > Reviewed-by: Josh Steadmon <steadmon@google.com> The topic branch has a lot more commits than these 17; I am wondering if the reviewed-by applies only to the bottom 17, or as the whole? I recall that the upper half was expecting at least some documentation updates. Thanks.
On 2021.02.16 14:47, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > >> Emily Shaffer (17): > >> doc: propose hooks managed by the config > >> hook: scaffolding for git-hook subcommand > >> hook: add list command > >> hook: include hookdir hook in list > >> hook: respect hook.runHookDir > >> hook: implement hookcmd.<name>.skip > >> parse-options: parse into strvec > >> hook: add 'run' subcommand > >> hook: replace find_hook() with hook_exists() > >> hook: support passing stdin to hooks > >> run-command: allow stdin for run_processes_parallel > >> hook: allow parallel hook execution > >> hook: allow specifying working directory for hooks > >> run-command: add stdin callback for parallelization > >> hook: provide stdin by string_list or callback > >> run-command: allow capturing of collated output > >> hooks: allow callers to capture output > >> > > Sorry for the delayed reply. I am happy with this series as-is. Thanks > > for all your work on it! > > > > Reviewed-by: Josh Steadmon <steadmon@google.com> > > The topic branch has a lot more commits than these 17; I am > wondering if the reviewed-by applies only to the bottom 17, or as > the whole? I recall that the upper half was expecting at least some > documentation updates. > > Thanks. Just to these 17, sorry for being unclear.
Josh Steadmon <steadmon@google.com> writes: >> The topic branch has a lot more commits than these 17; I am >> wondering if the reviewed-by applies only to the bottom 17, or as >> the whole? I recall that the upper half was expecting at least some >> documentation updates. >> >> Thanks. > > Just to these 17, sorry for being unclear. Thanks for reading them through. I am tempted to say we should merge these "mechanism" part down to 'next', hoping that the "rewrite existing ones using the new mechansim" part can follow soon.
Junio C Hamano <gitster@pobox.com> writes: > Josh Steadmon <steadmon@google.com> writes: > >>> The topic branch has a lot more commits than these 17; I am >>> wondering if the reviewed-by applies only to the bottom 17, or as >>> the whole? I recall that the upper half was expecting at least some >>> documentation updates. >>> >>> Thanks. >> >> Just to these 17, sorry for being unclear. > > Thanks for reading them through. > > I am tempted to say we should merge these "mechanism" part down to > 'next', hoping that the "rewrite existing ones using the new > mechansim" part can follow soon. I said this on Feb 17th, but since then I think I saw you answer "I'll do that" in responses to JTan's reviews in the past few days (e.g. <YC7o2rUQOEdiMdqh@google.com>). Would I regret if I merge the topic down to 'next' today? Thanks.
On Thu, Feb 25, 2021 at 11:50:11AM -0800, Junio C Hamano wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Josh Steadmon <steadmon@google.com> writes: > > > >>> The topic branch has a lot more commits than these 17; I am > >>> wondering if the reviewed-by applies only to the bottom 17, or as > >>> the whole? I recall that the upper half was expecting at least some > >>> documentation updates. > >>> > >>> Thanks. > >> > >> Just to these 17, sorry for being unclear. > > > > Thanks for reading them through. > > > > I am tempted to say we should merge these "mechanism" part down to > > 'next', hoping that the "rewrite existing ones using the new > > mechansim" part can follow soon. > > I said this on Feb 17th, but since then I think I saw you answer > "I'll do that" in responses to JTan's reviews in the past few days > (e.g. <YC7o2rUQOEdiMdqh@google.com>). Would I regret if I merge the > topic down to 'next' today? Bah, I'm sorry I missed this - I had a broken mutt config and wasn't seeing replies, my own fault. Argh. I have some pretty significant changes from JTan's reviews, so I'd prefer if you would wait since it would be tricky to turn them into a patch commit now. But if you'd rather merge it and see a patch instead, that is fine with me. - Emily
Emily Shaffer <emilyshaffer@google.com> writes: >> I said this on Feb 17th, but since then I think I saw you answer >> "I'll do that" in responses to JTan's reviews in the past few days >> (e.g. <YC7o2rUQOEdiMdqh@google.com>). Would I regret if I merge the >> topic down to 'next' today? > > Bah, I'm sorry I missed this - I had a broken mutt config and wasn't > seeing replies, my own fault. Argh. > > I have some pretty significant changes from JTan's reviews, so I'd > prefer if you would wait since it would be tricky to turn them into a > patch commit now. But if you'd rather merge it and see a patch instead, > that is fine with me. OK, I still have it outside 'next', I think.