Subject: Re: [PATCH v8 11/15] bugreport: collect list of populated hooks

From: Emily Shaffer <[email-masked]>
Date: Tue, 25 Feb 2020 15:19:18 -0800

On Thu, Feb 20, 2020 at 12:58:32PM -0800, Junio C Hamano wrote:
> Emily Shaffer <[email-masked]> 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.
> 
> > +	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.

> 
> > +	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.

> 
> > +		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) {