threads / patch / 38752

v2, 4 partsgit-credential-store: support multiple credential files

Subject: [PATCH v2 1/4] git-credential-store: support multiple credential files

## tl;dr

8 messages between Mar 8, 2015 and Mar 10, 2015. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Paul Tan· Mar 8, 2015, 07:58 UTC · lore

[PATCH v2 0/4] git-credential-store: XDG user-specific config file support

The previous patch series can be found at [1].
[1] http://thread.gmane.org/gmane.comp.version-control.git/264682
The changes are as follows:
* Code refactor: instead of a gigantic blob of code in main(),
  store_credential(), remove_credential() and lookup_credential() have been
  modified to take a string_list of precedence-ordered file paths. Although in
  this patch only support for XDG_CONFIG_HOME (user-specific config files) are
  implemented, this opens the door for support of XDG_CONFIG_DIRS (system-wide
  config files) to be implemented as well.
* parse_credential_file() returns the value of found_credential at all times.
  (Thanks Junio for pointing this out)
* parse_credential_file(), and thus "get" ignores unreadable/non-existing files
  instead of warning the user. This is done to follow the XDG base dir spec,
  which states that: "if for any reason a file in a certain directory is
  unaccessible, e.g. because the directory is non-existant, the file is
  non-existant or the user is not authorized to open the file, then the
  processing of the file in that directory should be skipped."[2]
[2] http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
* "store" now only write to a single file instead of writing to one and erasing
  from the rest, unlike the behavior in the 1st patch, as the complexity
  introduced by implementing this behavior is probably unnecessary. If the user
  really wanted all matching credentials to be erased, the user can simply just
  issue an erase. (Thanks Matthieu, Junio and Jeff for this suggestion)
* ~/.git-credentials now has greater precedence than the xdg credentials file.
  This is to be consistent with the behavior of git-config.
* Support for $XDG_CONFIG_HOME/git/credentials has been documented in
  git-credentials-store.
The changes for the tests are as follows:
* Instead of repeating ${XDG_CONFIG_HOME:-$HOME/.config} all over
  the place, add tests that test for $HOME/.config/git/credentials (when
  XDG_CONFIG_HOME is unset) and for $XDG_CONFIG_HOME/git/credentials with a
  custom $XDG_CONFIG_HOME directory set. This is to test that the new code
  respects the $XDG_CONFIG_HOME environment variable. (Thanks Junio)
* Use test_path_is_missing to test if files exist. (Thanks Matthieu)
* All code is now within test_expect_success and will now fail if any
  error occurs in the code block through the use of "&&". Furthermore, tests do
  not rely on previous tests passing as the credential files are overwritten in
  each test. (Thanks Matthieu and Junio for your code review)

The most current version of the patch queue is published in the xdg branch at [3]. I try to push -f regularly.

For your feedback, please.
[3] https://github.com/pyokagan/git
Paul Tan (4):
  git-credential-store: support multiple credential files
  git-credential-store: support XDG_CONFIG_HOME
  docs/git-credential-store: document XDG file and precedence
  t0302: test credential-store support for XDG_CONFIG_HOME
 Documentation/git-credential-store.txt | 37 +++++++++++++-
 credential-store.c                     | 86 +++++++++++++++++++++----------
 t/t0302-credential-store.sh            | 92 ++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+), 29 deletions(-)
-- 
2.1.4
Paul Tan· Mar 8, 2015, 07:58 UTC · re: Paul Tan · lore

Previously, git-credential-store only supported storing credentials in a single file: ~/.git-credentials. In order to support the XDG base directory specification[1], git-credential-store needs to be able to lookup and erase credentials from multiple files, as well as to pick the appropriate file to write to so that the credentials can be found on subsequent lookups.

