[PATCH v4] pull, fetch: fix segfault in --set-upstream option

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

Fix a segfault in the --set-upstream option added in
24bc1a12926 (pull, fetch: add --set-upstream option, 2019-08-19) added
in v2.24.0.

The code added there did not do the same checking we do for "git
branch" itself since 8efb8899cfe (branch: segfault fixes and
validation, 2013-02-23), which in turn fixed the same sort of segfault
I'm fixing now in "git branch --set-upstream-to", see
6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).

The warning message I'm adding here is an amalgamation of the error
added for "git branch" in 8efb8899cfe, and the error output
install_branch_config() itself emits, i.e. it trims "refs/heads/" from
the name and says "branch X on remote", not "branch refs/heads/X on
remote".

I think it would make more sense to simply die() here, but in the
other checks for --set-upstream added in 24bc1a12926 we issue a
warning() instead. Let's do the same here for consistency for now.

There was an earlier submitted alternate way of fixing this in [1],
due to that patch breaking threading with the original report at [2] I
didn't notice it before authoring this version. I think the more
detailed warning message here is better, and we should also have tests
for this behavior.

The --no-rebase option to "git pull" is needed as of the recently
merged 7d0daf3f12f (Merge branch 'en/pull-conflicting-options',
2021-08-30).

1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/
2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/

Reported-by: Clemens Fruhwirth <clemens@endorphin.org>
Reported-by: Jan Pokorný <poki@fnusa.cz>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A re-submission for a semantic conflict with the now-merged
7d0daf3f12f on master.

On Mon, Aug 30 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> There was an earlier submitted alternate way of fixing this in [1],
>> due to that patch breaking threading with the original report at [2] I
>> didn't notice it before authoring this version. I think the more
>> detailed warning message here is better, and we should also have tests
>> for this behavior.
>
> I do not think it is necessarily an improvement to give more info,
> if it is irrelevant to explain what the error is.  And the point of
> the error message here is that we cannot set the upstream of
> detached HEAD, no matter what the value of old source ref or new
> source ref are.
>
> The original from Clemens gives a warning message that omits the
> piece of information that does not contribute to the error.

I see your point, but looking at it again I decided to keep it as-is
in this re-roll. As noted in the commit message this is for
consistency with the output we emit when running a commind like "git
branch --set-upstream-to <upstream> <branchname>", which as you'll see
if you "git checkout HEAD^0" we'll quote the wanted upstream back at
you.

So yeah, arguably we should just punt on that whole thing and tell the
user "you're on a detached HEAD, this will never work" or something
like that, but let's consider that UI change separately, and then do
it for all of "branch", "fetch" and "pull", rather than leave the
latter two inconsistent with "branch" with this fix.

> Testing the new behaviour is a good idea.  I also agree with you
> that die() would be more appropriate and does not risk regression,
> if the original behaviour was to segfault.

Indeed. I changed it due to your upthread
<xmqqsg0anxix.fsf@gitster.g>.

I think doing s/warning/die/ here makes sense, but similarly to the
above comment: Let's punt on that and do it separately from this
narrow segfault fix. If and when we change that we should change
various other "warning()" around this codepath to "die()" as well.

Range-diff against v3:
1:  68899471206 ! 1:  0caa9a89a86 pull, fetch: fix segfault in --set-upstream option
    @@ Commit message
         detailed warning message here is better, and we should also have tests
         for this behavior.
     
    +    The --no-rebase option to "git pull" is needed as of the recently
    +    merged 7d0daf3f12f (Merge branch 'en/pull-conflicting-options',
    +    2021-08-30).
    +
         1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/
         2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/
     
    @@ t/t5553-set-upstream.sh: test_expect_success 'pull --set-upstream with valid URL
     +	cat >expect <<-\EOF &&
     +	warning: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
     +	EOF
    -+	git pull --set-upstream upstream main 2>actual.raw &&
    ++	git pull --no-rebase --set-upstream upstream main 2>actual.raw &&
     +	grep ^warning: actual.raw >actual &&
     +	test_cmp expect actual
     +'

 builtin/fetch.c         | 10 ++++++++++
 t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e064687dbdc..28fa168133a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1625,6 +1625,16 @@ static int do_fetch(struct transport *transport,
 			}
 		}
 		if (source_ref) {
+			if (!branch) {
+				const char *shortname = source_ref->name;
+				skip_prefix(shortname, "refs/heads/", &shortname);
+
+				warning(_("could not set upstream of HEAD to '%s' from '%s' when "
+					  "it does not point to any branch."),
+					shortname, transport->remote->name);
+				goto skip;
+			}
+
 			if (!strcmp(source_ref->name, "HEAD") ||
 			    starts_with(source_ref->name, "refs/heads/"))
 				install_branch_config(0,
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index 9c12c0f8c32..48050162c27 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -91,6 +91,17 @@ test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '
 	check_config_missing other2
 '
 
+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.
+	EOF
+	git fetch --set-upstream upstream main 2>actual.raw &&
+	grep ^warning: actual.raw >actual &&
+	test_cmp expect actual
+'
+
 # tests for pull --set-upstream
 
 test_expect_success 'setup bare parent pull' '
@@ -178,4 +189,15 @@ test_expect_success 'pull --set-upstream with valid URL and branch sets branch'
 	check_config_missing other2
 '
 
+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.
+	EOF
+	git pull --no-rebase --set-upstream upstream main 2>actual.raw &&
+	grep ^warning: actual.raw >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.33.0.805.g739b16c2189