Re: [PATCH v3] git-credential-store: skip empty lines and comments from store
From: Carlo Marcelo Arenas Belón <hidden>
Date: 2020-04-27 23:49:15
Possibly related (same subject, not in this thread)
- 2020-04-29 · Re: [PATCH v3] git-credential-store: skip empty lines and comments from store · Philip Oakley <hidden>
- 2020-04-28 · Re: [PATCH v3] git-credential-store: skip empty lines and comments from store · Junio C Hamano <hidden>
- 2020-04-28 · Re: [PATCH v3] git-credential-store: skip empty lines and comments from store · Carlo Marcelo Arenas Belón <hidden>
- 2020-04-28 · Re: [PATCH v3] git-credential-store: skip empty lines and comments from store · Junio C Hamano <hidden>
- 2020-04-28 · Re: [PATCH v3] git-credential-store: skip empty lines and comments from store · Carlo Marcelo Arenas Belón <hidden>
On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:quoted
On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote:quoted
quoted
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':
quoted hunk ↗ jump to hunk
quoted
quoted
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 <redacted> --- 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