Re: [PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG

[Pranit Bauva <pranit.bauva@gmail.com>]

Hey Junio,

On Tue, May 24, 2016 at 12:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> This is a follow up commit for f932729c (memoize common git-path
>> "constant" files, 10-Aug-2015).
>>
>> It serves two purposes:
>>   1. It reduces the number of calls to git_path() .
>>
>>   2. It serves the benefits of using GIT_PATH_FUNC as mentioned in the
>>      commit message of f932729c.
>
> All of that is a good idea, but I have huge doubts about its use.
>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 391126e..ffa242c 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -92,8 +92,10 @@ N_("If you wish to skip this commit, use:\n"
>>  "Then \"git cherry-pick --continue\" will resume cherry-picking\n"
>>  "the remaining commits.\n");
>>
>> +static GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
>> +
>>  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[]?  In order for git_pathdup() to
> produce a meaningful result, it needs to know where .git/ directory
> is, which (roughly) means setup_git_dir() must have been called from
> a callchain from main() somewhere already.
>
> But I do not think the linker knows that fact.

I think otherwise. git_pathdup() calls get_worktree_git_dir() which
calls get_git_dir() which if uninitialized calls setup_git_env(). So
technically the code gets to know the .git/ directory quite early.
Though I am not very sure whether this one is a desirable fact. There
would be later instances which would in turn call to know where the
.git/ directory.

>
>> @@ -771,9 +773,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>>               hook_arg2 = "";
>>       }
>>
>
> Instead, what you could do is to call git_path_commit_editmsg() when
> you refer to that global variable whose initialization is suspect.
>
>> -     s->fp = fopen_for_writing(git_path(commit_editmsg));
>> +     s->fp = fopen_for_writing(commit_editmsg_path);
>
> i.e.
>
>         s->fp = fopen_for_writing(git_path_commit_editmsg());
>
> As you can see in its definition, when the original code used to
> call git_path(), it is safe to call git_path_commit_editmsg(),
> because for the original git_path() to be correct, the code should
> already have established where $GIT_DIR is, so it is safe to call
> git_pathdup(), too.  Also, as you can see in its definition, calling
> the function many times would not cause git_path() called many
> times.  The first invocation will keep its value that is constant
> within the program that works with a constant $GIT_DIR.

I agree that it is actually not required to again compute the location
of .git/ directory and can only return the value.

Overall I agree to your idea of just using git_path_commit_editmsg()
instead of git_path() so as to not disturb any previous
implementations which can lead to some complications. Also if I am
changing some internal semantics there should be a valid reason which
there isn't really as I don't see any benefit in getting the location
of .git/ early in the program.

> And you do not free its return value.
This is one of the thing that bugging me with GIT_PATH_FUNC. Wouldn't
not freeing the memory lead to memory leaks?

Regards,
Pranit Bauva