Re: [PATCH 1/2] git-credential-store: support XDG config dir

[Jeff King <peff@peff.net>]

On Wed, Mar 04, 2015 at 04:24:58AM +0800, Paul Tan wrote:

> @@ -111,8 +114,7 @@ static void remove_credential(const char *fn, struct credential *c)
>  
>  static int lookup_credential(const char *fn, struct credential *c)
>  {
> -	parse_credential_file(fn, c, print_entry, NULL);
> -	return c->username && c->password;
> +	return parse_credential_file(fn, c, print_entry, NULL);
>  }

I wondered if we were losing something here, as the return value from
parse_credential_file is not the same as "did we find both a username
and a password". But then I realized that the existing "return" line is
nonsensical. The "c" variable here is our template of what to look for,
not the return.

I think this is leftover from an earlier iteration, where our callback
filled in the values, rather than directly printing them. Nobody noticed
because we didn't actually look at the return value of lookup_credential
at all.

So I think regardless of the end goal, it is nice to see this oddity
cleaned up.

> +	if (!strcmp(op, "get")) {
> +		if (file) {
> +			lookup_credential(file, &c);
> +		} else {
> +			if (xdg_file && access_or_warn(xdg_file, R_OK, 0) == 0)
> +				ret = lookup_credential(xdg_file, &c);
> +			if (!ret && home_file && access_or_warn(home_file, R_OK, 0) == 0)
> +				lookup_credential(home_file, &c);
> +		}
> +	} else if (!strcmp(op, "erase")) {
> +		if (file) {
> +			remove_credential(file, &c);
> +		} else {
> +			if (xdg_file && access(xdg_file, F_OK) == 0)
> +				remove_credential(xdg_file, &c);
> +			if (home_file && access(home_file, F_OK) == 0)
> +				remove_credential(home_file, &c);
> +		}

The lookup rules here all look sane. Thanks for paying such attention
to the details. Like Matthieu, I was unclear on the inconsistent use of
access_or_warn.

If we can use the same access variant everywhere, I wonder if it would
be more readable to hoist it into a function like:

  int has_config_file(const char *file)
  {
	return file && access_or_warn(file, F_OK) == 0;
  }

It's a tiny function, but then your repetitious "if" statements drop
some of the noise:

  if (has_config_file(xdg_file))
	ret = lookup_credential(xdg_file, &c);
  if (!ret && has_config_file(home_file))
	lookup_credential(home_file, &c);

> +	} else if (!strcmp(op, "store")) {
> +		if (file) {
> +			store_credential(file, &c);
> +		} else if (xdg_file && access(xdg_file, F_OK) == 0) {
> +			store_credential(xdg_file, &c);
> +			if (home_file && access(home_file, F_OK) == 0 &&
> +			    c.protocol && (c.host || c.path) && c.username
> +			    && c.password)
> +				remove_credential(home_file, &c);

I like that you take care not to lose information during the migration,
but I really don't like that we have to replicate the "is this a
fully-formed credential" logic. I think I'd rather just store the
credential in the xdg_file, and leave it be in home_file. The lookup
will prefer the xdg version, and if we ever issue an "erase" (e.g.,
because the credential changes), it should remove both of them.

-Peff