| Message ID | 0535908dd7ea4487b342c0f86182579279c57b34.1587728992.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | credential: handle partial URLs in config settings again | expand |
On Fri, Apr 24, 2020 at 11:49:52AM +0000, Johannes Schindelin via GitGitGadget wrote: > diff --git a/credential.c b/credential.c > index 7dbbf26f174..c1a9ca4e485 100644 > --- a/credential.c > +++ b/credential.c > @@ -35,6 +35,10 @@ int credential_match(const struct credential *want, > #undef CHECK > } > > + > +static int credential_from_potentially_partial_url(struct credential *c, > + const char *url); > + > static int credential_config_callback(const char *var, const char *value, > void *data) > { something like credential_from_url_partial might had been more grep friendly but this would work as well. > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh > index efed3ea2955..f796bbfd48b 100755 > --- a/t/t0300-credentials.sh > +++ b/t/t0300-credentials.sh > @@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' ' > test_i18ncmp expect stderr > ' > > +test_expect_success 'credential config with partial URLs' ' > + echo "echo password=yep" | write_script git-credential-yep && > + test_write_lines url=https://user@example.com/repo.git >stdin && > + for partial in \ > + example.com \ > + user@example.com \ even if it works, configurations with a username might not be worth the trouble to support IMHO maybe better not to include them in the tests then either? other than that, like the previous version (which is functionally equivalent) should be IMHO good to go. Carlo
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value, > char *url = xmemdupz(key, dot - key); > int matched; > > - credential_from_url(&want, url); > + if (credential_from_potentially_partial_url(&want, url) < 0) { > + warning(_("skipping credential lookup for key: %s"), > + var); > + credential_clear(&want); > + free(url); > + return 0; > + } > matched = credential_match(&want, c); Unfortunately, the whole section of this code that is being patched here goes away with 46fd7b39 (credential: allow wildcard patterns when matching config, 2020-02-20), that delegates large part of the logic to urlmatch. Dscho and Brian, how do we want to proceed? As the conflicting change has already been in 'next' for more than a month and a half, we'd need this "partial URL" logic build to work well with it. Thanks.
Hi Junio, On Fri, 24 Apr 2020, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value, > > char *url = xmemdupz(key, dot - key); > > int matched; > > > > - credential_from_url(&want, url); > > + if (credential_from_potentially_partial_url(&want, url) < 0) { > > + warning(_("skipping credential lookup for key: %s"), > > + var); > > + credential_clear(&want); > > + free(url); > > + return 0; > > + } > > matched = credential_match(&want, c); > > Unfortunately, the whole section of this code that is being patched > here goes away with 46fd7b39 (credential: allow wildcard patterns > when matching config, 2020-02-20), that delegates large part of the > logic to urlmatch. > > Dscho and Brian, how do we want to proceed? As the conflicting > change has already been in 'next' for more than a month and a half, > we'd need this "partial URL" logic build to work well with it. I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not necessarily feel 100% comfortable with that approach yet, but at least it lets the new test case of t0300 pass. Ciao, Dscho
Hi Carlo, On Fri, 24 Apr 2020, Carlo Marcelo Arenas Belón wrote: > On Fri, Apr 24, 2020 at 11:49:52AM +0000, Johannes Schindelin via GitGitGadget wrote: > > diff --git a/credential.c b/credential.c > > index 7dbbf26f174..c1a9ca4e485 100644 > > --- a/credential.c > > +++ b/credential.c > > @@ -35,6 +35,10 @@ int credential_match(const struct credential *want, > > #undef CHECK > > } > > > > + > > +static int credential_from_potentially_partial_url(struct credential *c, > > + const char *url); > > + > > static int credential_config_callback(const char *var, const char *value, > > void *data) > > { > > something like credential_from_url_partial might had been more grep friendly > but this would work as well. It might be more grep'able, but it sounds really awkward to me, that's why I did not use that name (it was my first candidate). > > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh > > index efed3ea2955..f796bbfd48b 100755 > > --- a/t/t0300-credentials.sh > > +++ b/t/t0300-credentials.sh > > @@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' ' > > test_i18ncmp expect stderr > > ' > > > > +test_expect_success 'credential config with partial URLs' ' > > + echo "echo password=yep" | write_script git-credential-yep && > > + test_write_lines url=https://user@example.com/repo.git >stdin && > > + for partial in \ > > + example.com \ > > + user@example.com \ > > even if it works, configurations with a username might not be worth the > trouble to support IMHO > > maybe better not to include them in the tests then either? Let me counter this: - It would take extra code _to prevent_ the username from being used, and - There is precedent where the user name _does_ matter: it is relatively normal to access different orgs' repositories at https://dev.azure.com/<org>/<repo>/_git via different accounts. Together, those points convince me that special-casing the username _and explicitly ignoring it_ would not make sense. > other than that, like the previous version (which is functionally equivalent) > should be IMHO good to go. Thank you for reviewing it! Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Fri, 24 Apr 2020, Junio C Hamano wrote: > >> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> >> writes: >> >> > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value, >> > char *url = xmemdupz(key, dot - key); >> > int matched; >> > >> > - credential_from_url(&want, url); >> > + if (credential_from_potentially_partial_url(&want, url) < 0) { >> > + warning(_("skipping credential lookup for key: %s"), >> > + var); >> > + credential_clear(&want); >> > + free(url); >> > + return 0; >> > + } >> > matched = credential_match(&want, c); >> >> Unfortunately, the whole section of this code that is being patched >> here goes away with 46fd7b39 (credential: allow wildcard patterns >> when matching config, 2020-02-20), that delegates large part of the >> logic to urlmatch. >> >> Dscho and Brian, how do we want to proceed? As the conflicting >> change has already been in 'next' for more than a month and a half, >> we'd need this "partial URL" logic build to work well with it. > > I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not > necessarily feel 100% comfortable with that approach yet, but at least it > lets the new test case of t0300 pass. The -2.17 patch series (your [v3 3/3]) ends t0300 like so + done && + + git -c credential.$partial.helper=yep \ + -c credential.with%0anewline.username=uh-oh \ + credential fill <stdin >stdout 2>stderr && + test_i18ngrep "skipping credential lookup for key" stderr + +' + test_done while the other branch lacks the extra blank line just before the single quote is closed. I queued 850ef627 (SQUASH??? lose excess blank line to match the other side of the eventual merge, 2020-04-24) on top so that the "-s ours" merge would be without unnecessary evil-merge noise. You probably would want to squash it in. Thanks.
Hi Junio, On Tue, 28 Apr 2020, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Hi Junio, > > > > On Fri, 24 Apr 2020, Junio C Hamano wrote: > > > >> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > >> writes: > >> > >> > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value, > >> > char *url = xmemdupz(key, dot - key); > >> > int matched; > >> > > >> > - credential_from_url(&want, url); > >> > + if (credential_from_potentially_partial_url(&want, url) < 0) { > >> > + warning(_("skipping credential lookup for key: %s"), > >> > + var); > >> > + credential_clear(&want); > >> > + free(url); > >> > + return 0; > >> > + } > >> > matched = credential_match(&want, c); > >> > >> Unfortunately, the whole section of this code that is being patched > >> here goes away with 46fd7b39 (credential: allow wildcard patterns > >> when matching config, 2020-02-20), that delegates large part of the > >> logic to urlmatch. > >> > >> Dscho and Brian, how do we want to proceed? As the conflicting > >> change has already been in 'next' for more than a month and a half, > >> we'd need this "partial URL" logic build to work well with it. > > > > I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not > > necessarily feel 100% comfortable with that approach yet, but at least it > > lets the new test case of t0300 pass. > > The -2.17 patch series (your [v3 3/3]) ends t0300 like so > > + done && > + > + git -c credential.$partial.helper=yep \ > + -c credential.with%0anewline.username=uh-oh \ > + credential fill <stdin >stdout 2>stderr && > + test_i18ngrep "skipping credential lookup for key" stderr > + > +' > + > test_done > > while the other branch lacks the extra blank line just before the > single quote is closed. I queued 850ef627 (SQUASH??? lose excess > blank line to match the other side of the eventual merge, > 2020-04-24) on top so that the "-s ours" merge would be without > unnecessary evil-merge noise. You probably would want to squash it > in. Thank you for pointing that out. I already did that: https://github.com/gitgitgadget/git/compare/0535908dd7ea4487b342c0f86182579279c57b34..d59738ecf741a5fddd70f133eaaf69768e245bcc Do you want me to send another iteration, for completeness' sake? Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> while the other branch lacks the extra blank line just before the >> single quote is closed. I queued 850ef627 (SQUASH??? ... > > Thank you for pointing that out. I already did that: ... > Do you want me to send another iteration, for completeness' sake? Then if I squash 850ef627 on my end, we'd be in sync, so unless there is any other change, no need to resend it. Have we got enough eyeballs that looked at the patches already? Thanks.
Hi Junio, On Wed, 29 Apr 2020, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> while the other branch lacks the extra blank line just before the > >> single quote is closed. I queued 850ef627 (SQUASH??? ... > > > > Thank you for pointing that out. I already did that: ... > > Do you want me to send another iteration, for completeness' sake? > > Then if I squash 850ef627 on my end, we'd be in sync, so unless > there is any other change, no need to resend it. Perfect, thanks! > Have we got enough eyeballs that looked at the patches already? I would obviously prefer more reviews. Having said that, GitHub Desktop rolled out a new release this past Monday with the MinGit backport I prepared with these here patches, and the only additional report at https://github.com/desktop/desktop/issues/9597 talks about macOS (and the MinGit backport is Windows-only). Which I take as good news for the robustness and correctness of the fixes. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > I would obviously prefer more reviews. > > Having said that, GitHub Desktop rolled out a new release this past Monday > with the MinGit backport I prepared with these here patches, and the only > additional report at https://github.com/desktop/desktop/issues/9597 talks > about macOS (and the MinGit backport is Windows-only). Which I take as > good news for the robustness and correctness of the fixes. Good. My preference actually is to fast-track this down to 'master' without the usual 'at least about a week in next' gestation period.
diff --git a/credential.c b/credential.c index 7dbbf26f174..c1a9ca4e485 100644 --- a/credential.c +++ b/credential.c @@ -35,6 +35,10 @@ int credential_match(const struct credential *want, #undef CHECK } + +static int credential_from_potentially_partial_url(struct credential *c, + const char *url); + static int credential_config_callback(const char *var, const char *value, void *data) { @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value, char *url = xmemdupz(key, dot - key); int matched; - credential_from_url(&want, url); + if (credential_from_potentially_partial_url(&want, url) < 0) { + warning(_("skipping credential lookup for key: %s"), + var); + credential_clear(&want); + free(url); + return 0; + } matched = credential_match(&want, c); credential_clear(&want); @@ -430,6 +440,12 @@ static int credential_from_url_1(struct credential *c, const char *url, return 0; } +static int credential_from_potentially_partial_url(struct credential *c, + const char *url) +{ + return credential_from_url_1(c, url, 1, 0); +} + int credential_from_url_gently(struct credential *c, const char *url, int quiet) { return credential_from_url_1(c, url, 0, quiet); diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index efed3ea2955..f796bbfd48b 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' ' test_i18ncmp expect stderr ' +test_expect_success 'credential config with partial URLs' ' + echo "echo password=yep" | write_script git-credential-yep && + test_write_lines url=https://user@example.com/repo.git >stdin && + for partial in \ + example.com \ + user@example.com \ + https:// \ + https://example.com \ + https://example.com/ \ + https://user@example.com \ + https://user@example.com/ \ + https://example.com/repo.git \ + https://user@example.com/repo.git \ + /repo.git + do + git -c credential.$partial.helper=yep \ + credential fill <stdin >stdout && + grep yep stdout || + return 1 + done && + + for partial in \ + dont.use.this \ + http:// \ + /repo + do + git -c credential.$partial.helper=yep \ + credential fill <stdin >stdout && + ! grep yep stdout || + return 1 + done && + + git -c credential.$partial.helper=yep \ + -c credential.with%0anewline.username=uh-oh \ + credential fill <stdin >stdout 2>stderr && + test_i18ngrep "skipping credential lookup for key" stderr + +' + test_done