On Thu, Jun 13, 2024 at 06:24:09AM -0400, Jeff King wrote:
> > I was expecting (with excitement) a mess, but the above is as clean
> > as we can make the idea, I would say. Lack of documentation and
> > tests do count as incompleteness though of course.
>
> Yeah, and we should probably do the same for pushurl. And I think there
> could be some cleanup of the memory ownership handling of add_url().
So as always with this crufty 2009-era code, there turned out to be some
subtleties. ;)
The good news is that I think dealing with them left the code in a
better place. It's easier to reason about, and a few possible leaks have
been plugged (I don't know if they were triggered in the test suite or
not; if so they weren't enough to tip any scripts over to being
leak-free).
We can split the series into segments:
[01/11]: archive: fix check for missing url
A nearby trivial bugfix.
[02/11]: remote: refactor alias_url() memory ownership
[03/11]: remote: transfer ownership of memory in add_url(), etc
[04/11]: remote: use strvecs to store remote url/pushurl
[05/11]: remote: simplify url/pushurl selection
Fixing memory handling weirdness, which is a necessary prereq for
the "reset" operation to avoid leaking. The switch to using a strvec
isn't strictly necessary, but it does make the code (including the
later patch 7) simpler.
[06/11]: config: document remote.*.url/pushurl interaction
[07/11]: remote: allow resetting url list
The actual change is in patch 7 here, but it was hard to add new
docs to the rather anemic existing ones. Hence patch 6.
[08/11]: t5801: make remote-testgit GIT_DIR setup more robust
[09/11]: t5801: test remote.*.vcs config
[10/11]: remote: always require at least one url in a remote
[11/11]: remote: drop checks for zero-url case
This is a related cleanup I found while working in the area.
Arguably it could be a separate topic, though it does depend
textually on what came before.
Documentation/config/remote.txt | 11 +++-
builtin/archive.c | 4 +-
builtin/clone.c | 4 +-
builtin/ls-remote.c | 6 +--
builtin/push.c | 28 ++--------
builtin/remote.c | 88 +++++++++---------------------
remote-curl.c | 2 +-
remote.c | 94 ++++++++++++++++++---------------
remote.h | 13 ++---
t/helper/test-bundle-uri.c | 2 -
t/t5505-remote.sh | 36 +++++++++++++
t/t5801-remote-helpers.sh | 23 ++++++++
t/t5801/git-remote-nourl | 3 ++
t/t5801/git-remote-testgit | 3 +-
transport.c | 19 +++----
15 files changed, 174 insertions(+), 162 deletions(-)
create mode 100755 t/t5801/git-remote-nourl
-Peff
