git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin 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] credential.c: fix credential reading with regards to CR/LF
Date: Fri, 14 Feb 2020 09:55:54 -0800	[thread overview]
Message-ID: <xmqqmu9lnjdh.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <pull.710.git.git.1581688196706.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Fri, 14 Feb 2020 13:49:56 +0000")

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Nikita Leonov <nykyta.leonov@gmail.com>
>
> This fix makes using Git credentials more friendly to Windows users. In
> previous version it was unable to finish input correctly without
> configuration changes (tested in PowerShell, CMD, Cygwin).
>
> We know credential filling should be finished by empty input, but the
> current implementation does not take into account CR/LF ending, and
> hence instead of the empty string we get '\r', which is interpreted as
> an incorrect string.
>
> So this commit changes default reading function to a more Windows
> compatible reading function.
>
> Signed-off-by: Nikita Leonov <nykyta.leonov@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

In the older days, strbuf_getline() used to be what we have as
strbuf_getdelim() today, i.e. explicitly request what record
separator to use to grab a line.

The use of strbuf_getline_lf() we see in the pre-image of this patch
came from 8f309aeb ("strbuf: introduce strbuf_getline_{lf,nul}()",
2016-01-13), which mechanically replaced the use of
strbuf_getline(... '\n'), in anticipation for later effort to
identify ones that are better to accept CRLF and turn them into
_crlf variant.  Later all the callers of (old) strbuf_getline() went
away, and strbuf_getline_crlf() that allowed both LF and CRLF
termination (which is most friendly to human-readable line of text)
took over the shortest and sweetest name, strbuf_getline().

So, the "later" effort just happend to this code.  It was a bit
overdue, but it's better late than never.

>  credential.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/credential.c b/credential.c
> index 62be651b03..65989dfa4d 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -146,7 +146,7 @@ int credential_read(struct credential *c, FILE *fp)
>  {
>  	struct strbuf line = STRBUF_INIT;
>  
> -	while (strbuf_getline_lf(&line, fp) != EOF) {
> +	while (strbuf_getline(&line, fp) != EOF) {
>  		char *key = line.buf;
>  		char *value = strchr(key, '=');

There are many more conversions of strbuf_getline(..., '\n') to
strbuf_getline_lf(...) made by 8f309aeb to other parts of credential
stuff.  Has anybody from the Windows side made sure these other ones
are also better to accept CRLF, too?  

I'd wait for a bit to hear either "oh, we found two more and here is
an updated patch" or "we looked at them and this is the only one",
before touching this patch.

Thanks.




  reply	other threads:[~2020-02-14 17:56 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 13:49 Johannes Schindelin via GitGitGadget
2020-02-14 17:55 ` Junio C Hamano [this message]
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
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=xmqqmu9lnjdh.fsf@gitster-ct.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] credential.c: fix credential reading with regards to CR/LF' \
    /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).