Re: [PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- May 24, 2016, 08:11 UTC
- Message-ID
- <vpq1t4rri2a.fsf@anie.imag.fr>
- In-Reply-To
- <xmqq7feka8kk.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> static const char *use_message_buffer;
>> -static const char commit_editmsg[] = "COMMIT_EDITMSG";
>> +static const char commit_editmsg_path[] = git_path_commit_editmsg();
>
> The function defined with the macro looks like
>
> const char *git_path_commit_editmsg(void)
> {
> static char *ret;
> if (!ret)
> ret = git_pathdup("COMMIT_EDITMSG");
> return ret;
> }
>
> so receiving its result to "const char v[]" looks somewhat
> suspicious.
>
> More importantly, when is this function evaluated and returned value
> used to fill commit_editmsg_path[]?I may have missed something, but I'd say "never", as the code is not compilable at least with my gcc:
builtin/commit.c:98:1: error: invalid initializer static const char commit_editmsg_path[] = git_path_commit_editmsg(); ^
AFAIK, initializing a global variable with a function call is allowed in C++, but not in C.
And indeed, this construct is a huge source of trouble, as it would mean that git_path_commit_editmsg() is called 1) unconditionnally, and 2) before entering main().
1) means that the function call is made even when git is called for another command. This is terrible for the startup time: if all git commands have a not-totally-immediate initializer, then all commands would need to run the initializers for all other commands. 2) means it's a nightmare to debug, as you can hardly predict when the code will be executed.
-- Matthieu Moy http://www-verimag.imag.fr/~moy/