Re: [PATCH v2] pull, fetch: fix segfault in --set-upstream option
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 24, 2021, 08:49 UTC
- Message-ID
- <87bl5nfcof.fsf@evledraar.gmail.com>
- In-Reply-To
- <CAG6gW_s6FHG6muJ6Hai-Sb2ntmygLymQZr8YrN212_SV1VhBAw@mail.gmail.com>
On Tue, Aug 24 2021, Clemens Fruhwirth wrote:
Show 9 quoted lines
> On Mon, 23 Aug 2021 at 14:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> >> A proposed replacement of cf/fetch-set-upstream-while-detached which >> as noted in What's Cooking has been languishing for a while. I changed >> the die() in my version to a warning() as suggested by Junio & updated >> the test and commit message accordingly. > > Thank you Ævar, for taking care of that and adding tests and my > apologizes for not responding in a timely manner.
No worries.
Show 23 quoted lines
>> builtin/fetch.c | 11 +++++++++++
>> t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 25740c13df1..ca487edd805 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1623,6 +1623,17 @@ static int do_fetch(struct transport *transport,
>> }
>> }
>> if (source_ref) {
>> + if (!branch) {
>> + const char *shortname = NULL;
>> + if (!skip_prefix(source_ref->name,
>> + "refs/heads/", &shortname))
>> + shortname = source_ref->name;
>
> Is skip_prefix ever able to fail?
>
> - If yes, then shortname will be left pointing to NULL when we print
> the warning below. Warning probably won't die because of NULL, but it
> still will print something weird.If it fails we'll use source_ref->name, so it won't be NULL...
> - If no, then an assert(!skip_prefix(source_ref->name, "refs/heads/", > &shortname)) might be more idiomatic.
Which means that this would be a bug, since it's not handling the case where the source doesn't start with refs/heads/*.
I.e. it's the same as doing it this way (on top of this v2), which perhaps is a clearer way to express the same idea. What do you think?
diff --git a/builtin/fetch.c b/builtin/fetch.c index ca487edd805..2bc9159690d 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1624,10 +1624,9 @@ static int do_fetch(struct transport *transport, } if (source_ref) { if (!branch) { - const char *shortname = NULL; - if (!skip_prefix(source_ref->name, - "refs/heads/", &shortname)) - shortname = source_ref->name; + 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);