Re: [PATCH v2 1/7] path.c: implement xdg_config_home()
On Sat, Apr 18, 2015 at 3:51 AM, Paul Tan pyoka...@gmail.com wrote:
On Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan pyoka...@gmail.com wrote:
+ * Returns the newly allocated string $XDG_CONFIG_HOME/git/{filename}.
If
+ * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string
+ * $HOME/.config/git/{filename}. Returns NULL if filename is NULL or an
error
+ * occurred.
This is better than the original which said $XDG_CONFIG_HOME/git/%s,
but is still potentially confusing. When I read the earlier iteration,
I was left with the impression that it was returning that literal
string, with '$' and '%s' embedded, and that the caller would have to
process it further to have '$' and '%s' expanded. Perhaps rephrasing
it something like this will help?
Return a newly allocated string with value xdg+/git/+filename
where xdg is the interpolated value of $XDG_CONFIG_HOME if
defined and non-empty, otherwise $HOME/.config. Return NULL
upon error.
Personally I think interpolated strings are easier to read than
concatenated strings. $VARIABLE (all upper case) in shell scripting is
understood to be an environment variable, and $variable (all lower
case) to be a local variable.
Thinking about it again, I should not be using python-style format
strings either ;-). So I would write it as
$XDG_CONFIG_HOME/git/$filename.
But anyway, I don't have strong opinions on documentation, so I will
leave this to majority opinion. I will change it if you strongly
disagree :-).
Other than being enuinely confused by the original, and having to
check the actual implementation for clarification, I don't feel
strongly about it either. Perhaps mentioning evaluation like this
might help?
Return a newly allocated string with the evaluation of
$XDG_CONFIG_HOME/git/$filename if $XDG_CONFIG_HOME is
non-empty, otherwise $HOME/.config/git/$filename. Return
NULL upon error.
More below.
+ if (!config_home || !config_home[0]) {
+ const char *home = getenv(HOME);
+
+ if (!home)
+ return NULL;
+ return mkpathdup(%s/.config/git/%s, home, filename);
+ } else
+ return mkpathdup(%s/git/%s, config_home, filename);
This logic is more difficult to follow than it ought to be due to the
use of 'config_home' so distant from the 'if' which checked it, and
due to the nested 'if'. It could be expressed more straight-forwardly
as:
if (config_home *config_home)
return mkpathdup(%s/git/%s, config_home, filename);
home = getenv(HOME);
if (home)
return mkpathdup(%s/.config/git/%s, home, filename);
return NULL;
Ah, flipping the conditionals definitely makes it look nicer. I guess
I will need your sign off to use your code? Thanks!
My sign-off is probably overkill. I merely re-arranged some lines of
code which you wrote. A simple Helped-by: is sufficient if you want to
mention my name.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/7] path.c: implement xdg_config_home()
Hi,
On Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan pyoka...@gmail.com wrote:
Below is the fixed patch. I also decided to return NULL if `filename` is
NULL because such an input usually indicated an uncaught error.
Unfortunately, this blurs the line between programmer error (passing
NULL for filename) and a user/configuration error (XDG_CONFIG_HOME and
HOME being undefined). If there is indeed no valid interpretation for
filename==NULL, then it may be better to die() or assert() here to
flag the programmer error as early as possible, rather than returning
NULL.
More below.
---
diff --git a/cache.h b/cache.h
index 3d3244b..2db10b8 100644
--- a/cache.h
+++ b/cache.h
@@ -836,6 +836,14 @@ char *strip_path_suffix(const char *path, const char
*suffix);
int daemon_avoid_alias(const char *path);
extern int is_ntfs_dotgit(const char *name);
+/**
+ * Returns the newly allocated string $XDG_CONFIG_HOME/git/{filename}. If
+ * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string
+ * $HOME/.config/git/{filename}. Returns NULL if filename is NULL or an
error
+ * occurred.
+ */
This is better than the original which said $XDG_CONFIG_HOME/git/%s,
but is still potentially confusing. When I read the earlier iteration,
I was left with the impression that it was returning that literal
string, with '$' and '%s' embedded, and that the caller would have to
process it further to have '$' and '%s' expanded. Perhaps rephrasing
it something like this will help?
Return a newly allocated string with value xdg+/git/+filename
where xdg is the interpolated value of $XDG_CONFIG_HOME if
defined and non-empty, otherwise $HOME/.config. Return NULL
upon error.
Personally I think interpolated strings are easier to read than
concatenated strings. $VARIABLE (all upper case) in shell scripting is
understood to be an environment variable, and $variable (all lower
case) to be a local variable.
Thinking about it again, I should not be using python-style format
strings either ;-). So I would write it as
$XDG_CONFIG_HOME/git/$filename.
But anyway, I don't have strong opinions on documentation, so I will
leave this to majority opinion. I will change it if you strongly
disagree :-).
Also, for consistency with other API documentation, say Return
rather than Returns.
Okay, will fix.
More below.
+extern char *xdg_config_home(const char *filename);
+
/* object replacement */
#define LOOKUP_REPLACE_OBJECT 1
extern void *read_sha1_file_extended(const unsigned char *sha1, enum
object_type *type, unsigned long *size, unsigned flag);
diff --git a/path.c b/path.c
index e608993..8ee7191 100644
--- a/path.c
+++ b/path.c
@@ -856,3 +856,19 @@ int is_ntfs_dotgit(const char *name)
len = -1;
}
}
+
+char *xdg_config_home(const char *filename)
+{
+ const char *config_home = getenv(XDG_CONFIG_HOME);
+
+ if (!filename)
+ return NULL;
See above regarding conflation of programmer error and user/configuration
error.
+ if (!config_home || !config_home[0]) {
On this project, *config_home is usually favored over config_home[0].
+ const char *home = getenv(HOME);
+
+ if (!home)
+ return NULL;
+ return mkpathdup(%s/.config/git/%s, home, filename);
+ } else
+ return mkpathdup(%s/git/%s, config_home, filename);
This logic is more difficult to follow than it ought to be due to the
use of 'config_home' so distant from the 'if' which checked it, and
due to the nested 'if'. It could be expressed more straight-forwardly
as:
if (config_home *config_home)
return mkpathdup(%s/git/%s, config_home, filename);
home = getenv(HOME);
if (home)
return mkpathdup(%s/.config/git/%s, home, filename);
return NULL;
Ah, flipping the conditionals definitely makes it look nicer. I guess
I will need your sign off to use your code? Thanks!
Regards,
Paul
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/7] path.c: implement xdg_config_home()
Hi, On Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan pyoka...@gmail.com wrote: Below is the fixed patch. I also decided to return NULL if `filename` is NULL because such an input usually indicated an uncaught error. Unfortunately, this blurs the line between programmer error (passing NULL for filename) and a user/configuration error (XDG_CONFIG_HOME and HOME being undefined). If there is indeed no valid interpretation for filename==NULL, then it may be better to die() or assert() here to flag the programmer error as early as possible, rather than returning NULL. I'm inclined to agree, but off the top of my head an API user may wish to do: xdg_config_home(function_which_returns_NULL_on_error()) And wish for the error to propagate, but that may be considered sloppy programming and so it's better to treat a NULL input as a bug. Will fix this by calling assert(). Thanks. Regards, Paul -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/7] path.c: implement xdg_config_home()
On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan pyoka...@gmail.com wrote:
Below is the fixed patch. I also decided to return NULL if `filename` is
NULL because such an input usually indicated an uncaught error.
Unfortunately, this blurs the line between programmer error (passing
NULL for filename) and a user/configuration error (XDG_CONFIG_HOME and
HOME being undefined). If there is indeed no valid interpretation for
filename==NULL, then it may be better to die() or assert() here to
flag the programmer error as early as possible, rather than returning
NULL.
More below.
8
The XDG base dir spec[1] specifies that configuration files be stored in
a subdirectory in $XDG_CONFIG_HOME. To construct such a configuration
file path, home_config_paths() can be used. However, home_config_paths()
combines distinct functionality:
1. Retrieve the home git config file path ~/.gitconfig
2. Construct the XDG config path of the file specified by `file`.
This function was introduced in commit 21cf3227 (read (but not write)
from $XDG_CONFIG_HOME/git/config file). While the intention of the
function was to allow the home directory configuration file path and the
xdg directory configuration file path to be retrieved with one function
call, the hard-coding of the path ~/.gitconfig prevents it from being
used for other configuration files. Furthermore, retrieving a file path
relative to the user's home directory can be done with
expand_user_path(). Hence, it can be seen that home_config_paths()
introduces unnecessary complexity, especially if a user just wants to
retrieve the xdg config file path.
As such, implement a simpler function xdg_config_home() for constructing
the XDG base dir spec configuration file path. This function, together
with expand_user_path(), can replace all uses of home_config_paths().
[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html
Signed-off-by: Paul Tan pyoka...@gmail.com
---
diff --git a/cache.h b/cache.h
index 3d3244b..2db10b8 100644
--- a/cache.h
+++ b/cache.h
@@ -836,6 +836,14 @@ char *strip_path_suffix(const char *path, const char
*suffix);
int daemon_avoid_alias(const char *path);
extern int is_ntfs_dotgit(const char *name);
+/**
+ * Returns the newly allocated string $XDG_CONFIG_HOME/git/{filename}. If
+ * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string
+ * $HOME/.config/git/{filename}. Returns NULL if filename is NULL or an
error
+ * occurred.
+ */
This is better than the original which said $XDG_CONFIG_HOME/git/%s,
but is still potentially confusing. When I read the earlier iteration,
I was left with the impression that it was returning that literal
string, with '$' and '%s' embedded, and that the caller would have to
process it further to have '$' and '%s' expanded. Perhaps rephrasing
it something like this will help?
Return a newly allocated string with value xdg+/git/+filename
where xdg is the interpolated value of $XDG_CONFIG_HOME if
defined and non-empty, otherwise $HOME/.config. Return NULL
upon error.
Also, for consistency with other API documentation, say Return
rather than Returns.
More below.
+extern char *xdg_config_home(const char *filename);
+
/* object replacement */
#define LOOKUP_REPLACE_OBJECT 1
extern void *read_sha1_file_extended(const unsigned char *sha1, enum
object_type *type, unsigned long *size, unsigned flag);
diff --git a/path.c b/path.c
index e608993..8ee7191 100644
--- a/path.c
+++ b/path.c
@@ -856,3 +856,19 @@ int is_ntfs_dotgit(const char *name)
len = -1;
}
}
+
+char *xdg_config_home(const char *filename)
+{
+ const char *config_home = getenv(XDG_CONFIG_HOME);
+
+ if (!filename)
+ return NULL;
See above regarding conflation of programmer error and user/configuration error.
+ if (!config_home || !config_home[0]) {
On this project, *config_home is usually favored over config_home[0].
+ const char *home = getenv(HOME);
+
+ if (!home)
+ return NULL;
+ return mkpathdup(%s/.config/git/%s, home, filename);
+ } else
+ return mkpathdup(%s/git/%s, config_home, filename);
This logic is more difficult to follow than it ought to be due to the
use of 'config_home' so distant from the 'if' which checked it, and
due to the nested 'if'. It could be expressed more straight-forwardly
as:
if (config_home *config_home)
return mkpathdup(%s/git/%s, config_home, filename);
home = getenv(HOME);
if (home)
return mkpathdup(%s/.config/git/%s, home, filename);
return NULL;
+}
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
