Re: [PATCH 4/6] hook: support reordering of hook list

[Junio C Hamano <gitster@pobox.com>]

Emily Shaffer <emilyshaffer@google.com> writes:

>   $ grep -A2 "\[hook\]" ~/.gitconfig
>   [hook]
>           pre-commit = 001:~/test.sh
>           pre-commit = 999:~/baz.sh
>   $ grep -A2 "\[hook\]" ~/git/.git/config
>   [hook]
>           pre-commit = 900:~/bar.sh
>           pre-commit = 050:~/baz.sh
>   $ ./bin-wrappers/git hook --list pre-commit
>   001     global  ~/test.sh
>   050     repo    ~/baz.sh
>   900     repo    ~/bar.sh
>
> In the above example, '~/baz.sh' is provided in the global config with
> order position 999. Then, in the local config, that order is overridden
> to 050. Instead of running ~/baz.sh twice (at order 050 and at order
> 999), only run it once, in the position specified last in config order.

Doesn't that depend on the nature of the hook?  A hook that is
general enough to be used to inspect if another hook's effect is
sane and reject the result may want to be run after invocation of
each hook that is not itself, so I would prefer to avoid a design
that forbids the same command to be specified twice.

I would love it if it were possible without the precedence order and
instead the order of appearance in git_config() stream were usable
to decide the order these hooks are executed.  Unfortunately, there
is a fixed order that the configuration files are read, and I do not
see a way short of adding <number>: prefix like this design does to
ensure that a hook defined in the local config can run before or
after a hook defined in the global config, so <number>: in the above
design is probably a necessary evil X-<.

Having said that, I have a suspicion that the config file itself
should be kept simple---if a hook appears twice with different
numbers, they would be run twice, for example---and the tooling
around it (e.g. "git hook add/edit/replace/reorder") should
implement such a policy (e.g. "the same hook can run only once, so
remove the other entries when adding the same") if desired.

Which would mean that overriding/disabling an entry in the same
configuration file should be done by replacing or removing the
entry.  Adding another entry for the same command with different
precedence should mean the command would run twice.

And you would need a notation to override or disable an entry in a
different configuration file (e.g. global tells us to run foo.sh at
level 50 with "hook.pre-commit=50:foo.sh"; repository wants to say
not to run it at all, or run it at 80 instead).  I would think you'd
just need a notation to kill an existing entry (e.g. the local one
adds "hook.pre-commit=-50:foo.sh" to countermand the entry in the
earlier example, and then can add another one at level 80 if it
desires).

I am also tempted to say that the precedence level may not stay to
be the only attribute for a <hookname, executable> pair wants to
keep.  Instead of

	[hook]
                pre-commit = 900:bar.sh

it may have to become more like

	[hook "pre-commit"]
		level = 900
		path = bar.sh

if we do not want to paint us into a corner from which we cannot get
out of.  I dunno.

Doesn't this require coordination between the three configuration
sources how numbers are assigned and used, by the way?  Between the
per-user and the per-repository config, they are set by the same
person anyway, so there is not much to coordinate, but I am not sure
what the expectations are to allow reading from the system-wide
configuration (or, should we just disable reading from the
system-wide configuration for, say, security reasons?)