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

Message

Jeff King June 14, 2024, 10:24 a.m. UTC

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

Comments

Elijah Newren June 25, 2024, 5:44 p.m. UTC | #1

On Fri, Jun 14, 2024 at 3:25 AM Jeff King <[e-mail]> wrote:
>
> 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).

I agree with this good news after reviewing the series.

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

I only managed to find a few typos in commit messages, but I looked
through patches 1-8 pretty closely.  I only skimmed 9 & 10 -- I don't
really have an opinion on the remote helpers.  I agree that the issue
you bring up in the patches makes sense to discuss, and the route you
picked looks reasonable to me, but I don't feel motivated to try to
use or understand the remote helpers enough to form an opinion.
However, I'm a fan of the cleanup in patch 11 that your changes in 9 &
10 enabled, so if everyone's as ambivalent as me (and 15 years of
things being broken suggests everyone is likely to be as ambivalent as
me) then I'd say just go with your changes in 9 & 10 and call it a
day.

Jeff King June 26, 2024, 8:40 p.m. UTC | #2

On Tue, Jun 25, 2024 at 10:44:03AM -0700, Elijah Newren wrote:

> I only managed to find a few typos in commit messages, but I looked
> through patches 1-8 pretty closely.  I only skimmed 9 & 10 -- I don't
> really have an opinion on the remote helpers.  I agree that the issue
> you bring up in the patches makes sense to discuss, and the route you
> picked looks reasonable to me, but I don't feel motivated to try to
> use or understand the remote helpers enough to form an opinion.
> However, I'm a fan of the cleanup in patch 11 that your changes in 9 &
> 10 enabled, so if everyone's as ambivalent as me (and 15 years of
> things being broken suggests everyone is likely to be as ambivalent as
> me) then I'd say just go with your changes in 9 & 10 and call it a
> day.

Thanks for taking a look. I do think patches 9 and 10 are the most
controversial in their goals. But for the reasons given there, I don't
think anybody will care about the direction much either way. And
certainly they are not making anything _worse_, since the thing they
disallow is already broken. They are merely shutting off the option of
"fixing" it to match the original intent.

So I stand by the direction I took in the patches, but I wanted to point
out that if anybody wants to be extra careful, those are the ones to
look at.

-Peff