Re: [TOPIC 2/17] Hooks in the future

[Emily Shaffer <emilyshaffer@google.com>]

On Fri, Mar 13, 2020 at 10:56:59AM -0700, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:

Phew - now that git-bugreport looks to be on the path to 'next' I get to
work on hooks again :) Forgive me for the late reply.

> 
> > This means that we could do something like this:
> >
> > [hook "/path/to/executable.sh"]
> > 	event = pre-commit
> > 	order = 123
> > 	mustSucceed = false
> > 	parallelizable = true
> >
> > etc, etc as needed.
> 
> You can do
> 
>     [hook "pre-commit"]
> 	order = 123
> 	path = "/path/to/executable.sh"
> 
>     [hook "pre-commit"]
> 	order = 234
> 	path = "/path/to/another-executable.sh"
> 
> as well, and using the second level for what hook the (sub)section
> is about, instead of "we have this path that is used for a hook.
> What hook is it?", feels (at least to me) more natural.

Yeah, I see what you mean, and it's true I misread the notes and
misremembered Peff's suggestion. I was reworking my RFC patch some
today, and noticed that the following two configs:

A.gitconfig:
  [hook "pre-commit"]
    command = "/path/to/executable.sh"
    option = foo
  [hook "pre-commit"]
    command = "/path/to/another-executable.sh"

B.gitconfig:
  [hook "pre-commit"]
    command = "/path/to/executable.sh"
  [hook "pre-commit"]
    option = foo
    command = "/path/to/another-executable.sh"

are indistinguishable during the config parse - both show up during the
config callback looking like:

  value = "hook.pre-commit.command"; var = "/path/to/executable.sh"
  value = "hook.pre-commit.option"; var = "foo"
  value = "hook.pre-commit.command"; var = "/path/to/another-executable.sh"

I didn't see anything to get around this in the config parser library;
if I missed it I'd love to know.

Using the hook path as the subsection still doesn't help, of course. I
think the only way I see around it is to require a specific value at the
beginning of each hook config section, e.g. "each hook entry must begin
with 'command'"; that means that the config parser callback can look
something like:

  parse section, subsection, key
  if section.subsection = "hook.pre-commit":
    if key = "command":
      add a new hook to the hook list
    else:
      operate on the tail of the hook list

The price of this is poor user experience for those handcrafting their
own hook configs, but I don't think it's poorer than carefully spelling
out "123:~/my-hook-path.sh:whatever:other:options" or something. I'll
add that I had planned to teach 'git-hook' to write and modify config
files for the user with an interactive-rebase-like UI, so a brittle
config layout might not be the end of the world.

Or, I suppose, we could teach the config parser how to understand
"structlike" configs like this where repeated header entries need to be
collated together. That seems to be contrary to the semantics of the
config file right now, though, and it looks like it'd require a rework
of the config_set implementation: today config_set_element looks like

  struct config_set_element {
          struct hashmap_entry ent;
          char *key; /* "hook.pre-commit.command" */
          struct string_list value_list; /* "/path/to/executable.sh"
	                                  * "path/to/another-executable.sh"
					  */
  };

I'm not very keen on the idea of changing the way configs are stored for
everyone, although if folks are unsatisfied with the way it is now and
want to do that, I guess it's an option. But it's certainly more
overhead than my earlier suggestion.

Thoughts?

 - Emily