On Fri, Apr 30 2021, Derrick Stolee via GitGitGadget wrote: Just nits on the patch, will reply to the idea in another message: > [...] > +test_expect_success 'enable username:password urls' ' > + git config --global core.allowUsernamePasswordUrls true > +' Hrm, --global? In any case isn't it also better here to tweak this for specific tests? > test_expect_success 'push status output scrubs password' ' > cd "$ROOT_PATH/test_repo_clone" && > + git config core.allowUsernamePasswordUrls true && > git push --porcelain \ > "$HTTPD_URL_USER_PASS/smart/test_repo.git" \ > +HEAD:scrub >status && > @@ -469,9 +470,11 @@ test_expect_success 'push status output scrubs password' ' Use test_config instead, unless this is really "setup for the rest of the tests" in disguise, but IMO even more of a reason to use test_config for each one. > test_expect_success 'clone/fetch scrubs password from reflogs' ' > cd "$ROOT_PATH" && > - git clone "$HTTPD_URL_USER_PASS/smart/test_repo.git" \ > + git -c core.allowUsernamePasswordUrls=true clone \ > + "$HTTPD_URL_USER_PASS/smart/test_repo.git" \ > reflog-test && > cd reflog-test && > + git config core.allowUsernamePasswordUrls true && Ditto. Although redundant in your patch, no, since we've set it to true above? > +test_expect_success 'clone fails when using username:password' ' > + test_must_fail git clone https://username:password@xxxxxxxxx 2>err && > + test_i18ngrep "attempted to parse a URL with a plain-text username and password" err > +' > + Just use grep, not test_i18ngrep. GETTEXT_POISON is gone. > test_expect_success 'clone from hooks' ' > > test_create_repo r0 && > diff --git a/urlmatch.c b/urlmatch.c > index 33a2ccd306b6..e81ec9e1fc0b 100644 > --- a/urlmatch.c > +++ b/urlmatch.c > @@ -1,5 +1,6 @@ > #include "cache.h" > #include "urlmatch.h" > +#include "config.h" > > #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" > #define URL_DIGIT "0123456789" > @@ -106,6 +107,18 @@ static int match_host(const struct url_info *url_info, > return (!url_len && !pat_len); > } > > +static void die_if_username_password_not_allowed(void) > +{ > + int opt_in = 0; > + if (!git_config_get_bool("core.allowusernamepasswordurls", &opt_in) && > + opt_in) > + return; API use nit: You need to either initialize "opt_in = 0" or check the git_config_get_bool() return value. Doing both isn't strictly needed. I.e. either of these would work: int opt_in = 0; git_config_get_bool(..., &opt_in); if (opt_in) ...; Or: int opt_in; if (!git_config_get_bool(..., &opt_in) && opt_in) No?