[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html

Note that some credential storage files may not be owned, readable or writable by the user, as they may be system-wide files that are meant to apply to every user.

Instead of a single file path, lookup_credential(), remove_credential() and store_credential() now take a precedence-ordered string_list of file paths. lookup_credential() expects both user-specific and system-wide credential files to be provided to support the use case of system administrators setting default credentials for users. remove_credential() and store_credential() expect only the user-specific credential files to be provided as usually the only config files that users are allowed to edit are their own user-specific ones.

lookup_credential() will read these (user-specific and system-wide) file paths in order until it finds the 1st matching credential and print it. As some files may be private and thus unreadable, any file which cannot be read will be ignored silently.

remove_credential() will erase credentials from all (user-specific) files in the list. This is because if credentials are only erased from the file with the highest precedence, a matching credential may still be found in a file further down the list. (Note that due to the lockfile code, this requires the directory to be writable, which should be so for user-specific config files)

store_credential() will write the credentials to the first existing (user-specific) file in the list. If none of the files in the list exist, store_credential() will write to the filename specified by default_index, thus creating it. For backwards compatibility, ~/.git-credentials should be the file specified by default_index.

Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 credential-store.c | 77 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 52 insertions(+), 25 deletions(-)
Show changes to credential-store.c +52 −25
diff --git a/credential-store.c b/credential-store.c
index 925d3f4..3455d7a 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)
+		if (errno != ENOENT && errno != EACCES)
 			die_errno("unable to open %s", fn);
-		return;
+		return found_credential;
 	}
 
 	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)
@@ -64,21 +67,10 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 		die_errno("unable to commit credential store");
 }
 
-static void store_credential(const char *fn, struct credential *c)
+static void store_credential_file(const char *fn, struct credential *c)
 {
 	struct strbuf buf = STRBUF_INIT;
 
-	/*
-	 * Sanity check that what we are storing is actually sensible.
-	 * In particular, we can't make a URL without a protocol field.
-	 * Without either a host or pathname (depending on the scheme),
-	 * we have no primary key. And without a username and password,
-	 * we are not actually storing a credential.
-	 */
-	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, ':');
@@ -95,8 +87,34 @@ static void store_credential(const char *fn, struct credential *c)
 	strbuf_release(&buf);
 }
 
-static void remove_credential(const char *fn, struct credential *c)
+static void store_credential(const struct string_list *fns, struct credential *c,
+			     unsigned int default_index)
 {
+	struct string_list_item *fn;
+
+	/*
+	 * Sanity check that what we are storing is actually sensible.
+	 * In particular, we can't make a URL without a protocol field.
+	 * Without either a host or pathname (depending on the scheme),
+	 * we have no primary key. And without a username and password,
+	 * we are not actually storing a credential.
+	 */
+	if (!c->protocol || !(c->host || c->path) || !c->username || !c->password)
+		return;
+
+	for_each_string_list_item(fn, fns)
+		if (!access(fn->string, F_OK)) {
+			store_credential_file(fn->string, c);
+			return;
+		}
+	/* Write credential to the filename at default_index, creating it */
+	store_credential_file(fns->items[default_index].string, c);
+}
+
+static void remove_credential(const struct string_list *fns, struct credential *c)
+{
+	struct string_list_item *fn;
+
 	/*
 	 * Sanity check that we actually have something to match
 	 * against. The input we get is a restrictive pattern,
@@ -105,14 +123,20 @@ static void remove_credential(const char *fn, struct credential *c)
 	 * to empty input. So explicitly disallow it, and require that the
 	 * pattern have some actual content to match.
 	 */
-	if (c->protocol || c->host || c->path || c->username)
-		rewrite_credential_file(fn, c, NULL);
+	if (!c->protocol && !c->host && !c->path && !c->username)
+		return;
+	for_each_string_list_item(fn, fns)
+		if (!access(fn->string, F_OK))
+			rewrite_credential_file(fn->string, c, NULL);
 }
 
-static int lookup_credential(const char *fn, struct credential *c)
+static void lookup_credential(const struct string_list *fns, struct credential *c)
 {
-	parse_credential_file(fn, c, print_entry, NULL);
-	return c->username && c->password;
+	struct string_list_item *fn;
+
+	for_each_string_list_item(fn, fns)
+		if (parse_credential_file(fn->string, c, print_entry, NULL))
+			return; /* Found credential */
 }
 
 int main(int argc, char **argv)
@@ -123,6 +147,7 @@ int main(int argc, char **argv)
 	};
 	const char *op;
 	struct credential c = CREDENTIAL_INIT;
