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

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

[Paul Tan]

The previous patch series can be found at [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/265042

The changes for "git-credential-store: support multiple credential files" are
as follows:

* store_credential(), instead of taking an index to the string_list for the
default filename, takes a filename string instead as it leads to a more
flexible API.

The changes for "t0302: test credential-store support for XDG_CONFIG_HOME" are
as follows:

* Corrected code style violations: All tests are now separated by newlines.

* After $XDG_CONFIG_HOME is set to "$HOME/xdg", use $XDG_CONFIG_HOME directly
for all paths instead of "$HOME/xdg".

Thanks Matthieu for the code review.

The diff between the previous patch series is as follows:

diff --git a/credential-store.c b/credential-store.c
index 7b22a3a..b00f80f 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -88,7 +88,7 @@ static void store_credential_file(const char *fn, struct credential *c)
 }
 
 static void store_credential(const struct string_list *fns, struct credential *c,
-			     unsigned int default_index)
+			     const char *default_fn)
 {
 	struct string_list_item *fn;
 
@@ -107,8 +107,9 @@ static void store_credential(const struct string_list *fns, struct credential *c
 			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);
+	/* Write credential to default_fn, thus creating it */
+	if (default_fn)
+		store_credential_file(default_fn, c);
 }
 
 static void remove_credential(const struct string_list *fns, struct credential *c)
@@ -182,7 +183,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, 0);
+		store_credential(&fns, &c, fns.items[0].string);
 	else
 		; /* Ignore unknown operation. */
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 7fe832d..34752ea 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -5,41 +5,53 @@ 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" &&
+	mkdir -p "$XDG_CONFIG_HOME/git" &&
+	>"$XDG_CONFIG_HOME/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_when_finished "unset XDG_CONFIG_HOME" &&
+	test -s "$XDG_CONFIG_HOME/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" &&
@@ -55,6 +67,7 @@ test_expect_success 'get: return credentials from home file if matches are found
 	--
 	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" &&
@@ -70,6 +83,7 @@ test_expect_success 'get: return credentials from xdg file if the home files do
 	--
 	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" &&
@@ -86,6 +100,7 @@ test_expect_success 'get: return credentials from home file if xdg files are unr
 	--
 	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" &&

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                     |  87 ++++++++++++++++++---------
 t/t0302-credential-store.sh            | 107 +++++++++++++++++++++++++++++++++
 3 files changed, 202 insertions(+), 29 deletions(-)

-- 
2.1.4

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

[Paul Tan]

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_fn, thus creating it. For backwards compatibility, default_fn
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>
---

