On Thu, Mar 13, 2025 at 5:28 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> 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.
