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

From: Carlo Arenas <carenas@gmail.com>
Date: Sep 28, 2020, 23:26 UTC
Message-ID: <CAPUEspgW9CFO3WtbiuTUsmXp05fPqr2Cs81piDJFJ0g3KcTy3A@mail.gmail.com>
In-Reply-To: <f69076036fe4dfe8b57fc1d4329c7be3f7346850.1601293224.git.gitgitgadget@gmail.com>

On Mon, Sep 28, 2020 at 4:41 AM Nikita Leonov via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 33 quoted lines

> 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/