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@xxxxxxxxxxxxx/ 2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@xxxxxxxxxxxxxx/ Reported-by: Clemens Fruhwirth <clemens@xxxxxxxxxxxxx> Reported-by: Jan Pokorný <poki@xxxxxxxx> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> --- 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@xxxxxxxxx> 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@xxxxxxxxxxxxx/ 2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@xxxxxxxxxxxxxx/ @@ 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
