threads / patch / 42422

patchbuiltin/commit.c: memoize git-path for COMMIT_EDITMSG

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

## tl;dr

12 messages between May 23, 2016 and Jun 16, 2016. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Pranit Bauva· May 23, 2016, 18:16 UTC · lore

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(-)
Show changes to builtin/commit.c +9 −7
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
Junio C Hamano· May 23, 2016, 19:16 UTC · re: Pranit Bauva · lore

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

Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 8 quoted lines
> 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.
Show 13 quoted lines
> 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.
Pranit Bauva· May 24, 2016, 05:54 UTC · re: Junio C Hamano · lore

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

Hey Junio,
On Tue, May 24, 2016 at 12:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 47 quoted lines
> 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.

Show 24 quoted lines
>
>> @@ -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

Pranit Bauva· May 24, 2016, 06:35 UTC · re: Pranit Bauva · lore

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

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
Matthieu Moy· May 24, 2016, 08:11 UTC · re: Junio C Hamano · lore

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

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/
Pranit Bauva· May 24, 2016, 11:41 UTC · re: Matthieu Moy · lore

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

Hey Matthieu,

On Tue, May 24, 2016 at 1:41 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 33 quoted lines
> 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.
Show 10 quoted lines
> 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.

Show 7 quoted lines
> --
> 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
Junio C Hamano· May 24, 2016, 22:25 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 5 quoted lines
>> 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.
Pranit Bauva· May 24, 2016, 19:19 UTC · re: Pranit Bauva · lore

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

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(-)
Show changes to builtin/commit.c +8 −7
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
Pranit Bauva· Jun 7, 2016, 14:55 UTC · re: Pranit Bauva · lore

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

On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 91 quoted lines
> 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

Jeff King· Jun 9, 2016, 06:58 UTC · re: Pranit Bauva · lore

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

On Tue, Jun 07, 2016 at 08:25:17PM +0530, Pranit Bauva wrote:
Show 14 quoted lines
> 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
Pranit Bauva· Jun 9, 2016, 09:54 UTC · re: Jeff King · lore

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

Hey Jeff,
On Thu, Jun 9, 2016 at 12:28 PM, Jeff King <peff@peff.net> wrote:
Show 21 quoted lines
> 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

Junio C Hamano· Jun 9, 2016, 17:04 UTC · re: Pranit Bauva · lore

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

Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 16 quoted lines
> 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.

← back to recent threads