[PATCH] [GSoC15] git-credentials-store: support XDG config dir

[PATCH] [GSoC15] git-credentials-store: support XDG config dir

[Paul Tan]

Hi all,

This is my initial implementation for the GSoC15 microproject for
supporting both ~/.git-credentials and the XDG standard
$XDG_CONFIG_HOME/git/credentials.

I wrote the XDG tests in t0302-credential-store.sh in the end because it
depends on the helper_test and check functions defined in
lib-credential.sh.

For your feedback please.

Regards,
Paul

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

[Paul Tan]

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>
---
 credential-store.c | 60 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 15 deletions(-)

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;
 	}
 
 	while (strbuf_getline(&line, fh, '\n') != EOF) {
 		credential_from_url(&entry, line.buf);
 		if (entry.username && entry.password &&
 		    credential_match(c, &entry)) {
+			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
 				break;
@@ -38,6 +40,7 @@ static void parse_credential_file(const char *fn,
 	credential_clear(&entry);
 	strbuf_release(&line);
 	fclose(fh);
+	return found_credential;
 }
 
 static void print_entry(struct credential *c)
@@ -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);
 }
 
 int main(int argc, char **argv)
@@ -124,6 +126,9 @@ int main(int argc, char **argv)
 	const char *op;
 	struct credential c = CREDENTIAL_INIT;
 	char *file = NULL;
+	char *home_file = NULL;
+	char *xdg_file = NULL;
+	int ret = 0;
 	struct option options[] = {
 		OPT_STRING(0, "file", &file, "path",
 			   "fetch and store credentials in <path>"),
@@ -137,21 +142,46 @@ int main(int argc, char **argv)
 		usage_with_options(usage, options);
 	op = argv[0];
 
-	if (!file)
-		file = expand_user_path("~/.git-credentials");
-	if (!file)
-		die("unable to set up default path; use --file");
+	if (!file) {
+		home_config_paths(NULL, &xdg_file, "credentials");
+		home_file = expand_user_path("~/.git-credentials");
+		if (!xdg_file && !home_file)
+			die("unable to set up default path; use --file");
+	}
 
 	if (credential_read(&c, stdin) < 0)
 		die("unable to read credential");
 
-	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);
+		}
+	} 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);
+		} else
+			store_credential(home_file, &c);
+	} else
 		; /* Ignore unknown operation. */
 
 	return 0;
-- 
2.1.4

[PATCH 2/2] tests: test credential-store XDG support

[Paul Tan]

* "helper_test store" is run for when $XDG_CONFIG_HOME/git/credentials
  exists and ~/.git-credentials does not and the other way around.
* Test that credentials are stored in XDG file if both XDG and HOME
  files exist.
* Test that credentials from XDG file are used if matching credentials
  are found in both XDG and HOME files.
* Test that credentials from HOME file are used if a matching credential
  could not be found in the XDG file.
* Test that when erasing credentials, matching credentials in both the
  XDG and HOME files are erase.d

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t0302-credential-store.sh | 74 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index f61b40c..80893b9 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -4,6 +4,80 @@ test_description='credential-store tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
+# Tests for when ~/.git-credentials exists but
+# $XDG_CONFIG_HOME/git/credentials does not
 helper_test store
+test_expect_success '~/.git-credentials is written to when XDG git-credentials does not exist' '
+	test -s "$HOME/.git-credentials"
+'
+test_expect_success 'XDG credentials will not be created if it does not exist' '
+	test ! -e "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
+'
+
+# Tests for when $XDG_CONFIG_HOME/git/credentials exists but
+# ~/.git-credentials does not.
+rm "$HOME/.git-credentials"
+mkdir -p "${XDG_CONFIG_HOME:-$HOME/.config}/git"
+echo '' > "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
+helper_test store
+test_expect_success '~/.git-credentials will not be created if XDG git-credentials exist' '
+	test ! -e "$HOME/.git-credentials"
+'
+test_expect_success 'XDG credentials will be written to if it exists' '
+	test -s "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
+'
+
+# Tests for when both $XDG_CONFIG_HOME/git/credentials and
+# ~/.git-credentials exists.
+echo '' > "$HOME/.git-credentials"
+echo '' > "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
+test_expect_success 'Credentials are stored in XDG file if both XDG and HOME files exist' '
+	check approve store <<-\EOF
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	read contents < "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
+	test ! -z "$contents"
+	read contents < "$HOME/.git-credentials"
+	test -z "$contents"
+'
+test_expect_success 'Credentials from XDG file are used if the credentials exist in both XDG and HOME files' '
+	echo "https://baduser:badpass@example.com" > "$HOME/.git-credentials"
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	--
+	EOF
+'
+test_expect_success 'Credentials from HOME file are used if the XDG files does not have them' '
+	echo "" > "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=baduser
+	password=badpass
+	--
+	EOF
+'
+test_expect_success 'Credentials from both XDG and HOME files meeting the criteria are erased' '
+	check reject $HELPER <<-\EOF &&
+	protocol=https
+	host=example.com
+	EOF
+	read contents < "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
+	test -z "$contents"
+	read contents < "$HOME/.git-credentials"
+	test -z "$contents"
+'
 
 test_done
