Message ID | f69076036fe4dfe8b57fc1d4329c7be3f7346850.1601293224.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Prepare git credential to read input with DOS line endings | expand |
"Nikita Leonov via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Nikita Leonov <nykyta.leonov@gmail.com> > > This commit makes reading process regarding credentials compatible with > 'CR/LF' line ending. It makes using git more convenient on systems like > Windows. I can see why this is a good thing for "store" and the two updated pieces of the test script demonstrate it very well. But it is unclear why and how cache-daemon benefits from this change. That needs to be justified. As to the log message (this is 70% style, but there are consequences), we tend not to say "This commit does X"---because such a statement is often insufficient to illustrate if the commit indeed does X, and explain why it is a good thing to do X in the first place. Instead, we first explain that the current system does not do X (in present tense, so we do NOT say "previously we did not do X"), then explain why doing X would be a good thing, and finally give an order to the codebase to start doing X. For this change, it might look like this: The credential subsystem has assumed that lines always end with LF. On a system whose text file ends with CRLF (e.g. Windows), this assumption causes the CR at the end of the line as an extra byte appended to the data, and worse, there is no way to send an "empty line" to signal an end of a group of lines, because such a line would be taken as a line with a lone CR on it. Because credential-store writes to and reads from a text file on disk, which users may manually edit with their editor and turn the line-ending to CRLF. Accept lines that end with CR/LF, as well as those that end with LF. Note that the credential-cache--daemon is not touched, because it is only about interacting with in-core cache, and there is nowhere CRLF comes into the picture. Note: I didn't know why we need to touch the daemon, and that is why I wrote the paragraph on it as such, but I expect that the real log message to justify the change would be quite different and explain why the daemon code needs the same change well---such a description would not be "Note that", but would come before the "Accept lines that end with..." and after the paragraph about credential-store. Thanks. > Signed-off-by: Nikita Leonov <nykyta.leonov@gmail.com> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/credential-cache--daemon.c | 4 ++-- > builtin/credential-store.c | 2 +- > t/t0302-credential-store.sh | 16 ++++++---------- > 3 files changed, 9 insertions(+), 13 deletions(-) > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index c61f123a3b..17664bb0d5 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -99,12 +99,12 @@ static int read_request(FILE *fh, struct credential *c, > static struct strbuf item = STRBUF_INIT; > const char *p; > > - strbuf_getline_lf(&item, fh); > + strbuf_getline(&item, fh); > if (!skip_prefix(item.buf, "action=", &p)) > return error("client sent bogus action line: %s", item.buf); > strbuf_addstr(action, p); > > - strbuf_getline_lf(&item, fh); > + strbuf_getline(&item, fh); > if (!skip_prefix(item.buf, "timeout=", &p)) > return error("client sent bogus timeout line: %s", item.buf); > *timeout = atoi(p); > diff --git a/builtin/credential-store.c b/builtin/credential-store.c > index 5331ab151a..d4e90b68df 100644 > --- a/builtin/credential-store.c > +++ b/builtin/credential-store.c > @@ -23,7 +23,7 @@ static int parse_credential_file(const char *fn, > return found_credential; > } > > - while (strbuf_getline_lf(&line, fh) != EOF) { > + while (strbuf_getline(&line, fh) != EOF) { > if (!credential_from_url_gently(&entry, line.buf, 1) && > entry.username && entry.password && > credential_match(c, &entry)) { > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > index 716bf1af9f..f2c672e4b6 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -142,7 +142,7 @@ invalid_credential_test "scheme" ://user:pass@example.com > invalid_credential_test "valid host/path" https://user:pass@ > invalid_credential_test "username/password" https://pass@example.com > > -test_expect_success 'get: credentials with DOS line endings are invalid' ' > +test_expect_success 'get: credentials with DOS line endings are valid' ' > printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" && > check fill store <<-\EOF > protocol=https > @@ -150,11 +150,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid' ' > -- > protocol=https > host=example.com > - username=askpass-username > - password=askpass-password > + username=user > + password=pass > -- > - askpass: Username for '\''https://example.com'\'': > - askpass: Password for '\''https://askpass-username@example.com'\'': > -- > EOF > ' > @@ -172,7 +170,7 @@ test_expect_success 'get: credentials with path and DOS line endings are valid' > EOF > ' > > -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' ' > +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' ' > printf "https://user:pass@example.com/repo.git\r\n" >"$HOME/.git-credentials" && > test_config credential.useHttpPath true && > check fill store <<-\EOF > @@ -181,11 +179,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid if path > protocol=https > host=example.com > path=repo.git > - username=askpass-username > - password=askpass-password > + username=user > + password=pass > -- > - askpass: Username for '\''https://example.com/repo.git'\'': > - askpass: Password for '\''https://askpass-username@example.com/repo.git'\'': > -- > EOF > '
On Mon, Sep 28, 2020 at 4:41 AM Nikita Leonov via GitGitGadget <gitgitgadget@gmail.com> wrote: > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > index 716bf1af9f..f2c672e4b6 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -142,7 +142,7 @@ invalid_credential_test "scheme" ://user:pass@example.com > invalid_credential_test "valid host/path" https://user:pass@ > invalid_credential_test "username/password" https://pass@example.com > > -test_expect_success 'get: credentials with DOS line endings are invalid' ' > +test_expect_success 'get: credentials with DOS line endings are valid' ' > printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" && > check fill store <<-\EOF > protocol=https > @@ -150,11 +150,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid' ' > -- > protocol=https > host=example.com > - username=askpass-username > - password=askpass-password > + username=user > + password=pass > -- > - askpass: Username for '\''https://example.com'\'': > - askpass: Password for '\''https://askpass-username@example.com'\'': > -- > EOF > ' > @@ -172,7 +170,7 @@ test_expect_success 'get: credentials with path and DOS line endings are valid' > EOF > ' > > -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' ' > +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' ' note that this test was put in place to protect users from regressions like the one we got after the release of 2.26.1 where users that had '\r' as part of their credentials were getting an error[1] while I am sympathetic to the change (indeed I proposed something similar, but was reminded by Peff that while it looks like a text file it was designed to be considered opaque and therefore should use UNIX LF as record terminator by specification), I am concerned this could result in a similar regression since we know they are still users out there that had modified this file manually (something that was not recommended) and are currently relying on the fact that these lines are invalid and therefore silently ignored. Carlo [1] https://lore.kernel.org/git/ad80aa0d-3a35-6d7e-7958-b3520e16c855@ed4u.de/
Carlo Arenas <carenas@gmail.com> writes: >> -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' ' >> +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' ' > > note that this test was put in place to protect users from regressions > like the one we got after the release of 2.26.1 where users that had > '\r' as part of their credentials were getting an error[1] > > while I am sympathetic to the change (indeed I proposed something > similar, but was reminded by Peff that while it looks like a text file > it was designed to be considered opaque and therefore should use UNIX > LF as record terminator by specification), I am concerned this could > result in a similar regression since we know they are still users out > there that had modified this file manually (something that was not > recommended) and are currently relying on the fact that these lines > are invalid and therefore silently ignored. > > Carlo > > [1] https://lore.kernel.org/git/ad80aa0d-3a35-6d7e-7958-b3520e16c855@ed4u.de/ I think you meant to remind us and this thread of the earlier review and discussion thread, which begins at https://lore.kernel.org/git/20200426234750.40418-1-carenas@gmail.com/ And thanks for doing so---I totally forgot about it.
On Mon, Sep 28, 2020 at 04:26:38PM -0700, Carlo Arenas wrote: > > -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' ' > > +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' ' > > note that this test was put in place to protect users from regressions > like the one we got after the release of 2.26.1 where users that had > '\r' as part of their credentials were getting an error[1] > > while I am sympathetic to the change (indeed I proposed something > similar, but was reminded by Peff that while it looks like a text file > it was designed to be considered opaque and therefore should use UNIX > LF as record terminator by specification), I am concerned this could > result in a similar regression since we know they are still users out > there that had modified this file manually (something that was not > recommended) and are currently relying on the fact that these lines > are invalid and therefore silently ignored. Going over that old thread, I'm not sure that anybody raised a real use case where somebody expected a CR at the end of a line. The discussion was mostly about whether proposed changes would or would not be compatible with existing behavior. I think that: - we'd never write a raw CR ourselves, as we'd urlencode the character - if somebody did put in a raw CR manually like: https://example.com\r\n then we'd currently fail to match "example.com". Which is probably not what they wanted. I suspect that \r in a hostname is bogus anyway (certainly curl will complain about it). - you could do the same in a path: https://example.com/foo\r\n which _is_ legal, but I think we'd generally ignore it completely unless credential.usehttppath is set (for network-accessible requests, that is; you could probably point your local cert file at something bogus, but I think the main areas of interest here are people manipulating the credential protocol via malicious urls). So I'm OK loosening the format in the name of convenience, as long as we're confident that it's not opening up any security problems. I can't think of any such problems, though. It sounds from your email like you may have found me arguing the opposite. That's entirely possible. ;) But I couldn't find it in the thread Junio linked. -Peff
On Mon, Sep 28, 2020 at 01:58:03PM -0700, Junio C Hamano wrote: > "Nikita Leonov via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Nikita Leonov <nykyta.leonov@gmail.com> > > > > This commit makes reading process regarding credentials compatible with > > 'CR/LF' line ending. It makes using git more convenient on systems like > > Windows. > > I can see why this is a good thing for "store" and the two updated > pieces of the test script demonstrate it very well. > > But it is unclear why and how cache-daemon benefits from this change. > That needs to be justified. I suspect it doesn't need touched, because it is internal to git-daemon. But it does handle CRLF for some lines, because the first patch modified credential_read(), which the daemon builds on (and which _is_ user-facing via git-credential). So there's perhaps an argument that these calls should just be made consistent, even though the only one who would write them is our matching client. If that is the argument to be made, I think it would make sense to do so in a separate patch, since there's no functional change. (I'm also slightly puzzled that anybody on Windows would care about credential-cache, since it require unix sockets. But maybe in a world of WSL people are actually able to mix the two. I confess I haven't kept up with the state of things in Windows). -Peff
Jeff King <peff@peff.net> writes: > I think that: > > - we'd never write a raw CR ourselves, as we'd urlencode the character > > - if somebody did put in a raw CR manually like: > > https://example.com\r\n > > then we'd currently fail to match "example.com". Which is probably > not what they wanted. I suspect that \r in a hostname is bogus > anyway (certainly curl will complain about it). I may be misremembering, but an argument I recall against the kind of change we are dicussing now was that we ignore such an entry right now, and the user may have added an entry for the host anew, possibly with a more recent password. Changing the parsing to ignore CR would silently resurrect such a stale entry that the user has written off as unused, and depending on the order of entries in the file, a site that used to work may start failing suddenly. I don't think I'd care too heavily either way. As long as other people will deal with possible backward-incompatibility fallout, I do not mind seeing the change ;-). I still don't see why we need to touch the cache-daemon, though. Thanks.
On Mon, Sep 28, 2020 at 05:41:14PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I think that: > > > > - we'd never write a raw CR ourselves, as we'd urlencode the character > > > > - if somebody did put in a raw CR manually like: > > > > https://example.com\r\n > > > > then we'd currently fail to match "example.com". Which is probably > > not what they wanted. I suspect that \r in a hostname is bogus > > anyway (certainly curl will complain about it). > > I may be misremembering, but an argument I recall against the kind > of change we are dicussing now was that we ignore such an entry > right now, and the user may have added an entry for the host anew, > possibly with a more recent password. Changing the parsing to > ignore CR would silently resurrect such a stale entry that the user > has written off as unused, and depending on the order of entries in > the file, a site that used to work may start failing suddenly. Yeah, that is probably what would happen. I have to admit that it's such an obscure case that I'm not sure I really care. It's unlikely in practice, and if somebody did report such a case, I think my first response would be "well, why did you have a broken entry stuck in your file?". > I still don't see why we need to touch the cache-daemon, though. Yeah, I touched on that more in another response. -Peff
Jeff King <peff@peff.net> writes: > Yeah, that is probably what would happen. I have to admit that it's such > an obscure case that I'm not sure I really care. It's unlikely in > practice, and if somebody did report such a case, I think my first > response would be "well, why did you have a broken entry stuck in your > file?". I think we know the likely answer. "I once used Windows to edit the file manually". After which the file looks broken, so the user may have re-added via the credential API (with LF line ending) a new entry for the host and have been happily using the result.
On Mon, Sep 28, 2020 at 05:54:37PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Yeah, that is probably what would happen. I have to admit that it's such > > an obscure case that I'm not sure I really care. It's unlikely in > > practice, and if somebody did report such a case, I think my first > > response would be "well, why did you have a broken entry stuck in your > > file?". > > I think we know the likely answer. "I once used Windows to edit the > file manually". > > After which the file looks broken, so the user may have re-added via > the credential API (with LF line ending) a new entry for the host > and have been happily using the result. I wrote something less charitable towards the user at first, and then toned it down. But I think I toned it down too much. Maybe my response would be "garbage in, garbage out; it was lucky that it worked before". -Peff
Jeff King <peff@peff.net> writes: > On Mon, Sep 28, 2020 at 05:54:37PM -0700, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >> > Yeah, that is probably what would happen. I have to admit that it's such >> > an obscure case that I'm not sure I really care. It's unlikely in >> > practice, and if somebody did report such a case, I think my first >> > response would be "well, why did you have a broken entry stuck in your >> > file?". >> >> I think we know the likely answer. "I once used Windows to edit the >> file manually". >> >> After which the file looks broken, so the user may have re-added via >> the credential API (with LF line ending) a new entry for the host >> and have been happily using the result. > > I wrote something less charitable towards the user at first, and then > toned it down. But I think I toned it down too much. Maybe my response > would be "garbage in, garbage out; it was lucky that it worked before". OK, so what's the final verdict from us? This is good enough to move forward as-is?
Jeff King <peff@peff.net> writes: > On Mon, Sep 28, 2020 at 01:58:03PM -0700, Junio C Hamano wrote: > >> "Nikita Leonov via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >> > From: Nikita Leonov <nykyta.leonov@gmail.com> >> > >> > This commit makes reading process regarding credentials compatible with >> > 'CR/LF' line ending. It makes using git more convenient on systems like >> > Windows. >> >> I can see why this is a good thing for "store" and the two updated >> pieces of the test script demonstrate it very well. >> >> But it is unclear why and how cache-daemon benefits from this change. >> That needs to be justified. > > I suspect it doesn't need touched, because it is internal to git-daemon. > But it does handle CRLF for some lines, because the first patch > modified credential_read(), which the daemon builds on (and which _is_ > user-facing via git-credential). So there's perhaps an argument that > these calls should just be made consistent, even though the only one who > would write them is our matching client. If that is the argument to be > made, I think it would make sense to do so in a separate patch, since > there's no functional change. > > (I'm also slightly puzzled that anybody on Windows would care about > credential-cache, since it require unix sockets. But maybe in a world of > WSL people are actually able to mix the two. I confess I haven't kept up > with the state of things in Windows). True, so some, if not all, parts of these changes start to look more and more like "I change LF to CRLF purely for political correctness, even I know nobody sends CRLF in these cases (or "even I do not know if anybody sends CRLF in these cases", which essentially amounts to the same thing but may be worse)", needless code churns. Sigh.
On Wed, Sep 30, 2020 at 03:25:09PM -0700, Junio C Hamano wrote: > OK, so what's the final verdict from us? This is good enough to > move forward as-is? I'd prefer to see the credential-cache--daemon changes either dropped, or split out into a separate patch with the justification of: this probably doesn't matter in practice, but it makes the whole protocol between client and server treat CRLF consistently. I had also raised a question in: https://lore.kernel.org/git/20200929004220.GC898702@coredump.intra.peff.net/ about whether they just care about the empty line, or about CRLF on data lines. If the former (which is what the commit message claims), then I think we can do something much simpler. But I suspect it is the latter, and it is simply the commit message that is a bit misleading. Other than those nits, I think the series is OK. -Peff
Jeff King <peff@peff.net> writes: > On Wed, Sep 30, 2020 at 03:25:09PM -0700, Junio C Hamano wrote: > >> OK, so what's the final verdict from us? This is good enough to >> move forward as-is? > > I'd prefer to see the credential-cache--daemon changes either dropped, > or split out into a separate patch with the justification of: this > probably doesn't matter in practice, but it makes the whole protocol > between client and server treat CRLF consistently. > > I had also raised a question in: > > https://lore.kernel.org/git/20200929004220.GC898702@coredump.intra.peff.net/ > > about whether they just care about the empty line, or about CRLF on data > lines. If the former (which is what the commit message claims), then I > think we can do something much simpler. But I suspect it is the latter, > and it is simply the commit message that is a bit misleading. > > Other than those nits, I think the series is OK. Sure. But credential-store side is also iffy; it is not like they want CRLF on data lines (if they want CR in data, that needs to be encoded). The only reason I can think of that the change to "-store" makes any difference is when people edit it, but the file is not designed to be manually edited, so even that part of the series needs a better justification. It's not like "We want to be compatible" without "why it is better to be compatible" is a good rationale, when we define the file format not to be manually edited in the first place. Thanks.
On Wed, Sep 30, 2020 at 03:56:27PM -0700, Junio C Hamano wrote: > > Other than those nits, I think the series is OK. > > Sure. But credential-store side is also iffy; it is not like they > want CRLF on data lines (if they want CR in data, that needs to be > encoded). The only reason I can think of that the change to > "-store" makes any difference is when people edit it, but the file > is not designed to be manually edited, so even that part of the > series needs a better justification. It's not like "We want to be > compatible" without "why it is better to be compatible" is a good > rationale, when we define the file format not to be manually edited > in the first place. Yeah, I agree that just teaching git-credential's stdin to handle CR would be an OK stopping point. I don't have a strong opinion on credential-store's on disk format. At least allowing CRLF there is _plausibly_ useful, unlike credential-cache--daemon's pipe. And I doubt that making the change would hurt anybody. -Peff
Jeff King <peff@peff.net> writes: > Yeah, I agree that just teaching git-credential's stdin to handle CR > would be an OK stopping point. I don't have a strong opinion on > credential-store's on disk format. At least allowing CRLF there is > _plausibly_ useful, unlike credential-cache--daemon's pipe. And I doubt > that making the change would hurt anybody. I agree 100% with all of the above, including the part that says it is also OK to let credential-store read from its files with line ending converted to CRLF. Thanks.
Hi Junio & Peff, On Wed, 30 Sep 2020, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Mon, Sep 28, 2020 at 01:58:03PM -0700, Junio C Hamano wrote: > > > >> "Nikita Leonov via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> > >> > From: Nikita Leonov <nykyta.leonov@gmail.com> > >> > > >> > This commit makes reading process regarding credentials compatible with > >> > 'CR/LF' line ending. It makes using git more convenient on systems like > >> > Windows. > >> > >> I can see why this is a good thing for "store" and the two updated > >> pieces of the test script demonstrate it very well. > >> > >> But it is unclear why and how cache-daemon benefits from this change. > >> That needs to be justified. > > > > I suspect it doesn't need touched, because it is internal to git-daemon. > > But it does handle CRLF for some lines, because the first patch > > modified credential_read(), which the daemon builds on (and which _is_ > > user-facing via git-credential). So there's perhaps an argument that > > these calls should just be made consistent, even though the only one who > > would write them is our matching client. If that is the argument to be > > made, I think it would make sense to do so in a separate patch, since > > there's no functional change. > > > > (I'm also slightly puzzled that anybody on Windows would care about > > credential-cache, since it require unix sockets. But maybe in a world of > > WSL people are actually able to mix the two. I confess I haven't kept up > > with the state of things in Windows). > > True, so some, if not all, parts of these changes start to look more > and more like "I change LF to CRLF purely for political correctness, > even I know nobody sends CRLF in these cases (or "even I do not know > if anybody sends CRLF in these cases", which essentially amounts to > the same thing but may be worse)", needless code churns. Indeed. I did not make the connection that `credential-cache--daemon` uses Unix sockets internally (and hence if you try to use it on Windows, you are greeted with the message "credential-cache--daemon unavailable; no unix socket support"). Will drop this part from the next iteration. Ciao, Dscho
Hi, On Thu, 1 Oct 2020, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Yeah, I agree that just teaching git-credential's stdin to handle CR > > would be an OK stopping point. I don't have a strong opinion on > > credential-store's on disk format. At least allowing CRLF there is > > _plausibly_ useful, unlike credential-cache--daemon's pipe. And I doubt > > that making the change would hurt anybody. > > I agree 100% with all of the above, including the part that says it > is also OK to let credential-store read from its files with line > ending converted to CRLF. After mulling over this, I am more inclined to drop the credential-store changes, too. That file _is_ an implementation detail, and not intended for interactive editing. And as Carlo pointed out so correctly, the regression tests were not introduced "just for fun", they really want to make sure that we keep handling CR/LF the way we currently do. So I will drop both credential-cache--daemon and credential-store changes from the next iteration. Ciao, Dscho
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index c61f123a3b..17664bb0d5 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -99,12 +99,12 @@ static int read_request(FILE *fh, struct credential *c, static struct strbuf item = STRBUF_INIT; const char *p; - strbuf_getline_lf(&item, fh); + strbuf_getline(&item, fh); if (!skip_prefix(item.buf, "action=", &p)) return error("client sent bogus action line: %s", item.buf); strbuf_addstr(action, p); - strbuf_getline_lf(&item, fh); + strbuf_getline(&item, fh); if (!skip_prefix(item.buf, "timeout=", &p)) return error("client sent bogus timeout line: %s", item.buf); *timeout = atoi(p); diff --git a/builtin/credential-store.c b/builtin/credential-store.c index 5331ab151a..d4e90b68df 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -23,7 +23,7 @@ static int parse_credential_file(const char *fn, return found_credential; } - while (strbuf_getline_lf(&line, fh) != EOF) { + while (strbuf_getline(&line, fh) != EOF) { if (!credential_from_url_gently(&entry, line.buf, 1) && entry.username && entry.password && credential_match(c, &entry)) { diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index 716bf1af9f..f2c672e4b6 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -142,7 +142,7 @@ invalid_credential_test "scheme" ://user:pass@example.com invalid_credential_test "valid host/path" https://user:pass@ invalid_credential_test "username/password" https://pass@example.com -test_expect_success 'get: credentials with DOS line endings are invalid' ' +test_expect_success 'get: credentials with DOS line endings are valid' ' printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" && check fill store <<-\EOF protocol=https @@ -150,11 +150,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid' ' -- protocol=https host=example.com - username=askpass-username - password=askpass-password + username=user + password=pass -- - askpass: Username for '\''https://example.com'\'': - askpass: Password for '\''https://askpass-username@example.com'\'': -- EOF ' @@ -172,7 +170,7 @@ test_expect_success 'get: credentials with path and DOS line endings are valid' EOF ' -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' ' +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' ' printf "https://user:pass@example.com/repo.git\r\n" >"$HOME/.git-credentials" && test_config credential.useHttpPath true && check fill store <<-\EOF @@ -181,11 +179,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid if path protocol=https host=example.com path=repo.git - username=askpass-username - password=askpass-password + username=user + password=pass -- - askpass: Username for '\''https://example.com/repo.git'\'': - askpass: Password for '\''https://askpass-username@example.com/repo.git'\'': -- EOF '