On Thu, Mar 13, 2025 at 5:40 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Christian Couder <christian.couder@xxxxxxxxx> 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? Maybe they don't share the same directory but there is a symlink to or a mount of the remote directory. I agree it's a rare case, but the case with no URL is a rare case too. Also I just tested the following: $ mkdir test_git $ cd test_git $ git init $ git config remote."../git".fetch '+refs/heads/*:refs/remotes/git/*' $ git fetch "../git" which works if ../git is a valid path to a repo. So even if `git remote add "../stuff" url` is rejected, one can actually create working remotes with names that point to any directory on the current filesystem. > 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. I am fine with this solution. The case with no URL isn't likely to happen in the first place, and if needed, it can be easily worked around by just specifying an URL that can be the same as the remote name. So in the next version, only remotes with an URL configured are pushed into the 'names' and 'urls' strvecs. > >> ... 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. I am fine with not worrying about this. Then I think it's just simpler to ignore any remote with an empty URL and not even push them into the strvecs in the first place, like we are now also doing for remote with no URL. So I have implemented this in the next version. It just simplifies a lot of things. It also seems that when an URL is empty, Git uses the remote name to fetch, like when the URL is missing. So it makes sense to process missing and empty URLs in the same way. > >> 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. I think you mean "the nicknames are case sensitive" here. > "remote.origin.fetch" and "Remote.origin.FETCH" are the same thing, > but "remote.Origin.fetch" and "remote.origin.fetch" are different). Ok, then I have fixed this in a separate patch in the next version.
