Re: [PATCH v2 2/3] credentials: make line reading Windows compatible

[Carlo Arenas <carenas@gmail.com>]

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/