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

Emily Shaffer <emilyshaffer@google.com> writes:

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?)