* [PATCH 1/1] switch/restore: consistently pass through exit code of `post-checkout`
2019-10-10 12:01 [PATCH 0/1] Pass through the exit code of post-checkout Johannes Schindelin via GitGitGadget
@ 2019-10-10 12:01 ` Johannes Schindelin via GitGitGadget
2019-10-11 4:22 ` [PATCH 0/1] Pass through the exit code of post-checkout Junio C Hamano
1 sibling, 0 replies; 3+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-10 12:01 UTC (permalink / raw)
To: git
Cc: Jonathan Nieder, Magne Land, Johannes Schindelin, Junio C Hamano,
Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Currently, the exit code of the `post-checkout` hook becomes the exit
code of the `git restore` (or the equivalent `git checkout`) command,
_iff_ no error occurred during the `restore` operation.
That allows versatile scripting where the `post-checkout` failure modes
can be discerned from the `git restore` failure modes.
There is a problem, though: if that `git restore` operation fails
_before_ the `post-checkout` hook is run, -1 is returned from the
`checkout_paths()` function, which yields the misleading exit code 127
(which is reserved to indicate "command not found").
Another problem: a non-zero exit code of the `post-checkout` hook as run
at the end of a `git switch` run will always causes that command to exit
with exit code 1.
This is inconsistent. Let's pass through the exit code of the
`post-checkout` hook verbatim (unless the Git operation fails, in which
case we want to exit with code 1, even if the hook returns another exit
code).
Document this, and add a regression test for this behavior, too.
Inspired-by-a-patch-by: Magne Land <magne.land@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/githooks.txt | 3 ++-
builtin/checkout.c | 8 +++++---
t/t5403-post-checkout-hook.sh | 9 +++++++++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 50365f2914..12c756a37b 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -193,7 +193,8 @@ worktree. The hook is given three parameters: the ref of the previous HEAD,
the ref of the new HEAD (which may or may not have changed), and a flag
indicating whether the checkout was a branch checkout (changing branches,
flag=1) or a file checkout (retrieving a file from the index, flag=0).
-This hook cannot affect the outcome of `git switch` or `git checkout`.
+If this hook exits with an exit code other than 0, it causes the calling
+`git switch` or `git checkout` command to fail with the same exit code.
It is also run after linkgit:git-clone[1], unless the `--no-checkout` (`-n`) option is
used. The first parameter given to the hook is the null-ref, the second the
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 1283727761..56afc3c1a8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -400,7 +400,7 @@ static int checkout_paths(const struct checkout_opts *opts,
static char *ps_matched;
struct object_id rev;
struct commit *head;
- int errs = 0;
+ int errs = 0, ret;
struct lock_file lock_file = LOCK_INIT;
int checkout_index;
@@ -548,8 +548,8 @@ static int checkout_paths(const struct checkout_opts *opts,
read_ref_full("HEAD", 0, &rev, NULL);
head = lookup_commit_reference_gently(the_repository, &rev, 1);
- errs |= post_checkout_hook(head, head, 0);
- return errs;
+ ret = post_checkout_hook(head, head, 0);
+ return !errs ? ret : (errs < 0 ? 1 : errs);
}
static void show_local_changes(struct object *head,
@@ -1062,6 +1062,8 @@ static int switch_branches(const struct checkout_opts *opts,
ret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1);
free(path_to_free);
+ if (ret > 0)
+ return ret;
return ret || writeout_error;
}
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index a39b3b5c78..60ce81f6db 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -73,4 +73,13 @@ test_expect_success 'post-checkout hook is triggered by clone' '
test -f clone3/.git/post-checkout.args
'
+test_expect_success 'exit code of post-checkout hook is passed through' '
+ mkdir -p exit-code &&
+ echo "exit 123" | write_script exit-code/post-checkout &&
+ test_expect_code 123 \
+ git -c core.hooksPath="$PWD/exit-code" switch -c trigger-exit-code &&
+ test_expect_code 123 \
+ git -c core.hooksPath="$PWD/exit-code" restore .
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 0/1] Pass through the exit code of post-checkout
2019-10-10 12:01 [PATCH 0/1] Pass through the exit code of post-checkout Johannes Schindelin via GitGitGadget
2019-10-10 12:01 ` [PATCH 1/1] switch/restore: consistently pass through exit code of `post-checkout` Johannes Schindelin via GitGitGadget
@ 2019-10-11 4:22 ` Junio C Hamano
1 sibling, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2019-10-11 4:22 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Jonathan Nieder, Magne Land, Johannes Schindelin
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> To answer Jonathan's question, at long last, yes, it is useful. A hook is
> not only an opportunity to run code at given points in Git's life cycle, but
> also an opportunity to stop Git in its tracks.
In general that is correct, and especially so for pre_$git_action
hooks, which are about interfering and vetoing an action that is
being carried out.
On the other hand, post_$git_action hooks are specifically defined
as a way to trigger an extra action and documented that they cannot
affect the outcome of the operation (in other words, they trigger at
a point in the operation flow that is too late to affect the
outcome).
Now, it is somewhat debatable how the "outcome" should be defined.
A post-rebase hook that drops the last commit in the sequence, for
example, should not be allowed (the rebase has rebuilt a sequence
and that final sequence of commits should be left), but should it be
allowed to signal back to "git rebase" and affect its exit status?
I am not 100% sure if it is a good idea to allow post-checkout to
affect the exit status of "git checkout" or its friends. If one
codepath in "git checkout" or its friends lets post-checkout to
influence the exit status while another codepath ignores, it makes
sense to discuss if the inconsistency needs to be removed, but in
such a case, I think it would make sense to unify in the direction
of ignoring (i.e. not allowing post-* hook to affect the outcome).
^ permalink raw reply [flat|nested] 3+ messages in thread