[PATCH 0/11] allow overriding remote.*.url

From: Jeff King <peff@peff.net>
Date: Jun 14, 2024, 10:24 UTC
Message-ID: <20240614102439.GA222287@coredump.intra.peff.net>
In-Reply-To: <20240613102409.GE817573@coredump.intra.peff.net>

On Thu, Jun 13, 2024 at 06:24:09AM -0400, Jeff King wrote:

Show 6 quoted lines

> > 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