{"thread":{"id":"39053","subject":"[PATCH 2/7] attr.c: replace home_config_paths() with xdg_config_home()","startedAt":"2015-04-12T07:46:06Z","lastAt":"2015-05-06T08:01:04Z","messageCount":26,"participants":["Paul Tan","Johannes Schindelin","Matthieu Moy","Stefan Beller","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"259254","messageId":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":null,"subject":"[PATCH 1/7] path.c: implement xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-12T07:46:06Z","receivedAt":"2015-04-12T07:46:06Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"The XDG base dir spec[1] specifies that configuration files be stored in\na subdirectory in $XDG_CONFIG_HOME. To construct such a configuration\nfile path, home_config_paths() can be used. However, home_config_paths()\ncombines distinct functionality:\n\n1. Retrieve the home git config file path ~/.gitconfig\n\n2. Construct the XDG config path of the file specified by `file`.\n\nThis function was introduced in commit 21cf3227 (\"read (but not write)\nfrom $XDG_CONFIG_HOME/git/config file\").  While the intention of the\nfunction was to allow the home directory configuration file path and the\nxdg directory configuration file path to be retrieved with one function\ncall, the hard-coding of the path ~/.gitconfig prevents it from being\nused for other configuration files. Furthermore, retrieving a file path\nrelative to the user's home directory can be done with\nexpand_user_path(). Hence, it can be seen that home_config_paths()\nintroduces unnecessary complexity, especially if a user just wants to\nretrieve the xdg config file path.\n\nAs such, implement a simpler function xdg_config_home() for constructing\nthe XDG base dir spec configuration file path. This function, together\nwith expand_user_path(), can replace all uses of home_config_paths().\n\n[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n cache.h |  7 +++++++\n path.c  | 16 ++++++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..7f9bab0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const char *suffix);\n int daemon_avoid_alias(const char *path);\n extern int is_ntfs_dotgit(const char *name);\n \n+/**\n+ * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/%s\".  If\n+ * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n+ * \"$HOME/.config/git/%s\". Returns NULL if an error occurred.\n+ */\n+extern char *xdg_config_home(const char *fn);\n+\n /* object replacement */\n #define LOOKUP_REPLACE_OBJECT 1\n extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);\ndiff --git a/path.c b/path.c\nindex e608993..4c32d16 100644\n--- a/path.c\n+++ b/path.c\n@@ -856,3 +856,19 @@ int is_ntfs_dotgit(const char *name)\n \t\t\tlen = -1;\n \t\t}\n }\n+\n+char *xdg_config_home(const char *fn)\n+{\n+\tconst char *config_home = getenv(\"XDG_CONFIG_HOME\");\n+\n+\tif (!fn)\n+\t\tfn = \"\";\n+\tif (!config_home || !config_home[0]) {\n+\t\tconst char *home = getenv(\"HOME\");\n+\n+\t\tif (!home)\n+\t\t\treturn NULL;\n+\t\treturn mkpathdup(\"%s/.config/git/%s\", home, fn);\n+\t} else\n+\t\treturn mkpathdup(\"%s/git/%s\", config_home, fn);\n+}\n-- \n2.1.4\n"},{"id":"259253","messageId":"1428824772-8736-2-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 2/7] attr.c: replace home_config_paths() with xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-12T07:46:07Z","receivedAt":"2015-04-12T07:46:07Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since only the xdg attributes file path is required, simplify the code\nby using xdg_config_home() instead of home_config_paths().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n attr.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 1f9eebd..868dce3 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -488,7 +488,6 @@ static int git_attr_system(void)\n static void bootstrap_attr_stack(void)\n {\n \tstruct attr_stack *elem;\n-\tchar *xdg_attributes_file;\n \n \tif (attr_stack)\n \t\treturn;\n@@ -507,10 +506,8 @@ static void bootstrap_attr_stack(void)\n \t\t}\n \t}\n \n-\tif (!git_attributes_file) {\n-\t\thome_config_paths(NULL, &xdg_attributes_file, \"attributes\");\n-\t\tgit_attributes_file = xdg_attributes_file;\n-\t}\n+\tif (!git_attributes_file)\n+\t\tgit_attributes_file = xdg_config_home(\"attributes\");\n \tif (git_attributes_file) {\n \t\telem = read_attr_from_file(git_attributes_file, 1);\n \t\tif (elem) {\n-- \n2.1.4\n"},{"id":"259255","messageId":"1428824772-8736-3-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 3/7] dir.c: replace home_config_paths() with xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-12T07:46:08Z","receivedAt":"2015-04-12T07:46:08Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since only the xdg excludes file path is required, simplify the code by\nreplacing use of home_config_paths() with xdg_config_home().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n dir.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 0943a81..335dc7e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1665,14 +1665,11 @@ int remove_dir_recursively(struct strbuf *path, int flag)\n void setup_standard_excludes(struct dir_struct *dir)\n {\n \tconst char *path;\n-\tchar *xdg_path;\n \n \tdir->exclude_per_dir = \".gitignore\";\n \tpath = git_path(\"info/exclude\");\n-\tif (!excludes_file) {\n-\t\thome_config_paths(NULL, &xdg_path, \"ignore\");\n-\t\texcludes_file = xdg_path;\n-\t}\n+\tif (!excludes_file)\n+\t\texcludes_file = xdg_config_home(\"ignore\");\n \tif (!access_or_warn(path, R_OK, 0))\n \t\tadd_excludes_from_file(dir, path);\n \tif (excludes_file && !access_or_warn(excludes_file, R_OK, 0))\n-- \n2.1.4\n"},{"id":"259257","messageId":"1428824772-8736-4-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 4/7] credential-store.c: replace home_config_paths() with xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-12T07:46:09Z","receivedAt":"2015-04-12T07:46:09Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since only the xdg credentials file path is required, and\nhome_config_paths() is unable to construct the path ~/.git-credentials,\nsimplify the code by replacing home_config_paths() with\nxdg_config_home().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n credential-store.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 8b22251..f692509 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -170,7 +170,7 @@ int main(int argc, char **argv)\n \t} else {\n \t\tif ((file = expand_user_path(\"~/.git-credentials\")))\n \t\t\tstring_list_append_nodup(&fns, file);\n-\t\thome_config_paths(NULL, &file, \"credentials\");\n+\t\tfile = xdg_config_home(\"credentials\");\n \t\tif (file)\n \t\t\tstring_list_append_nodup(&fns, file);\n \t}\n-- \n2.1.4\n"},{"id":"259258","messageId":"1428824772-8736-5-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 5/7] git-commit: replace use of home_config_paths()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-12T07:46:10Z","receivedAt":"2015-04-12T07:46:10Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since home_config_paths() combines two distinct functionality already\nimplemented by expand_user_path() and xdg_config_home(), and hides the\nhome config file path ~/.gitconfig. Make the code more explicit by\nreplacing the use of home_config_paths() with expand_user_path() and\nxdg_config_home().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n builtin/commit.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex da79ac4..c2ebea4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1398,12 +1398,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \n static const char *implicit_ident_advice(void)\n {\n-\tchar *user_config = NULL;\n-\tchar *xdg_config = NULL;\n-\tint config_exists;\n+\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n+\tchar *xdg_config = xdg_config_home(\"config\");\n+\tint config_exists = file_exists(user_config) || file_exists(xdg_config);\n \n-\thome_config_paths(&user_config, &xdg_config, \"config\");\n-\tconfig_exists = file_exists(user_config) || file_exists(xdg_config);\n \tfree(user_config);\n \tfree(xdg_config);\n \n-- \n2.1.4\n"},{"id":"259259","messageId":"1428824772-8736-6-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 6/7] git-config: replace use of home_config_paths()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-12T07:46:11Z","receivedAt":"2015-04-12T07:46:11Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since home_config_paths() combines distinct functionality already\nimplemented by expand_user_path() and xdg_config_home(), and hides the\nhome config file path ~/.gitconfig. Make the code more explicit by\nreplacing the use of home_config_paths() with expand_user_path() and\nxdg_config_home().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n builtin/config.c | 6 ++----\n config.c         | 6 ++----\n 2 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d32c532..5e61f4a 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -488,10 +488,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (use_global_config) {\n-\t\tchar *user_config = NULL;\n-\t\tchar *xdg_config = NULL;\n-\n-\t\thome_config_paths(&user_config, &xdg_config, \"config\");\n+\t\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n+\t\tchar *xdg_config = xdg_config_home(\"config\");\n \n \t\tif (!user_config)\n \t\t\t/*\ndiff --git a/config.c b/config.c\nindex 66c0a51..3a619da 100644\n--- a/config.c\n+++ b/config.c\n@@ -1185,10 +1185,8 @@ int git_config_system(void)\n int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n {\n \tint ret = 0, found = 0;\n-\tchar *xdg_config = NULL;\n-\tchar *user_config = NULL;\n-\n-\thome_config_paths(&user_config, &xdg_config, \"config\");\n+\tchar *xdg_config = xdg_config_home(\"config\");\n+\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n \n \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n-- \n2.1.4\n"},{"id":"259256","messageId":"1428824772-8736-7-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 7/7] path.c: remove home_config_paths()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-12T07:46:12Z","receivedAt":"2015-04-12T07:46:12Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"home_config_paths() combines distinct functionality already implemented\nby expand_user_path() and xdg_config_home(), and it also hard-codes the\npath ~/.gitconfig, which makes it unsuitable to use for other home\nconfig file paths. Since its use will just add unnecessary complexity to\nthe code, remove it.\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n cache.h |  1 -\n path.c  | 28 ----------------------------\n 2 files changed, 29 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 7f9bab0..19d75ef 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -816,7 +816,6 @@ enum scld_error safe_create_leading_directories(char *path);\n enum scld_error safe_create_leading_directories_const(const char *path);\n \n int mkdir_in_gitdir(const char *path);\n-extern void home_config_paths(char **global, char **xdg, char *file);\n extern char *expand_user_path(const char *path);\n const char *enter_repo(const char *path, int strict);\n static inline int is_absolute_path(const char *path)\ndiff --git a/path.c b/path.c\nindex 4c32d16..84ec589 100644\n--- a/path.c\n+++ b/path.c\n@@ -130,34 +130,6 @@ char *git_path(const char *fmt, ...)\n \treturn ret;\n }\n \n-void home_config_paths(char **global, char **xdg, char *file)\n-{\n-\tchar *xdg_home = getenv(\"XDG_CONFIG_HOME\");\n-\tchar *home = getenv(\"HOME\");\n-\tchar *to_free = NULL;\n-\n-\tif (!home) {\n-\t\tif (global)\n-\t\t\t*global = NULL;\n-\t} else {\n-\t\tif (!xdg_home) {\n-\t\t\tto_free = mkpathdup(\"%s/.config\", home);\n-\t\t\txdg_home = to_free;\n-\t\t}\n-\t\tif (global)\n-\t\t\t*global = mkpathdup(\"%s/.gitconfig\", home);\n-\t}\n-\n-\tif (xdg) {\n-\t\tif (!xdg_home)\n-\t\t\t*xdg = NULL;\n-\t\telse\n-\t\t\t*xdg = mkpathdup(\"%s/git/%s\", xdg_home, file);\n-\t}\n-\n-\tfree(to_free);\n-}\n-\n char *git_path_submodule(const char *path, const char *fmt, ...)\n {\n \tchar *pathname = get_pathname();\n-- \n2.1.4\n"},{"id":"259303","messageId":"e1bc6f19af608db796a2212dbf00ba45@www.dscho.org","threadId":"39053","inReplyTo":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 1/7] path.c: implement xdg_config_home()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-13T15:50:49Z","receivedAt":"2015-04-13T15:50:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nmaybe it would be a good idea to add a `0/7` mail that describes the overall goal of this patch series, much like a Pull Request? I found it very useful -- even for myself -- to set a description via `git branch --edit-description` and to let `git format-patch` use that via the `--cover-letter` option.\n\nbelow just two minor nits because the rest of the patches looks fine to me:\n\nOn 2015-04-12 09:46, Paul Tan wrote:\n\n\n> diff --git a/cache.h b/cache.h\n> index 3d3244b..7f9bab0 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const\n> char *suffix);\n>  int daemon_avoid_alias(const char *path);\n>  extern int is_ntfs_dotgit(const char *name);\n>  \n> +/**\n> + * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/%s\".  If\n> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n> + * \"$HOME/.config/git/%s\". Returns NULL if an error occurred.\n> + */\n> +extern char *xdg_config_home(const char *fn);\n\nShould this not be inserted close to home_config_paths()? Also, the name \"fn\" sounds more like \"function\" than like \"filename\" to me, especially keeping the name `config_fn_t` in mind. Maybe call the parameter \"filename\" to avoid confusion?\n\nThanks,\nDscho\n"},{"id":"259311","messageId":"vpqwq1fn1pf.fsf@anie.imag.fr","threadId":"39053","inReplyTo":"1428824772-8736-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 1/7] path.c: implement xdg_config_home()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-04-13T21:43:56Z","receivedAt":"2015-04-13T21:43:56Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> As such, implement a simpler function xdg_config_home() for constructing\n> the XDG base dir spec configuration file path. This function, together\n> with expand_user_path(), can replace all uses of home_config_paths().\n\nIndeed. The code looks much better after your patch series than before.\n\nI agree with Dscho's remark that \"fn\" sounds like \"function\" more than\n\"filename\". Perhaps just \"name\" would be better.\n\nAnyway, the series is\n\nReviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"259314","messageId":"CAGZ79kYnmYu54XbEOtP8n_sgvLs4Zx=Z4Cr_U=qOrTmZCEFr=g@mail.gmail.com","threadId":"39053","inReplyTo":"vpqwq1fn1pf.fsf@anie.imag.fr","subject":"Re: [PATCH 1/7] path.c: implement xdg_config_home()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-14T00:18:13Z","receivedAt":"2015-04-14T00:18:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Apr 13, 2015 at 2:43 PM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> Paul Tan <pyokagan@gmail.com> writes:\n>\n>> As such, implement a simpler function xdg_config_home() for constructing\n>> the XDG base dir spec configuration file path. This function, together\n>> with expand_user_path(), can replace all uses of home_config_paths().\n>\n> Indeed. The code looks much better after your patch series than before.\n>\n> I agree with Dscho's remark that \"fn\" sounds like \"function\" more than\n> \"filename\". Perhaps just \"name\" would be better.\n\nI'd go with fname but that's just bikeshedding now.\n\n>\n> Anyway, the series is\n>\n> Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n\nand\nReviewed-by: Stefan Beller <sbeller@google.com>\n\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"259362","messageId":"20150414172854.GA27623@yoshi.chippynet.com","threadId":"39053","inReplyTo":"e1bc6f19af608db796a2212dbf00ba45@www.dscho.org","subject":"[PATCH v2 1/7] path.c: implement xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-14T17:28:54Z","receivedAt":"2015-04-14T17:28:54Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Mon, Apr 13, 2015 at 05:50:49PM +0200, Johannes Schindelin wrote:\n> maybe it would be a good idea to add a `0/7` mail that describes the\n> overall goal of this patch series, much like a Pull Request? I found\n> it very useful -- even for myself -- to set a description via `git\n> branch --edit-description` and to let `git format-patch` use that via\n> the `--cover-letter` option.\n\nIn this case I felt that the first patch's commit message was\ndescriptive enough and a cover message would simply repeat it.\n\n> below just two minor nits because the rest of the patches looks fine to me:\n> \n> On 2015-04-12 09:46, Paul Tan wrote:\n> > diff --git a/cache.h b/cache.h\n> > index 3d3244b..7f9bab0 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const\n> > char *suffix);\n> >  int daemon_avoid_alias(const char *path);\n> >  extern int is_ntfs_dotgit(const char *name);\n> >  \n> > +/**\n> > + * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/%s\".  If\n> > + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n> > + * \"$HOME/.config/git/%s\". Returns NULL if an error occurred.\n> > + */\n> > +extern char *xdg_config_home(const char *fn);\n> \n> Should this not be inserted close to home_config_paths()?\n\nA personal style thing, but I wanted to add the function's docstring to\ncache.h (where I personally think it belongs), but I didn't want to\nbreak up the huge block of path function declarations. Hence, it was\nadded at the end.\n\n> Also, the name \"fn\" sounds more like \"function\" than like \"filename\"\n> to me, especially keeping the name `config_fn_t` in mind. Maybe call\n> the parameter \"filename\" to avoid confusion?\n\nThat's true, especially since there is still lots of horizontal space\nfor this name.\n\nBelow is the fixed patch. I also decided to return NULL if `filename` is\nNULL because such an input usually indicated an uncaught error. The\ndocstring has also been modified to be a little clearer.\n\nThanks all for the reviews.\n\n---- >8 ----\nThe XDG base dir spec[1] specifies that configuration files be stored in\na subdirectory in $XDG_CONFIG_HOME. To construct such a configuration\nfile path, home_config_paths() can be used. However, home_config_paths()\ncombines distinct functionality:\n\n1. Retrieve the home git config file path ~/.gitconfig\n\n2. Construct the XDG config path of the file specified by `file`.\n\nThis function was introduced in commit 21cf3227 (\"read (but not write)\nfrom $XDG_CONFIG_HOME/git/config file\").  While the intention of the\nfunction was to allow the home directory configuration file path and the\nxdg directory configuration file path to be retrieved with one function\ncall, the hard-coding of the path ~/.gitconfig prevents it from being\nused for other configuration files. Furthermore, retrieving a file path\nrelative to the user's home directory can be done with\nexpand_user_path(). Hence, it can be seen that home_config_paths()\nintroduces unnecessary complexity, especially if a user just wants to\nretrieve the xdg config file path.\n\nAs such, implement a simpler function xdg_config_home() for constructing\nthe XDG base dir spec configuration file path. This function, together\nwith expand_user_path(), can replace all uses of home_config_paths().\n\n[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n cache.h |  8 ++++++++\n path.c  | 16 ++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..2db10b8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -836,6 +836,14 @@ char *strip_path_suffix(const char *path, const char *suffix);\n int daemon_avoid_alias(const char *path);\n extern int is_ntfs_dotgit(const char *name);\n \n+/**\n+ * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/{filename}\".  If\n+ * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n+ * \"$HOME/.config/git/{filename}\". Returns NULL if filename is NULL or an error\n+ * occurred.\n+ */\n+extern char *xdg_config_home(const char *filename);\n+\n /* object replacement */\n #define LOOKUP_REPLACE_OBJECT 1\n extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);\ndiff --git a/path.c b/path.c\nindex e608993..8ee7191 100644\n--- a/path.c\n+++ b/path.c\n@@ -856,3 +856,19 @@ int is_ntfs_dotgit(const char *name)\n \t\t\tlen = -1;\n \t\t}\n }\n+\n+char *xdg_config_home(const char *filename)\n+{\n+\tconst char *config_home = getenv(\"XDG_CONFIG_HOME\");\n+\n+\tif (!filename)\n+\t\treturn NULL;\n+\tif (!config_home || !config_home[0]) {\n+\t\tconst char *home = getenv(\"HOME\");\n+\n+\t\tif (!home)\n+\t\t\treturn NULL;\n+\t\treturn mkpathdup(\"%s/.config/git/%s\", home, filename);\n+\t} else\n+\t\treturn mkpathdup(\"%s/git/%s\", config_home, filename);\n+}\n-- \n2.1.4\n"},{"id":"259370","messageId":"xmqqzj6a1m3c.fsf@gitster.dls.corp.google.com","threadId":"39053","inReplyTo":"e1bc6f19af608db796a2212dbf00ba45@www.dscho.org","subject":"Re: [PATCH 1/7] path.c: implement xdg_config_home()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-14T20:39:03Z","receivedAt":"2015-04-14T20:39:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n>> diff --git a/cache.h b/cache.h\n>> index 3d3244b..7f9bab0 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const\n>> char *suffix);\n>>  int daemon_avoid_alias(const char *path);\n>>  extern int is_ntfs_dotgit(const char *name);\n>>  \n>> +/**\n>> + * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/%s\".  If\n>> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n>> + * \"$HOME/.config/git/%s\". Returns NULL if an error occurred.\n>> + */\n>> +extern char *xdg_config_home(const char *fn);\n>\n> Should this not be inserted close to home_config_paths()? Also, the\n> name \"fn\" sounds more like \"function\" than like \"filename\" to me,\n> especially keeping the name `config_fn_t` in mind. Maybe call the\n> parameter \"filename\" to avoid confusion?\n\nIt is OK to omit the name in the extern declaration here.  We have\nto have a sensible variable name in the definition in path.c, of\ncourse ;-), and \"filename\" sounds like a very sensible suggestion.\n"},{"id":"259380","messageId":"CAGZ79kaO_1QMMTY0ni9k3hrkrt_PhqRDRzXkhZEiYuJ0EsE9Tw@mail.gmail.com","threadId":"39053","inReplyTo":"xmqqzj6a1m3c.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/7] path.c: implement xdg_config_home()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-14T22:28:14Z","receivedAt":"2015-04-14T22:28:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Apr 14, 2015 at 1:39 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>\n>>> diff --git a/cache.h b/cache.h\n>>> index 3d3244b..7f9bab0 100644\n>>> --- a/cache.h\n>>> +++ b/cache.h\n>>> @@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const\n>>> char *suffix);\n>>>  int daemon_avoid_alias(const char *path);\n>>>  extern int is_ntfs_dotgit(const char *name);\n>>>\n>>> +/**\n>>> + * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/%s\".  If\n>>> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n>>> + * \"$HOME/.config/git/%s\". Returns NULL if an error occurred.\n>>> + */\n>>> +extern char *xdg_config_home(const char *fn);\n>>\n>> Should this not be inserted close to home_config_paths()? Also, the\n>> name \"fn\" sounds more like \"function\" than like \"filename\" to me,\n>> especially keeping the name `config_fn_t` in mind. Maybe call the\n>> parameter \"filename\" to avoid confusion?\n>\n> It is OK to omit the name in the extern declaration here.  We have\n> to have a sensible variable name in the definition in path.c, of\n> course ;-), and \"filename\" sounds like a very sensible suggestion.\n>\n\nIt is OK, but I think this is bad practice.\nHave a look at strbuf.h, there you'll find:\n\n    extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n\nIt's not clear what the 2 ints are, probably a fd and a max size?\nEven if guessed correctly, you'd still need another guess for the order.\n"},{"id":"259382","messageId":"xmqqd2361gx8.fsf@gitster.dls.corp.google.com","threadId":"39053","inReplyTo":"CAGZ79kaO_1QMMTY0ni9k3hrkrt_PhqRDRzXkhZEiYuJ0EsE9Tw@mail.gmail.com","subject":"Re: [PATCH 1/7] path.c: implement xdg_config_home()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-14T22:30:43Z","receivedAt":"2015-04-14T22:30:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Tue, Apr 14, 2015 at 1:39 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> It is OK to omit the name in the extern declaration here.\n>\n> It is OK, but I think this is bad practice.\n\nTake a special note on the word \"here\", meaning \"in this particular\ncase.\"  It is perfectly fine when the meaning of the parameter is\nclear from its type.\n\nI was assuming that our developers have common sense to disambiguate\nambiguous cases, of course ;-)\n"},{"id":"259383","messageId":"CAGZ79kZvTdmGpoQozNjNefB4CWm31C05S1JoEBASFofxpYUXYg@mail.gmail.com","threadId":"39053","inReplyTo":"xmqqd2361gx8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/7] path.c: implement xdg_config_home()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-14T22:34:12Z","receivedAt":"2015-04-14T22:34:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I need to learn to read the whole sentence. :(\nApologies.\n\nOn Tue, Apr 14, 2015 at 3:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Tue, Apr 14, 2015 at 1:39 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>> It is OK to omit the name in the extern declaration here.\n>>\n>> It is OK, but I think this is bad practice.\n>\n> Take a special note on the word \"here\", meaning \"in this particular\n> case.\"  It is perfectly fine when the meaning of the parameter is\n> clear from its type.\n>\n> I was assuming that our developers have common sense to disambiguate\n> ambiguous cases, of course ;-)\n"},{"id":"259542","messageId":"CAPig+cTrErOBwPteeA1d_gdve5SiyLnbyFPpQ1sTN7aajGJfeA@mail.gmail.com","threadId":"39053","inReplyTo":"20150414172854.GA27623@yoshi.chippynet.com","subject":"Re: [PATCH v2 1/7] path.c: implement xdg_config_home()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-16T21:41:19Z","receivedAt":"2015-04-16T21:41:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan <pyokagan@gmail.com> wrote:\n> Below is the fixed patch. I also decided to return NULL if `filename` is\n> NULL because such an input usually indicated an uncaught error.\n\nUnfortunately, this blurs the line between programmer error (passing\nNULL for filename) and a user/configuration error (XDG_CONFIG_HOME and\nHOME being undefined). If there is indeed no valid interpretation for\nfilename==NULL, then it may be better to die() or assert() here to\nflag the programmer error as early as possible, rather than returning\nNULL.\n\nMore below.\n\n> ---- >8 ----\n> The XDG base dir spec[1] specifies that configuration files be stored in\n> a subdirectory in $XDG_CONFIG_HOME. To construct such a configuration\n> file path, home_config_paths() can be used. However, home_config_paths()\n> combines distinct functionality:\n>\n> 1. Retrieve the home git config file path ~/.gitconfig\n>\n> 2. Construct the XDG config path of the file specified by `file`.\n>\n> This function was introduced in commit 21cf3227 (\"read (but not write)\n> from $XDG_CONFIG_HOME/git/config file\").  While the intention of the\n> function was to allow the home directory configuration file path and the\n> xdg directory configuration file path to be retrieved with one function\n> call, the hard-coding of the path ~/.gitconfig prevents it from being\n> used for other configuration files. Furthermore, retrieving a file path\n> relative to the user's home directory can be done with\n> expand_user_path(). Hence, it can be seen that home_config_paths()\n> introduces unnecessary complexity, especially if a user just wants to\n> retrieve the xdg config file path.\n>\n> As such, implement a simpler function xdg_config_home() for constructing\n> the XDG base dir spec configuration file path. This function, together\n> with expand_user_path(), can replace all uses of home_config_paths().\n>\n> [1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html\n>\n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n> diff --git a/cache.h b/cache.h\n> index 3d3244b..2db10b8 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -836,6 +836,14 @@ char *strip_path_suffix(const char *path, const char *suffix);\n>  int daemon_avoid_alias(const char *path);\n>  extern int is_ntfs_dotgit(const char *name);\n>\n> +/**\n> + * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/{filename}\".  If\n> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n> + * \"$HOME/.config/git/{filename}\". Returns NULL if filename is NULL or an error\n> + * occurred.\n> + */\n\nThis is better than the original which said \"$XDG_CONFIG_HOME/git/%s\",\nbut is still potentially confusing. When I read the earlier iteration,\nI was left with the impression that it was returning that literal\nstring, with '$' and '%s' embedded, and that the caller would have to\nprocess it further to have '$' and '%s' expanded. Perhaps rephrasing\nit something like this will help?\n\n    Return a newly allocated string with value xdg+\"/git/\"+filename\n    where xdg is the interpolated value of $XDG_CONFIG_HOME if\n    defined and non-empty, otherwise \"$HOME/.config\". Return NULL\n    upon error.\n\nAlso, for consistency with other API documentation, say \"Return\"\nrather than \"Returns\".\n\nMore below.\n\n> +extern char *xdg_config_home(const char *filename);\n> +\n>  /* object replacement */\n>  #define LOOKUP_REPLACE_OBJECT 1\n>  extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);\n> diff --git a/path.c b/path.c\n> index e608993..8ee7191 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -856,3 +856,19 @@ int is_ntfs_dotgit(const char *name)\n>                         len = -1;\n>                 }\n>  }\n> +\n> +char *xdg_config_home(const char *filename)\n> +{\n> +       const char *config_home = getenv(\"XDG_CONFIG_HOME\");\n> +\n> +       if (!filename)\n> +               return NULL;\n\nSee above regarding conflation of programmer error and user/configuration error.\n\n> +       if (!config_home || !config_home[0]) {\n\nOn this project, *config_home is usually favored over config_home[0].\n\n> +               const char *home = getenv(\"HOME\");\n> +\n> +               if (!home)\n> +                       return NULL;\n> +               return mkpathdup(\"%s/.config/git/%s\", home, filename);\n> +       } else\n> +               return mkpathdup(\"%s/git/%s\", config_home, filename);\n\nThis logic is more difficult to follow than it ought to be due to the\nuse of 'config_home' so distant from the 'if' which checked it, and\ndue to the nested 'if'. It could be expressed more straight-forwardly\nas:\n\n    if (config_home && *config_home)\n        return mkpathdup(\"%s/git/%s\", config_home, filename);\n\n    home = getenv(\"HOME\");\n    if (home)\n        return mkpathdup(\"%s/.config/git/%s\", home, filename);\n\n    return NULL;\n\n> +}\n> --\n> 2.1.4\n"},{"id":"259615","messageId":"CACRoPnR7MQsP_dAE0RV_80JDbU9h6tfu30yL2OOK+JGYLzVLNQ@mail.gmail.com","threadId":"39053","inReplyTo":"CAPig+cTrErOBwPteeA1d_gdve5SiyLnbyFPpQ1sTN7aajGJfeA@mail.gmail.com","subject":"Re: [PATCH v2 1/7] path.c: implement xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-18T07:51:48Z","receivedAt":"2015-04-18T07:51:48Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan <pyokagan@gmail.com> wrote:\n>> Below is the fixed patch. I also decided to return NULL if `filename` is\n>> NULL because such an input usually indicated an uncaught error.\n>\n> Unfortunately, this blurs the line between programmer error (passing\n> NULL for filename) and a user/configuration error (XDG_CONFIG_HOME and\n> HOME being undefined). If there is indeed no valid interpretation for\n> filename==NULL, then it may be better to die() or assert() here to\n> flag the programmer error as early as possible, rather than returning\n> NULL.\n>\n> More below.\n>\n>> ---\n>> diff --git a/cache.h b/cache.h\n>> index 3d3244b..2db10b8 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -836,6 +836,14 @@ char *strip_path_suffix(const char *path, const char *suffix);\n>>  int daemon_avoid_alias(const char *path);\n>>  extern int is_ntfs_dotgit(const char *name);\n>>\n>> +/**\n>> + * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/{filename}\".  If\n>> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n>> + * \"$HOME/.config/git/{filename}\". Returns NULL if filename is NULL or an error\n>> + * occurred.\n>> + */\n>\n> This is better than the original which said \"$XDG_CONFIG_HOME/git/%s\",\n> but is still potentially confusing. When I read the earlier iteration,\n> I was left with the impression that it was returning that literal\n> string, with '$' and '%s' embedded, and that the caller would have to\n> process it further to have '$' and '%s' expanded. Perhaps rephrasing\n> it something like this will help?\n>\n>     Return a newly allocated string with value xdg+\"/git/\"+filename\n>     where xdg is the interpolated value of $XDG_CONFIG_HOME if\n>     defined and non-empty, otherwise \"$HOME/.config\". Return NULL\n>     upon error.\n>\n\nPersonally I think interpolated strings are easier to read than\nconcatenated strings. $VARIABLE (all upper case) in shell scripting is\nunderstood to be an environment variable, and $variable (all lower\ncase) to be a local variable.\n\nThinking about it again, I should not be using python-style format\nstrings either ;-). So I would write it as\n\"$XDG_CONFIG_HOME/git/$filename\".\n\nBut anyway, I don't have strong opinions on documentation, so I will\nleave this to majority opinion. I will change it if you strongly\ndisagree :-).\n\n> Also, for consistency with other API documentation, say \"Return\"\n> rather than \"Returns\".\n\nOkay, will fix.\n\n>\n> More below.\n>\n>> +extern char *xdg_config_home(const char *filename);\n>> +\n>>  /* object replacement */\n>>  #define LOOKUP_REPLACE_OBJECT 1\n>>  extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);\n>> diff --git a/path.c b/path.c\n>> index e608993..8ee7191 100644\n>> --- a/path.c\n>> +++ b/path.c\n>> @@ -856,3 +856,19 @@ int is_ntfs_dotgit(const char *name)\n>>                         len = -1;\n>>                 }\n>>  }\n>> +\n>> +char *xdg_config_home(const char *filename)\n>> +{\n>> +       const char *config_home = getenv(\"XDG_CONFIG_HOME\");\n>> +\n>> +       if (!filename)\n>> +               return NULL;\n>\n> See above regarding conflation of programmer error and user/configuration error.\n>\n>> +       if (!config_home || !config_home[0]) {\n>\n> On this project, *config_home is usually favored over config_home[0].\n>\n>> +               const char *home = getenv(\"HOME\");\n>> +\n>> +               if (!home)\n>> +                       return NULL;\n>> +               return mkpathdup(\"%s/.config/git/%s\", home, filename);\n>> +       } else\n>> +               return mkpathdup(\"%s/git/%s\", config_home, filename);\n>\n> This logic is more difficult to follow than it ought to be due to the\n> use of 'config_home' so distant from the 'if' which checked it, and\n> due to the nested 'if'. It could be expressed more straight-forwardly\n> as:\n>\n>     if (config_home && *config_home)\n>         return mkpathdup(\"%s/git/%s\", config_home, filename);\n>\n>     home = getenv(\"HOME\");\n>     if (home)\n>         return mkpathdup(\"%s/.config/git/%s\", home, filename);\n>\n>     return NULL;\n>\n\nAh, flipping the conditionals definitely makes it look nicer. I guess\nI will need your sign off to use your code? Thanks!\n\nRegards,\nPaul\n"},{"id":"259618","messageId":"CACRoPnRMHDO3VC1-S2rpx7jgcv=c_ym6wRSx2xxNRk=k_vtXvA@mail.gmail.com","threadId":"39053","inReplyTo":"CAPig+cTrErOBwPteeA1d_gdve5SiyLnbyFPpQ1sTN7aajGJfeA@mail.gmail.com","subject":"Re: [PATCH v2 1/7] path.c: implement xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-18T08:48:35Z","receivedAt":"2015-04-18T08:48:35Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan <pyokagan@gmail.com> wrote:\n>> Below is the fixed patch. I also decided to return NULL if `filename` is\n>> NULL because such an input usually indicated an uncaught error.\n>\n> Unfortunately, this blurs the line between programmer error (passing\n> NULL for filename) and a user/configuration error (XDG_CONFIG_HOME and\n> HOME being undefined). If there is indeed no valid interpretation for\n> filename==NULL, then it may be better to die() or assert() here to\n> flag the programmer error as early as possible, rather than returning\n> NULL.\n\nI'm inclined to agree, but off the top of my head an API user may wish to do:\n\n    xdg_config_home(function_which_returns_NULL_on_error())\n\nAnd wish for the error to propagate, but that may be considered sloppy\nprogramming and so it's better to treat a NULL input as a bug.\n\nWill fix this by calling assert(). Thanks.\n\nRegards,\nPaul\n"},{"id":"259652","messageId":"CAPig+cQD9BH8kib98fCsiUHnsSAAMkwxAnFVxD8Gq5JMwXVjog@mail.gmail.com","threadId":"39053","inReplyTo":"CACRoPnR7MQsP_dAE0RV_80JDbU9h6tfu30yL2OOK+JGYLzVLNQ@mail.gmail.com","subject":"Re: [PATCH v2 1/7] path.c: implement xdg_config_home()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-20T00:39:44Z","receivedAt":"2015-04-20T00:39:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 18, 2015 at 3:51 AM, Paul Tan <pyokagan@gmail.com> wrote:\n> On Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan <pyokagan@gmail.com> wrote:\n>>> + * Returns the newly allocated string \"$XDG_CONFIG_HOME/git/{filename}\".  If\n>>> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string\n>>> + * \"$HOME/.config/git/{filename}\". Returns NULL if filename is NULL or an error\n>>> + * occurred.\n>> This is better than the original which said \"$XDG_CONFIG_HOME/git/%s\",\n>> but is still potentially confusing. When I read the earlier iteration,\n>> I was left with the impression that it was returning that literal\n>> string, with '$' and '%s' embedded, and that the caller would have to\n>> process it further to have '$' and '%s' expanded. Perhaps rephrasing\n>> it something like this will help?\n>>\n>>     Return a newly allocated string with value xdg+\"/git/\"+filename\n>>     where xdg is the interpolated value of $XDG_CONFIG_HOME if\n>>     defined and non-empty, otherwise \"$HOME/.config\". Return NULL\n>>     upon error.\n>\n> Personally I think interpolated strings are easier to read than\n> concatenated strings. $VARIABLE (all upper case) in shell scripting is\n> understood to be an environment variable, and $variable (all lower\n> case) to be a local variable.\n> Thinking about it again, I should not be using python-style format\n> strings either ;-). So I would write it as\n> \"$XDG_CONFIG_HOME/git/$filename\".\n>\n> But anyway, I don't have strong opinions on documentation, so I will\n> leave this to majority opinion. I will change it if you strongly\n> disagree :-).\n\nOther than being enuinely confused by the original, and having to\ncheck the actual implementation for clarification, I don't feel\nstrongly about it either. Perhaps mentioning \"evaluation\" like this\nmight help?\n\n    Return a newly allocated string with the evaluation of\n    \"$XDG_CONFIG_HOME/git/$filename\" if $XDG_CONFIG_HOME is\n    non-empty, otherwise \"$HOME/.config/git/$filename\". Return\n    NULL upon error.\n\nMore below.\n\n>>> +       if (!config_home || !config_home[0]) {\n>>> +               const char *home = getenv(\"HOME\");\n>>> +\n>>> +               if (!home)\n>>> +                       return NULL;\n>>> +               return mkpathdup(\"%s/.config/git/%s\", home, filename);\n>>> +       } else\n>>> +               return mkpathdup(\"%s/git/%s\", config_home, filename);\n>>\n>> This logic is more difficult to follow than it ought to be due to the\n>> use of 'config_home' so distant from the 'if' which checked it, and\n>> due to the nested 'if'. It could be expressed more straight-forwardly\n>> as:\n>>\n>>     if (config_home && *config_home)\n>>         return mkpathdup(\"%s/git/%s\", config_home, filename);\n>>\n>>     home = getenv(\"HOME\");\n>>     if (home)\n>>         return mkpathdup(\"%s/.config/git/%s\", home, filename);\n>>\n>>     return NULL;\n>\n> Ah, flipping the conditionals definitely makes it look nicer. I guess\n> I will need your sign off to use your code? Thanks!\n\nMy sign-off is probably overkill. I merely re-arranged some lines of\ncode which you wrote. A simple Helped-by: is sufficient if you want to\nmention my name.\n"},{"id":"259725","messageId":"20150421040627.GA5415@yoshi.chippynet.com","threadId":"39053","inReplyTo":"CAPig+cQD9BH8kib98fCsiUHnsSAAMkwxAnFVxD8Gq5JMwXVjog@mail.gmail.com","subject":"[PATCH v3 1/7] path.c: implement xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-04-21T04:06:27Z","receivedAt":"2015-04-21T04:06:27Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Sun, Apr 19, 2015 at 08:39:44PM -0400, Eric Sunshine wrote:\n> Other than being enuinely confused by the original, and having to\n> check the actual implementation for clarification, I don't feel\n> strongly about it either. Perhaps mentioning \"evaluation\" like this\n> might help?\n> \n>     Return a newly allocated string with the evaluation of\n>     \"$XDG_CONFIG_HOME/git/$filename\" if $XDG_CONFIG_HOME is\n>     non-empty, otherwise \"$HOME/.config/git/$filename\". Return\n>     NULL upon error.\n> \n\nThis is perfect, thanks.\n\nRe-rolled patch below now uses assert() to check if filename is\nnon-NULL, and re-arranges the control flow.\n\n--- >8 ---\n\nThe XDG base dir spec[1] specifies that configuration files be stored in\na subdirectory in $XDG_CONFIG_HOME. To construct such a configuration\nfile path, home_config_paths() can be used. However, home_config_paths()\ncombines distinct functionality:\n\n1. Retrieve the home git config file path ~/.gitconfig\n\n2. Construct the XDG config path of the file specified by `file`.\n\nThis function was introduced in commit 21cf3227 (\"read (but not write)\nfrom $XDG_CONFIG_HOME/git/config file\").  While the intention of the\nfunction was to allow the home directory configuration file path and the\nxdg directory configuration file path to be retrieved with one function\ncall, the hard-coding of the path ~/.gitconfig prevents it from being\nused for other configuration files. Furthermore, retrieving a file path\nrelative to the user's home directory can be done with\nexpand_user_path(). Hence, it can be seen that home_config_paths()\nintroduces unnecessary complexity, especially if a user just wants to\nretrieve the xdg config file path.\n\nAs such, implement a simpler function xdg_config_home() for constructing\nthe XDG base dir spec configuration file path. This function, together\nwith expand_user_path(), can replace all uses of home_config_paths().\n\n[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n cache.h |  7 +++++++\n path.c  | 15 +++++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..3512d32 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const char *suffix);\n int daemon_avoid_alias(const char *path);\n extern int is_ntfs_dotgit(const char *name);\n \n+/**\n+ * Return a newly allocated string with the evaluation of\n+ * \"$XDG_CONFIG_HOME/git/$filename\" if $XDG_CONFIG_HOME is non-empty, otherwise\n+ * \"$HOME/.config/git/$filename\". Return NULL upon error.\n+ */\n+extern char *xdg_config_home(const char *filename);\n+\n /* object replacement */\n #define LOOKUP_REPLACE_OBJECT 1\n extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);\ndiff --git a/path.c b/path.c\nindex 595da81..c28b8f5 100644\n--- a/path.c\n+++ b/path.c\n@@ -851,3 +851,18 @@ int is_ntfs_dotgit(const char *name)\n \t\t\tlen = -1;\n \t\t}\n }\n+\n+char *xdg_config_home(const char *filename)\n+{\n+\tconst char *home, *config_home;\n+\n+\tassert(filename);\n+\tconfig_home = getenv(\"XDG_CONFIG_HOME\");\n+\tif (config_home && *config_home)\n+\t\treturn mkpathdup(\"%s/git/%s\", config_home, filename);\n+\n+\thome = getenv(\"HOME\");\n+\tif (home)\n+\t\treturn mkpathdup(\"%s/.config/git/%s\", home, filename);\n+\treturn NULL;\n+}\n-- \n2.1.4\n"},{"id":"260653","messageId":"1430899264-18447-1-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"20150421040627.GA5415@yoshi.chippynet.com","subject":"[PATCH v3 2/7] attr.c: replace home_config_paths() with xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-06T08:00:59Z","receivedAt":"2015-05-06T08:00:59Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since only the xdg attributes file path is required, simplify the code\nby using xdg_config_home() instead of home_config_paths().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n\nRe-sending the patch series. There are no changes.\n\n attr.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 1f9eebd..868dce3 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -488,7 +488,6 @@ static int git_attr_system(void)\n static void bootstrap_attr_stack(void)\n {\n \tstruct attr_stack *elem;\n-\tchar *xdg_attributes_file;\n \n \tif (attr_stack)\n \t\treturn;\n@@ -507,10 +506,8 @@ static void bootstrap_attr_stack(void)\n \t\t}\n \t}\n \n-\tif (!git_attributes_file) {\n-\t\thome_config_paths(NULL, &xdg_attributes_file, \"attributes\");\n-\t\tgit_attributes_file = xdg_attributes_file;\n-\t}\n+\tif (!git_attributes_file)\n+\t\tgit_attributes_file = xdg_config_home(\"attributes\");\n \tif (git_attributes_file) {\n \t\telem = read_attr_from_file(git_attributes_file, 1);\n \t\tif (elem) {\n-- \n2.1.4\n"},{"id":"260654","messageId":"1430899264-18447-2-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1430899264-18447-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 3/7] dir.c: replace home_config_paths() with xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-06T08:01:00Z","receivedAt":"2015-05-06T08:01:00Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since only the xdg excludes file path is required, simplify the code by\nreplacing use of home_config_paths() with xdg_config_home().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n\nRe-sending the patch series. There are no changes.\n\n dir.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 0943a81..335dc7e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1665,14 +1665,11 @@ int remove_dir_recursively(struct strbuf *path, int flag)\n void setup_standard_excludes(struct dir_struct *dir)\n {\n \tconst char *path;\n-\tchar *xdg_path;\n \n \tdir->exclude_per_dir = \".gitignore\";\n \tpath = git_path(\"info/exclude\");\n-\tif (!excludes_file) {\n-\t\thome_config_paths(NULL, &xdg_path, \"ignore\");\n-\t\texcludes_file = xdg_path;\n-\t}\n+\tif (!excludes_file)\n+\t\texcludes_file = xdg_config_home(\"ignore\");\n \tif (!access_or_warn(path, R_OK, 0))\n \t\tadd_excludes_from_file(dir, path);\n \tif (excludes_file && !access_or_warn(excludes_file, R_OK, 0))\n-- \n2.1.4\n"},{"id":"260655","messageId":"1430899264-18447-3-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1430899264-18447-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 4/7] credential-store.c: replace home_config_paths() with xdg_config_home()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-06T08:01:01Z","receivedAt":"2015-05-06T08:01:01Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since only the xdg credentials file path is required, and\nhome_config_paths() is unable to construct the path ~/.git-credentials,\nsimplify the code by replacing home_config_paths() with\nxdg_config_home().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n\nRe-sending the patch series. There are no changes.\n\n credential-store.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 8b22251..f692509 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -170,7 +170,7 @@ int main(int argc, char **argv)\n \t} else {\n \t\tif ((file = expand_user_path(\"~/.git-credentials\")))\n \t\t\tstring_list_append_nodup(&fns, file);\n-\t\thome_config_paths(NULL, &file, \"credentials\");\n+\t\tfile = xdg_config_home(\"credentials\");\n \t\tif (file)\n \t\t\tstring_list_append_nodup(&fns, file);\n \t}\n-- \n2.1.4\n"},{"id":"260656","messageId":"1430899264-18447-4-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1430899264-18447-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 5/7] git-commit: replace use of home_config_paths()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-06T08:01:02Z","receivedAt":"2015-05-06T08:01:02Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since home_config_paths() combines two distinct functionality already\nimplemented by expand_user_path() and xdg_config_home(), and hides the\nhome config file path ~/.gitconfig. Make the code more explicit by\nreplacing the use of home_config_paths() with expand_user_path() and\nxdg_config_home().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n\nRe-sending the patch series. There are no changes.\n\n builtin/commit.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex da79ac4..c2ebea4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1398,12 +1398,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \n static const char *implicit_ident_advice(void)\n {\n-\tchar *user_config = NULL;\n-\tchar *xdg_config = NULL;\n-\tint config_exists;\n+\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n+\tchar *xdg_config = xdg_config_home(\"config\");\n+\tint config_exists = file_exists(user_config) || file_exists(xdg_config);\n \n-\thome_config_paths(&user_config, &xdg_config, \"config\");\n-\tconfig_exists = file_exists(user_config) || file_exists(xdg_config);\n \tfree(user_config);\n \tfree(xdg_config);\n \n-- \n2.1.4\n"},{"id":"260657","messageId":"1430899264-18447-5-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1430899264-18447-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 6/7] git-config: replace use of home_config_paths()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-06T08:01:03Z","receivedAt":"2015-05-06T08:01:03Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since home_config_paths() combines distinct functionality already\nimplemented by expand_user_path() and xdg_config_home(), and hides the\nhome config file path ~/.gitconfig. Make the code more explicit by\nreplacing the use of home_config_paths() with expand_user_path() and\nxdg_config_home().\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n\nRe-sending the patch series. There are no changes.\n\n builtin/config.c | 6 ++----\n config.c         | 6 ++----\n 2 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d32c532..5e61f4a 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -488,10 +488,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (use_global_config) {\n-\t\tchar *user_config = NULL;\n-\t\tchar *xdg_config = NULL;\n-\n-\t\thome_config_paths(&user_config, &xdg_config, \"config\");\n+\t\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n+\t\tchar *xdg_config = xdg_config_home(\"config\");\n \n \t\tif (!user_config)\n \t\t\t/*\ndiff --git a/config.c b/config.c\nindex 66c0a51..3a619da 100644\n--- a/config.c\n+++ b/config.c\n@@ -1185,10 +1185,8 @@ int git_config_system(void)\n int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n {\n \tint ret = 0, found = 0;\n-\tchar *xdg_config = NULL;\n-\tchar *user_config = NULL;\n-\n-\thome_config_paths(&user_config, &xdg_config, \"config\");\n+\tchar *xdg_config = xdg_config_home(\"config\");\n+\tchar *user_config = expand_user_path(\"~/.gitconfig\");\n \n \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n-- \n2.1.4\n"},{"id":"260658","messageId":"1430899264-18447-6-git-send-email-pyokagan@gmail.com","threadId":"39053","inReplyTo":"1430899264-18447-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v3 7/7] path.c: remove home_config_paths()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-06T08:01:04Z","receivedAt":"2015-05-06T08:01:04Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"home_config_paths() combines distinct functionality already implemented\nby expand_user_path() and xdg_config_home(), and it also hard-codes the\npath ~/.gitconfig, which makes it unsuitable to use for other home\nconfig file paths. Since its use will just add unnecessary complexity to\nthe code, remove it.\n\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n\nRe-sending the patch series. There are no changes.\n\n cache.h |  1 -\n path.c  | 28 ----------------------------\n 2 files changed, 29 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3512d32..f6970bb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -816,7 +816,6 @@ enum scld_error safe_create_leading_directories(char *path);\n enum scld_error safe_create_leading_directories_const(const char *path);\n \n int mkdir_in_gitdir(const char *path);\n-extern void home_config_paths(char **global, char **xdg, char *file);\n extern char *expand_user_path(const char *path);\n const char *enter_repo(const char *path, int strict);\n static inline int is_absolute_path(const char *path)\ndiff --git a/path.c b/path.c\nindex c28b8f5..6b537cc 100644\n--- a/path.c\n+++ b/path.c\n@@ -130,34 +130,6 @@ char *git_path(const char *fmt, ...)\n \treturn ret;\n }\n \n-void home_config_paths(char **global, char **xdg, char *file)\n-{\n-\tchar *xdg_home = getenv(\"XDG_CONFIG_HOME\");\n-\tchar *home = getenv(\"HOME\");\n-\tchar *to_free = NULL;\n-\n-\tif (!home) {\n-\t\tif (global)\n-\t\t\t*global = NULL;\n-\t} else {\n-\t\tif (!xdg_home) {\n-\t\t\tto_free = mkpathdup(\"%s/.config\", home);\n-\t\t\txdg_home = to_free;\n-\t\t}\n-\t\tif (global)\n-\t\t\t*global = mkpathdup(\"%s/.gitconfig\", home);\n-\t}\n-\n-\tif (xdg) {\n-\t\tif (!xdg_home)\n-\t\t\t*xdg = NULL;\n-\t\telse\n-\t\t\t*xdg = mkpathdup(\"%s/git/%s\", xdg_home, file);\n-\t}\n-\n-\tfree(to_free);\n-}\n-\n char *git_path_submodule(const char *path, const char *fmt, ...)\n {\n \tchar *pathname = get_pathname();\n-- \n2.1.4\n"}]}