Hey Junio,
On Tue, May 24, 2016 at 12:46 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Pranit Bauva <pranit.bauva@xxxxxxxxx> 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
--
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