+	struct string_list fns = STRING_LIST_INIT_NODUP;
 	char *file = NULL;
 	struct option options[] = {
 		OPT_STRING(0, "file", &file, "path",
@@ -139,18 +164,20 @@ int main(int argc, char **argv)
 
 	if (!file)
 		file = expand_user_path("~/.git-credentials");
-	if (!file)
+	if (file)
+		string_list_append_nodup(&fns, file);
+	else
 		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);
+		lookup_credential(&fns, &c);
 	else if (!strcmp(op, "erase"))
-		remove_credential(file, &c);
+		remove_credential(&fns, &c);
 	else if (!strcmp(op, "store"))
-		store_credential(file, &c);
+		store_credential(&fns, &c, fns.nr - 1);
 	else
 		; /* Ignore unknown operation. */
 
-- 
2.1.4
Paul Tan· Mar 8, 2015, 07:58 UTC · re: Paul Tan · lore

[PATCH v2 2/4] git-credential-store: support XDG_CONFIG_HOME

Add $XDG_CONFIG_HOME/git/credentials to the default credential search path of git-credential-store. This allows git-credential-store to support user-specific configuration files in accordance with the XDG base directory specification[1].

[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html

~/.git-credentials has a higher precedence than $XDG_CONFIG_HOME/git/credentials when looking up credentials. This means that if any duplicate matching credentials are found in the xdg file (due to ~/.git-credentials being updated by old versions of git or outdated tools), they will not be used at all. This is to give the user some leeway in switching to old versions of git while keeping the xdg directory. This is consistent with the behavior of git-config.

However, the higher precedence of ~/.git-credentials means that as long as ~/.git-credentials exist, all credentials will be written to the ~/.git-credentials file even if the user has an xdg file as having a ~/.git-credentials file indicates that the user wants to preserve backwards-compatibility. This is also consistent with the behavior of git-config.

Since the xdg file will not be used unless it actually exists, to prevent the situation where some credentials are present in the xdg file while some are present in the home file, users are recommended to not create the xdg file if they require compatibility with old versions of git or outdated tools. Note, though, that "erase" can be used to explicitly erase matching credentials from all files.

Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 credential-store.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
Show changes to credential-store.c +10 −5
diff --git a/credential-store.c b/credential-store.c
index 3455d7a..7b22a3a 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -162,11 +162,16 @@ int main(int argc, char **argv)
 		usage_with_options(usage, options);
 	op = argv[0];
 
-	if (!file)
-		file = expand_user_path("~/.git-credentials");
-	if (file)
+	if (file) {
 		string_list_append_nodup(&fns, file);
-	else
+	} else {
+		if ((file = expand_user_path("~/.git-credentials")))
+			string_list_append_nodup(&fns, file);
+		home_config_paths(NULL, &file, "credentials");
+		if (file)
+			string_list_append_nodup(&fns, file);
+	}
+	if (!fns.nr)
 		die("unable to set up default path; use --file");
 
 	if (credential_read(&c, stdin) < 0)
@@ -177,7 +182,7 @@ int main(int argc, char **argv)
 	else if (!strcmp(op, "erase"))
 		remove_credential(&fns, &c);
 	else if (!strcmp(op, "store"))
-		store_credential(&fns, &c, fns.nr - 1);
+		store_credential(&fns, &c, 0);
 	else
 		; /* Ignore unknown operation. */
 
-- 
2.1.4
Paul Tan· Mar 10, 2015, 13:43 UTC · re: Paul Tan · lore

Re: [PATCH v2 2/4] git-credential-store: support XDG_CONFIG_HOME

On Sun, Mar 8, 2015 at 3:58 PM, Paul Tan <pyokagan@gmail.com> wrote:
Show 6 quoted lines
>                 remove_credential(&fns, &c);
>         else if (!strcmp(op, "store"))
> -               store_credential(&fns, &c, fns.nr - 1);
> +               store_credential(&fns, &c, 0);
>         else
>                 ; /* Ignore unknown operation. */

Whoops, seems like I squashed some commits in the wrong order (but there is no functional difference). Will re-roll the patch series.

Also, store_credential() should take a string as the default filename instead of an index into the filename string_list for extra API flexibility.

Thanks.
Paul Tan· Mar 8, 2015, 07:58 UTC · re: Paul Tan · lore

[PATCH v2 3/4] docs/git-credential-store: document XDG file and precedence

git-credential-store now supports an additional default credential file at $XDG_CONFIG_HOME/git/credentials. However, ~/.git-credentials takes precedence over it for backwards compatibility. To make the precedence ordering explicit, add a new section FILES that lists out the credential file paths in their order of precedence, and explains how the ordering affects the lookup, storage and erase operations.

Also update documentation for --store to briefly explain the operations on multiple files if the --store option is not provided.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Documentation/git-credential-store.txt | 37 ++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)
Show changes to Documentation/git-credential-store.txt +35 −2
diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index bc97071..451c4fa 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -31,10 +31,43 @@ OPTIONS
 
 --file=<path>::
 
-	Use `<path>` to store credentials. The file will have its
+	Use `<path>` to lookup and store credentials. The file will have its
 	filesystem permissions set to prevent other users on the system
 	from reading it, but will not be encrypted or otherwise
-	protected. Defaults to `~/.git-credentials`.
+	protected. If not specified, credentials will be searched for from
+	`~/.git-credentials` and `$XDG_CONFIG_HOME/git/credentials`, and
+	credentials will be written to `~/.git-credentials` if it exists, or
+	`$XDG_CONFIG_HOME/git/credentials` if it exists and the former does
+	not. See also <<FILES>>.
+
+[[FILES]]
+FILES
+-----
+
+If not set explicitly with '--file', there are two files where
+git-credential-store will search for credentials in order of precedence:
+
+~/.git-credentials::
+	User-specific credentials file.
+
+$XDG_CONFIG_HOME/git/credentials::
+	Second user-specific credentials file. If '$XDG_CONFIG_HOME' is not set
+	or empty, `$HOME/.config/git/credentials` will be used. Any credentials
+	stored in this file will not be used if `~/.git-credentials` has a
+	matching credential as well. It is a good idea not to create this file
+	if you sometimes use older versions of Git, as support for this file
+	was added fairly recently.
+
+
+For credential lookups, the files are read in the order given above, with the
+first matching credential found taking precedence over credentials found in
+files further down the list.
+
+Credential storage will per default write to the first existing file in the
+list. If none of these files exist, `~/.git-credentials` will be created and
+written to.
+
+When erasing credentials, matching credentials will be erased from all files.
 
 EXAMPLES
 --------
-- 
2.1.4
Paul Tan· Mar 8, 2015, 07:58 UTC · re: Paul Tan · lore

[PATCH v2 4/4] t0302: test credential-store support for XDG_CONFIG_HOME

t0302 now tests git-credential-store's support for the XDG user-specific configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:

* Ensure that the XDG file is strictly opt-in. It should not be created
  by git at all times if it does not exist.
* On the flip side, if the XDG file exists, ~/.git-credentials should
  not be created at all times.
* If both the XDG file and ~/.git-credentials exists, then both files
  should be used for credential lookups. However, credentials should
  only be written to ~/.git-credentials.
* Credentials must be erased from both files.
* $XDG_CONFIG_HOME can be a custom directory set by the user as per the
  XDG base directory specification. Test that git-credential-store
  respects that, but defaults to "~/.config/git/credentials" if it does
  not exist or is empty.
Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t0302-credential-store.sh | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)
Show changes to t/t0302-credential-store.sh +92 −0
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index f61b40c..7fe832d 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -5,5 +5,97 @@ test_description='credential-store tests'
 . "$TEST_DIRECTORY"/lib-credential.sh
 
 helper_test store
