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

[Junio C Hamano <gitster@pobox.com>]

"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
>  '