Subject: Re: [PATCH v4 3/9] hook: add list command

From: Emily Shaffer <[email-masked]>
Date: Tue, 6 Oct 2020 13:46:08 -0700

On Wed, Sep 23, 2020 at 04:04:51PM -0700, Jonathan Tan wrote:
> 
> >   $ git hook list pre-commit
> >   ~/baz/from/hookcmd.sh
> >   ~/bar.sh
> 
> In the tests below, there is a "local:" prefix (or similar). It's
> clearer if the commit message has that too.
> 
> Also, looking at a later commit, the "list" command probably should
> include the legacy hook if it exists.

Have added it as a separate patch for v5, hopefully that will make more
sense.

> 
> > +static void emplace_hook(struct list_head *pos, const char *command)
> > +{
> > +	struct hook *to_add = malloc(sizeof(struct hook));
> > +	to_add->origin = current_config_scope();
> > +	strbuf_init(&to_add->command, 0);
> > +	/* even with use_shell, run_command() needs quotes */
> > +	strbuf_addf(&to_add->command, "'%s'", command);
> > +
> > +	list_add_tail(&to_add->list, pos);
> > +}
> 
> It might be odd to a programmer reading this that an existing "struct
> hook" with the same name is not reused - the scanning of the list done
> in hook_config_lookup() could probably go here instead.

Sure, done.

> 
> > +test_expect_success 'git hook list orders by config order' '
> > +	setup_hooks &&
> > +
> > +	cat >expected <<-EOF &&
> > +	global:	$ROOT/path/def
> > +	local:	$ROOT/path/ghi
> 
> Will the "global" strings etc. be translated? If yes, it's probably not
> worth it to align the paths in this way.

Asked more offline. Jonathan was saying that translation might result in
scope name + tab character leaving the path in different columns
depending on the scope anyways, so there's no point in using a tab
character instead of a space character here. That seems reasonable; I'll
switch.

 - Emily