Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

From: Philip Oakley <philipoakley@iee.email>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, dirk@ed4u.de, sunshine@sunshineco.com,
	peff@peff.net
Subject: Re: [PATCH v3] git-credential-store: skip empty lines and comments from store
Date: Wed, 29 Apr 2020 11:09:31 +0100	[thread overview]
Message-ID: <c64b8f26-7f59-c701-20d2-7ca2e52f686d@iee.email> (raw)
In-Reply-To: <20200428014926.GE61348@Carlos-MBP>

Hi Carlo,
Thanks for the clarification.
Philip

On 28/04/2020 02:49, Carlo Marcelo Arenas Belón wrote:
> On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote:
>> On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote:
>>> with the added checks for invalid URLs in credentials, any locally
>>> modified store files which might have empty lines or even comments
>>> were reported[1] failing to parse as valid credentials.
>>>
>>> using the store file in this manner wasn't intended by the original
>>> code and it had latent issues which the new code dutifully prevented
>>> but since the strings used wouldn't had been valid credentials anyway
>>> we could instead detect them and skip the matching logic and therefore
>>> formalize this "feature".
>>>
>>> trim all lines as they are being read from the store file and skip the
>> Does trimming affect any credentials that may have spaces either end?
> all credentials are url encoded so the only spaces that are affected are
> the ones that were added by careless editing of that file.
>
> as Eric pointed out, any tabs will be silently "cleaned" as well.
>
>> Should the trimming of leading/trailing spaces be mentioned?
> I wanted to keep that as an undocumented "feature" as it is just meant
> to help people to avoid a fatal error during the transition but didn't
> want to encourage people thinking it is a supported part or even worse
> encourage people to start editing their files to add tabs and spaces.
>
> Junio's suggested documentation fix[1] makes that clearer that I could
>
>> Also the git-credential page mentions that the credential must end with
>> a blank line ("don't forget.."). Should that be mentioned here, or have
>> I misunderstood?
> that is for the protocol part of it (which is used between git and the
> credential helper), the file format for this specific helper doesn't
> require that.
>
> Carlo
>
> [1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/