Re: [PATCH v3] Expand ~ and ~user in core.excludesfile, commit.template

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 28, 2008 at 02:09:38AM -0700, Karl Chen wrote:
>
>>  builtin-commit.c |    2 +-
>>  cache.h          |    2 +
>>  config.c         |   13 +++++++-
>>  path.c           |   88 +++++++++++++++++++++++++++++++++--------------------
>>  4 files changed, 69 insertions(+), 36 deletions(-)
>
> Documentation?
>
>>  	if (!strcmp(k, "commit.template"))
>> -		return git_config_string(&template_file, k, v);
>> +		return git_config_userdir(&template_file, k, v);
>
> I like this.

Likewise, except for the name.  It is more like "pathname"; "userdir" is
stressing only one aspect of the magic we would do to a value that is a
pathname compared to a value that is a string without any magic.

>> +int git_config_userdir(const char **dest, const char *var, const char *value) {
>> +	if (!value)
>> +		return config_error_nonbool(var);
>> +	*dest = expand_user_path(NULL, value, 0);
>> +	if (!*dest || !**dest) die("Failed to expand user dir in: '%s'", value);
>> +	return 0;
>> +}
>
> I am not sure about !**dest here. This precludes somebody from using "".
> While it might not matter here, if there are other users of
> git_config_userdir(), they might want to allow a blank entry.

True again.

>> +	if (begin_username == end_username) {
>> +		return getpwuid(getuid());
>> +	} else {
>
> Style: omit braces on one-liner conditionals:

... except in cases like this where "else" side is a multi-statement
block, in which case the above is fine.  But as you pointed out, the early
return from here makes the else block unnecessary so you do not need the
braces around "if" side.

>> +		char *username = alloca(username_len + 1);
>
> I don't think we use alloca() anywhere else. I don't know if there are
> portability issues.

Avoidance of alloca() and c99 dynamic array on stack is deliberate in the
current codebase.  Portable use of alloca() is quite hard to get right.

>> +static inline const char *strchr_or_end(const char *str, char c)
>> +{
>> +	while (*str && *str != c) ++str;
>> +	return str;
>> +}
>
> This really seems like premature optimization to me.

So is overuse of inline, it seems.