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

From: Jeff King <peff@peff.net>
Date: Apr 27, 2020, 21:10 UTC
Message-ID: <20200427211013.GB1732530@coredump.intra.peff.net>
In-Reply-To: <xmqqv9lk7j7p.fsf@gitster.c.googlers.com>

On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> >> 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, exactly. If I start with:

  cat >creds <<\EOF
  # this is a comment
  https://user:pass@example.com/
  EOF

then:

  echo url=https://other:pass@other.example.com |
  git credential-store --file=creds store

with v2.26.0 results in:

  https://other:pass@other.example.com
  # this is a comment
  https://user:pass@example.com

but applying the patch on top:

  https://other:pass@other.example.com
  https://user:pass@example.com/

That raises another issue about comments, too: we make no promises about where we write new entries. They could be inserted between a comment line and one it is meant to annotate (that line could even be moved if we reject and then re-approve a credential).

Show 6 quoted lines

> 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.

The more I look into it, the more negative I am on adding such a comment feature.

> -- >8 --
> Subject: credential-store: document the file format a bit more

Yeah, this certainly clarifies things. I suppose one could ask why we'd bother documenting the format at all, then, though I do think it could be helpful for debugging.

> 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.

s/torelate/tolerate/

-Peff