Jeff King <peff@xxxxxxxx> writes:
> +static void parse_credential_file(const char *fn,
> + struct credential *c,
> + void (*match_cb)(struct credential *),
> + void (*other_cb)(struct strbuf *))
> +{
> + FILE *fh;
> + struct strbuf line = STRBUF_INIT;
> + struct credential entry = CREDENTIAL_INIT;
> +
> + fh = fopen(fn, "r");
> + if (!fh) {
> + if (errno != ENOENT)
> + die_errno("unable to open %s", fn);
> + return;
> + }
> +
> + 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?
> + if (match_cb) {
> + match_cb(&entry);
> + break;
Multiple matches not allowed, which sounds fine.
> + }
> + }
> + else if (other_cb)
> + other_cb(&line);
> + }
> +
> + credential_clear(&entry);
> + strbuf_release(&line);
> + fclose(fh);
> +}
> +
> +static void print_entry(struct credential *c)
> +{
> + printf("username=%s\n", c->username);
> + printf("password=%s\n", c->password);
> +}
> +
> +static void print_line(struct strbuf *buf)
> +{
> + strbuf_addch(buf, '\n');
> + write_or_die(credential_lock.fd, buf->buf, buf->len);
> +}
> +
> +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.
> + 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?
> + 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;
> +
> + strbuf_addf(&buf, "%s://", c->protocol);
> + strbuf_addstr_urlencode(&buf, c->username, 1);
> + strbuf_addch(&buf, ':');
> + strbuf_addstr_urlencode(&buf, c->password, 1);
> + strbuf_addch(&buf, '@');
> + if (c->host)
> + strbuf_addstr_urlencode(&buf, c->host, 1);
> + if (c->path) {
> + strbuf_addch(&buf, '/');
> + strbuf_addstr_urlencode(&buf, c->path, 0);
> + }
> +
> + rewrite_credential_file(fn, c, &buf);
> + strbuf_release(&buf);
> +}
> +
> +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.
This filtering matches what lookup_credential() does but shouldn't it be
implemented at a single place in any case?
> + rewrite_credential_file(fn, c, NULL);
> +}
> +
> +static int lookup_credential(const char *fn, struct credential *c)
> +{
> + if (!c->protocol || !(c->host || c->path))
> + return 0;
> + parse_credential_file(fn, c, print_entry, NULL);
> + return c->username && c->password;
> +}
--
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