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

[Junio C Hamano <gitster@pobox.com>]

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?

>> 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.
+
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
 each entry in the credentials file.  If the protocol, hostname, and