+test_expect_success '~/.git-credentials is written to when xdg credentials file does not exist' '
+	test -s "$HOME/.git-credentials"
+'
+test_expect_success 'xdg credentials file will not be created if it does not exist' '
+	test_path_is_missing "$HOME/.config/git/credentials"
+'
+test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '
+	test_might_fail rm "$HOME/.git-credentials" &&
+	mkdir -p "$HOME/.config/git" &&
+	>"$HOME/.config/git/credentials"
+'
+helper_test store
+test_expect_success 'xdg credentials file will be written to if it exists' '
+	test -s "$HOME/.config/git/credentials"
+'
+test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '
+	test_path_is_missing "$HOME/.git-credentials"
+'
+test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '
+	XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME &&
+	mkdir -p "$HOME/xdg/git" &&
+	>"$HOME/xdg/git/credentials" &&
+	>"$HOME/.config/git/credentials"
+'
+helper_test store
+test_expect_success 'custom XDG_CONFIG_HOME credentials file will be written to if it exists' '
+	unset XDG_CONFIG_HOME &&
+	test -s "$HOME/xdg/git/credentials"
+'
+test_expect_success '~/.config/git/credentials file will not be written to if a custom XDG_CONFIG_HOME is set' '
+	test_must_be_empty "$HOME/.config/git/credentials"
+'
+test_expect_success '~/.git-credentials will not be created if xdg credentials file exists' '
+	test_path_is_missing "$HOME/.git-credentials"
+'
+test_expect_success 'get: return credentials from home file if matches are found in both home and xdg files' '
+	mkdir -p "$HOME/.config/git" &&
+	echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=home-user
+	password=home-pass
+	--
+	EOF
+'
+test_expect_success 'get: return credentials from xdg file if the home files do not have them' '
+	mkdir -p "$HOME/.config/git" &&
+	>"$HOME/.git-credentials" &&
+	echo "https://home-user:home-pass@example.com" >"$HOME/.config/git/credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=home-user
+	password=home-pass
+	--
+	EOF
+'
+test_expect_success 'get: return credentials from home file if xdg files are unreadable' '
+	mkdir -p "$HOME/.config/git" &&
+	echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
+	chmod -r "$HOME/.config/git/credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=home-user
+	password=home-pass
+	--
+	EOF
+'
+test_expect_success 'erase: erase matching credentials from both xdg and home files' '
+	mkdir -p "$HOME/.config/git" &&
+	echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
+	check reject store <<-\EOF
+	protocol=https
+	host=example.com
+	EOF
+	test_must_be_empty "$HOME/.config/git/credentials" &&
+	test_must_be_empty "$HOME/.git-credentials"
+'
 
 test_done
