Update behavior for postCommand.strategy=post-index-change (#748)

This feature was introduced in #736 but had a few pain points that were
discovered when testing the full replacement of the existing
post-index-change and post-command hooks in our internal environment:

1. When using `core.hooksPath` to point to a directory within the
worktree, the sentinel files were being written into the worktree. Write
to `$GITDIR/info/` instead.
2. The existing internal post-index-change hook only signaled that work
needed to be done if actually the _worktree_ changed. Make the sentinel
file logic more restrictive to only write in these cases.

Tests are added to confirm both of these behavior changes.

To reflect the changed behavior, the name `post-index-change` was renamed
to `worktree-change`.

Author: dscho

Documentation/config/postcommand.adoc

@@ -7,7 +7,7 @@ postCommand.strategy::
 `always`;;
 	run the `post-command` hook on every process (default).
 
-`post-index-change`;;
+`worktree-change`;;
 	run the `post-command` hook only if the current process wrote to
-	the index.
+	the index and updated the worktree.
 ----

hook.c

@@ -190,7 +190,11 @@ static char *get_post_index_change_sentinel_name(struct repository *r)
 	if (slash)
 		*slash = 0;
 
-	repo_git_path_replace(r, &path, "hooks/index-change-%s.snt", sid);
+	/*
+	 * Do not write to hooks directory, as it could be redirected
+	 * somewhere like the source tree.
+	 */
+	repo_git_path_replace(r, &path, "info/index-change-%s.snt", sid);
 
 	return strbuf_detach(&path, NULL);
 }
@@ -229,6 +233,37 @@ static int post_index_change_sentinel_exists(struct repository *r)
 	return res;
 }
 
+/**
+ * See if we can replace the requested hook with an internal behavior.
+ * Returns 0 if the real hook should run. Returns nonzero if we instead
+ * executed custom internal behavior and the real hook should not run.
+ */
+static int handle_hook_replacement(struct repository *r,
+				   const char *hook_name,
+				   struct strvec *args)
+{
+	const char *strval;
+	if (repo_config_get_string_tmp(r, "postcommand.strategy", &strval) ||
+	    strcasecmp(strval, "worktree-change"))
+		return 0;
+
+	if (!strcmp(hook_name, "post-index-change")) {
+		/* Create a sentinel file only if the worktree changed. */
+		if (!strcmp(args->v[0], "1"))
+			write_post_index_change_sentinel(r);
+
+		/* We don't skip post-index-change hooks that exist. */
+		return 0;
+	}
+	if (!strcmp(hook_name, "post-command") &&
+	    !post_index_change_sentinel_exists(r)) {
+		/* We skip the post-command hook in this case. */
+		return 1;
+	}
+
+	return 0;
+}
+
 int run_hooks_opt(struct repository *r, const char *hook_name,
 		  struct run_hooks_opt *options)
 {
@@ -255,17 +290,9 @@ int run_hooks_opt(struct repository *r, const char *hook_name,
 	};
 
 	/* Interject hook behavior depending on strategy. */
-	if (r && r->gitdir) {
-		const char *strval;
-		if (!repo_config_get_string_tmp(r, "postcommand.strategy", &strval) &&
-		    !strcasecmp(strval, "post-index-change")) {
-			if (!strcmp(hook_name, "post-index-change"))
-				return write_post_index_change_sentinel(r);
-			if (!strcmp(hook_name, "post-command") &&
-			    !post_index_change_sentinel_exists(r))
-				return 0;
-		}
-	}
+	if (r && r->gitdir &&
+	    handle_hook_replacement(r, hook_name, &options->args))
+		return 0;
 
 	hook_path = find_hook(r, hook_name);
 

t/t0401-post-command-hook.sh

@@ -32,14 +32,20 @@ test_expect_success 'with failing pre-command hook' '
 '
 
 test_expect_success 'with post-index-change config' '
-	mkdir -p .git/hooks &&
-	write_script .git/hooks/post-command <<-EOF &&
+	mkdir -p internal-hooks &&
+	write_script internal-hooks/post-command <<-EOF &&
 	echo ran >post-command.out
 	EOF
-	write_script .git/hooks/post-index-change <<-EOF &&
+	write_script internal-hooks/post-index-change <<-EOF &&
 	echo ran >post-index-change.out
 	EOF
 
+	# prevent writing of sentinel files to this directory.
+	test_when_finished chmod 775 internal-hooks &&
+	chmod a-w internal-hooks &&
+
+	git config core.hooksPath internal-hooks &&
+
 	# First, show expected behavior.
 	echo ran >expect &&
 	rm -f post-command.out post-index-change.out &&
@@ -57,18 +63,26 @@ test_expect_success 'with post-index-change config' '
 	test_cmp expect post-command.out &&
 
 	# Now, show configured behavior
-	git config postCommand.strategy post-index-change &&
-	rm -f post-command.out post-index-change.out &&
+	git config postCommand.strategy worktree-change &&
 
 	# rev-parse leaves index intact and thus skips post-command.
+	rm -f post-command.out post-index-change.out &&
 	git rev-parse HEAD &&
 	test_path_is_missing post-index-change.out &&
 	test_path_is_missing post-command.out &&
 
 	echo stuff >>file &&
-	# add updates the index and runs post-command.
+	# add keeps the worktree the same, so does not run post-command.
+	rm -f post-command.out post-index-change.out &&
 	git add file &&
-	test_path_is_missing post-index-change.out &&
+	test_cmp expect post-index-change.out &&
+	test_path_is_missing post-command.out &&
+
+	echo stuff >>file &&
+	# reset --hard updates the worktree.
+	rm -f post-command.out post-index-change.out &&
+	git reset --hard &&
+	test_cmp expect post-index-change.out &&
 	test_cmp expect post-command.out
 '