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

From: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>,
	git@vger.kernel.org, dirk@ed4u.de, sunshine@sunshineco.com
Subject: Re: [PATCH v3] git-credential-store: skip empty lines and comments from store
Date: Mon, 27 Apr 2020 16:49:09 -0700	[thread overview]
Message-ID: <20200427234909.GC61348@Carlos-MBP> (raw)
In-Reply-To: <xmqqv9lk7j7p.fsf@gitster.c.googlers.com>

On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote:
> >
> >> > modified store files which might have empty lines or even comments
> >> > were reported[1] failing to parse as valid credentials.
> >> 
> >> These files are not supposed to be viewed or edited without the help
> >> of the credential helpers.  Do these blank lines and comments even
> >> survive when a new credential is approved, or do we just overwrite
> >> and lose them?
> >
> > That's a good question. In the older code we do save them, because
> > credential-store passes through lines which don't match the credential
> > we're operating on.
> >
> > But in Carlo's patch, the immediate "continue" means we wouldn't ever
> > call "other_cb", which is what does that pass-through.
> 
> So, does that mean the patch that started this thread will still help
> users who wrote custom comments and blank lines in their credential
> store by letting git-credential-store start up, but leaves a ticking
> bomb for them to lose these precious comments and blanks once they
> add a new site, change password, etc., at which point the user realizes
> that comments and blanks are not supported after all?

yes, and it also helps users that might have added spaces or tabs
around their credentials while editing them to still be able to use those
instead of just failing to match them.

IMHO the only "regression" I was fixing was the fact that the current
code will get to throw a fatal error with an unhelpful message and prevent
access to valid credentials as shown by :

$ /git credential fill
protocol=http
host=example.com

warning: url has no scheme:
fatal: credential url cannot be parsed:
Username for 'http://example.com':

> >> I'd rather not to do either, if we did not have to, but if it were
> >> necessary for us to do something, I am OK to ignore empty lines.
> >> But I'd prefer not to mix the new "# comment" feature in, if we did
> >> not have to.
> >> 
> >> Also, triming the lines that are not empty is unwarranted.  IIUC,
> >> what the "store" action writes encodes whitespaces, so as soon as
> >> you see whitespace on either end, (or anywhere on the line for that
> >> matter), it is a hand-edited cruft in the file.  If you ignore
> >> comments, you probably should ignore those lines, too.
> >
> > Yeah, all of that seems quite sensible.
> 
> I think the first patch we need is a (belated) documentation patch,
> that adds to the existing "STORAGE FORMAT".  We already say "Each
> credential is stored on its own line as a URL", but we do not say
> anything about allowing other cruft in the file.  We probably
> should.  Adding a "comment" feature, if anybody feels like it, is OK
> and we can loosen the paragraph when that happens.
> 
> -- >8 --
> Subject: credential-store: document the file format a bit more
> 
> Reading a malformed credential URL line and silently ignoring it
> does not mean that we promise to torelate and/or keep empty lines
> and "# commented" lines forever.
> 
> Some people seem to take anything that is not explicitly forbidden
> as allowed, but the world does not work that way.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-credential-store.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 693dd9d9d7..76b0798856 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -94,6 +94,10 @@ stored on its own line as a URL like:
>  https://user:pass@example.com
>  ------------------------------
>  
> +No other kinds of lines (e.g. empty lines or comment lines) are
> +allowed in the file, even though some may be silently ignored. Do
> +not view or edit the file with editors.

view should be ok; mentioning that any typos or extraneous characters
will compromise the validation of credentials like I mentioned in my
proposed documentation update probably worth doing here instead, too.

Carlo