-- 
2.1.4

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

[Matthieu Moy]

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/

Re: [PATCH 2/2] tests: test credential-store XDG support

[Matthieu Moy]

Paul Tan <pyokagan@gmail.com> writes:

> +# Tests for when $XDG_CONFIG_HOME/git/credentials exists but
> +# ~/.git-credentials does not.

As much as possible, put text in the $1 of test_expect_success instead
of comments.

> +rm "$HOME/.git-credentials"
> +mkdir -p "${XDG_CONFIG_HOME:-$HOME/.config}/git"
> +echo '' > "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
> +helper_test store

Don't write code outside test_expect_success (read "Do's, don'ts &
things to keep in mind" in t/README).

> +test_expect_success '~/.git-credentials will not be created if XDG git-credentials exist' '
> +	test ! -e "$HOME/.git-credentials"
> +'

test_path_is_missing (see test-lib-functions.sh)

> +echo '' > "$HOME/.git-credentials"
> +echo '' > "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"

Not even that outside test_expect_success ;-).

Also, no space after > (Documentation/CodingGuidelines).


> +test_expect_success 'Credentials are stored in XDG file if both XDG and HOME files exist' '
> +	check approve store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	read contents < "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
> +	test ! -z "$contents"
> +	read contents < "$HOME/.git-credentials"

No space after <.

> +	test -z "$contents"
> +'
> +test_expect_success 'Credentials from XDG file are used if the
> +	credentials exist in both XDG and HOME files' '

Blank line between tests please.

> +test_expect_success 'Credentials from both XDG and HOME files meeting the criteria are erased' '
> +	check reject $HELPER <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	EOF
> +	read contents < "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
> +	test -z "$contents"
> +	read contents < "$HOME/.git-credentials"
> +	test -z "$contents"
> +'

I'd rather do stg like

echo >expected
test_cmp expected "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
test_cmp expected $HOME/.git-credentials

so that a test failure shows a diagnosis.

Regards,

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

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

[Junio C Hamano]

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?

Re: [PATCH 2/2] tests: test credential-store XDG support

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> * "helper_test store" is run for when $XDG_CONFIG_HOME/git/credentials
>   exists and ~/.git-credentials does not and the other way around.
> * Test that credentials are stored in XDG file if both XDG and HOME
>   files exist.
> * Test that credentials from XDG file are used if matching credentials
>   are found in both XDG and HOME files.
> * Test that credentials from HOME file are used if a matching credential
>   could not be found in the XDG file.
> * Test that when erasing credentials, matching credentials in both the
>   XDG and HOME files are erase.d
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---

Again, I agree with everything Matthieu said ;-)

> +test_expect_success 'XDG credentials will not be created if it does not exist' '
> +	test ! -e "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
> +'

You repeat this ${XDG_CONFIG_HOME:-$HOME/.config} all over, but is
that necessary?  The test environment is under your control, so if
you know you have XDG_CONFIG_HOME at this point in the test, don't
use the fallback.  If you want to test _both_, on the other hand,
then test both in separate tests.

> +# Tests for when both $XDG_CONFIG_HOME/git/credentials and
> +# ~/.git-credentials exists.
> +echo '' > "$HOME/.git-credentials"
> +echo '' > "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"

