Re: [PATCH v8 11/15] bugreport: collect list of populated hooks
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Feb 25, 2020, 23:19 UTC
- Message-ID
- <20200225231918.GC212281@google.com>
- In-Reply-To
- <xmqqeeup9ds7.fsf@gitster-ct.c.googlers.com>
On Thu, Feb 20, 2020 at 12:58:32PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> Emily Shaffer <emilyshaffer@google.com> writes:
>
> > +static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> > +{
> > + /*
> > + * Doesn't look like there is a list of all possible hooks; so below is
> > + * a transcription of `git help hooks`.
> > + */
>
> It may want to become a NEEDSWORK comment.Oh, good point. I'll rephrase it.
Show 26 quoted lines
> > > + const char *hooks = "applypatch-msg," > > + "pre-applypatch," > > + "post-applypatch," > > + "pre-commit," > > + "pre-merge-commit," > > + "prepare-commit-msg," > > + "commit-msg," > > + "post-commit," > > + "pre-rebase," > > + "post-checkout," > > + "post-merge," > > + "pre-push," > > + "pre-receive," > > + "update," > > + "post-receive," > > + "post-update," > > + "push-to-checkout," > > + "pre-auto-gc," > > + "post-rewrite," > > + "sendemail-validate," > > + "fsmonitor-watchman," > > + "p4-pre-submit," > > + "post-index-changex"; > > Typo here?
Yep, I imagine from trying to press 'x' to remove the ',' from my list.
Show 37 quoted lines
>
> > + struct string_list hooks_list = STRING_LIST_INIT_DUP;
> > + struct string_list_item *iter = NULL;
> > +
> > +
> > + if (nongit) {
> > + strbuf_addstr(hook_info,
> > + "not run from a git repository - no hooks to show\n");
> > + return;
> > + }
> > +
> > + string_list_split(&hooks_list, hooks, ',', -1);
> > +
> > + for_each_string_list_item(iter, &hooks_list) {
>
> I do not get why you want to use string_list for this, especially if
> you need to use string_list_split.
>
> To me,
>
> int i;
> const char *hook[] = {
> "applypatch-msg",
> "pre-applypatch",
> ...
> "post-index-change",
> };
>
> for (i = 0; i < ARRAY_SIZE(hook); i++) {
> if (hook[i] is enabled)
> strbuf_addf(hook_info, "%s\n", hook[i]);
> }
>
> would be far easier to understand. Do you have an external source
> that can feed you a single long string of comma separated hook names
> in mind, so that the initialization of *hooks will become simpler
> that way, or something?Huh. I'm having a hard time remembering why I did it this way. I think I wanted to use the string_list foreach? But you're right that it is pretty hard to understand, plus expensive; I'll just use an array like you suggest.
Show 21 quoted lines
>
> > + if (find_hook(iter->string)) {
> > + strbuf_addstr(hook_info, iter->string);
> > + strbuf_complete_line(hook_info);
> > + }
> > + }
> > +}
> > +
> > static const char * const bugreport_usage[] = {
> > N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
> > NULL
> > @@ -166,6 +216,9 @@ int cmd_main(int argc, const char **argv)
> > get_header(&buffer, "Safelisted Config Info");
> > get_safelisted_config(&buffer);
> >
> > + get_header(&buffer, "Enabled Hooks");
> > + get_populated_hooks(&buffer, nongit_ok);
> > +
> > report = fopen_for_writing(report_path.buf);
> >
> > if (report == NULL) {