Re: [PATCH 12/13] credentials: add "store" helper

On Tue, Nov 29, 2011 at 10:19:06AM -0800, Junio C Hamano wrote:

quoted

+	while (strbuf_getline(&line, fh, '\n') != EOF) {
+		credential_from_url(&entry, line.buf);
+		if (entry.username && entry.password &&
+		    credential_match(c, &entry)) {

This looks curious; isn't checking .username and .password part of the
responsibility of credential_match()? And even if entry lacks password
(which won't happen in the context of this program, given the
implementation of store_credential() below) shouldn't it still be
considered a match?

credential_match will check .username, if the pattern mentions it. It
will never check .password. My intent here was to enforce well-formed
entries in the credential file. So you could add:

  http://example.com/

to the credential file, but it's just meaningless noise. It
doesn't actually tell us a username or password.

The helper won't add such an entry itself, but given the simplicity of
the format, I wanted to leave the door open for curious hackers to
populate it manually if they choose.

I think you're right that:

  http://user@example.com/

is potentially meaningful, and this would skip that. OTOH, you would be
much better served to just do:

  git config credential.http://example.com.username user

So I consider it a slight abuse of this helper in the first place.

quoted

+static void rewrite_credential_file(const char *fn, struct credential *c,
+				    struct strbuf *extra)
+{
+	umask(077);

Curious placement of umask(). I would expect a function that has its own
call to umask() restore it before it returns, and a stand-alone program
whose sole purpose is to work with a private file, setting a tight umask
upfront at the beginning of main() may be easier to understand.

I think that is largely a holdover from the original implementation,
which set the umask and did other black magic before calling
git_config_set. I agree it would make more sense at the beginning of the
program. Will change.

quoted

+	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
+		die_errno("unable to get credential storage lock");
+	parse_credential_file(fn, c, NULL, print_line);
+	if (extra)
+		print_line(extra);

An entry for a newly updated password comes at the end of the file,
instead of replacing an entry already in the file in-place? Given that
parse_credential_file() when processing a look-up request (which is the
majority of the case) stops upon finding a match, it might make more sense
to have the new one (which may be expected to be used often) at the
beginning instead, no?

Yeah. It's a linear search. Your worst-case is always going to be O(n),
but I just assumed n would remain relatively small and we wouldn't care
(if it isn't, the right solution is probably a smarter data structure).

But your optimization is trivial to implement, so it's probably worth
doing.

quoted

+	if (commit_lock_file(&credential_lock) < 0)
+		die_errno("unable to commit credential store");
+}
+
+static void store_credential(const char *fn, struct credential *c)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!c->protocol || !(c->host || c->path) ||
+	    !c->username || !c->password)
+		return;

[...]

quoted

+static void remove_credential(const char *fn, struct credential *c)
+{
+	if (!c->protocol || !(c->host || c->path))
+		return;

The choice of the fields looks rather arbitrary. I cannot say "remove all
the credentials whose username is 'gitster' at 'github.com' no matter what
protocol is used", but I can say "remove all credentials under any name
for any host as long as the transfer goes over 'https' and accesses a
repository at 'if/xyzzy' path", it seems.

It is kind of arbitrary. The storage format is URLs, which is why
store_credential is a little pedantic. We can't store something that
doesn't have a protocol part, as that is a required part of the URL
(actually, in URL-speak this is the "scheme"; I wonder if we should use
the same term here).

I was thinking we need a protocol for the same reason in
remove_credential, but I think you are right. We never actually convert
it to a URL, so in theory you could do:

  git credential-store erase <<\EOF
  username=gitster
  host=github.com
  EOF

Again, not an operation that git will ever perform, but I guess
something that people might want to do (I had always assumed the
"$EDITOR ~/.git-credentials" was going to be the preferred way of doing
such operations :) ).

I don't think there's any harm in loosening that condition.

-Peff