{"thread":{"id":"46562","subject":"[PATCH] commit: skip discarding the index if there is no pre-commit hook","startedAt":"2017-08-10T18:54:26Z","lastAt":"2017-08-15T18:05:52Z","messageCount":8,"participants":["Kevin Willford","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"326067","messageId":"20170810185416.8224-1-kewillf@microsoft.com","threadId":"46562","inReplyTo":null,"subject":"[PATCH] commit: skip discarding the index if there is no pre-commit hook","fromName":"Kevin Willford","fromEmail":"kcwillford@gmail.com","sentAt":"2017-08-10T18:54:16Z","receivedAt":"2017-08-10T18:54:26Z","isPatch":true,"sender":{"key":"kcwillford@gmail.com","avatar":null},"body":"If there is not a pre-commit hook, there is no reason to discard\nthe index and reread it.\n\nThis change checks to presence of a pre-commit hook and then only\ndiscards the index if there was one.\n\nSigned-off-by: Kevin Willford <kewillf@microsoft.com>\n---\n builtin/commit.c | 29 +++++++++++++++++++++--------\n 1 file changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e7a2cb6285..443949d87b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -671,12 +671,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tconst char *hook_arg2 = NULL;\n \tint clean_message_contents = (cleanup_mode != CLEANUP_NONE);\n \tint old_display_comment_prefix;\n+\tconst char *precommit_hook = NULL;\n \n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n-\t\treturn 0;\n+\n+\tif (!no_verify) {\n+\t\t/*\n+\t\t * Check to see if there is a pre-commit hook\n+\t\t * If there not one we can skip discarding the index later on\n+\t\t */\n+\t\tprecommit_hook = find_hook(\"pre-commit\");\n+\t\tif (precommit_hook &&\n+\t\t    run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\t\t\treturn 0;\n+\t}\n \n \tif (squash_message) {\n \t\t/*\n@@ -940,12 +950,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\t/*\n-\t * Re-read the index as pre-commit hook could have updated it,\n-\t * and write it out as a tree.  We must do this before we invoke\n-\t * the editor and after we invoke run_status above.\n-\t */\n-\tdiscard_cache();\n+\tif (!no_verify && precommit_hook) {\n+\t\t/*\n+\t\t * Re-read the index as pre-commit hook could have updated it,\n+\t\t * and write it out as a tree.  We must do this before we invoke\n+\t\t * the editor and after we invoke run_status above.\n+\t\t */\n+\t\tdiscard_cache();\n+\t}\n+\n \tread_cache_from(index_file);\n \tif (update_main_cache_tree(0)) {\n \t\terror(_(\"Error building trees\"));\n-- \n2.14.0.rc0.286.g44127d70e4\n\n"},{"id":"326071","messageId":"20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net","threadId":"46562","inReplyTo":"20170810185416.8224-1-kewillf@microsoft.com","subject":"Re: [PATCH] commit: skip discarding the index if there is no pre-commit hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-10T19:16:13Z","receivedAt":"2017-08-10T19:16:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 10, 2017 at 02:54:16PM -0400, Kevin Willford wrote:\n\n> If there is not a pre-commit hook, there is no reason to discard\n> the index and reread it.\n> \n> This change checks to presence of a pre-commit hook and then only\n> discards the index if there was one.\n\nSounds like a smart optimization.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e7a2cb6285..443949d87b 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -671,12 +671,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \tconst char *hook_arg2 = NULL;\n>  \tint clean_message_contents = (cleanup_mode != CLEANUP_NONE);\n>  \tint old_display_comment_prefix;\n> +\tconst char *precommit_hook = NULL;\n\nThe return value from find_hook() points to storage that may later be\noverwritten or even freed.  I know your patch doesn't actually look at\nthe contents of precommit_hook, but it seems like it's setting up a\npotential bomb for somebody later.\n\nCan we make this:\n\n  int have_precommit_hook = 0;\n  ...\n  have_precommit_hook = !!find_hook(\"pre-commit\");\n\n? Though see below.\n\n> -\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n> -\t\treturn 0;\n> +\n> +\tif (!no_verify) {\n> +\t\t/*\n> +\t\t * Check to see if there is a pre-commit hook\n> +\t\t * If there not one we can skip discarding the index later on\n> +\t\t */\n> +\t\tprecommit_hook = find_hook(\"pre-commit\");\n> +\t\tif (precommit_hook &&\n> +\t\t    run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n> +\t\t\treturn 0;\n> +\t}\n\nWe'll find the hook again in run_commit_hook(), but it's not so\nexpensive that it's worth trying to pass down the hook path we found\n(and if we switch to an integer flag we don't have the path anyway ;) ).\n\nBut note that we don't even care about precommit_hook here. We could\njust call run_commit_hook() regardless. We only care later whether we\nran it or not. So we could drop this hunk and the precommit_hook\nvariable entirely, and...\n\n> @@ -940,12 +950,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\treturn 0;\n>  \t}\n>  \n> -\t/*\n> -\t * Re-read the index as pre-commit hook could have updated it,\n> -\t * and write it out as a tree.  We must do this before we invoke\n> -\t * the editor and after we invoke run_status above.\n> -\t */\n> -\tdiscard_cache();\n> +\tif (!no_verify && precommit_hook) {\n> +\t\t/*\n> +\t\t * Re-read the index as pre-commit hook could have updated it,\n> +\t\t * and write it out as a tree.  We must do this before we invoke\n> +\t\t * the editor and after we invoke run_status above.\n> +\t\t */\n> +\t\tdiscard_cache();\n> +\t}\n\nJust make this:\n\n  if (!no_verify && find_hook(\"pre-commit\"))\n     ... discard cache ...\n\nThat is racy if somebody removes the hook while we're running (so is\nyour patch, but in the opposite direction). What we really want to know\nis \"did we run the hook\" and annoyingly run_hook_ve() doesn't tell us\nthat.  So I think the most robust solution would be refactoring that to\npass out the information, and then use the flag here. But I'm not sure\nit's actually worth worrying about such a race in practice.\n\n-Peff\n"},{"id":"326110","messageId":"xmqqo9rn12rl.fsf@gitster.mtv.corp.google.com","threadId":"46562","inReplyTo":"20170810185416.8224-1-kewillf@microsoft.com","subject":"Re: [PATCH] commit: skip discarding the index if there is no pre-commit hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-10T22:22:54Z","receivedAt":"2017-08-10T22:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford <kcwillford@gmail.com> writes:\n\n> If there is not a pre-commit hook, there is no reason to discard\n> the index and reread it.\n>\n> This change checks to presence of a pre-commit hook and then only\n> discards the index if there was one.\n>\n> Signed-off-by: Kevin Willford <kewillf@microsoft.com>\n> ---\n\nPeff already has done a good job reviewing the patch text, and I\nagree that this is a worthwhile optimization.\n\nCould Microsoft folks all make sure that their signed-off-by lines\nmatch their From: address (or leave an in-body From: to override\nthe From: address your MUA places in your messages)?\n\nThanks.\n\n>  builtin/commit.c | 29 +++++++++++++++++++++--------\n>  1 file changed, 21 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e7a2cb6285..443949d87b 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -671,12 +671,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \tconst char *hook_arg2 = NULL;\n>  \tint clean_message_contents = (cleanup_mode != CLEANUP_NONE);\n>  \tint old_display_comment_prefix;\n> +\tconst char *precommit_hook = NULL;\n>  \n>  \t/* This checks and barfs if author is badly specified */\n>  \tdetermine_author_info(author_ident);\n>  \n> -\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n> -\t\treturn 0;\n> +\n> +\tif (!no_verify) {\n> +\t\t/*\n> +\t\t * Check to see if there is a pre-commit hook\n> +\t\t * If there not one we can skip discarding the index later on\n> +\t\t */\n> +\t\tprecommit_hook = find_hook(\"pre-commit\");\n> +\t\tif (precommit_hook &&\n> +\t\t    run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n> +\t\t\treturn 0;\n> +\t}\n>  \n>  \tif (squash_message) {\n>  \t\t/*\n> @@ -940,12 +950,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\treturn 0;\n>  \t}\n>  \n> -\t/*\n> -\t * Re-read the index as pre-commit hook could have updated it,\n> -\t * and write it out as a tree.  We must do this before we invoke\n> -\t * the editor and after we invoke run_status above.\n> -\t */\n> -\tdiscard_cache();\n> +\tif (!no_verify && precommit_hook) {\n> +\t\t/*\n> +\t\t * Re-read the index as pre-commit hook could have updated it,\n> +\t\t * and write it out as a tree.  We must do this before we invoke\n> +\t\t * the editor and after we invoke run_status above.\n> +\t\t */\n> +\t\tdiscard_cache();\n> +\t}\n> +\n>  \tread_cache_from(index_file);\n>  \tif (update_main_cache_tree(0)) {\n>  \t\terror(_(\"Error building trees\"));\n"},{"id":"326323","messageId":"20170814215425.23784-1-kewillf@microsoft.com","threadId":"46562","inReplyTo":"20170810185416.8224-1-kewillf@microsoft.com","subject":"[PATCH v2] commit: skip discarding the index if there is no pre-commit hook","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-08-14T21:54:25Z","receivedAt":"2017-08-14T21:54:49Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"If there is not a pre-commit hook, there is no reason to discard\nthe index and reread it.\n\nThis change checks to presence of a pre-commit hook and then only\ndiscards the index if there was one.\n\nSigned-off-by: Kevin Willford <kewillf@microsoft.com>\n---\n builtin/commit.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e7a2cb6285..ab71b93518 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -940,12 +940,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\t/*\n-\t * Re-read the index as pre-commit hook could have updated it,\n-\t * and write it out as a tree.  We must do this before we invoke\n-\t * the editor and after we invoke run_status above.\n-\t */\n-\tdiscard_cache();\n+\tif (!no_verify && find_hook(\"pre-commit\")) {\n+\t\t/*\n+\t\t * Re-read the index as pre-commit hook could have updated it,\n+\t\t * and write it out as a tree.  We must do this before we invoke\n+\t\t * the editor and after we invoke run_status above.\n+\t\t */\n+\t\tdiscard_cache();\n+\t}\n+\n \tread_cache_from(index_file);\n \tif (update_main_cache_tree(0)) {\n \t\terror(_(\"Error building trees\"));\n-- \n2.14.1.481.g785c1dc9ae\n\n"},{"id":"326327","messageId":"20170814221309.tg7wizmvx3gtzfc7@sigill.intra.peff.net","threadId":"46562","inReplyTo":"20170814215425.23784-1-kewillf@microsoft.com","subject":"Re: [PATCH v2] commit: skip discarding the index if there is no pre-commit hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-14T22:13:10Z","receivedAt":"2017-08-14T22:13:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 03:54:25PM -0600, Kevin Willford wrote:\n\n> If there is not a pre-commit hook, there is no reason to discard\n> the index and reread it.\n> \n> This change checks to presence of a pre-commit hook and then only\n> discards the index if there was one.\n> \n> Signed-off-by: Kevin Willford <kewillf@microsoft.com>\n> ---\n>  builtin/commit.c | 15 +++++++++------\n>  1 file changed, 9 insertions(+), 6 deletions(-)\n\nThanks, this looks nice and simple.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e7a2cb6285..ab71b93518 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -940,12 +940,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\treturn 0;\n>  \t}\n>  \n> -\t/*\n> -\t * Re-read the index as pre-commit hook could have updated it,\n> -\t * and write it out as a tree.  We must do this before we invoke\n> -\t * the editor and after we invoke run_status above.\n> -\t */\n> -\tdiscard_cache();\n> +\tif (!no_verify && find_hook(\"pre-commit\")) {\n> +\t\t/*\n> +\t\t * Re-read the index as pre-commit hook could have updated it,\n> +\t\t * and write it out as a tree.  We must do this before we invoke\n> +\t\t * the editor and after we invoke run_status above.\n> +\t\t */\n> +\t\tdiscard_cache();\n> +\t}\n> +\n>  \tread_cache_from(index_file);\n\nThis read_cache_from() should be a noop, right, because it immediately\nsees istate->initialized is set? So it shouldn't matter that it is not\nin the conditional with discard_cache(). Though if its only purpose is\nto re-read the just-discarded contents, perhaps it makes sense to put it\nthere for readability.\n\n-Peff\n"},{"id":"326364","messageId":"DM2PR21MB004160EA994A445A89BD50F7B78D0@DM2PR21MB0041.namprd21.prod.outlook.com","threadId":"46562","inReplyTo":"20170814221309.tg7wizmvx3gtzfc7@sigill.intra.peff.net","subject":"RE: [PATCH v2] commit: skip discarding the index if there is no pre-commit hook","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-08-15T04:23:50Z","receivedAt":"2017-08-15T04:23:58Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"> On Mon, Aug 14, 2017 at 03:54:25PM -0600, Kevin Willford wrote:\n> \n> > If there is not a pre-commit hook, there is no reason to discard\n> > the index and reread it.\n> >\n> > This change checks to presence of a pre-commit hook and then only\n> > discards the index if there was one.\n> >\n> > Signed-off-by: Kevin Willford <kewillf@microsoft.com>\n> > ---\n> >  builtin/commit.c | 15 +++++++++------\n> >  1 file changed, 9 insertions(+), 6 deletions(-)\n> \n> Thanks, this looks nice and simple.\n> \n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index e7a2cb6285..ab71b93518 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -940,12 +940,15 @@ static int prepare_to_commit(const char *index_file,\n> const char *prefix,\n> >  \t\treturn 0;\n> >  \t}\n> >\n> > -\t/*\n> > -\t * Re-read the index as pre-commit hook could have updated it,\n> > -\t * and write it out as a tree.  We must do this before we invoke\n> > -\t * the editor and after we invoke run_status above.\n> > -\t */\n> > -\tdiscard_cache();\n> > +\tif (!no_verify && find_hook(\"pre-commit\")) {\n> > +\t\t/*\n> > +\t\t * Re-read the index as pre-commit hook could have updated it,\n> > +\t\t * and write it out as a tree.  We must do this before we invoke\n> > +\t\t * the editor and after we invoke run_status above.\n> > +\t\t */\n> > +\t\tdiscard_cache();\n> > +\t}\n> > +\n> >  \tread_cache_from(index_file);\n> \n> This read_cache_from() should be a noop, right, because it immediately\n> sees istate->initialized is set? So it shouldn't matter that it is not\n> in the conditional with discard_cache(). Though if its only purpose is\n> to re-read the just-discarded contents, perhaps it makes sense to put it\n> there for readability.\n> \n> -Peff\n\nI thought about that and didn't know if there were cases when this would be called\nand the cache has not been loaded.  It didn't look like it since it is only called from \ncmd_commit and prepare_index is called before it.  Also if in the future this call would\nbe made when it had not read the index yet so thought it was safest just to leave\nthis as always being called since it is basically a noop if the istate->initialized is set.\n\nAlso based on this commit\nhttps://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48\nit looked like the discard_cache was added independent of the read_cache_from call,\nwhich made me think that the two were not tied together.\n\nKevin\n"},{"id":"326365","messageId":"20170815045313.njpq5wf7iuredhoe@sigill.intra.peff.net","threadId":"46562","inReplyTo":"DM2PR21MB004160EA994A445A89BD50F7B78D0@DM2PR21MB0041.namprd21.prod.outlook.com","subject":"Re: [PATCH v2] commit: skip discarding the index if there is no pre-commit hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-15T04:53:14Z","receivedAt":"2017-08-15T04:53:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 15, 2017 at 04:23:50AM +0000, Kevin Willford wrote:\n\n> > This read_cache_from() should be a noop, right, because it immediately\n> > sees istate->initialized is set? So it shouldn't matter that it is not\n> > in the conditional with discard_cache(). Though if its only purpose is\n> > to re-read the just-discarded contents, perhaps it makes sense to put it\n> > there for readability.\n> \n> I thought about that and didn't know if there were cases when this would be called\n> and the cache has not been loaded.  It didn't look like it since it is only called from \n> cmd_commit and prepare_index is called before it.  Also if in the future this call would\n> be made when it had not read the index yet so thought it was safest just to leave\n> this as always being called since it is basically a noop if the istate->initialized is set.\n\nYeah, I agree it might be safer as you have it. And hopefully the\ncomment above the discard would point anybody in the right direction.\n\n> Also based on this commit\n> https://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48\n> it looked like the discard_cache was added independent of the read_cache_from call,\n> which made me think that the two were not tied together.\n\nYeah, I tried to dig in the history and figure it out, but it was not\nnearly as clear as I would have liked. :)\n\n-Peff\n"},{"id":"326424","messageId":"xmqq60doso3x.fsf@gitster.mtv.corp.google.com","threadId":"46562","inReplyTo":"20170815045313.njpq5wf7iuredhoe@sigill.intra.peff.net","subject":"Re: [PATCH v2] commit: skip discarding the index if there is no pre-commit hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T18:05:38Z","receivedAt":"2017-08-15T18:05:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Aug 15, 2017 at 04:23:50AM +0000, Kevin Willford wrote:\n>\n>> > This read_cache_from() should be a noop, right, because it immediately\n>> > sees istate->initialized is set? So it shouldn't matter that it is not\n>> > in the conditional with discard_cache(). Though if its only purpose is\n>> > to re-read the just-discarded contents, perhaps it makes sense to put it\n>> > there for readability.\n>> \n>> I thought about that and didn't know if there were cases when this would be called\n>> and the cache has not been loaded.  It didn't look like it since it is only called from \n>> cmd_commit and prepare_index is called before it.  Also if in the future this call would\n>> be made when it had not read the index yet so thought it was safest just to leave\n>> this as always being called since it is basically a noop if the istate->initialized is set.\n>\n> Yeah, I agree it might be safer as you have it. And hopefully the\n> comment above the discard would point anybody in the right direction.\n\nI agree that it would not hurt to have it outside the conditional,\nbecause read_cache_from() is a no-op when it is already populated.\nI however do not think it is sensible to call prepare_to_commit()\nwithout having populated the in-core index---in the function, nobody\nconditionally reads the in-core index, expecting that the caller\nmight not have prepared the in-core index, when we need to do the\nwt-status thing, for example.\n\n>> Also based on this commit\n>> https://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48\n>> it looked like the discard_cache was added independent of the read_cache_from call,\n>> which made me think that the two were not tied together.\n\nThat one comes from the first commit of the C reimplementation,\nf5bbc322 (\"Port git commit to C.\", 2007-11-08).\n\nIf I am reading the old code correctly, read_cache_from() is called\nfor entirely different reasons.  Back in the old code, when doing\n\"commit --patch\", prepare_index() called interactive_add(), which\nwould return to us after updating the index file in the filesystem.\nThe caller of prepare_index(), which was cmd_commit(), needed to\nread that off of the filesystem hence the call is there.\n\nWhen not doing \"--patch\", prepare_index() called read_cache(), so\nthe read_cache_from() there was a no-op.\n\n2888605c (\"builtin-commit: fix partial-commit support\", 2007-11-18)\nwas to fix the mistake of not re-reading the index from the file at\nthat point for non \"--patch\" cases.  Having read_cache_from() that\nis most of the time no-op was not sufficient and needed additional\ndiscard_cache() there.\n"}]}