On 6/15/22 6:53 AM, Ævar Arnfjörð Bjarmason wrote: > This is on top of [1], and given the "rc" phase is an RFC. This: > > * Fixes the issue of the transfer.credentialsInUrl (now renamed) You are burying the lede here (and I had to look hard to see where you renamed it in patch 4). I think this rename makes sense, as long as we do that replacement before the release. Having a patch that does only that rename as the first patch (and updates all docs, too) would be helpful. The doc updates should include the reference in the release notes, too. > not > finding passwords in "pushurl" URLs (in my case, the only place > where I'd actually put a password in a URL in a config...) ... > * 5/5 fixes the (major) blind spot of the warning missing "pushurl" config. I think this is a valuable extension of the feature, and justifies the rename. I'm mixed on whether we should add this before the release or save it for the next cycle. > * 1/5 fixes a bug in an existing test, but I didn't think it was > worth bothering with for 2.37.0. It's a good find, and a test fix is easy to do during the release cycle, I think. > * Adds missing test coverage for reading the config from a file, not > the CLI. Again, test coverage is good. No functionality needs to change. However, one test added requires the pushurl change. > * 3/5 is a WIP CI target to spot the type of issue I fixed in [2], > it's not the first time where we have a NO_CURL=Y breakage land on > master... I think that we should use CI to prevent these kinds of issues, so I support adding this as well as leaving room for it to be changed in the future if we notice other build issues that we can group into this build. > * 4/5 attemps to "really" fix the duplicate warnings we emit, I think > the approach there is good, especially the part where we shouldn't > emit it twice in-process. > > But this currently misses e.g. "git ls-remote". I wonder if we > should just stick that git_config_push_parameter() condition into > packet_trace_identity() and call it a day. I think these duplicate warning things should absolutely be left until after the release. We do not have "warn" on by default, so it will not disturb users who don't opt-in. We _should_ pursue this in the next cycle, but with the time we can take to really be sure it is the right approach to solving that problem. > I think this is all non-RFC quality, except the "ls-remote" case, and > us missing tests for that & other transport users that aren't > clone/fetch/push. There are some patch-order things that need to be cleaned up. I agree that most of the code looks right. > Derrick: Are you interested in picking this up & pursuing it after the > release, with whatever fix-ups/rewrites etc. that you find > appropriate? I'm happy to review a new version of this series during the release window that takes the high-priority items (renaming the config, fixing tests, adding the CI build). I'd also be happy to review the other changes as a follow-up. Thanks, -Stolee
