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

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> It could happen that the server, the client and the common promisor
> remote are all on the same filesystem. Then it would make sense for
> both the server and the client to rely on just the remote name,
> without any URL configured, to access the promisor remote. So if we
> want things to work in this case, then I think the server should
> advertise the remote name in the "url=" field.

Meaning the server and all the clients share the short-and-sweet
string that is suitable as a remote nickname (i.e. something you the
client driver would type to "git fetch" command) as a pathname that
is relative to the current working directory, and because the server
and these clients must refer to the same repository using this
mechanism, this in turn means that the server and all the clients
share the same current working directory?

It may be possible, but would that make _any_ practical sense?  I
doubt it.  I would understand perfectly well that the local single
machine situation as a good justification to use file:// URL in such
a setting, but not for the r->name fallback.

>> What other uses do the name/url vectors prepared by
>> promisor_info_vecs() have?  Is it that we use them only to advertise
>> with this code, and then match with what they advertise?
>
> Yes, I think so.
> ...
> Other call sites don't use promisor_info_vecs(). It was introduced by
> the lop patch series which doesn't change how other code gets the
> remote names and URLs.

Then it should be simpler to remove r->name entries at the source in
that function, than having to filter it from the strvec whenever the
strvec elements are used.

>> ... would it be so different to pass an empty string as to pass a
>> misspelt URL received from the other end?  Wouldn't the end result
>> the same (i.e., we thought we had a URL usable as a promisor remote,
>> but it turns out that we cannot reach it)?
>
> Perhaps but I think it would be weird if URLs are matching when they
> are empty on both sides. I think it makes more sense and is more
> helpful to warn with a clear error message and just reject the remote
> if any of the URL is empty.

Smells like over-engineering for nonexisting case to me.

>> The 'i' was obtained by calling remote_nick_find(), which uses
>> strcasecmp() to find named remote (which I doubt it is a sensible
>> design by the way).  This code should be consistent with whatever
>> comparison used there.
>
> I think comparing remote names case insensitively is fair. It's likely
> to just make things a bit easier for users.

Meaning it makes it impossible to have two remotes with nicknames
that are different in case?

Because the "[remote "nick"] fetch = ..." configuration variables
have the nickname in the second part, the nicknames are case
insensitive, unlike the first and the third component (i.e.
"remote.origin.fetch" and "Remote.origin.FETCH" are the same thing,
but "remote.Origin.fetch" and "remote.origin.fetch" are different).