[RFC,0/5] fix issues in transfer.credentialsInUrl

Message

Ævar Arnfjörð Bjarmason June 15, 2022, 10:53 a.m. UTC

This is on top of [1], and given the "rc" phase is an RFC. This:

 * Fixes the issue of the transfer.credentialsInUrl (now renamed) 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...)

 * 1/5 fixes a bug in an existing test, but I didn't think it was
   worth bothering with for 2.37.0.

 * Adds missing test coverage for reading the config from a file, not
   the CLI.

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

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

 * 5/5 fixes the (major) blind spot of the warning missing "pushurl" config.

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.

Derrick: Are you interested in picking this up & pursuing it after the
release, with whatever fix-ups/rewrites etc. that you find
appropriate?

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/

Ævar Arnfjörð Bjarmason (5):
  push tests: add a missing "test_line_count"
  fetch+push tests: add missing coverage for 6dcbdc0d661
  CI: add a linux-BUILD-vars job
  fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings
  transport: check remote.<name>pushurl with transfer.credentialsInUrl

 .github/workflows/main.yml |  3 ++
 builtin/clone.c            |  5 ++-
 builtin/fetch.c            |  4 ++
 builtin/push.c             |  6 ++-
 ci/run-build-and-tests.sh  | 30 ++++++++++++++
 remote.c                   | 82 +++++++++++++++++++++++++++-----------
 remote.h                   | 14 +++++++
 t/t5516-fetch-push.sh      | 46 ++++++++++++++++++++-
 t/t5601-clone.sh           |  2 +-
 9 files changed, 164 insertions(+), 28 deletions(-)

Comments

Derrick Stolee June 15, 2022, 12:52 p.m. UTC | #1

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