On Fri, Jun 14, 2024 at 3:25 AM Jeff King <peff@xxxxxxxx> 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.
