From: Christian Couder Date: Fri, 14 Mar 2025 14:10:09 GMT Subject: Re: [PATCH v4] promisor-remote: fix segfault when remote URL is missing Message-ID: In-Reply-To: On Thu, Mar 13, 2025 at 5:28 PM Junio C Hamano wrote: > > Christian Couder 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. > > 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.