Re: [PATCH v2 2/3] credentials: make line reading Windows compatible
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 28, 2020, 20:58 UTC
- Message-ID
- <xmqqk0wdliuc.fsf@gitster.c.googlers.com>
- In-Reply-To
- <f69076036fe4dfe8b57fc1d4329c7be3f7346850.1601293224.git.gitgitgadget@gmail.com>
"Nikita Leonov via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 90 quoted lines
> 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
> '