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

[Clemens Fruhwirth <clemens@endorphin.org>]

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.

>  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 no, then an assert(!skip_prefix(source_ref->name, "refs/heads/",
&shortname)) might be more idiomatic.

> diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
> index b1d614ce18c..7d12ceff702 100755
> --- a/t/t5553-set-upstream.sh
> +++ b/t/t5553-set-upstream.sh

Thank for you adding tests. LGTM.

Regards,
-- 
Fruhwirth Clemens http://clemens.endorphin.org