git/remote.c, branch v2.46.2

Merge branch 'jk/remote-wo-url'

2024-07-02T16:59:01Z

Memory ownership rules for the in-core representation of remote.*.url configuration values have been straightened out, which resulted in a few leak fixes and code clarification. * jk/remote-wo-url: remote: drop checks for zero-url case remote: always require at least one url in a remote t5801: test remote.*.vcs config t5801: make remote-testgit GIT_DIR setup more robust remote: allow resetting url list config: document remote.*.url/pushurl interaction remote: simplify url/pushurl selection remote: use strvecs to store remote url/pushurl remote: transfer ownership of memory in add_url(), etc remote: refactor alias_url() memory ownership archive: fix check for missing url

global: introduce `USE_THE_REPOSITORY_VARIABLE` macro

2024-06-14T17:26:33Z

Use of the `the_repository` variable is deprecated nowadays, and we slowly but steadily convert the codebase to not use it anymore. Instead, callers should be passing down the repository to work on via parameters. It is hard though to prove that a given code unit does not use this variable anymore. The most trivial case, merely demonstrating that there is no direct use of `the_repository`, is already a bit of a pain during code reviews as the reviewer needs to manually verify claims made by the patch author. The bigger problem though is that we have many interfaces that implicitly rely on `the_repository`. Introduce a new `USE_THE_REPOSITORY_VARIABLE` macro that allows code units to opt into usage of `the_repository`. The intent of this macro is to demonstrate that a certain code unit does not use this variable anymore, and to keep it from new dependencies on it in future changes, be it explicit or implicit For now, the macro only guards `the_repository` itself as well as `the_hash_algo`. There are many more known interfaces where we have an implicit dependency on `the_repository`, but those are not guarded at the current point in time. Over time though, we should start to add guards as required (or even better, just remove them). Define the macro as required in our code units. As expected, most of our code still relies on the global variable. Nearly all of our builtins rely on the variable as there is no way yet to pass `the_repository` to their entry point. For now, declare the macro in "biultin.h" to keep the required changes at least a little bit more contained. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

hash: require hash algorithm in `oidread()` and `oidclr()`

2024-06-14T17:26:32Z

Both `oidread()` and `oidclr()` use `the_repository` to derive the hash function that shall be used. Require callers to pass in the hash algorithm to get rid of this implicit dependency. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

remote: always require at least one url in a remote

2024-06-14T16:34:38Z

When we return a struct from remote_get(), the result _almost_ always has at least one url. In remotes_remote_get_1(), we do this: if (name_given && !valid_remote(ret)) add_url_alias(remote_state, ret, name); if (!valid_remote(ret)) return NULL; So if the remote doesn't have a url, we give it one based on the name (this is how unconfigured urls are used as remotes). And if that doesn't work, we return NULL. But there's a catch: valid_remote() checks that we have at least one url _unless_ the remote.*.vcs field is set. This comes from c578f51d52 (Add a config option for remotes to specify a foreign vcs, 2009-11-18), and the whole idea was to support remote helpers that don't have their own url. However, that mode has been broken since 25d5cc488a (Pass unknown protocols to external protocol handlers, 2009-12-09)! That commit unconditionally looks at the url in get_helper(), causing a segfault with something like: git -c remote.foo.vcs=bar fetch foo We could fix that now, of course. But given that it has been broken for almost 15 years and nobody noticed, there's a better option. This weird "there might not be a url" special case requires checks all over the code base, and it's not clear if there are other similar segfaults lurking. It would be nice if we could drop that special case. So instead, let's let the "the remote name is the url" code kick in. If you have "remote.foo.vcs", then your url (unless otherwise configured) is "foo". This does have a visible effect compared to what 25d5cc488a was trying to do. The idea back then is that for a remote without a url, we'd run: # only one command-line option! git-remote-bar foo whereas with our default url, now we'll run: git-remote-bar foo foo Again, in practice nobody can be relying on this because it has been segfaulting for 15 years. We should consider just removing this "vcs" config option entirely, but that would be a user-visible breakage. So by fixing it this way, we can keep things working that have been working, and simplify away one special case inside our code. This fixes the segfault from 25d5cc488a (demonstrated by the test), and we can build further cleanups on top. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

remote: allow resetting url list

2024-06-14T16:34:38Z

Because remote.*.url is treated as a multi-valued key, there is no way to override previous config. So for example if you have remote.origin.url set to some wrong value, doing: git -c remote.origin.url=right fetch would not work. It would append "right" to the list, which means we'd still fetch from "wrong" (since subsequent values are used only as push urls). Let's provide a mechanism to reset the list, like we do for other multi-valued keys (e.g., credential.helper, http.extraheaders, and merge.suppressDest all use this "empty string means reset" pattern). Reported-by: Mathew George Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

