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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2020, 22:56 UTC
Message-ID: <xmqq5z7ug9gk.fsf@gitster.c.googlers.com>
In-Reply-To: <20200930223913.GB1908000@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

> On Wed, Sep 30, 2020 at 03:25:09PM -0700, Junio C Hamano wrote:
>
>> OK, so what's the final verdict from us?  This is good enough to
>> move forward as-is?
>
> I'd prefer to see the credential-cache--daemon changes either dropped,
> or split out into a separate patch with the justification of: this
> probably doesn't matter in practice, but it makes the whole protocol
> between client and server treat CRLF consistently.
>
> I had also raised a question in:
>
>   https://lore.kernel.org/git/20200929004220.GC898702@coredump.intra.peff.net/
>
> about whether they just care about the empty line, or about CRLF on data
> lines. If the former (which is what the commit message claims), then I
> think we can do something much simpler. But I suspect it is the latter,
> and it is simply the commit message that is a bit misleading.
>
> Other than those nits, I think the series is OK.

Sure. But credential-store side is also iffy; it is not like they want CRLF on data lines (if they want CR in data, that needs to be encoded). The only reason I can think of that the change to "-store" makes any difference is when people edit it, but the file is not designed to be manually edited, so even that part of the series needs a better justification. It's not like "We want to be compatible" without "why it is better to be compatible" is a good rationale, when we define the file format not to be manually edited in the first place.

Thanks.