{"thread":{"id":"45041","subject":"[PATCH] commit: Optimize number of lstat() calls","startedAt":"2017-02-03T23:22:47Z","lastAt":"2017-02-04T10:44:00Z","messageCount":6,"participants":["Gumbel, Matthew K","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"310823","messageId":"DA0A42D68346B1469147552440A645039A9C9988@ORSMSX115.amr.corp.intel.com","threadId":"45041","inReplyTo":null,"subject":"[PATCH] commit: Optimize number of lstat() calls","fromName":"Gumbel, Matthew K","fromEmail":"matthew.k.gumbel@intel.com","sentAt":"2017-02-03T23:22:30Z","receivedAt":"2017-02-03T23:22:47Z","isPatch":true,"sender":{"key":"matthew.k.gumbel@intel.com","avatar":"https://gravatar.com/avatar/b890e13658e390e8dbf289304ea863589f6679161577e2bbb16a7626a267950b?d=mp&s=160"},"body":"When making a --only commit, original behavior was to do a full cache\nupdate for the purposes of giving the pre-commit hook an up-to-date set \nof stat data. That would result in long runtime for git-commit in a big \nrepo on NFS (>60s for a 54k-file repo).\n\nWith this change, when doing a --only commit and no pre-commit hook is\npresent, the cache update is skipped since it is known a priori which\nfiles are to be committed.\n\nThis was discussed on the mailing list here:\nhttps://public-inbox.org/git/DA0A42D68346B1469147552440A645039A9C56D4@ORSMSX115.amr.corp.intel.com/\n\nSigned-off-by: Matthew K. Gumbel \n---\nbuiltin/commit.c | 8 ++++++--\n1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2de5f6c..1df3d71 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -470,7 +470,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n\n    hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n    add_remove_files(&partial);\n-   refresh_cache(REFRESH_QUIET);\n+    if (find_hook(\"pre-commit\")) {\n+        refresh_cache(REFRESH_QUIET);\n+    }\n    update_main_cache_tree(WRITE_TREE_SILENT);\n    if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n        die(_(\"unable to write new_index file\"));\n@@ -482,7 +484,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n\n    create_base_index(current_head);\n    add_remove_files(&partial);\n-   refresh_cache(REFRESH_QUIET);\n+    if (find_hook(\"pre-commit\")) {\n+        refresh_cache(REFRESH_QUIET);\n+    }\n\n    if (write_locked_index(&the_index, &false_lock, CLOSE_LOCK))\n        die(_(\"unable to write temporary index file\"));\n-- \n2.8.4\n\n\n"},{"id":"310824","messageId":"xmqqh94adev3.fsf@gitster.mtv.corp.google.com","threadId":"45041","inReplyTo":"DA0A42D68346B1469147552440A645039A9C9988@ORSMSX115.amr.corp.intel.com","subject":"Re: [PATCH] commit: Optimize number of lstat() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-03T23:38:40Z","receivedAt":"2017-02-03T23:38:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Gumbel, Matthew K\" writes:\n\n> When making a --only commit, original behavior was to do a full cache\n> update for the purposes of giving the pre-commit hook an up-to-date set \n> of stat data. That would result in long runtime for git-commit in a big \n> repo on NFS (>60s for a 54k-file repo).\n>\n> With this change, when doing a --only commit and no pre-commit hook is\n> present, the cache update is skipped since it is known a priori which\n> files are to be committed.\n\nDid you determine that \"pre-commit\" hook is the only thing that\nneeds the special case and if so how? With that stated in the log\nmessage, we would feel safe to take this patch.\n\n> Signed-off-by: Matthew K. Gumbel \n> ---\n> builtin/commit.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2de5f6c..1df3d71 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -470,7 +470,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n>\n>     hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n>     add_remove_files(&partial);\n> -   refresh_cache(REFRESH_QUIET);\n> +    if (find_hook(\"pre-commit\")) {\n> +        refresh_cache(REFRESH_QUIET);\n> +    }\n>     update_main_cache_tree(WRITE_TREE_SILENT);\n>     if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n>         die(_(\"unable to write new_index file\"));\n\nI wonder why this patch is full of 0xa0 bytes instead of spaces.\nSomebody between your editor and mailing list is munging your\npatches?\n\n> @@ -482,7 +484,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n>\n>     create_base_index(current_head);\n>     add_remove_files(&partial);\n> -   refresh_cache(REFRESH_QUIET);\n> +    if (find_hook(\"pre-commit\")) {\n> +        refresh_cache(REFRESH_QUIET);\n> +    }\n>\n>     if (write_locked_index(&the_index, &false_lock, CLOSE_LOCK))\n>         die(_(\"unable to write temporary index file\"));\n\nI'd prefer if you introduced a small helper function, name it\ncan_cheat_on_refreshing() or something, and encapsulate the call to\nfind_hook(\"pre-commit\") inside it---if there are other corner cases\nyou didn't think of, or if in the future more hooks are added that\nneed their incoming index file already refreshed, we do not want to\nupdate these two places---rather we'd want to limit the need for\nupdating to the single helper function.\n\n"},{"id":"310838","messageId":"xmqqwpd678lx.fsf@gitster.mtv.corp.google.com","threadId":"45041","inReplyTo":"DA0A42D68346B1469147552440A645039A9C9988@ORSMSX115.amr.corp.intel.com","subject":"Re: [PATCH] commit: Optimize number of lstat() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-04T06:50:18Z","receivedAt":"2017-02-04T06:50:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aside from whitespace breakage, I think we should never skip the\nrefreshing of the real index that is left after \"git commit\"\nfinishes.\n\nBetween these two calls to refresh_cache(), the one that writes out\na temporary index that contains the contents of HEAD plus the\ncontents of the working tree for the specified paths may be fine\nwithout refreshing, unless somebody else (like the pre-commit hook)\nlooks at it. But the other one refreshes the real index file that\nwill be used after \"git commit\" returns the control. Users and\nscripts that run \"git commit\" inside expect that the entries in the\nresulting index are refreshed after \"git commit\" returns, and I do\nnot think of a safe way to optimizing it out; unlike the other one,\nto which we can say \"as long as there is no pre-commit hook, nobody\nwill look at it after we are done\", there does not an easy-to-check\nset of conditions that we can use to decide when it is safe to skip\nrefreshing.\n\nBesides, leaving the main index not refreshed would mean the user\nhas to pay the refreshing cost when s/he runs other commands \"git\ndiff\", \"git status\", etc. after \"git commit\" for the first time;\nso...\n\n"},{"id":"310839","messageId":"alpine.DEB.2.20.1702040129430.3496@virtualbox","threadId":"45041","inReplyTo":"xmqqwpd678lx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: Optimize number of lstat() calls","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-04T00:35:56Z","receivedAt":"2017-02-04T07:15:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 3 Feb 2017, Junio C Hamano wrote:\n\n> Aside from whitespace breakage, I think we should never skip the\n> refreshing of the real index that is left after \"git commit\"\n> finishes.\n> \n> Between these two calls to refresh_cache(), the one that writes out\n> a temporary index that contains the contents of HEAD plus the\n> contents of the working tree for the specified paths may be fine\n> without refreshing, unless somebody else (like the pre-commit hook)\n> looks at it. But the other one refreshes the real index file that\n> will be used after \"git commit\" returns the control. Users and\n> scripts that run \"git commit\" inside expect that the entries in the\n> resulting index are refreshed after \"git commit\" returns, and I do\n> not think of a safe way to optimizing it out; unlike the other one,\n> to which we can say \"as long as there is no pre-commit hook, nobody\n> will look at it after we are done\", there does not an easy-to-check\n> set of conditions that we can use to decide when it is safe to skip\n> refreshing.\n> \n> Besides, leaving the main index not refreshed would mean the user\n> has to pay the refreshing cost when s/he runs other commands \"git\n> diff\", \"git status\", etc. after \"git commit\" for the first time;\n> so...\n\nI am not sure... when you run `git diff` and `git status`, the index is\nrefreshed *anyway*, so with the patch under discussion we would save one\nround of lstat() calls, for the same effect.\n\nI could imagine that there is a third option we should consider, too: only\nlstat() and update the paths that match the pathspec(s) provided on the\ncommand line (this is the semantic meaning of the --only option, after\nall: \"I am only interested in these paths as far as this commit is\nconcerned\"). What do you think?\n\nCiao,\nJohannes\n"},{"id":"310840","messageId":"xmqqd1ey5shc.fsf@gitster.mtv.corp.google.com","threadId":"45041","inReplyTo":"alpine.DEB.2.20.1702040129430.3496@virtualbox","subject":"Re: [PATCH] commit: Optimize number of lstat() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-04T07:23:59Z","receivedAt":"2017-02-04T07:24:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Besides, leaving the main index not refreshed would mean the user\n>> has to pay the refreshing cost when s/he runs other commands \"git\n>> diff\", \"git status\", etc. after \"git commit\" for the first time;\n>> so...\n>\n> I am not sure... when you run `git diff` and `git status`, the index is\n> refreshed *anyway*, so with the patch under discussion we would save one\n> round of lstat() calls, for the same effect.\n\nYeah, you're right. The only ones that could be affected are\nplumbing commands, and scripts that use plumbing are expected to be\nwritten without relying on the \"refreshed\"-ness of the index they\nare given (iow, if they want to rely on, they are expected to refresh\nfirst before using plumbing commands). So there is no downside of\nleaving the index in an unrefreshed state as long as everbody plays\nby the rule.\n\n> I could imagine that there is a third option we should consider, too: only\n> lstat() and update the paths that match the pathspec(s) provided on the\n> command line (this is the semantic meaning of the --only option, after\n> all: \"I am only interested in these paths as far as this commit is\n> concerned\"). What do you think?\n\nI wondered that myself when I read the first message from Matthew\nand noticed that we always refresh the entire index. But if it is\nOK to leave the entire index un-refreshed, that would even be\nsimpler ;-)\n"},{"id":"310843","messageId":"alpine.DEB.2.20.1702041142320.3496@virtualbox","threadId":"45041","inReplyTo":"xmqqd1ey5shc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] commit: Optimize number of lstat() calls","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-04T10:42:56Z","receivedAt":"2017-02-04T10:44:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\n\nOn Fri, 3 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > I could imagine that there is a third option we should consider, too: only\n> > lstat() and update the paths that match the pathspec(s) provided on the\n> > command line (this is the semantic meaning of the --only option, after\n> > all: \"I am only interested in these paths as far as this commit is\n> > concerned\"). What do you think?\n> \n> I wondered that myself when I read the first message from Matthew\n> and noticed that we always refresh the entire index. But if it is\n> OK to leave the entire index un-refreshed, that would even be\n> simpler ;-)\n\nHah! You're right, that would be much simpler ;-)\n\nCiao,\nJohannes\n"}]}