[4/6] hook: support reordering of hook list
diff mbox series

Message ID 20191210023335.49987-5-emilyshaffer@google.com
State New
Headers show
Series
  • configuration-based hook management
Related show

Commit Message

Emily Shaffer Dec. 10, 2019, 2:33 a.m. UTC
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(+)

Comments

Junio C Hamano Dec. 11, 2019, 7:21 p.m. UTC | #1
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?)

Patch
diff mbox series

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