Re: [PATCH v4] promisor-remote: fix segfault when remote URL is missing

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 14, 2025, 14:10 UTC
Message-ID: <CAP8UFD3O9_22br2vxUp2YqsE39JYHwyf-G0FJ3PqQg6mO5q3+w@mail.gmail.com>
In-Reply-To: <xmqqv7scq4iu.fsf@gitster.g>

On Thu, Mar 13, 2025 at 5:28 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > While at it, let's warn and reject the remote, in the 'KnownUrl' case,
> > when no URL or an empty URL is advertised by the server, or when an
> > empty URL is configured on the client for a remote name advertised by
> > the server and configured on the client. This is on par with a warning
> > already emitted when URLs are different in the same case.
>
> That explanation makes it unclear why we need a new one.  If the
> configured and davertised are both empty and the same, according to
> that "warning already emitted", that is not a warning-worthy event,
> is it?

We have to check that remote_url is not NULL before using it in strcmp(). If it is NULL, we need to reject the remote, and it makes sense to warn before doing that with `return 0;` because we warn otherwise when a remote is rejected to try to help diagnose things at the end of the function.

And while we are checking that remote_url is not NULL and warning if it is, it makes sense to also help diagnose the case where remote_url is empty with something like:

    if (!remote_url || !*remote_url) {
        warning(_("no or empty URL advertised for remote '%s'"), remote_name);
        return 0;
    }

I have used the above in the next version. Also I think this part deserves its own patch too, so it is in a separate patch in the next version.

Show 11 quoted lines

> > Let's also warn if the remote is rejected because name and url are the
> > same, as it could mean the url has not been configured.
>
> Are we rejecting a remote _because_ r->name is used?  I thought the
> code did something quite different.  We reject because the url does
> not match, and then after that give an extra warning if remote nick
> was used as a fallback URL.  Even if URL is configured as 'orogin'
> for a remote with nick 'origin', the code would have rejected the
> remote with the same logic in the same code path, wouldn't it?  It
> is a bit confusiong to call such a situation "rejected because name
> and URL are the same".

Yeah, I have removed the above code as it's not needed anyway if we don't process the remotes when they don't have a non-empty URL configured.

Thanks.