Re: [RFC PATCH 0/5] fix issues in transfer.credentialsInUrl

[Derrick Stolee <derrickstolee@github.com>]

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