remote: simplify url/pushurl selection

2024-06-14T16:34:38Z

When we want to know the push urls for a remote, there is some simple logic: - if the user configured any remote.*.pushurl keys, then those make the complete set of push urls - otherwise we push to all urls in remote.*.url Many spots implement this with a level of indirection, assigning to a local url/url_nr pair. But since both arrays are now strvecs, we can just use a pointer to select the appropriate strvec, shortening the code a bit. Even though this is now a one-liner, since it is application logic that is present in so many places, it's worth abstracting a helper function. In fact, we already have such a function, but it's local to builtin/push.c. So we'll just make it available everywhere via remote.h. There are two spots to pay special attention to here: 1. in builtin/remote.c's get_url(), we are selecting first based on push_mode and then falling back to "url" when we're in push_mode but no pushurl is defined. The updated code makes that much more clear, compared to the original which had an "else" fall-through. 2. likewise in that file's set_url(), we _only_ respect push_mode, sine the point is that we are adding to pushurl in that case (whether it is empty or not). And thus it does not use our helper function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

remote: use strvecs to store remote url/pushurl

2024-06-14T16:34:38Z

Now that the url/pushurl fields of "struct remote" own their strings, we can switch from bare arrays to strvecs. This has a few advantages: - push/clear are now one-liners - likewise the free+assigns in alias_all_urls() can use strvec_replace() - we now use size_t for storage, avoiding possible overflow - this will enable some further cleanups in future patches There's quite a bit of fallout in the code that reads these fields, as it tends to access these arrays directly. But it's mostly a mechanical replacement of "url_nr" with "url.nr", and "url[i]" with "url.v[i]", with a few variations (e.g. "*url" could become "*url.v", but I used "url.v[0]" for consistency). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

remote: transfer ownership of memory in add_url(), etc

2024-06-14T16:34:37Z

Many of the internal functions in remote.c take const strings and store them forever in instances of "struct remote". Since the functions are internal and callers are aware of the convention, this seems to mostly work and not cause leaks. But there are some issues: - it's impossible to clear any of the arrays, because the data dependencies between them are too muddled (if you free() a string, it might also be referenced from another array, causing a user-after-free; but if you don't, that might be the last reference, causing a leak). This is mostly of interest for further refactoring and features, but there's at least one spot that's already a problem. In alias_all_urls(), we replace elements of remote->url and remote->pushurl with their aliased forms, dropping references to the original. - sometimes strings from outside callers make their way in. For example, calling remote_get("foo") when there is no configured "foo" remote will create a remote struct with the single url "foo". But we'll do so by holding on to the string passed to remote_get() forever. In practice I think this works out because we'd usually pass in a string that lasts the length of the program (a string literal, or argv reference, or other data structure allocated in the main function). But it's a rather subtle requirement. Instead, let's have remote->url and remote->pushurl own their string memory. They'll copy the const strings that are passed in, and callers can stop making their own copies. Likewise, when we overwrite an entry, we can free the memory it points to, fixing the leak mentioned above. We'll leave the struct members as "const" since they are visible to the outside world, and shouldn't usually be touched. This requires casting on free() for now, but we'll clean that further in a future patch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

remote: refactor alias_url() memory ownership

2024-06-14T16:34:37Z

The alias_url() function may return either a newly allocated string (which the caller must take ownership of), or the original const "url" parameter that was passed in. This often works OK because callers are generally passing in a "url" that they expect to retain ownership of anyway. So whether we got back the original or a new string, we're always interested in storing it forever. But I suspect there are some possible leaks here (e.g., add_url_alias() may end up discarding the original "url"). Whether there are active leaks or not, this is a confusing setup that makes further refactoring of memory ownership harder. So instead of returning the original string, return NULL, forcing callers to decide what to do with it explicitly. We can then build further cleanups on top of that. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

Merge branch 'ps/leakfixes'

2024-06-06T19:49:23Z

Leakfixes. * ps/leakfixes: builtin/mv: fix leaks for submodule gitfile paths builtin/mv: refactor to use `struct strvec` builtin/mv duplicate string list memory builtin/mv: refactor `add_slash()` to always return allocated strings strvec: add functions to replace and remove strings submodule: fix leaking memory for submodule entries commit-reach: fix memory leak in `ahead_behind()` builtin/credential: clear credential before exit config: plug various memory leaks config: clarify memory ownership in `git_config_string()` builtin/log: stop using globals for format config builtin/log: stop using globals for log config convert: refactor code to clarify ownership of check_roundtrip_encoding diff: refactor code to clarify memory ownership of prefixes config: clarify memory ownership in `git_config_pathname()` http: refactor code to clarify memory ownership checkout: clarify memory ownership in `unique_tracking_name()` strbuf: fix leak when `appendwholeline()` fails with EOF transport-helper: fix leaking helper name