On Tue, Nov 29, 2011 at 10:19:06AM -0800, Junio C Hamano wrote:
> > + 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@xxxxxxxxxxx/
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.
> > +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.
> > + 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.
> > + 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;
> [...]
> > +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html