| Message ID | 20191210023335.49987-5-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | configuration-based hook management | expand |
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?)
diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index a141884239..0f7115f826 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -20,6 +20,14 @@ This command parses the default configuration files for lines which look like single hook. Hooks are sorted in ascending order by order number; in the event of an order number conflict, they are sorted in configuration order. +The order number of a hook can be changed at a more local scope, e.g.: + + git config --add --global hook.pre-commit "001:/foo.sh" + git config --add --local hook.pre-commit "005:/foo.sh" + +When the order number is respecified this way, the previously specified hook +configuration is overridden. + OPTIONS ------- diff --git a/hook.c b/hook.c index f8d1109084..a7dcd18a2e 100644 --- a/hook.c +++ b/hook.c @@ -64,6 +64,13 @@ static int check_config_for_hooks(const char *var, const char *value, void *hook emplace_hook(pos, order, command); added = 1; } + + /* + * if the command already exists, this entry should be + * replacing it. + */ + if (!strcmp(item->command.buf, command)) + remove_hook(pos); } if (!added) diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh index 1434051db3..1af43ef18d 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -47,4 +47,18 @@ test_expect_success 'order number collisions resolved in config order' ' test_cmp expected actual ' +test_expect_success 'adding a command with a different number reorders list' ' + cat >expected <<-\EOF && + 010 repo /path/abc + 050 repo /path/def + 100 repo /path/ghi + 990 repo /path/rst + 999 global /path/uvw + EOF + + git config --add --local hook.pre-commit "050:/path/def" && + git hook --list pre-commit >actual && + test_cmp expected actual +' + test_done
It's possible that in most cases a user wants to run pre-commit hook 'A', but in exactly one repo that user wants to run pre-commit hook 'A' first instead. Teach 'git hook' to support this by allowing a user to specify a new order number for a hook after the initial hook has been specified. For example: $ 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. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/git-hook.txt | 8 ++++++++ hook.c | 7 +++++++ t/t1360-config-based-hooks.sh | 14 ++++++++++++++ 3 files changed, 29 insertions(+)