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

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

[Pranit Bauva]

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.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/commit.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

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();
 static struct lock_file index_lock; /* real index */
 static struct lock_file false_lock; /* used only for partial commits */
 static enum {
@@ -771,9 +773,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg2 = "";
 	}
 
-	s->fp = fopen_for_writing(git_path(commit_editmsg));
+	s->fp = fopen_for_writing(commit_editmsg_path);
 	if (s->fp == NULL)
-		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
+		die_errno(_("could not open '%s'"), commit_editmsg_path);
 
 	/* Ignore status.displayCommentPrefix: we do need comments in COMMIT_EDITMSG. */
 	old_display_comment_prefix = s->display_comment_prefix;
@@ -950,7 +952,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
-			    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+			    commit_editmsg_path, hook_arg1, hook_arg2, NULL))
 		return 0;
 
 	if (use_editor) {
@@ -958,7 +960,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		const char *env[2] = { NULL };
 		env[0] =  index;
 		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path(commit_editmsg), NULL, env)) {
+		if (launch_editor(commit_editmsg_path, NULL, env)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
@@ -966,7 +968,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_commit_hook(use_editor, index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+	    run_commit_hook(use_editor, index_file, "commit-msg", commit_editmsg_path, NULL)) {
 		return 0;
 	}
 
@@ -1728,7 +1730,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	/* Finally, get the commit message */
 	strbuf_reset(&sb);
-	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
+	if (strbuf_read_file(&sb, commit_editmsg_path, 0) < 0) {
 		int saved_errno = errno;
 		rollback_index_files();
 		die(_("could not read commit message: %s"), strerror(saved_errno));
-- 
2.8.2

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

[Junio C Hamano]

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.

> @@ -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.

And you do not free its return value.

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

[Pranit Bauva]

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

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

[Pranit Bauva]

Hey Junio

On Tue, May 24, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> 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?

Slight misunderstanding. I got it now. Thanks!

> Regards,
> Pranit Bauva

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> 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/

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

[Pranit Bauva]

Hey Matthieu,

On Tue, May 24, 2016 at 1:41 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.

I wasn't aware of this fact. Thanks.

> 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.

Yes I agree that this approach will definitely cause a lot of
problems. I will re-roll with how Junio suggested.

> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

This is a follow up commit for f932729c (memoize common git-path
"constant" files, 10-Aug-2015).

The many function calls to git_path() are replaced by
git_path_commit_editmsg() and which thus eliminates the need to repeatedly
compute the location of "COMMIT_EDITMSG".

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
Link for v1[1].

Changes wrt v1:

 * Remove the call to git_path_commit_editmsg() which would directly assign
   the value to the string.
 * Remove the string commit_editmsg[] as it is redundant now.
 * Call git_path_commit_editmsg() everytime when it is needed.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/295345

 builtin/commit.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 391126e..01b921f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -92,8 +92,9 @@ 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 struct lock_file index_lock; /* real index */
 static struct lock_file false_lock; /* used only for partial commits */
 static enum {
@@ -771,9 +772,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg2 = "";
 	}
 
-	s->fp = fopen_for_writing(git_path(commit_editmsg));
+	s->fp = fopen_for_writing(git_path_commit_editmsg());
 	if (s->fp == NULL)
-		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
+		die_errno(_("could not open '%s'"), git_path_commit_editmsg());
 
 	/* Ignore status.displayCommentPrefix: we do need comments in COMMIT_EDITMSG. */
 	old_display_comment_prefix = s->display_comment_prefix;
@@ -950,7 +951,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
-			    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+			    git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))
 		return 0;
 
 	if (use_editor) {
@@ -958,7 +959,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		const char *env[2] = { NULL };
 		env[0] =  index;
 		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path(commit_editmsg), NULL, env)) {
+		if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
@@ -966,7 +967,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_commit_hook(use_editor, index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+	    run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
 		return 0;
 	}
 
@@ -1728,7 +1729,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	/* Finally, get the commit message */
 	strbuf_reset(&sb);
-	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
+	if (strbuf_read_file(&sb, git_path_commit_editmsg(), 0) < 0) {
 		int saved_errno = errno;
 		rollback_index_files();
 		die(_("could not read commit message: %s"), strerror(saved_errno));
-- 
2.8.3

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> 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:

It was a rhetorical question ;-)  But "the more important part" was
that initialization by calling non-trivial function is not a good
idea even in C++ where it is allowed, as you said below.

> 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().

Indeed.  Thanks.

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

[Pranit Bauva]

On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> This is a follow up commit for f932729c (memoize common git-path
> "constant" files, 10-Aug-2015).
>
> The many function calls to git_path() are replaced by
> git_path_commit_editmsg() and which thus eliminates the need to repeatedly
> compute the location of "COMMIT_EDITMSG".
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> Link for v1[1].
>
> Changes wrt v1:
>
>  * Remove the call to git_path_commit_editmsg() which would directly assign
>    the value to the string.
>  * Remove the string commit_editmsg[] as it is redundant now.
>  * Call git_path_commit_editmsg() everytime when it is needed.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/295345
>
>  builtin/commit.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 391126e..01b921f 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -92,8 +92,9 @@ 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 struct lock_file index_lock; /* real index */
>  static struct lock_file false_lock; /* used only for partial commits */
>  static enum {
> @@ -771,9 +772,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>                 hook_arg2 = "";
>         }
>
> -       s->fp = fopen_for_writing(git_path(commit_editmsg));
> +       s->fp = fopen_for_writing(git_path_commit_editmsg());
>         if (s->fp == NULL)
> -               die_errno(_("could not open '%s'"), git_path(commit_editmsg));
> +               die_errno(_("could not open '%s'"), git_path_commit_editmsg());
>
>         /* Ignore status.displayCommentPrefix: we do need comments in COMMIT_EDITMSG. */
>         old_display_comment_prefix = s->display_comment_prefix;
> @@ -950,7 +951,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>         }
>
>         if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
> -                           git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
> +                           git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))
>                 return 0;
>
>         if (use_editor) {
> @@ -958,7 +959,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>                 const char *env[2] = { NULL };
>                 env[0] =  index;
>                 snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> -               if (launch_editor(git_path(commit_editmsg), NULL, env)) {
> +               if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
>                         fprintf(stderr,
>                         _("Please supply the message using either -m or -F option.\n"));
>                         exit(1);
> @@ -966,7 +967,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>         }
>
>         if (!no_verify &&
> -           run_commit_hook(use_editor, index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
> +           run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
>                 return 0;
>         }
>
> @@ -1728,7 +1729,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>
>         /* Finally, get the commit message */
>         strbuf_reset(&sb);
> -       if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
> +       if (strbuf_read_file(&sb, git_path_commit_editmsg(), 0) < 0) {
>                 int saved_errno = errno;
>                 rollback_index_files();
>                 die(_("could not read commit message: %s"), strerror(saved_errno));
> --
> 2.8.3
>

Anyone any comments?

Regards,
Pranit Bauva

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

[Jeff King]

On Tue, Jun 07, 2016 at 08:25:17PM +0530, Pranit Bauva wrote:

> On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> > This is a follow up commit for f932729c (memoize common git-path
> > "constant" files, 10-Aug-2015).
> >
> > The many function calls to git_path() are replaced by
> > git_path_commit_editmsg() and which thus eliminates the need to repeatedly
> > compute the location of "COMMIT_EDITMSG".
> >
> > Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> > ---
> [...]
> Anyone any comments?

Looks good to me. You may want to re-post without the quoting to make it
easier for the maintainer to pick up, and feel free to add my:

  Reviewed-by: Jeff King <peff@peff.net>

-Peff

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

[Pranit Bauva]

Hey Jeff,

On Thu, Jun 9, 2016 at 12:28 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 07, 2016 at 08:25:17PM +0530, Pranit Bauva wrote:
>
>> On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> > This is a follow up commit for f932729c (memoize common git-path
>> > "constant" files, 10-Aug-2015).
>> >
>> > The many function calls to git_path() are replaced by
>> > git_path_commit_editmsg() and which thus eliminates the need to repeatedly
>> > compute the location of "COMMIT_EDITMSG".
>> >
>> > Mentored-by: Lars Schneider <larsxschneider@gmail.com>
>> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> > ---
>> [...]
>> Anyone any comments?
>
> Looks good to me. You may want to re-post without the quoting to make it
> easier for the maintainer to pick up, and feel free to add my:
>
>   Reviewed-by: Jeff King <peff@peff.net>

Sure I could re-post it. Thanks for your tag! :)

Regards,
Pranit Bauva

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

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> This is a follow up commit for f932729c (memoize common git-path
>> "constant" files, 10-Aug-2015).
>>
>> The many function calls to git_path() are replaced by
>> git_path_commit_editmsg() and which thus eliminates the need to repeatedly
>> compute the location of "COMMIT_EDITMSG".
>>
>> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
>> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> Link for v1[1].
>> ...
>
> Anyone any comments?

It seems that nobody saw anything that needs further polishing?
Thanks for pinging.  Will queue.