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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 15, 2022, 12:52 UTC
Message-ID: <7e5488bd-a17b-3997-072d-69e3ae0a9821@github.com>
In-Reply-To: <RFC-cover-0.5-00000000000-20220615T104503Z-avarab@gmail.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.

Show 7 quoted lines

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