Re: [PATCH v3] Expand ~ and ~user in core.excludesfile, commit.template
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 29, 2008, 04:08 UTC
- Message-ID
- <7vod3ca2ey.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <20080829032630.GA7024@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> 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.
Show 11 quoted lines
>> +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.
Show 5 quoted lines
>> + 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.
Show 7 quoted lines
>> +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.