Previous version can be found at [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/265042/focus=265038

The changes are as follows:

* store_credential(), instead of taking an index to the string_list for the
default filename, takes a filename string instead as it leads to a more
flexible API.

 credential-store.c | 78 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 25 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index 925d3f4..803bed2 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,
+			     const char *default_fn)
 {
+	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 default_fn, thus creating it */
+	if (default_fn)
+		store_credential_file(default_fn, 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_NODUP;
 	char *file = NULL;
 	struct option options[] = {
 		OPT_STRING(0, "file", &file, "path",
@@ -139,18 +165,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.items[0].string);
 	else
 		; /* Ignore unknown operation. */
 
-- 
2.1.4

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

[Paul Tan]

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 | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index 803bed2..b00f80f 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_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)
-- 
2.1.4

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

[Paul Tan]

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

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

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

[Paul Tan]

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

The previous version can be found at [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/265042/focus=265041

The changes are as follows:

* Corrected code style violations: All tests are now separated by newlines.

* After $XDG_CONFIG_HOME is set to "$HOME/xdg", use $XDG_CONFIG_HOME directly
for all paths instead of "$HOME/xdg".

Thanks Matthieu for the code review.

 t/t0302-credential-store.sh | 107 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index f61b40c..34752ea 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -6,4 +6,111 @@ test_description='credential-store tests'
 
 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 "$XDG_CONFIG_HOME/git" &&
+	>"$XDG_CONFIG_HOME/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' '
+	test_when_finished "unset XDG_CONFIG_HOME" &&
+	test -s "$XDG_CONFIG_HOME/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

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

[Eric Sunshine]

On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@gmail.com> wrote:
> 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>
> ---
> 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
> +[[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.

The final sentence won't age well: "fairly recently" is too nebulous.
It may be sufficient merely to advise the reader to avoid this file if
she also uses an older version of Git which doesn't support XDG for
credentials.

Other than this minor point, the patch series seems well prepared and
quite nicely done. Thanks.

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

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

[Eric Sunshine]

On Wed, Mar 11, 2015 at 2:49 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:
>
> * 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.

Regarding the final sentence: I don't see a test corresponding to the
restriction that only ~/.git-credentials will be modified if both
files exist. Am I missing something?

More below.

> * 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>
> ---
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index f61b40c..34752ea 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -6,4 +6,111 @@ test_description='credential-store tests'
>
>  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"

Since these two tests are complementary, each checking a facet of the
same behavioral rule, it seems like they ought to be combined. For
people reading the tests, having them separate implies incorrectly
that they are unrelated, making it difficult to understand the overall
picture of how the pieces relate to one another. In prose, you would
describe the behavior as:

    When XDG credentials does not exist, write only to
    ~/.git-credentials; do not create XDG credentials.

It's one thought; easy to read and understand. The test should reflect
the same intent:

    test_expect_success '...' '
        test -s "$HOME/.git-credentials" &&
        test_path_is_missing "$HOME/.config/git/credentials"
    '

The same observation applies to several other tests below.

> +'
> +
> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '
> +       test_might_fail rm "$HOME/.git-credentials" &&

Can this just be "rm -f $HOME/.git-credentials &&" instead?

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

Ditto: It seems like these two tests should be combined.

> +'
> +
> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '
> +       XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME &&
> +       mkdir -p "$XDG_CONFIG_HOME/git" &&
> +       >"$XDG_CONFIG_HOME/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' '
> +       test_when_finished "unset XDG_CONFIG_HOME" &&
> +       test -s "$XDG_CONFIG_HOME/git/credentials"
> +'

It feels wrong to set global state (XDG_CONFIG_HOME) in one test and
then clear it later in another test, and it's not obvious to the
casual reader that global state is being modified. An alternative
would be to set XDG_CONFIG_HOME outside of the first test to which it
applies, clear it after the final test. In reality, however, it only
needs to be defined for the "helper_test store" invocation, so it also
could be highly localized:

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

A final alternative would be to wrap the block of tests needing
XDG_CONFIG_HOME within a subshell and set the variable only within the
subshell. Then, there's no need to unset it, and it's clear to the
reader that only the tests within the subshell are affected by it.

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

For clarity, the three above tests probably ought to be combined.

> +'
> +
> +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" &&

In the other tests, credentials in the XDG file are
"xdg-user:xdg-pass", and credentials in ~/.git-credentials are
"home-user:home-pass". It's not at all clear why, in this test, you
instead use "home-user:home-pass" for the XDG credentials. It seems
like an arbitrary and unnecessary change from the norm, and thus
confuses the reader.

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

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

[Paul Tan]

Hi,

Thanks for taking the time to write such a detailed review and
catching all of my careless mistakes.

On Wed, Mar 11, 2015 at 4:40 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Mar 11, 2015 at 2:49 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:
>>
>> * 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.
>
> Regarding the final sentence: I don't see a test corresponding to the
> restriction that only ~/.git-credentials will be modified if both
> files exist. Am I missing something?

Whoops, the test is indeed missing.

>> +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"
>
> Since these two tests are complementary, each checking a facet of the
> same behavioral rule, it seems like they ought to be combined. For
> people reading the tests, having them separate implies incorrectly
> that they are unrelated, making it difficult to understand the overall
> picture of how the pieces relate to one another. In prose, you would
> describe the behavior as:
>
>     When XDG credentials does not exist, write only to
>     ~/.git-credentials; do not create XDG credentials.
>
> It's one thought; easy to read and understand. The test should reflect
> the same intent:
>
>     test_expect_success '...' '
>         test -s "$HOME/.git-credentials" &&
>         test_path_is_missing "$HOME/.config/git/credentials"
>     '
>
> The same observation applies to several other tests below.

Indeed, it looks much cleaner. Thanks for the suggestion.

>> +'
>> +
>> +test_expect_success 'create $XDG_CONFIG_HOME/git/credentials file' '
>> +       test_might_fail rm "$HOME/.git-credentials" &&
>
> Can this just be "rm -f $HOME/.git-credentials &&" instead?

I picked this pattern up in t1306-xdg-files.sh so I thought it is the
norm, but turns out it's not. I think I can see the rationale though.
At this point in the tests the file ~/.git-credentials is expected to
exist, so if it does not exist a diagnostic message should be printed
to stderr (which -f will not do). Whether this actually helps is
purely theoretical though, so I will change it to "rm -f" on the next
iteration.

>> +'
>> +
>> +test_expect_success 'set up custom XDG_CONFIG_HOME credential file' '
>> +       XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME &&
>> +       mkdir -p "$XDG_CONFIG_HOME/git" &&
>> +       >"$XDG_CONFIG_HOME/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' '
>> +       test_when_finished "unset XDG_CONFIG_HOME" &&
>> +       test -s "$XDG_CONFIG_HOME/git/credentials"
>> +'
>
> It feels wrong to set global state (XDG_CONFIG_HOME) in one test and
> then clear it later in another test, and it's not obvious to the
> casual reader that global state is being modified. An alternative
> would be to set XDG_CONFIG_HOME outside of the first test to which it
> applies, clear it after the final test. In reality, however, it only
> needs to be defined for the "helper_test store" invocation, so it also
> could be highly localized:
>
>     XDG_CONFIG_HOME="$HOME/xdg"
>     export XDG_CONFIG_HOME
>     helper_test store
>     unset XDG_CONFIG_HOME
>
> A final alternative would be to wrap the block of tests needing
> XDG_CONFIG_HOME within a subshell and set the variable only within the
> subshell. Then, there's no need to unset it, and it's clear to the
> reader that only the tests within the subshell are affected by it.

I agree that the setting of XDG_CONFIG_HOME needs to be more
localized. This is a by-product of me following the "no code outside
tests" rule too strictly. A quick grep for "export" in the tests show
that quite a lot of tests do use export outside of test scripts,
though, so I guess I will change this to export XDG_CONFIG_HOME just
before running "helper_test store". Thanks for the suggestion.

>> +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" &&
>
> In the other tests, credentials in the XDG file are
> "xdg-user:xdg-pass", and credentials in ~/.git-credentials are
> "home-user:home-pass". It's not at all clear why, in this test, you
> instead use "home-user:home-pass" for the XDG credentials. It seems
> like an arbitrary and unnecessary change from the norm, and thus
> confuses the reader.

This is a careless mistake on my part when I was renaming all the
usernames and passwords to the current form. Thanks for catching it.

Regards,
Paul

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

[Paul Tan]

On Wed, Mar 11, 2015 at 3:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Mar 11, 2015 at 2:49 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> +
>> +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.
>
> The final sentence won't age well: "fairly recently" is too nebulous.
> It may be sufficient merely to advise the reader to avoid this file if
> she also uses an older version of Git which doesn't support XDG for
> credentials.

I copied this part from the documentation of git-config. I couldn't
find the exact patch in the archives where "fairly recently" was
introduced, but I did find this patch[1] where apparently a version
number was supposed to be used instead.

[1] http://thread.gmane.org/gmane.comp.version-control.git/198837/focus=200552

So yes, at this point in time I think the sentence should be changed
to something like "It is a good idea not to create this file if you
use older versions of git that do not support this file.", although it
would be even more useful for users if the version where this feature
was introduced is stated as well. This patch series has not even hit
pu though ;)

> Other than this minor point, the patch series seems well prepared and
> quite nicely done. Thanks.

Thank you so much for the positive review. Will re-roll the documentation.

Regards,
Paul

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

[Jeff King]

On Wed, Mar 11, 2015 at 02:49:10PM +0800, Paul Tan wrote:

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

I looked over the code here and in the second patch, and I didn't see
any real problems. Thanks for iterating on this.

Two minor nits, that might not even be worth addressing:

> +static void store_credential(const struct string_list *fns, struct credential *c,
> +			     const char *default_fn)

I think you could even get away without passing default_fn here, and
just use the rule "the first file in the list is the default". Unless
you are anticipating ever passing something else, but I couldn't think
of a case where that would be useful.

> +	struct string_list fns = STRING_LIST_INIT_NODUP;

This is declared NODUP...

> -	if (!file)
> +	if (file)
> +		string_list_append_nodup(&fns, file);

So you can just call the regular string_list_append here (the idea of
declaring the list as DUP or NODUP is that the callers do not have to
care; string_list_append does the right thing).

-Peff

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

[Paul Tan]

On Fri, Mar 13, 2015 at 2:15 PM, Jeff King <peff@peff.net> wrote:
>> +static void store_credential(const struct string_list *fns, struct credential *c,
>> +                          const char *default_fn)
>
> I think you could even get away without passing default_fn here, and
> just use the rule "the first file in the list is the default". Unless
> you are anticipating ever passing something else, but I couldn't think
> of a case where that would be useful.

Even though in this case the store_credential() function is not used
anywhere else, from my personal API design experience I think that
cementing the rule of "the first file in the list is the default" in
the behavior of the function is not a good thing. For example, in the
future, we may wish to keep the precedence ordering the same, but if
none of the credential files exist, we create the XDG file by default
instead. It's a balance of flexibility, but in this case I think
putting the default filename in a separate argument is a good thing.

>> +     struct string_list fns = STRING_LIST_INIT_NODUP;
>
> This is declared NODUP...
>
>> -     if (!file)
>> +     if (file)
>> +             string_list_append_nodup(&fns, file);
>
> So you can just call the regular string_list_append here (the idea of
> declaring the list as DUP or NODUP is that the callers do not have to
> care; string_list_append does the right thing).

Actually, thinking about it again from a memory management
perspective, STRING_LIST_INIT_DUP should be used instead as the
string_list `fns` should own the memory of the strings inside it.
Thus, in this case since `file` is pulled from the argv array,
string_list_append() should be used to duplicate the memory, and for
expand_user_path(), string_list_append_nodup() should be used because
the returned path is newly-allocated memory. Finally,
string_list_clear() should be called at the end to release memory.

Thanks for taking the time to review the patches, I will work on v4
now. (Really keen on getting this to pu)

Regards,
Paul

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

[Jeff King]

On Sat, Mar 14, 2015 at 04:15:53PM +0800, Paul Tan wrote:

> Even though in this case the store_credential() function is not used
> anywhere else, from my personal API design experience I think that
> cementing the rule of "the first file in the list is the default" in
> the behavior of the function is not a good thing. For example, in the
> future, we may wish to keep the precedence ordering the same, but if
> none of the credential files exist, we create the XDG file by default
> instead. It's a balance of flexibility, but in this case I think
> putting the default filename in a separate argument is a good thing.

Yeah, I see your line of reasoning. I think this is probably a case of
YAGNI, but it is really a matter of personal preference. It's not a big
deal either way.

> > So you can just call the regular string_list_append here (the idea of
> > declaring the list as DUP or NODUP is that the callers do not have to
> > care; string_list_append does the right thing).
> 
> Actually, thinking about it again from a memory management
> perspective, STRING_LIST_INIT_DUP should be used instead as the
> string_list `fns` should own the memory of the strings inside it.
> Thus, in this case since `file` is pulled from the argv array,
> string_list_append() should be used to duplicate the memory, and for
> expand_user_path(), string_list_append_nodup() should be used because
> the returned path is newly-allocated memory. Finally,
> string_list_clear() should be called at the end to release memory.

Yeah, I had the same thought, but I noticed that you don't call
string_list_clear. Nor is it really necessary to do so. Since git
programs are generally short-lived, we often let the OS take care of
deallocating variables like this (it's not appropriate for all
variables, of course, but quite frequently there are variables that are
effectively allocated at program startup and used until program end).

Again, I think this is a matter of taste. I don't mind if you want to
string_list_clear at the end, and I agree that using nodup() is the
right thing in that case.

-Peff

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

[Jeff King]

On Sat, Mar 14, 2015 at 01:33:28PM -0400, Jeff King wrote:

> On Sat, Mar 14, 2015 at 04:15:53PM +0800, Paul Tan wrote:
> 
> > Even though in this case the store_credential() function is not used
> > anywhere else, from my personal API design experience I think that
> > cementing the rule of "the first file in the list is the default" in
> > the behavior of the function is not a good thing. For example, in the
> > future, we may wish to keep the precedence ordering the same, but if
> > none of the credential files exist, we create the XDG file by default
> > instead. It's a balance of flexibility, but in this case I think
> > putting the default filename in a separate argument is a good thing.
> 
> Yeah, I see your line of reasoning. I think this is probably a case of
> YAGNI, but it is really a matter of personal preference. It's not a big
> deal either way.

By the way, I hope this (and the other comment) do not come off as "you
are wrong, but I do not feel like arguing with you". I really do think
these are a matter of taste, and while we often express issues of taste
in reviews, it is ultimately up to the patch submitter (who is, after
all, doing most of the work) to have the final say on minor issues like
this.  Sometimes the response to taste issue is "oh, I didn't think to
do that, thanks for the suggestion" and sometimes it is "nah, I like it
better my way". And both are OK.

Of course there are also taste issues where we insist (e.g., consistent
whitespace), but I do not think this is one of them.  :)

Maybe that was all obvious, but since you are new to the list, I wanted
to make sure I was clear.

-Peff

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

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

>> I think you could even get away without passing default_fn here, and
>> just use the rule "the first file in the list is the default". Unless
>> you are anticipating ever passing something else, but I couldn't think
>> of a case where that would be useful.
>
> Even though in this case the store_credential() function is not used
> anywhere else, from my personal API design experience I think that
> cementing the rule of "the first file in the list is the default" in
> the behavior of the function is not a good thing. For example, in the
> future, we may wish to keep the precedence ordering the same, but if
> none of the credential files exist, we create the XDG file by default
> instead.

I am not sure if this is not a premature over-engineering---I am not
convinced that such a future need will be fulfilled by passing just
a single default_fn this version already passes, or it needs even
more parameters that this version does not pass yet, and the
interface to the function needs to be updated at that point when you
need it _anyways_. One thing that we all agree is that we don't need
the extra parameter within the context of what the current code does.

So, it smells like a case of YAGNI a bit, at least to me.

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Paul Tan <pyokagan@gmail.com> writes:
>
>>> I think you could even get away without passing default_fn here, and
>>> just use the rule "the first file in the list is the default". Unless
>>> you are anticipating ever passing something else, but I couldn't think
>>> of a case where that would be useful.
>>
>> Even though in this case the store_credential() function is not used
>> anywhere else, from my personal API design experience I think that
>> cementing the rule of "the first file in the list is the default" in
>> the behavior of the function is not a good thing. For example, in the
>> future, we may wish to keep the precedence ordering the same, but if
>> none of the credential files exist, we create the XDG file by default
>> instead.
>
> I am not sure if this is not a premature over-engineering

I would say so if having this default_fn made the code more complex, but
here the code is basically

+	if (default_fn)
+		store_credential_file(default_fn, c);

and

-		store_credential(file, &c);
+		store_credential(&fns, &c, fns.items[0].string);

Taking the first element in the list wouldn't change much.

I'm personally fine with both versions.

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Paul Tan <pyokagan@gmail.com> writes:
>>
>>>> I think you could even get away without passing default_fn here, and
>>>> just use the rule "the first file in the list is the default". Unless
>>>> you are anticipating ever passing something else, but I couldn't think
>>>> of a case where that would be useful.
>>>
>>> Even though in this case the store_credential() function is not used
>>> anywhere else, from my personal API design experience I think that
>>> cementing the rule of "the first file in the list is the default" in
>>> the behavior of the function is not a good thing. For example, in the
>>> future, we may wish to keep the precedence ordering the same, but if
>>> none of the credential files exist, we create the XDG file by default
>>> instead.
>>
>> I am not sure if this is not a premature over-engineering
>
> I would say so if having this default_fn made the code more complex,

True, or if it made caller(s) be redundant or repeat themselves
without a good reason.  Otherwise we would end up having to drop the
redundant and/or unnecessary arguments in future clean-up patches; I
had to de-conflict a series with 7ce7c760 (convert: drop arguments
other than 'path' from would_convert_to_git(), 2014-08-21) recently,
which reminded me of this point ;-).

> but
> here the code is basically
>
> +	if (default_fn)
> +		store_credential_file(default_fn, c);
>
> and
>
> -		store_credential(file, &c);
> +		store_credential(&fns, &c, fns.items[0].string);
>
> Taking the first element in the list wouldn't change much.
>
> I'm personally fine with both versions.

Turning the current code to drop the extra parameter and to use the
first element instead wouldn't be a big change, but these things
tend to add up, so unless this discussion immediately lead to a
future enhancement plan to make use of the flexibility the parameter
gives us, I'd rather see things kept simpler.

I do not terribly mind either way, either, though.

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

[Paul Tan]

Hi all, thanks for providing your feedback.

On Sun, Mar 15, 2015 at 6:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I am not sure if this is not a premature over-engineering---I am not
> convinced that such a future need will be fulfilled by passing just
> a single default_fn this version already passes, or it needs even
> more parameters that this version does not pass yet, and the
> interface to the function needs to be updated at that point when you
> need it _anyways_. One thing that we all agree is that we don't need
> the extra parameter within the context of what the current code does.

After considering everyone's responses, I've decided to remove the
argument in the v4 patch. As Junio says, when there is a policy change
the code can be modified anyway.

Regards,
Paul