Re: [PATCH v3] git-credential-store: skip empty lines and comments from store
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 27, 2020, 20:43 UTC
- Message-ID
- <xmqqv9lk7j7p.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20200427191858.GB1728884@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> 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?
Show 12 quoted lines
>> 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