cf/fetch-set-upstream-while-detached (was: What's cooking in git.git (Dec 2021, #01; Fri, 3))
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 7, 2021, 19:37 UTC
- Message-ID
- <211207.86pmq8mbtu.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqh7bpqhf0.fsf@gitster.g>
On Fri, Dec 03 2021, Junio C Hamano wrote:
Show 12 quoted lines
> * 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 &&