Do you need one empty line in each of these file, or is existence of
these two files what you care?  If the latter, then just redirect
into it without any command, like this:

	>"$HOME/.git-credentials"

> +test_expect_success 'Credentials are stored in XDG file if both XDG and HOME files exist' '
> +	check approve store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	read contents < "${XDG_CONFIG_HOME:-$HOME/.config}/git/credentials"
> +	test ! -z "$contents"

In an earlier part of this patch, you used "test -s FILE".  Do you
have to use "read $it && test [!] -z $it" here?

> +	read contents < "$HOME/.git-credentials"
> +	test -z "$contents"

Missing && everywhere (not just this test).

	check approve store <<-\EOF &&
        ...
        EOF
        read ... &&
        ...

If "check approve" exited with a non-zero status, that won't be
noticed as long as .../git/credentials file happens to have the
contents that passes the later part of the test, which is not what
we want.

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

[Jeff King]

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

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

[Paul Tan]

Hi all,

Thanks for the review. I apologize for rushing the patch out as I
wanted to get feedback on the new behavior before committing to any
more code changes. I also wanted to quickly clear any doubts that
people may have about my coding ability. (Or maybe it created more ;)
)


On Wed, Mar 4, 2015 at 6:00 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>> * 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).

Generally, I would like git to have full support for the XDG base dir
spec because it would allow it to become a good citizen in an
ecosystem of software which already support the XDG base dir spec by
default. Personally, I am annoyed when a piece of software does not
store its configuration into its own separate directory because I
version control the configuration of every piece of software (with git
;) ) in its own directory, and store them as submodules in my home
directory git repo.

Keeping consistent with the behavior of git-config, the presence of
the credentials file in the XDG git directory signals that the user
wants credentials to be stored in the XDG directory rather than the
HOME directory. However, we keep reading from the home credentials in
case the user is using old versions of git on the same home directory.

In fact, thinking about it again, I think the behavior implemented in
the patch may not be suitable. Comments below.

>> 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].

Eh? I thought lookup_credential has static linkage. The only other use
of lookup_credential is in credential-cache--daemon.c, and that has
its own function definition with static linkage.

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

For "get" even though the xdg file cannot be read I believe it should
not be a fatal error because the credential may be found in the home
file. We should still warn the user though because it may not be what
the user wants. However, I see now that I mistakenly broke
compatibility with the old behavior, which errors out if the home
credential file could not be read.

Thinking about it again, the behavior could go both ways. The user may
actually not want the xdg or home credential file to be unreadable,
and so we should error out if the files are unreadable. This is
something that the community should decide, I think, so opinions
please :) Personally, I think we should go with this behavior. If that
is so, access(file, F_OK) would be used instead of access_or_warn.

For "erase", as stated above, having the xdg credentials file existing
signals that the user wants the xdg location to be used. If the xdg
credentials file does not exist then git should just skip over it.
Hence, the use of access instead of access_or_warn.

(As a side note, I discovered the lockfile code does not respect write
permissions on the target file, and will just rename the lockfile
over. Perhaps git should do that?)

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

Yes, I agree that the entire code needs to be refactored. The code
needs to be moved into store_credential(), remove_credential() and
lookup_credential() instead of cluttering up main()

> 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.

Indeed, I committed a reasoning error there. I was thinking about what
happens if the user runs "store" on the new version of git, then
switches to an old one. Actually, "store" should write to both, so
that users will get the updated credentials regardless of whether they
use the old or new git.

In fact, the priority needs to be changed. To summarize, for
compatibility with older versions, git should read from the home
credentials file first, then the XDG one (since store will handle
updating of both). For writing, git should write to both files. For
erasing, git should erase from both files.

On Wed, Mar 4, 2015 at 7:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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".

Actually I think die_errno is a function that does not return at all.
The return is just to shut the compiler up. Perhaps I shall comment
that.

>> +     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.

Agreed, but I think I will re-factor out the credential check into its
separate function.

>
>> +                             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?

Actually, I think git should write to both files. See above.


On Wed, Mar 4, 2015 at 5:45 PM, Jeff King <peff@peff.net> wrote:
> 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.

