cf/fetch-set-upstream-while-detached (was: What's cooking in git.git (Dec 2021, #01; Fri, 3))

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Dec 03 2021, Junio C Hamano wrote:

> * cf/fetch-set-upstream-while-detached (2021-07-06) 1 commit
>  - fetch: fix segfault on --set-upstream while on a detached HEAD
>
>  "git fetch --set-upstream" while on detached HEAD segfaulted
>  instead of noticing that such an operation did not make sense.
>
>  Expecting a reroll.
>  A low hanging fruit to make this usable is an addition of a few tests;
>  any takers?
>  cf. <CAPig+cTRXTGe-MNTy=2gk1eX8G+0fa303nrLnEtX1uHUC2usmg@mail.gmail.com>
>  cf. <xmqqsg0ri5mq.fsf@gitster.g>
>  source: <20210706162238.575988-1-clemens@endorphin.org>

Yes, you can pick my [1] which solves the same issue, but has tests
(which I came up with independently, didn't notice the earlier fix due
to a broken In-Reply-To chain with that fix).

To refresh your memory (I had to refresh mine) you had the comment that
we shouldn't fix the segfault without also making that exit with
non-zero instead of warning().

I don't disagree, but if you read t/t5553-set-upstream.sh you'll see
that's how we do it now.

So i'd think that any proposed behavior change there should come
independent of a narrow segfault fix, let's fix that, and then change
how "fetch --set-upstream-to" works in general.

In any case, per your comment above there's no mention of the
s/warning/die/g, just that it needs tests. If that's all that's needed
you can just pick up [1].

You can even pick that [1] and combine it with
cf/fetch-set-upstream-while-detached and it'll pass the tests the former
adds if they're fixed up[2] to grep for the difference in the warning
text.

I think the warning I added is a bit better, but I really don't care. As
long as we finally fix this segfault we can tweak that wording, or
whether we use warning() or die() etc. as a folllow-up.

1. https://lore.kernel.org/git/patch-v4-1.1-0caa9a89a86-20210831T135212Z-avarab@gmail.com/
2.

diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index 48050162c27..631bd5a9366 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -95,7 +95,7 @@ test_expect_success 'fetch --set-upstream with a detached HEAD' '
 	git checkout HEAD^0 &&
 	test_when_finished "git checkout -" &&
 	cat >expect <<-\EOF &&
-	warning: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
+	warning: not on a branch to use --set-upstream with
 	EOF
 	git fetch --set-upstream upstream main 2>actual.raw &&
 	grep ^warning: actual.raw >actual &&
@@ -193,7 +193,7 @@ test_expect_success 'pull --set-upstream with a detached HEAD' '
 	git checkout HEAD^0 &&
 	test_when_finished "git checkout -" &&
 	cat >expect <<-\EOF &&
-	warning: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
+	warning: not on a branch to use --set-upstream with
 	EOF
 	git pull --no-rebase --set-upstream upstream main 2>actual.raw &&
 	grep ^warning: actual.raw >actual &&