Hi Junio
On 23/11/2019 09:54, Phillip Wood wrote:
Hi Junio
On 23/11/2019 02:03, Junio C Hamano wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
+ if (!(flags & ALLOW_EMPTY)) {
+ struct commit *first_parent = current_head;
+
+ if (flags & AMEND_MSG) {
+ if (current_head->parents) {
It is not apparent to me that somebody guarantees that this access
is safe; would we need to do things differently when !current_head?
We do actually check that there is a valid HEAD before we try to fixup a
commit. Though perhaps we should still change this patch as HEAD may be
changed by another process between that check and re-reading it here. If
you try to fixup a commit without a valid HEAD you get
error: need a HEAD to fixup
hint: Could not execute the todo command
hint:
hint: fixup faef1a5a7637ff91b3611aabd1b96541da5f5536 P
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
hint: git rebase --edit-todo
hint: git rebase --continue
error: could not copy '.git/rebase-merge/message-squash' to
'.git/rebase-merge/message'
The last error message is unfortunate but we do exit in an orderly
manner rather than segfaulting. It's a bit tricky to trigger this (there
isn't a test at the moment) but something like this does it
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d2f1d5bd23..4f55f0cd1c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -67,6 +67,21 @@ test_expect_success 'setup' '
SHELL=
export SHELL
+test_expect_success 'fixup on orphan branch errors out' '
+
+ test_when_finished "git switch master" &&
+ write_script switch-branch.sh <<-\EOF &&
+ git symbolic-ref HEAD refs/heads/does-not-exist &&
+ git rm -rf .
+ EOF
+ (
+ set_fake_editor &&
+ FAKE_LINES="exec_./switch-branch.sh \
+ fixup 1" git rebase -i HEAD^
+ ) &&
+ test_pause
+'
+
I think it would be useful to add something like this to the test suite
(changed to check the error message, with a better name for the script
and modified to expect failure) What do you think?
Best Wishes
Phillip
That's a good point, I'll fix it, thanks for catching this
Best Wishes
Phillip
+ first_parent = current_head->parents->item;
+ if (repo_parse_commit(r, first_parent)) {
+ res = error(_("could not parse HEAD commit"));
+ goto out;
+ }
+ } else {
+ first_parent = NULL;
+ }
+ }