Haha, you just reminded me of that issue when I was coding. At first,
when I was rapidly prototyping the changes, I thought that
lookup_credential would return some kind of true/false value, but then
I realized it kept on returning the same value all the time ;) Thus, I
implemented the function signature change in parse_credential_file().
Totally forgot about it when posting the patch though.

>> +     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.

I don't think the lookup rules are sane now ;) See above.

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

Yes, refactoring is required.

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

At first, I simply followed the existing behavior of
store_credential/parse_credential_file/credential_check as it will
remove duplicate credentials (matching on protocol, host, path,
username as keys). Now, however, I think that we should just write to
both the xdg and home files. See above.

Again, thanks everyone for taking time to review my code.

Regards,
Paul

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

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> On Wed, Mar 4, 2015 at 7:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Paul Tan <pyokagan@gmail.com> writes:
>>
>>>       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".
>
> Actually I think die_errno is a function that does not return at all.
> The return is just to shut the compiler up. Perhaps I shall comment
> that.

Commenting just on this part (I am not agreeing or disagreeing with
you on other parts of your message yet).

When fopen() fails because we cannot open an existing file for
reading, then die_errno() will trigger and we stop there, in which
case the return will not be reached.

But when we try to open fn and we fail only because fn does not
exist, we do not say "die".  We instead return to the caller,
telling it that we have not found any credential so far in the file
supplied by the caller.

So the return does matter, and spelling that zero with
found_credential does matter for readability, I would think.

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

[Matthieu Moy]

Paul Tan <pyokagan@gmail.com> writes:

> Hi all,
>
> Thanks for the review. I apologize for rushing the patch out as I
> wanted to get feedback on the new behavior before committing to any
> more code changes.

There is no problem sending unfinished versions for discussions. If
unsure, send it as [RFC/PATCH].

>> When writting a commit message, always insist on _why_ [...]
>
> Generally, I would like git to have full support for the XDG base dir
> spec

The point is not only why you implement XDG spec (which is not very
controversial), but also why you did it the way you did.

> In fact, thinking about it again, I think the behavior implemented in
> the patch may not be suitable. Comments below.

Writting more arguments in the commit message helps getting these
thoughts earlier ;-).

>>> 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].
>
> Eh? I thought lookup_credential has static linkage. The only other use
> of lookup_credential is in credential-cache--daemon.c, and that has
> its own function definition with static linkage.

Indeed, it was only me looking at "git grep" too fast. You're right.

>>> -     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)
>
> For "get" even though the xdg file cannot be read I believe it should
> not be a fatal error because the credential may be found in the home
> file. We should still warn the user though because it may not be what
> the user wants.

IMHO, this would deserve a short comment in the code, e.g. /* Warn the
user, but we may recover by finding credential in another file */ or so.

It's less sensitive, but there was more subtle breakages with the config
file (should Git do something at all when the config files can't be read
completely?).

> However, I see now that I mistakenly broke compatibility with the old
> behavior, which errors out if the home credential file could not be
> read.

You changed the behavior, but it's not really a compatibility breakage:
I doubt people _rely_ on Git dying in this case.

I have no strong opinion on what behavior is the best, I think yours
makes sense, but if you go for it, it should be documented in the commit
message (or even better: the change could be extracted into a separate
patch)

>> 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.
>
> Indeed, I committed a reasoning error there. I was thinking about what
> happens if the user runs "store" on the new version of git, then
> switches to an old one. Actually, "store" should write to both, so
> that users will get the updated credentials regardless of whether they
> use the old or new git.

Actually, your version made sense too. Credentials are usually
confidential data that you don't want to replicate too much. One reason
for using "store" can be that you want to overwrite your old password
with something else, and not leave your old password lying around
(because, e.g. it's the same password you use for your bank account and
nuclear launch code, and you haven't changed it there yet).

> In fact, the priority needs to be changed. To summarize, for
> compatibility with older versions, git should read from the home
> credentials file first, then the XDG one (since store will handle
> updating of both). For writing, git should write to both files. For
> erasing, git should erase from both files.

OTOH, if you really want compatibility with old versions, just don't
create .config/git/ and git will still write to ~/.git-credentials,
right? As long as XDG remains an opt-in feature, I wouldn't care too
much about backward compatibility.

At some point, I personnally think XDG should become the default, but we
can wait as much as needed to do that (and not everybody may agree with
me here).

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