{"thread":{"id":"42422","subject":"[PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","startedAt":"2016-05-23T18:16:30Z","lastAt":"2016-06-16T02:19:50Z","messageCount":12,"participants":["Pranit Bauva","Junio C Hamano","Matthieu Moy","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287266","messageId":"1464027390-1512-1-git-send-email-pranit.bauva@gmail.com","threadId":"42422","inReplyTo":null,"subject":"[PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-23T18:16:30Z","receivedAt":"2016-05-23T18:16:30Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"This is a follow up commit for f932729c (memoize common git-path\n\"constant\" files, 10-Aug-2015).\n\nIt serves two purposes:\n 1. It reduces the number of calls to git_path() .\n\n 2. It serves the benefits of using GIT_PATH_FUNC as mentioned in the\n commit message of f932729c.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/commit.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 391126e..ffa242c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -92,8 +92,10 @@ N_(\"If you wish to skip this commit, use:\\n\"\n \"Then \\\"git cherry-pick --continue\\\" will resume cherry-picking\\n\"\n \"the remaining commits.\\n\");\n \n+static GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n+\n static const char *use_message_buffer;\n-static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n+static const char commit_editmsg_path[] = git_path_commit_editmsg();\n static struct lock_file index_lock; /* real index */\n static struct lock_file false_lock; /* used only for partial commits */\n static enum {\n@@ -771,9 +773,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\thook_arg2 = \"\";\n \t}\n \n-\ts->fp = fopen_for_writing(git_path(commit_editmsg));\n+\ts->fp = fopen_for_writing(commit_editmsg_path);\n \tif (s->fp == NULL)\n-\t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n+\t\tdie_errno(_(\"could not open '%s'\"), commit_editmsg_path);\n \n \t/* Ignore status.displayCommentPrefix: we do need comments in COMMIT_EDITMSG. */\n \told_display_comment_prefix = s->display_comment_prefix;\n@@ -950,7 +952,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n-\t\t\t git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))\n+\t\t\t commit_editmsg_path, hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n \tif (use_editor) {\n@@ -958,7 +960,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tconst char *env[2] = { NULL };\n \t\tenv[0] = index;\n \t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path(commit_editmsg), NULL, env)) {\n+\t\tif (launch_editor(commit_editmsg_path, NULL, env)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n \t\t\texit(1);\n@@ -966,7 +968,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t run_commit_hook(use_editor, index_file, \"commit-msg\", git_path(commit_editmsg), NULL)) {\n+\t run_commit_hook(use_editor, index_file, \"commit-msg\", commit_editmsg_path, NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1728,7 +1730,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t/* Finally, get the commit message */\n \tstrbuf_reset(&sb);\n-\tif (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {\n+\tif (strbuf_read_file(&sb, commit_editmsg_path, 0) < 0) {\n \t\tint saved_errno = errno;\n \t\trollback_index_files();\n \t\tdie(_(\"could not read commit message: %s\"), strerror(saved_errno));\n-- \n2.8.2\n"},{"id":"287273","messageId":"xmqq7feka8kk.fsf@gitster.mtv.corp.google.com","threadId":"42422","inReplyTo":"1464027390-1512-1-git-send-email-pranit.bauva@gmail.com","subject":"Re: [PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-23T19:16:43Z","receivedAt":"2016-05-23T19:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> This is a follow up commit for f932729c (memoize common git-path\n> \"constant\" files, 10-Aug-2015).\n>\n> It serves two purposes:\n> 1. It reduces the number of calls to git_path() .\n>\n> 2. It serves the benefits of using GIT_PATH_FUNC as mentioned in the\n> commit message of f932729c.\n\nAll of that is a good idea, but I have huge doubts about its use.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 391126e..ffa242c 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -92,8 +92,10 @@ N_(\"If you wish to skip this commit, use:\\n\"\n> \"Then \\\"git cherry-pick --continue\\\" will resume cherry-picking\\n\"\n> \"the remaining commits.\\n\");\n> \n> +static GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n> +\n> static const char *use_message_buffer;\n> -static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n> +static const char commit_editmsg_path[] = git_path_commit_editmsg();\n\nThe function defined with the macro looks like\n\n\tconst char *git_path_commit_editmsg(void)\n {\n\t\tstatic char *ret;\n if (!ret)\n \tret = git_pathdup(\"COMMIT_EDITMSG\");\n\t\treturn ret;\n\t}\n\nso receiving its result to \"const char v[]\" looks somewhat\nsuspicious.\n\nMore importantly, when is this function evaluated and returned value\nused to fill commit_editmsg_path[]? In order for git_pathdup() to\nproduce a meaningful result, it needs to know where .git/ directory\nis, which (roughly) means setup_git_dir() must have been called from\na callchain from main() somewhere already.\n\nBut I do not think the linker knows that fact.\n\n> @@ -771,9 +773,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\thook_arg2 = \"\";\n> \t}\n> \n\nInstead, what you could do is to call git_path_commit_editmsg() when\nyou refer to that global variable whose initialization is suspect.\n\n> -\ts->fp = fopen_for_writing(git_path(commit_editmsg));\n> +\ts->fp = fopen_for_writing(commit_editmsg_path);\n\ni.e.\n\n\ts->fp = fopen_for_writing(git_path_commit_editmsg());\n\nAs you can see in its definition, when the original code used to\ncall git_path(), it is safe to call git_path_commit_editmsg(),\nbecause for the original git_path() to be correct, the code should\nalready have established where $GIT_DIR is, so it is safe to call\ngit_pathdup(), too. Also, as you can see in its definition, calling\nthe function many times would not cause git_path() called many\ntimes. The first invocation will keep its value that is constant\nwithin the program that works with a constant $GIT_DIR.\n\nAnd you do not free its return value.\n"},{"id":"287338","messageId":"CAFZEwPN3L5Y-7wNj6TMjg-jPb_oDQYjukBj1uL6OJ8rWAoqjcQ@mail.gmail.com","threadId":"42422","inReplyTo":"xmqq7feka8kk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-24T05:54:15Z","receivedAt":"2016-05-24T05:54:15Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Tue, May 24, 2016 at 12:46 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> This is a follow up commit for f932729c (memoize common git-path\n>> \"constant\" files, 10-Aug-2015).\n>>\n>> It serves two purposes:\n>> 1. It reduces the number of calls to git_path() .\n>>\n>> 2. It serves the benefits of using GIT_PATH_FUNC as mentioned in the\n>> commit message of f932729c.\n>\n> All of that is a good idea, but I have huge doubts about its use.\n>\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 391126e..ffa242c 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -92,8 +92,10 @@ N_(\"If you wish to skip this commit, use:\\n\"\n>> \"Then \\\"git cherry-pick --continue\\\" will resume cherry-picking\\n\"\n>> \"the remaining commits.\\n\");\n>>\n>> +static GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n>> +\n>> static const char *use_message_buffer;\n>> -static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n>> +static const char commit_editmsg_path[] = git_path_commit_editmsg();\n>\n> The function defined with the macro looks like\n>\n> const char *git_path_commit_editmsg(void)\n> {\n> static char *ret;\n> if (!ret)\n> ret = git_pathdup(\"COMMIT_EDITMSG\");\n> return ret;\n> }\n>\n> so receiving its result to \"const char v[]\" looks somewhat\n> suspicious.\n>\n> More importantly, when is this function evaluated and returned value\n> used to fill commit_editmsg_path[]? In order for git_pathdup() to\n> produce a meaningful result, it needs to know where .git/ directory\n> is, which (roughly) means setup_git_dir() must have been called from\n> a callchain from main() somewhere already.\n>\n> But I do not think the linker knows that fact.\n\nI think otherwise. git_pathdup() calls get_worktree_git_dir() which\ncalls get_git_dir() which if uninitialized calls setup_git_env(). So\ntechnically the code gets to know the .git/ directory quite early.\nThough I am not very sure whether this one is a desirable fact. There\nwould be later instances which would in turn call to know where the\n.git/ directory.\n\n>\n>> @@ -771,9 +773,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>> hook_arg2 = \"\";\n>> }\n>>\n>\n> Instead, what you could do is to call git_path_commit_editmsg() when\n> you refer to that global variable whose initialization is suspect.\n>\n>> - s->fp = fopen_for_writing(git_path(commit_editmsg));\n>> + s->fp = fopen_for_writing(commit_editmsg_path);\n>\n> i.e.\n>\n> s->fp = fopen_for_writing(git_path_commit_editmsg());\n>\n> As you can see in its definition, when the original code used to\n> call git_path(), it is safe to call git_path_commit_editmsg(),\n> because for the original git_path() to be correct, the code should\n> already have established where $GIT_DIR is, so it is safe to call\n> git_pathdup(), too. Also, as you can see in its definition, calling\n> the function many times would not cause git_path() called many\n> times. The first invocation will keep its value that is constant\n> within the program that works with a constant $GIT_DIR.\n\nI agree that it is actually not required to again compute the location\nof .git/ directory and can only return the value.\n\nOverall I agree to your idea of just using git_path_commit_editmsg()\ninstead of git_path() so as to not disturb any previous\nimplementations which can lead to some complications. Also if I am\nchanging some internal semantics there should be a valid reason which\nthere isn't really as I don't see any benefit in getting the location\nof .git/ early in the program.\n\n> And you do not free its return value.\nThis is one of the thing that bugging me with GIT_PATH_FUNC. Wouldn't\nnot freeing the memory lead to memory leaks?\n\nRegards,\nPranit Bauva\n"},{"id":"287340","messageId":"CAFZEwPN26xyrb1EGrNebW5ybn0OY066+dszvuzLJ54opToz5qg@mail.gmail.com","threadId":"42422","inReplyTo":"CAFZEwPN3L5Y-7wNj6TMjg-jPb_oDQYjukBj1uL6OJ8rWAoqjcQ@mail.gmail.com","subject":"Re: [PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-24T06:35:48Z","receivedAt":"2016-05-24T06:35:48Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio\n\nOn Tue, May 24, 2016 at 11:24 AM, Pranit Bauva wrote:\n>> And you do not free its return value.\n> This is one of the thing that bugging me with GIT_PATH_FUNC. Wouldn't\n> not freeing the memory lead to memory leaks?\n\nSlight misunderstanding. I got it now. Thanks!\n\n> Regards,\n> Pranit Bauva\n"},{"id":"287379","messageId":"vpq1t4rri2a.fsf@anie.imag.fr","threadId":"42422","inReplyTo":"xmqq7feka8kk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-24T08:11:57Z","receivedAt":"2016-05-24T08:11:57Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Pranit Bauva writes:\n>\n>> static const char *use_message_buffer;\n>> -static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n>> +static const char commit_editmsg_path[] = git_path_commit_editmsg();\n>\n> The function defined with the macro looks like\n>\n> \tconst char *git_path_commit_editmsg(void)\n> {\n> \t\tstatic char *ret;\n> if (!ret)\n> \tret = git_pathdup(\"COMMIT_EDITMSG\");\n> \t\treturn ret;\n> \t}\n>\n> so receiving its result to \"const char v[]\" looks somewhat\n> suspicious.\n>\n> More importantly, when is this function evaluated and returned value\n> used to fill commit_editmsg_path[]?\n\nI may have missed something, but I'd say \"never\", as the code is not\ncompilable at least with my gcc:\n\nbuiltin/commit.c:98:1: error: invalid initializer\n static const char commit_editmsg_path[] = git_path_commit_editmsg();\n ^\n\nAFAIK, initializing a global variable with a function call is allowed in\nC++, but not in C.\n\nAnd indeed, this construct is a huge source of trouble, as it would mean\nthat git_path_commit_editmsg() is called 1) unconditionnally, and 2)\nbefore entering main().\n\n1) means that the function call is made even when git is called for\nanother command. This is terrible for the startup time: if all git\ncommands have a not-totally-immediate initializer, then all commands\nwould need to run the initializers for all other commands. 2) means it's\na nightmare to debug, as you can hardly predict when the code will be\nexecuted.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287409","messageId":"CAFZEwPMmMEQMuT0+ap00e5UgccDfYZ75qbovRL_d8qWZ0_2XWw@mail.gmail.com","threadId":"42422","inReplyTo":"vpq1t4rri2a.fsf@anie.imag.fr","subject":"Re: [PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-24T11:41:20Z","receivedAt":"2016-05-24T11:41:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Matthieu,\n\nOn Tue, May 24, 2016 at 1:41 PM, Matthieu Moy\n wrote:\n> Junio C Hamano writes:\n>\n>> Pranit Bauva writes:\n>>\n>>> static const char *use_message_buffer;\n>>> -static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n>>> +static const char commit_editmsg_path[] = git_path_commit_editmsg();\n>>\n>> The function defined with the macro looks like\n>>\n>> const char *git_path_commit_editmsg(void)\n>> {\n>> static char *ret;\n>> if (!ret)\n>> ret = git_pathdup(\"COMMIT_EDITMSG\");\n>> return ret;\n>> }\n>>\n>> so receiving its result to \"const char v[]\" looks somewhat\n>> suspicious.\n>>\n>> More importantly, when is this function evaluated and returned value\n>> used to fill commit_editmsg_path[]?\n>\n> I may have missed something, but I'd say \"never\", as the code is not\n> compilable at least with my gcc:\n>\n> builtin/commit.c:98:1: error: invalid initializer\n> static const char commit_editmsg_path[] = git_path_commit_editmsg();\n> ^\n>\n> AFAIK, initializing a global variable with a function call is allowed in\n> C++, but not in C.\n\nI wasn't aware of this fact. Thanks.\n\n> And indeed, this construct is a huge source of trouble, as it would mean\n> that git_path_commit_editmsg() is called 1) unconditionnally, and 2)\n> before entering main().\n>\n> 1) means that the function call is made even when git is called for\n> another command. This is terrible for the startup time: if all git\n> commands have a not-totally-immediate initializer, then all commands\n> would need to run the initializers for all other commands. 2) means it's\n> a nightmare to debug, as you can hardly predict when the code will be\n> executed.\n\nYes I agree that this approach will definitely cause a lot of\nproblems. I will re-roll with how Junio suggested.\n\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"287445","messageId":"20160524191950.21889-1-pranit.bauva@gmail.com","threadId":"42422","inReplyTo":"1464027390-1512-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v2] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-24T19:19:50Z","receivedAt":"2016-05-24T19:19:50Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"This is a follow up commit for f932729c (memoize common git-path\n\"constant\" files, 10-Aug-2015).\n\nThe many function calls to git_path() are replaced by\ngit_path_commit_editmsg() and which thus eliminates the need to repeatedly\ncompute the location of \"COMMIT_EDITMSG\".\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\nLink for v1[1].\n\nChanges wrt v1:\n\n * Remove the call to git_path_commit_editmsg() which would directly assign\n the value to the string.\n * Remove the string commit_editmsg[] as it is redundant now.\n * Call git_path_commit_editmsg() everytime when it is needed.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/295345\n\n builtin/commit.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 391126e..01b921f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -92,8 +92,9 @@ N_(\"If you wish to skip this commit, use:\\n\"\n \"Then \\\"git cherry-pick --continue\\\" will resume cherry-picking\\n\"\n \"the remaining commits.\\n\");\n \n+static GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n+\n static const char *use_message_buffer;\n-static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n static struct lock_file index_lock; /* real index */\n static struct lock_file false_lock; /* used only for partial commits */\n static enum {\n@@ -771,9 +772,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\thook_arg2 = \"\";\n \t}\n \n-\ts->fp = fopen_for_writing(git_path(commit_editmsg));\n+\ts->fp = fopen_for_writing(git_path_commit_editmsg());\n \tif (s->fp == NULL)\n-\t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n+\t\tdie_errno(_(\"could not open '%s'\"), git_path_commit_editmsg());\n \n \t/* Ignore status.displayCommentPrefix: we do need comments in COMMIT_EDITMSG. */\n \told_display_comment_prefix = s->display_comment_prefix;\n@@ -950,7 +951,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n-\t\t\t git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))\n+\t\t\t git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n \tif (use_editor) {\n@@ -958,7 +959,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tconst char *env[2] = { NULL };\n \t\tenv[0] = index;\n \t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path(commit_editmsg), NULL, env)) {\n+\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n \t\t\texit(1);\n@@ -966,7 +967,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t run_commit_hook(use_editor, index_file, \"commit-msg\", git_path(commit_editmsg), NULL)) {\n+\t run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1728,7 +1729,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t/* Finally, get the commit message */\n \tstrbuf_reset(&sb);\n-\tif (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {\n+\tif (strbuf_read_file(&sb, git_path_commit_editmsg(), 0) < 0) {\n \t\tint saved_errno = errno;\n \t\trollback_index_files();\n \t\tdie(_(\"could not read commit message: %s\"), strerror(saved_errno));\n-- \n2.8.3\n"},{"id":"287459","messageId":"xmqqy46z2iwz.fsf@gitster.mtv.corp.google.com","threadId":"42422","inReplyTo":"vpq1t4rri2a.fsf@anie.imag.fr","subject":"Re: [PATCH] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-24T22:25:00Z","receivedAt":"2016-05-24T22:25:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>> More importantly, when is this function evaluated and returned value\n>> used to fill commit_editmsg_path[]?\n>\n> I may have missed something, but I'd say \"never\", as the code is not\n> compilable at least with my gcc:\n\nIt was a rhetorical question ;-) But \"the more important part\" was\nthat initialization by calling non-trivial function is not a good\nidea even in C++ where it is allowed, as you said below.\n\n> And indeed, this construct is a huge source of trouble, as it would mean\n> that git_path_commit_editmsg() is called 1) unconditionnally, and 2)\n> before entering main().\n\nIndeed. Thanks.\n"},{"id":"288609","messageId":"CAFZEwPOZSU315oCJSdawtacPmgZobCnkkguTnSy1_V7x_n09kw@mail.gmail.com","threadId":"42422","inReplyTo":"20160524191950.21889-1-pranit.bauva@gmail.com","subject":"Re: [PATCH v2] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-06-07T14:55:17Z","receivedAt":"2016-06-07T14:55:17Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva wrote:\n> This is a follow up commit for f932729c (memoize common git-path\n> \"constant\" files, 10-Aug-2015).\n>\n> The many function calls to git_path() are replaced by\n> git_path_commit_editmsg() and which thus eliminates the need to repeatedly\n> compute the location of \"COMMIT_EDITMSG\".\n>\n> Mentored-by: Lars Schneider \n> Mentored-by: Christian Couder \n> Signed-off-by: Pranit Bauva \n> ---\n> Link for v1[1].\n>\n> Changes wrt v1:\n>\n> * Remove the call to git_path_commit_editmsg() which would directly assign\n> the value to the string.\n> * Remove the string commit_editmsg[] as it is redundant now.\n> * Call git_path_commit_editmsg() everytime when it is needed.\n>\n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/295345\n>\n> builtin/commit.c | 15 ++++++++-------\n> 1 file changed, 8 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 391126e..01b921f 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -92,8 +92,9 @@ N_(\"If you wish to skip this commit, use:\\n\"\n> \"Then \\\"git cherry-pick --continue\\\" will resume cherry-picking\\n\"\n> \"the remaining commits.\\n\");\n>\n> +static GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n> +\n> static const char *use_message_buffer;\n> -static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n> static struct lock_file index_lock; /* real index */\n> static struct lock_file false_lock; /* used only for partial commits */\n> static enum {\n> @@ -771,9 +772,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> hook_arg2 = \"\";\n> }\n>\n> - s->fp = fopen_for_writing(git_path(commit_editmsg));\n> + s->fp = fopen_for_writing(git_path_commit_editmsg());\n> if (s->fp == NULL)\n> - die_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n> + die_errno(_(\"could not open '%s'\"), git_path_commit_editmsg());\n>\n> /* Ignore status.displayCommentPrefix: we do need comments in COMMIT_EDITMSG. */\n> old_display_comment_prefix = s->display_comment_prefix;\n> @@ -950,7 +951,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> }\n>\n> if (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n> - git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))\n> + git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n> return 0;\n>\n> if (use_editor) {\n> @@ -958,7 +959,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> const char *env[2] = { NULL };\n> env[0] = index;\n> snprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n> - if (launch_editor(git_path(commit_editmsg), NULL, env)) {\n> + if (launch_editor(git_path_commit_editmsg(), NULL, env)) {\n> fprintf(stderr,\n> _(\"Please supply the message using either -m or -F option.\\n\"));\n> exit(1);\n> @@ -966,7 +967,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> }\n>\n> if (!no_verify &&\n> - run_commit_hook(use_editor, index_file, \"commit-msg\", git_path(commit_editmsg), NULL)) {\n> + run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n> return 0;\n> }\n>\n> @@ -1728,7 +1729,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>\n> /* Finally, get the commit message */\n> strbuf_reset(&sb);\n> - if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {\n> + if (strbuf_read_file(&sb, git_path_commit_editmsg(), 0) < 0) {\n> int saved_errno = errno;\n> rollback_index_files();\n> die(_(\"could not read commit message: %s\"), strerror(saved_errno));\n> --\n> 2.8.3\n>\n\nAnyone any comments?\n\nRegards,\nPranit Bauva\n"},{"id":"288795","messageId":"20160609065805.GA19015@sigill.intra.peff.net","threadId":"42422","inReplyTo":"CAFZEwPOZSU315oCJSdawtacPmgZobCnkkguTnSy1_V7x_n09kw@mail.gmail.com","subject":"Re: [PATCH v2] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-09T06:58:06Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 08:25:17PM +0530, Pranit Bauva wrote:\n\n> On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva wrote:\n> > This is a follow up commit for f932729c (memoize common git-path\n> > \"constant\" files, 10-Aug-2015).\n> >\n> > The many function calls to git_path() are replaced by\n> > git_path_commit_editmsg() and which thus eliminates the need to repeatedly\n> > compute the location of \"COMMIT_EDITMSG\".\n> >\n> > Mentored-by: Lars Schneider \n> > Mentored-by: Christian Couder \n> > Signed-off-by: Pranit Bauva \n> > ---\n> [...]\n> Anyone any comments?\n\nLooks good to me. You may want to re-post without the quoting to make it\neasier for the maintainer to pick up, and feel free to add my:\n\n Reviewed-by: Jeff King \n\n-Peff\n"},{"id":"288799","messageId":"CAFZEwPPRirGKqA4=qY+TrSmkGomZVZjLqOG-ZKwciK8hLhhdHg@mail.gmail.com","threadId":"42422","inReplyTo":"20160609065805.GA19015@sigill.intra.peff.net","subject":"Re: [PATCH v2] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-06-09T09:54:54Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Jeff,\n\nOn Thu, Jun 9, 2016 at 12:28 PM, Jeff King wrote:\n> On Tue, Jun 07, 2016 at 08:25:17PM +0530, Pranit Bauva wrote:\n>\n>> On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva wrote:\n>> > This is a follow up commit for f932729c (memoize common git-path\n>> > \"constant\" files, 10-Aug-2015).\n>> >\n>> > The many function calls to git_path() are replaced by\n>> > git_path_commit_editmsg() and which thus eliminates the need to repeatedly\n>> > compute the location of \"COMMIT_EDITMSG\".\n>> >\n>> > Mentored-by: Lars Schneider \n>> > Mentored-by: Christian Couder \n>> > Signed-off-by: Pranit Bauva \n>> > ---\n>> [...]\n>> Anyone any comments?\n>\n> Looks good to me. You may want to re-post without the quoting to make it\n> easier for the maintainer to pick up, and feel free to add my:\n>\n> Reviewed-by: Jeff King \n\nSure I could re-post it. Thanks for your tag! :)\n\nRegards,\nPranit Bauva\n"},{"id":"288824","messageId":"xmqqinxii994.fsf@gitster.mtv.corp.google.com","threadId":"42422","inReplyTo":"CAFZEwPOZSU315oCJSdawtacPmgZobCnkkguTnSy1_V7x_n09kw@mail.gmail.com","subject":"Re: [PATCH v2] builtin/commit.c: memoize git-path for COMMIT_EDITMSG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T17:04:39Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> On Wed, May 25, 2016 at 12:49 AM, Pranit Bauva wrote:\n>> This is a follow up commit for f932729c (memoize common git-path\n>> \"constant\" files, 10-Aug-2015).\n>>\n>> The many function calls to git_path() are replaced by\n>> git_path_commit_editmsg() and which thus eliminates the need to repeatedly\n>> compute the location of \"COMMIT_EDITMSG\".\n>>\n>> Mentored-by: Lars Schneider \n>> Mentored-by: Christian Couder \n>> Signed-off-by: Pranit Bauva \n>> ---\n>> Link for v1[1].\n>> ...\n>\n> Anyone any comments?\n\nIt seems that nobody saw anything that needs further polishing?\nThanks for pinging. Will queue.\n"}]}