threads / patch / 38839

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

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

## tl;dr

15 messages between Mar 18, 2015 and Mar 23, 2015. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Paul Tan· Mar 18, 2015, 07:04 UTC · 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 the first item of the filename list. For backwards compatibility, this filename should be "~/.git-credentials".

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>
---
The previous version can be found at [1].
[1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265309
The changes are as follows:
* Remove default_fn argument from store_credential(). It now uses the
first item of the filenames string_list. Thanks Jeff, Matthieu and
Junio for your input in this discussion.
* Change filename string_list fns to duplicate strings by default
(STRING_LIST_INIT_DUP). This is to make memory ownership explicit --
when the string_list receives the file paths from xdg_config_home()
and expand_user_path(), it is responsible for freeing those strings.
Thanks Jeff for pointing this out.
 credential-store.c | 79 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 25 deletions(-)
Show changes to credential-store.c +54 −25
diff --git a/credential-store.c b/credential-store.c
index 925d3f4..8dad479 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,35 @@ 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)
 {
+	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 specified by fns->items[0], thus
+	 * creating it */
+	if (fns->nr)
+		store_credential_file(fns->items[0].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 +124,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 +148,7 @@ int main(int argc, char **argv)
 	};
 	const char *op;
 	struct credential c = CREDENTIAL_INIT;
+	struct string_list fns = STRING_LIST_INIT_DUP;
 	char *file = NULL;
 	struct option options[] = {
 		OPT_STRING(0, "file", &file, "path",
@@ -139,20 +165,23 @@ int main(int argc, char **argv)
 
 	if (!file)
 		file = expand_user_path("~/.git-credentials");
-	if (!file)
+	if (file)
+		string_list_append(&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);
 	else
 		; /* Ignore unknown operation. */
 
+	string_list_clear(&fns, 0);
 	return 0;
 }
-- 
2.1.4
Paul Tan· Mar 18, 2015, 07:04 UTC · re: Paul Tan · lore

[PATCH v4 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>
---
The previous version can be found at [1].
[1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308
There are no changes in this version.
 credential-store.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
Show changes to credential-store.c +9 −4
diff --git a/credential-store.c b/credential-store.c
index 8dad479..d805df7 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -163,11 +163,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(&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)
-- 
2.1.4
Paul Tan· Mar 18, 2015, 07:04 UTC · re: Paul Tan · lore

[PATCH v4 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.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
The previous version can be found at [1].
[1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308
The changes are as follows:
* Remove "support for this file was added fairly recently" statement, as
it will become out-dated with time. Thanks Eric for pointing this out.
 Documentation/git-credential-store.txt | 35 ++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)
Show changes to Documentation/git-credential-store.txt +33 −2
diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index bc97071..e16afd8 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -31,10 +31,41 @@ 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 that do not support it.
+
+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
Matthieu Moy· Mar 18, 2015, 16:23 UTC · re: Paul Tan · lore

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

I would personnally prefer to see this squashed with PATCH 2/4: pushing the "bisectable history" principle a bit, the state between patches 2 and 3 could be considered broken because the code does not do what the documentation says. And as a reviewer, I like having pieces of docs linked to the patch they document.

Paul Tan <pyokagan@gmail.com> writes:
> +Credential storage will per default

Not a native, but "per default" sounds weird and "by default" seems far more common.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Paul Tan· Mar 21, 2015, 05:59 UTC · re: Matthieu Moy · lore

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

Hi,

On Thu, Mar 19, 2015 at 12:23 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 5 quoted lines
> I would personnally prefer to see this squashed with PATCH 2/4: pushing
> the "bisectable history" principle a bit, the state between patches 2
> and 3 could be considered broken because the code does not do what the
> documentation says. And as a reviewer, I like having pieces of docs
> linked to the patch they document.
Yup, I can see what you mean. Will squash on the next version.
Show 6 quoted lines
> Paul Tan <pyokagan@gmail.com> writes:
>
>> +Credential storage will per default
>
> Not a native, but "per default" sounds weird and "by default" seems far
> more common.
Ah right, that definitely sounds better. Thanks.
Paul Tan· Mar 18, 2015, 07:04 UTC · re: Paul Tan · lore

[PATCH v4 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.
* Conversely, 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>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
The previous version can be found at [1].
[1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308
* Merge related, but previously separate, tests together in order to
  make the test suite easier to understand.
* Instead of setting/unsetting XDG_CONFIG_HOME in separate tests, set
  it, and unset it immediately before and after "helper_test store" is
  called in order to make it localized to only the command that it
  should affect.
* Add test, previously missing, to check that only the home credentials
  file is written to if both the xdg and home files exist.
* Correct mislabelling of "home-user"/"home-pass" to the proper
  "xdg-user"/"xdg-pass".
* Use "rm -f" instead of "test_might_fail rm".
Thanks Eric for the code review.
 t/t0302-credential-store.sh | 111 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)
Show changes to t/t0302-credential-store.sh +111 −0
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index f61b40c..5b0a666 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -6,4 +6,115 @@ test_description='credential-store tests'
 
 helper_test store
 
+test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '
+	test -s "$HOME/.git-credentials" &&
+	test_path_is_missing "$HOME/.config/git/credentials"
+'
+
+test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '
+	rm -f "$HOME/.git-credentials" &&
+	mkdir -p "$HOME/.config/git" &&
+	>"$HOME/.config/git/credentials"
+'
+
+helper_test store
+
+test_expect_success 'when xdg credentials file exists, only write to xdg file; do not create ~/.git-credentials' '
+	test -s "$HOME/.config/git/credentials" &&
+	test_path_is_missing "$HOME/.git-credentials"
+'
+
+test_expect_success 'set up custom XDG_CONFIG_HOME credential file at ~/xdg/git/credentials' '
+	mkdir -p "$HOME/xdg/git" &&
+	rm -f "$HOME/.config/git/credentials" &&
+	>"$HOME/xdg/git/credentials"
+'
+
+XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME && helper_test store
+unset XDG_CONFIG_HOME
+
+test_expect_success 'if custom XDG_CONFIG_HOME credentials file ~/xdg/git/credentials exists, it will only be written to; ~/.config/git/credentials and ~/.git-credentials will not be created' '
+	test_when_finished "rm -f $HOME/xdg/git/credentials" &&
+	test -s "$HOME/xdg/git/credentials" &&
+	test_path_is_missing "$HOME/.config/git/credentials"
+	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://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=xdg-user
+	password=xdg-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 'store: If both xdg and home files exist, only store in home file' '
+	mkdir -p "$HOME/.config/git" &&
+	>"$HOME/.config/git/credentials" &&
+	>"$HOME/.git-credentials" &&
+	check approve store <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	echo "https://store-user:store-pass@example.com" >expected &&
+	test_cmp expected "$HOME/.git-credentials" &&
+	test_must_be_empty "$HOME/.config/git/credentials"
+'
+
+
+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 18, 2015, 16:41 UTC · re: Paul Tan · lore

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

Similarly to the "merge doc and code", I personally prefer seeing code and tests in the same patch.

Actually, my preference goes for a first patch that introduces the tests with test_expect_failure for things that are not yet implemented (and you can check that tests do not pass yet before you code), and then the patch introducing the feature doing

-test_expect_failure +test_expect_success

which documents quite clearly and concisely that you just made the feature work.

Paul Tan <pyokagan@gmail.com> writes:
Show 9 quoted lines
> +test_expect_success 'if custom XDG_CONFIG_HOME credentials file
> + ~/xdg/git/credentials exists, it will only be written to;
> + ~/.config/git/credentials and ~/.git-credentials will not be
> + created' '
> +	test_when_finished "rm -f $HOME/xdg/git/credentials" &&
> +	test -s "$HOME/xdg/git/credentials" &&
> +	test_path_is_missing "$HOME/.config/git/credentials"
> +	test_path_is_missing "$HOME/.git-credentials"
> +'
Broken &&-chain. That is a real issue that must be fixed.
> +test_expect_success 'get: return credentials from home file if xdg files are unreadable' '
"files are" -> "file is", no?
> +'
> +
> +
> +test_expect_success 'erase: erase matching credentials from both xdg and home files' '
Double blank line.

On overall, except the broken &&-chain above, the whole series looks good. Feel free to ignore my other nitpicks if you prefer.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Eric Sunshine· Mar 18, 2015, 21:15 UTC · re: Matthieu Moy · lore

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

On Wed, Mar 18, 2015 at 12:41 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

> Similarly to the "merge doc and code", I personally prefer seeing code
> and tests in the same patch.

In this case, the patch introducing the tests is already quite long and intricate, almost to the point of being a burden to review. Combining the patches would likely push it over the edge. I'd elect to keep them separate.

Show 10 quoted lines
> Actually, my preference goes for a first patch that introduces the tests
> with test_expect_failure for things that are not yet implemented (and
> you can check that tests do not pass yet before you code), and then the
> patch introducing the feature doing
>
> -test_expect_failure
> +test_expect_success
>
> which documents quite clearly and concisely that you just made the
> feature work.

I also tend to favor adding "failure" tests which are flipped to "success" when appropriate, however, as this is an entirely new feature, this approach may be unsuitable (and perhaps overkill). Generally speaking, these new tests don't really make sense without the feature; and, more specifically, due to their nature, several of them will pass even without the feature implemented. As such, the current patch organization seems reasonable.

Matthieu Moy· Mar 19, 2015, 13:35 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
> I also tend to favor adding "failure" tests which are flipped to
> "success" when appropriate, however, as this is an entirely new
> feature, this approach may be unsuitable (and perhaps overkill).

I can buy the "overkill", but not unsuitable. Even for new features, the tests should fail before and pass after. Otherwise, the tests are not testing the feature. Actually, this is a strong principle in test-driven development: don't write code unless you have a failing test.

But I was just thinking out loudly, certainly not requesting a change.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Paul Tan· Mar 21, 2015, 10:01 UTC · re: Matthieu Moy · lore

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

Hi,

On Thu, Mar 19, 2015 at 9:35 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 12 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> I also tend to favor adding "failure" tests which are flipped to
>> "success" when appropriate, however, as this is an entirely new
>> feature, this approach may be unsuitable (and perhaps overkill).
>
> I can buy the "overkill", but not unsuitable. Even for new features, the
> tests should fail before and pass after. Otherwise, the tests are not
> testing the feature. Actually, this is a strong principle in test-driven
> development: don't write code unless you have a failing test.
>
> But I was just thinking out loudly, certainly not requesting a change.

I also think that the tests belong in their own patch as the patch is really long compared to the rest of the patch series and it makes it easier for me to respond to comments as well.

Putting the tests before the implementation, though, makes sense in this case as it is just an implementation of a well-defined set of requirements. Some tests will still pass with or without the feature, but that is the requirement of the feature -- which is to not be enabled until the user explicitly creates the xdg file. Will re-order the patch series in the next version.

Thanks.

Regards, Paul

Eric Sunshine· Mar 18, 2015, 19:26 UTC · re: Paul Tan · lore

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

On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 23 quoted lines
> t0302 now tests git-credential-store's support for the XDG user-specific
> configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:
> ---
>
> The previous version can be found at [1].
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308
>
> * Merge related, but previously separate, tests together in order to
>   make the test suite easier to understand.
>
> * Instead of setting/unsetting XDG_CONFIG_HOME in separate tests, set
>   it, and unset it immediately before and after "helper_test store" is
>   called in order to make it localized to only the command that it
>   should affect.
>
> * Add test, previously missing, to check that only the home credentials
>   file is written to if both the xdg and home files exist.
>
> * Correct mislabelling of "home-user"/"home-pass" to the proper
>   "xdg-user"/"xdg-pass".
>
> * Use "rm -f" instead of "test_might_fail rm".
This round looks much better. Thanks.

Most of the comments below are just nit-picky, with one or two genuine (minor) issues.

Show 11 quoted lines
> Thanks Eric for the code review.
>
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index f61b40c..5b0a666 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -6,4 +6,115 @@ test_description='credential-store tests'
>
>  helper_test store
>
> +test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '

These test descriptions are quite long, often mirroring in prose what the test body already says more concisely. I generally try to keep descriptions short while still being descriptive enough to give a general idea about what is being tested. I've rewritten a few test descriptions (below) to be very short, in fact probably too terse; but perhaps better descriptions would lie somewhere between the two extremes. First example, for this test:

    "!xdg: >.git-credentials !xdg"
Key: Space-separated items. Items before ":" are the pre-conditions,
and items after are the post-state. "!" file does not exist; ">"
output goes here.
Show 5 quoted lines
> +       test -s "$HOME/.git-credentials" &&
> +       test_path_is_missing "$HOME/.config/git/credentials"
> +'
> +
> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '
It's customary to call this "setup" rather than "create".
Terse version: "setup: -.git-redentials +xdg"
Key: "-" file removed; "+" file created.
Show 8 quoted lines
> +       rm -f "$HOME/.git-credentials" &&
> +       mkdir -p "$HOME/.config/git" &&
> +       >"$HOME/.config/git/credentials"
> +'
> +
> +helper_test store
> +
> +test_expect_success 'when xdg credentials file exists, only write to xdg file; do not create ~/.git-credentials' '
Terse version: "!.git-credentials xdg: !.git-credentials >xdg"
Show 5 quoted lines
> +       test -s "$HOME/.config/git/credentials" &&
> +       test_path_is_missing "$HOME/.git-credentials"
> +'
> +
> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file at ~/xdg/git/credentials' '
s/set up/setup/
Terse: "setup custom-xdg"
> +       mkdir -p "$HOME/xdg/git" &&
> +       rm -f "$HOME/.config/git/credentials" &&
> +       >"$HOME/xdg/git/credentials"

It would be easier to read this if you placed the two lines together which refer to the custom xdg file. Also, for completeness and to be self-contained, don't you want to remove ~/.git-credentials?

    rm -f "$HOME/.git-credentials" &&
    rm -f "$HOME/.config/git/credentials" &&
    mkdir -p "$HOME/xdg/git" &&
    >"$HOME/xdg/git/credentials"
> +'
> +
> +XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME && helper_test store
> +unset XDG_CONFIG_HOME

It's hard to spot the "helper_test store" at the end of line. I'd place it on a line by itself so that it is easy to see that it is wrapped by the setting and unsetting of the environment variable.

> +test_expect_success 'if custom XDG_CONFIG_HOME credentials file ~/xdg/git/credentials exists, it will only be written to; ~/.config/git/credentials and ~/.git-credentials will not be created' '
Terse: "!.git-credentials !xdg custom-xdg: !.git-credentials !xdg >custom-xdg"
> +       test_when_finished "rm -f $HOME/xdg/git/credentials" &&
> +       test -s "$HOME/xdg/git/credentials" &&
> +       test_path_is_missing "$HOME/.config/git/credentials"
Matthieu already pointed out the broken &&-chain.
> +       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' '
Terse: ".git-credentials xdg: <.git-credentials"
Key: "<" taken from here.
Show 16 quoted lines
> +       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' '
Terse: "!.git-credentials xdg: <xdg"
Show 16 quoted lines
> +       mkdir -p "$HOME/.config/git" &&
> +       >"$HOME/.git-credentials" &&
> +       echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
> +       check fill store <<-\EOF
> +       protocol=https
> +       host=example.com
> +       --
> +       protocol=https
> +       host=example.com
> +       username=xdg-user
> +       password=xdg-pass
> +       --
> +       EOF
> +'
> +
> +test_expect_success 'get: return credentials from home file if xdg files are unreadable' '

An earlier test showed that the home file is preferred if both it and the xdg file exist, so is this test actually telling us anything new? Did you mean instead to reverse the case and make the home file 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" &&

It would be a bit easier to see that the 'chmod' applies to the xdg file if it directly followed creation of the xdg file.

Show 13 quoted lines
> +       check fill store <<-\EOF
> +       protocol=https
> +       host=example.com
> +       --
> +       protocol=https
> +       host=example.com
> +       username=home-user
> +       password=home-pass
> +       --
> +       EOF
> +'
> +
> +test_expect_success 'store: If both xdg and home files exist, only store in home file' '
Inconsistent capitalization: s/If/if/
Show 30 quoted lines
> +       mkdir -p "$HOME/.config/git" &&
> +       >"$HOME/.config/git/credentials" &&
> +       >"$HOME/.git-credentials" &&
> +       check approve store <<-\EOF &&
> +       protocol=https
> +       host=example.com
> +       username=store-user
> +       password=store-pass
> +       EOF
> +       echo "https://store-user:store-pass@example.com" >expected &&
> +       test_cmp expected "$HOME/.git-credentials" &&
> +       test_must_be_empty "$HOME/.config/git/credentials"
> +'
> +
> +
> +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
Eric Sunshine· Mar 19, 2015, 21:53 UTC · re: Eric Sunshine · lore

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

On Wed, Mar 18, 2015 at 3:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 29 quoted lines
> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> t0302 now tests git-credential-store's support for the XDG user-specific
>> configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:
>> ---
>>
>> The previous version can be found at [1].
>>
>> [1] http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308
>>
>> * Merge related, but previously separate, tests together in order to
>>   make the test suite easier to understand.
>>
>> * Instead of setting/unsetting XDG_CONFIG_HOME in separate tests, set
>>   it, and unset it immediately before and after "helper_test store" is
>>   called in order to make it localized to only the command that it
>>   should affect.
>>
>> * Add test, previously missing, to check that only the home credentials
>>   file is written to if both the xdg and home files exist.
>>
>> * Correct mislabelling of "home-user"/"home-pass" to the proper
>>   "xdg-user"/"xdg-pass".
>>
>> * Use "rm -f" instead of "test_might_fail rm".
>
> This round looks much better. Thanks.
>
> Most of the comments below are just nit-picky, with one or two genuine
> (minor) issues.

I should add that the nit-picky items are not necessarily actionable. As the person doing the actual work, it's okay if you disagree and feel that they are not worth the effort of addressing.

(The genuine issues, on the other hand, ought to be addressed.)
Paul Tan· Mar 21, 2015, 05:46 UTC · re: Eric Sunshine · lore

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

Hi,
On Thu, Mar 19, 2015 at 3:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 24 quoted lines
> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
>> index f61b40c..5b0a666 100755
>> --- a/t/t0302-credential-store.sh
>> +++ b/t/t0302-credential-store.sh
>> @@ -6,4 +6,115 @@ test_description='credential-store tests'
>>
>>  helper_test store
>>
>> +test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '
>
> These test descriptions are quite long, often mirroring in prose what
> the test body already says more concisely. I generally try to keep
> descriptions short while still being descriptive enough to give a
> general idea about what is being tested. I've rewritten a few test
> descriptions (below) to be very short, in fact probably too terse; but
> perhaps better descriptions would lie somewhere between the two
> extremes. First example, for this test:
>
>     "!xdg: >.git-credentials !xdg"
>
> Key: Space-separated items. Items before ":" are the pre-conditions,
> and items after are the post-state. "!" file does not exist; ">"
> output goes here.

I will make the test descriptions shorter. However, I don't think the test descriptions need to be so terse such that a separate key is required. e.g. I will shorten the above to "when xdg file does not exist, it's not created.", or even terser: "when xdg file not exists, it's not created.". I don't think symbols should be used, as many other test descriptions do not use them.

Show 7 quoted lines
>> +       test -s "$HOME/.git-credentials" &&
>> +       test_path_is_missing "$HOME/.config/git/credentials"
>> +'
>> +
>> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '
>
> It's customary to call this "setup" rather than "create".
Will fix.
> Terse version: "setup: -.git-redentials +xdg"
>
> Key: "-" file removed; "+" file created.

How about just "setup xdg file" (the fact that ~/.git-credentials is not created/deleted is implied in the next test)

Show 10 quoted lines
>> +       rm -f "$HOME/.git-credentials" &&
>> +       mkdir -p "$HOME/.config/git" &&
>> +       >"$HOME/.config/git/credentials"
>> +'
>> +
>> +helper_test store
>> +
>> +test_expect_success 'when xdg credentials file exists, only write to xdg file; do not create ~/.git-credentials' '
>
> Terse version: "!.git-credentials xdg: !.git-credentials >xdg"
How about "when xdg file exists, home file not created"
Show 7 quoted lines
>> +       test -s "$HOME/.config/git/credentials" &&
>> +       test_path_is_missing "$HOME/.git-credentials"
>> +'
>> +
>> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file at ~/xdg/git/credentials' '
>
> s/set up/setup/
Will fix. Thanks.
> Terse: "setup custom-xdg"

It's a matter of taste, but I personally don't see the need for hyphenation. How about: "setup custom xdg file"

Show 9 quoted lines
>
>> +       mkdir -p "$HOME/xdg/git" &&
>> +       rm -f "$HOME/.config/git/credentials" &&
>> +       >"$HOME/xdg/git/credentials"
>
> It would be easier to read this if you placed the two lines together
> which refer to the custom xdg file.
> Also, for completeness and to be
> self-contained, don't you want to remove ~/.git-credentials?
Ah yes, thanks for the suggestion.
Show 13 quoted lines
>     rm -f "$HOME/.git-credentials" &&
>     rm -f "$HOME/.config/git/credentials" &&
>     mkdir -p "$HOME/xdg/git" &&
>     >"$HOME/xdg/git/credentials"
>
>> +'
>> +
>> +XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME && helper_test store
>> +unset XDG_CONFIG_HOME
>
> It's hard to spot the "helper_test store" at the end of line. I'd
> place it on a line by itself so that it is easy to see that it is
> wrapped by the setting and unsetting of the environment variable.

Thanks, will fix. Although now it looks weird that the "export" is the only one with a continuation on a single line, so I split all of them so that they each have their own line.

Show 9 quoted lines
>> +test_expect_success 'if custom XDG_CONFIG_HOME credentials file ~/xdg/git/credentials exists, it will only be written to; ~/.config/git/credentials and ~/.git-credentials will not be created' '
>
> Terse: "!.git-credentials !xdg custom-xdg: !.git-credentials !xdg >custom-xdg"
>
>> +       test_when_finished "rm -f $HOME/xdg/git/credentials" &&
>> +       test -s "$HOME/xdg/git/credentials" &&
>> +       test_path_is_missing "$HOME/.config/git/credentials"
>
> Matthieu already pointed out the broken &&-chain.
Will fix. Thanks for catching it.
Show 9 quoted lines
>
>> +       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' '
>
> Terse: ".git-credentials xdg: <.git-credentials"
>
> Key: "<" taken from here.

How about "use home file if both home and xdg files have matches". I wish to make it explicit that which files are used depends on whether they have the matching credential, not if they exist.

Show 19 quoted lines
>
>> +       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' '
>
> Terse: "!.git-credentials xdg: <xdg"
How about "use xdg file if home file has no matches".
Show 22 quoted lines
>
>> +       mkdir -p "$HOME/.config/git" &&
>> +       >"$HOME/.git-credentials" &&
>> +       echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
>> +       check fill store <<-\EOF
>> +       protocol=https
>> +       host=example.com
>> +       --
>> +       protocol=https
>> +       host=example.com
>> +       username=xdg-user
>> +       password=xdg-pass
>> +       --
>> +       EOF
>> +'
>> +
>> +test_expect_success 'get: return credentials from home file if xdg files are unreadable' '
>
> An earlier test showed that the home file is preferred if both it and
> the xdg file exist, so is this test actually telling us anything new?
> Did you mean instead to reverse the case and make the home file
> unreadable?

Ah yes, this is embarrassing. Apparently this test was written with the old precedence ordering. Thank you so much for catching this fatal error.

Show 7 quoted lines
>> +       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" &&
>
> It would be a bit easier to see that the 'chmod' applies to the xdg
> file if it directly followed creation of the xdg file.
Will fix (will chmod the home file instead)
Show 16 quoted lines
>
>> +       check fill store <<-\EOF
>> +       protocol=https
>> +       host=example.com
>> +       --
>> +       protocol=https
>> +       host=example.com
>> +       username=home-user
>> +       password=home-pass
>> +       --
>> +       EOF
>> +'
>> +
>> +test_expect_success 'store: If both xdg and home files exist, only store in home file' '
>
> Inconsistent capitalization: s/If/if/
Will fix.
Show 29 quoted lines
>
>> +       mkdir -p "$HOME/.config/git" &&
>> +       >"$HOME/.config/git/credentials" &&
>> +       >"$HOME/.git-credentials" &&
>> +       check approve store <<-\EOF &&
>> +       protocol=https
>> +       host=example.com
>> +       username=store-user
>> +       password=store-pass
>> +       EOF
>> +       echo "https://store-user:store-pass@example.com" >expected &&
>> +       test_cmp expected "$HOME/.git-credentials" &&
>> +       test_must_be_empty "$HOME/.config/git/credentials"
>> +'
>> +
>> +
>> +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
Thanks so much Eric and Matthieu for the review.
Eric Sunshine· Mar 23, 2015, 01:56 UTC · re: Paul Tan · lore

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

On Sat, Mar 21, 2015 at 1:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 25 quoted lines
> On Thu, Mar 19, 2015 at 3:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
>>> index f61b40c..5b0a666 100755
>>> --- a/t/t0302-credential-store.sh
>>> +++ b/t/t0302-credential-store.sh
>>> @@ -6,4 +6,115 @@ test_description='credential-store tests'
>>> +test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '
>>
>> These test descriptions are quite long, often mirroring in prose what
>> the test body already says more concisely. I generally try to keep
>> descriptions short while still being descriptive enough to give a
>> general idea about what is being tested. I've rewritten a few test
>> descriptions (below) to be very short, in fact probably too terse; but
>> perhaps better descriptions would lie somewhere between the two
>> extremes. First example, for this test:
>>
>>     "!xdg: >.git-credentials !xdg"
>>
> I will make the test descriptions shorter. However, I don't think the
> test descriptions need to be so terse such that a separate key is
> required. e.g. I will shorten the above to "when xdg file does not
> exist, it's not created.", or even terser: "when xdg file not exists,
> it's not created.". I don't think symbols should be used, as many
> other test descriptions do not use them.
Your updated test descriptions all sound fine.
Show 10 quoted lines
>>> +XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME && helper_test store
>>> +unset XDG_CONFIG_HOME
>>
>> It's hard to spot the "helper_test store" at the end of line. I'd
>> place it on a line by itself so that it is easy to see that it is
>> wrapped by the setting and unsetting of the environment variable.
>
> Thanks, will fix. Although now it looks weird that the "export" is the
> only one with a continuation on a single line, so I split all of them
> so that they each have their own line.

An &&-chain outside of a test is not meaningful in this case, so I meant either this:

    XDG_CONFIG_HOME="$HOME/xdg"
    export XDG_CONFIG_HOME
    helper_test store
    unset XDG_CONFIG_HOME

or, slightly more compact (using && just to combine the assignment and export on one line):

    XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME
    helper_test store
    unset XDG_CONFIG_HOME
Matthieu Moy· Mar 18, 2015, 16:17 UTC · re: Paul Tan · lore

Re: [PATCH v4 1/4] git-credential-store: support multiple credential files

Paul Tan <pyokagan@gmail.com> writes:
> +	/* Write credential to the filename specified by fns->items[0], thus
> +	 * creating it */
Just to show I'm following: misformatted multi-line comment.
Other than that, good job!
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

← back to recent threads