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

[Junio C Hamano <gitster@pobox.com>]

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:
>
> * 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.
>
> In order to support the above, parse_credential_file() now returns 1 if
> it finds a matching credential, and 0 if it does not. Likewise,
> lookup_credential() returns 1 if it could find the credential, and 0 if
> it could not.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---

I agree with everything Matthieu said ;-)

> diff --git a/credential-store.c b/credential-store.c
> index 925d3f4..18b8897 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -6,7 +6,7 @@
>  
>  static struct lock_file credential_lock;
>  
> -static void parse_credential_file(const char *fn,
> +static int parse_credential_file(const char *fn,
>  				  struct credential *c,
>  				  void (*match_cb)(struct credential *),
>  				  void (*other_cb)(struct strbuf *))
> @@ -14,18 +14,20 @@ static void parse_credential_file(const char *fn,
>  	FILE *fh;
>  	struct strbuf line = STRBUF_INIT;
>  	struct credential entry = CREDENTIAL_INIT;
> +	int found_credential = 0;
>  
>  	fh = fopen(fn, "r");
>  	if (!fh) {
>  		if (errno != ENOENT)
>  			die_errno("unable to open %s", fn);
> -		return;
> +		return 0;

Returning found_credential here would be easier to read, no?  After
all, that is why you explicitly initialized it to 0 up there to say
"no we haven't found any yet".

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

If you have to chomp, do it like this instead:

> +			if (home_file && access(home_file, F_OK) == 0 &&
> +			    c.protocol && (c.host || c.path) &&
> +			    c.username && c.password)

It would make it more clear that the second line is about the URL
being accessed while the last line is about the user.

> +				remove_credential(home_file, &c);
> +		} else
> +			store_credential(home_file, &c);

The repetitiousness of the above three blocks looked somewhat
disturbing, but I guess you cannot avoid it because of the subtle
differences among these codepaths.  When "getting", you want to get
from only one place, and while not having an earlier candidate is
not an error, an existing but unreadable file deserves a warning.
When "erasing", you want to erase from everywhere.

I am not sure if I understand what you are doing in the store
codepath.  If your "get" will read from xdg (if available) without
looking at home anyway, why do you need to remove it from home?
Wouldn't the leftover be ignored anyway?