git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Nikita Leonov via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Nikita Leonov <nykyta.leonov@gmail.com>
Subject: Re: [PATCH v2 2/3] credentials: make line reading Windows compatible
Date: Mon, 28 Sep 2020 13:58:03 -0700	[thread overview]
Message-ID: <xmqqk0wdliuc.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <f69076036fe4dfe8b57fc1d4329c7be3f7346850.1601293224.git.gitgitgadget@gmail.com> (Nikita Leonov via GitGitGadget's message of "Mon, 28 Sep 2020 11:40:23 +0000")

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

  reply	other threads:[~2020-09-28 20:58 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 13:49 [PATCH] credential.c: fix credential reading with regards to CR/LF Johannes Schindelin via GitGitGadget
2020-02-14 17:55 ` Junio C Hamano
2020-02-14 18:32 ` Jeff King
2020-09-28 11:40 ` [PATCH v2 0/3] Prepare git credential to read input with DOS line endings Johannes Schindelin via GitGitGadget
2020-09-28 11:40   ` [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF Nikita Leonov via GitGitGadget
2020-09-29  0:42     ` Jeff King
2020-10-02 11:37       ` Johannes Schindelin
2020-10-02 12:01         ` Jeff King
2020-10-02 12:27           ` Johannes Schindelin
2020-10-02 16:32         ` Junio C Hamano
2020-10-03 13:28           ` Johannes Schindelin
2020-09-28 11:40   ` [PATCH v2 2/3] credentials: make line reading Windows compatible Nikita Leonov via GitGitGadget
2020-09-28 20:58     ` Junio C Hamano [this message]
2020-09-29  0:35       ` Jeff King
2020-09-30 22:33         ` Junio C Hamano
2020-10-02  7:53           ` Johannes Schindelin
2020-09-28 23:26     ` Carlo Arenas
2020-09-28 23:41       ` Junio C Hamano
2020-09-29  0:30       ` Jeff King
2020-09-29  0:41         ` Junio C Hamano
2020-09-29  0:44           ` Jeff King
2020-09-29  0:54             ` Junio C Hamano
2020-09-29  3:00               ` Jeff King
2020-09-30 22:25                 ` Junio C Hamano
2020-09-30 22:39                   ` Jeff King
2020-09-30 22:56                     ` Junio C Hamano
2020-10-01 13:54                       ` Jeff King
2020-10-01 15:42                         ` Junio C Hamano
2020-10-02  8:07                           ` Johannes Schindelin
2020-09-28 11:40   ` [PATCH v2 3/3] docs: make notes regarding credential line reading Nikita Leonov via GitGitGadget
2020-09-28 20:31   ` [PATCH v2 0/3] Prepare git credential to read input with DOS line endings Junio C Hamano
2020-10-03 13:29   ` [PATCH v3] credential: treat CR/LF as line endings in the credential protocol Johannes Schindelin via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqk0wdliuc.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=nykyta.leonov@gmail.com \
    --subject='Re: [PATCH v2 2/3] credentials: make line reading Windows compatible' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).