-- 
2.1.4
Matthieu Moy· Mar 9, 2015, 12:36 UTC · re: Paul Tan · lore

Re: [PATCH v2 4/4] t0302: test credential-store support for XDG_CONFIG_HOME

Paul Tan <pyokagan@gmail.com> writes:
Show 12 quoted lines
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index f61b40c..7fe832d 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -5,5 +5,97 @@ test_description='credential-store tests'
>  . "$TEST_DIRECTORY"/lib-credential.sh
>  
>  helper_test store
> +test_expect_success '~/.git-credentials is written to when xdg credentials file does not exist' '
> +	test -s "$HOME/.git-credentials"
> +'
> +test_expect_success 'xdg credentials file will not be created if it does not exist' '
We usually put a blank line between tests.
> +helper_test store

That seems a bit "brute force" to me to re-run all tests several times. It's probably OK since the tests are quite fast though.

> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '
> +	XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME &&
> +	mkdir -p "$HOME/xdg/git" &&

I'd spell that "$XDG_CONFIG_HOME"/git instead, but that's not really important.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Paul Tan· Mar 10, 2015, 13:55 UTC · re: Matthieu Moy · lore

Re: [PATCH v2 4/4] t0302: test credential-store support for XDG_CONFIG_HOME

On Mon, Mar 9, 2015 at 8:36 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 5 quoted lines
> Paul Tan <pyokagan@gmail.com> writes:
>> +'
>> +test_expect_success 'xdg credentials file will not be created if it does not exist' '
>
> We usually put a blank line between tests.
Okay, will do that.
>> +helper_test store
>
> That seems a bit "brute force" to me to re-run all tests several times.
> It's probably OK since the tests are quite fast though.

The credential helper tests are re-run because I believe that tests should make no assumptions on the underlying implementation. e.g. the presence of the xdg file only may activate a code path that causes credential storage to succeed but retrieval to fail.

Show 6 quoted lines
>> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '
>> +     XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME &&
>> +     mkdir -p "$HOME/xdg/git" &&
>
> I'd spell that "$XDG_CONFIG_HOME"/git instead, but that's not really
> important.
Okay, will do that.
Thanks for the review.

← back to recent threads