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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Paul Tan <pyokagan@gmail.com> writes:

> Teach git-credential-store to read/write credentials from
> $XDG_CONFIG_HOME/git/credentials and ~/.git-credentials where
> appropriate:

Thanks for your patch. Below is a partial review. Don't take my comments
as negative criticisms, they are all directions for improvement. I'm
actually positively surprised by the quality for a first patch :-).
Looking forward to your version2.

> * get: call lookup_credential() on the XDG file first if it exists. If
>   the credential can't be found, call lookup_credential() on the HOME
>   file.
> * erase: Call remove_credential() on both the XDG file if it exists and
>   the HOME file if it exists.
> * store: If the XDG file exists, call store_credential() on the XDG file
>   and remove_credential() on the HOME file to prevent duplicates.
> * If "--file" is provided, use the file for all operations instead.

When writting a commit message, always insist on _why_ you did what you
did, not _what_ you did (the patch already says it). For example, your
proposal for erase makes sense because if you're using "erase", you
probably don't want to leave cleartext passwords in another file. But
you didn't give the argument.

In other words: I hate GNU-style changelogs ;-).

Also, we usually put blank lines between items (read the output of "git
log --no-merges" in git.git to get an idea of the conventions).

> Likewise,
> lookup_credential() returns 1 if it could find the credential, and 0 if
> it could not.

Err, you're changing the calling convention, and you're not the only
caller (git grep lookup_credential).

If you need to change this existing function, best is to start your
series with a preparatory patch that does the calling convention change,
adapts the other caller, and then write your change on top, as [PATCH 2].

> -	if (!strcmp(op, "get"))
> -		lookup_credential(file, &c);
> -	else if (!strcmp(op, "erase"))
> -		remove_credential(file, &c);
> -	else if (!strcmp(op, "store"))
> -		store_credential(file, &c);
> -	else
> +	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);

Why is it somethimes access_or_warn and sometimes just access? (genuine
question)

> +		}
> +	} 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)

It would make sense to introduce a helper like sensible_credential(c),
or sanity_check(c). It could be used in store_credential too.

I'm not convinced you need to remove the credential from home_file if
the xdg_file takes precedence. Not saying you shouldn't, but you should
argue more at least.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/