Hi,
On Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan <pyokagan@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html