{"thread":{"id":"63720","subject":"[PATCH] read-cache: report lock error when refreshing index","startedAt":"2025-07-01T11:57:27Z","lastAt":"2025-07-08T02:47:53Z","messageCount":6,"participants":["Han Young","Justin Tobler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"521018","messageId":"20250701115719.85226-1-hanyang.tony@bytedance.com","threadId":"63720","inReplyTo":null,"subject":"[PATCH] read-cache: report lock error when refreshing index","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-07-01T11:57:19Z","receivedAt":"2025-07-01T11:57:27Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In the repo_refresh_and_write_index of read-cache.c, we return -1 to\nindicate that writing the index to disk failed.\nHowever, callers do not use this information. Commands such as stash print\n \"could not write index\"\nand then exit, which does not help to discover the exact problem.\n\nWe can let repo_hold_locked_index print the error message if the locking\nfailed.\n\nSigned-off-by: Han Young \n---\n read-cache.c | 2 +-\n t/t3903-stash.sh | 15 +++------------\n 2 files changed, 4 insertions(+), 13 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex c0bb760ad..50e842bfa 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1456,7 +1456,7 @@ int repo_refresh_and_write_index(struct repository *repo,\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint fd, ret = 0;\n \n-\tfd = repo_hold_locked_index(repo, &lock_file, 0);\n+\tfd = repo_hold_locked_index(repo, &lock_file, gentle ? 0 : LOCK_REPORT_ON_ERROR);\n \tif (!gentle && fd < 0)\n \t\treturn -1;\n \tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 35b85c790..39098ade4 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1571,11 +1571,8 @@ test_expect_success 'stash create reports a locked index' '\n \t\techo change >A.file &&\n \t\ttouch .git/index.lock &&\n \n-\t\tcat >expect <<-EOF &&\n-\t\terror: could not write index\n-\t\tEOF\n \t\ttest_must_fail git stash create 2>err &&\n-\t\ttest_cmp expect err\n+\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n \t)\n '\n \n@@ -1588,11 +1585,8 @@ test_expect_success 'stash push reports a locked index' '\n \t\techo change >A.file &&\n \t\ttouch .git/index.lock &&\n \n-\t\tcat >expect <<-EOF &&\n-\t\terror: could not write index\n-\t\tEOF\n \t\ttest_must_fail git stash push 2>err &&\n-\t\ttest_cmp expect err\n+\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n \t)\n '\n \n@@ -1606,11 +1600,8 @@ test_expect_success 'stash apply reports a locked index' '\n \t\tgit stash push &&\n \t\ttouch .git/index.lock &&\n \n-\t\tcat >expect <<-EOF &&\n-\t\terror: could not write index\n-\t\tEOF\n \t\ttest_must_fail git stash apply 2>err &&\n-\t\ttest_cmp expect err\n+\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n \t)\n '\n \n-- \n2.50.0\n\n"},{"id":"521079","messageId":"t4czubzmfuihxzmefwwhcel5qyss35gmodhfhvkfyiwitb5osw@d33acdbtds63","threadId":"63720","inReplyTo":"20250701115719.85226-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH] read-cache: report lock error when refreshing index","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-07-01T16:08:52Z","receivedAt":"2025-07-01T16:14:21Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/07/01 07:57PM, Han Young wrote:\n> In the repo_refresh_and_write_index of read-cache.c, we return -1 to\n> indicate that writing the index to disk failed.\n> However, callers do not use this information. Commands such as stash print\n> \"could not write index\"\n> and then exit, which does not help to discover the exact problem.\n\nOk, so `repo_refresh_and_write_index()` returns -1 when the \"index.lock\"\ncannot be acquired via `repo_hold_locked_index()` or the index write\nfails via `write_locked_index()`. The function returns 1 when the index\nrefreshed fails via `refresh_index()`.\n\nCallers of `repo_refresh_and_write_index()` currently do not\ndifferentiate between any of these failure types though. This patch\nwants to begin printing an error message if the lock file fails to be\ncreated. This would provide more insight into why the failure occurred\nthan simply \"error: could not write index\". That makes sense.\n\n> We can let repo_hold_locked_index print the error message if the locking\n> failed.\n\nIt looks like `repo_hold_locked_index()` already has the\n`LOCK_REPORT_ON_ERROR` flag which will print the message we want.\n\n> Signed-off-by: Han Young \n> ---\n> read-cache.c | 2 +-\n> t/t3903-stash.sh | 15 +++------------\n> 2 files changed, 4 insertions(+), 13 deletions(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index c0bb760ad..50e842bfa 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1456,7 +1456,7 @@ int repo_refresh_and_write_index(struct repository *repo,\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \tint fd, ret = 0;\n> \n> -\tfd = repo_hold_locked_index(repo, &lock_file, 0);\n> +\tfd = repo_hold_locked_index(repo, &lock_file, gentle ? 0 : LOCK_REPORT_ON_ERROR);\n\nHere we begin passing the `LOCK_REPORT_ON_ERROR` flag to print the error\nmessage only if `gentle` is not set. Makes sense.\n\n> \tif (!gentle && fd < 0)\n> \t\treturn -1;\n> \tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 35b85c790..39098ade4 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1571,11 +1571,8 @@ test_expect_success 'stash create reports a locked index' '\n> \t\techo change >A.file &&\n> \t\ttouch .git/index.lock &&\n> \n> -\t\tcat >expect <<-EOF &&\n> -\t\terror: could not write index\n> -\t\tEOF\n> \t\ttest_must_fail git stash create 2>err &&\n> -\t\ttest_cmp expect err\n> +\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n\nThe test now checks for the explicit lock error message. The check for\nthe \"error: could not write index\" message is also removed even though\nit should still be present in the output. Should we also continue to\ngrep for that message too?\n\n> \t)\n> '\n> \n> @@ -1588,11 +1585,8 @@ test_expect_success 'stash push reports a locked index' '\n> \t\techo change >A.file &&\n> \t\ttouch .git/index.lock &&\n> \n> -\t\tcat >expect <<-EOF &&\n> -\t\terror: could not write index\n> -\t\tEOF\n> \t\ttest_must_fail git stash push 2>err &&\n> -\t\ttest_cmp expect err\n> +\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n\nSame question about testing for the previous error message here.\n\n> \t)\n> '\n> \n> @@ -1606,11 +1600,8 @@ test_expect_success 'stash apply reports a locked index' '\n> \t\tgit stash push &&\n> \t\ttouch .git/index.lock &&\n> \n> -\t\tcat >expect <<-EOF &&\n> -\t\terror: could not write index\n> -\t\tEOF\n> \t\ttest_must_fail git stash apply 2>err &&\n> -\t\ttest_cmp expect err\n> +\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n\nand here\n\n> \t)\n> '\n\nThanks,\n-Justin\n"},{"id":"521096","messageId":"xmqqv7obk98v.fsf@gitster.g","threadId":"63720","inReplyTo":"t4czubzmfuihxzmefwwhcel5qyss35gmodhfhvkfyiwitb5osw@d33acdbtds63","subject":"Re: [PATCH] read-cache: report lock error when refreshing index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-01T19:21:20Z","receivedAt":"2025-07-01T19:21:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n>> \t\ttest_must_fail git stash create 2>err &&\n>> -\t\ttest_cmp expect err\n>> +\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n>\n> The test now checks for the explicit lock error message. The check for\n> the \"error: could not write index\" message is also removed even though\n> it should still be present in the output. Should we also continue to\n> grep for that message too?\n\nProbably. I was wondering which part of the patch removed the\nexisting message while reading this update to the test.\n\nThanks.\n"},{"id":"521234","messageId":"20250703074502.45593-1-hanyang.tony@bytedance.com","threadId":"63720","inReplyTo":"20250701115719.85226-1-hanyang.tony@bytedance.com","subject":"[PATCH v2] read-cache: report lock error when refreshing index","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-07-03T07:45:02Z","receivedAt":"2025-07-03T07:45:21Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In the repo_refresh_and_write_index of read-cache.c, we return -1 to\nindicate that writing the index to disk failed.\nHowever, callers do not use this information. Commands such as stash print\n \"could not write index\"\nand then exit, which does not help to discover the exact problem.\n\nWe can let repo_hold_locked_index print the error message if the locking\nfailed.\n\nSigned-off-by: Han Young \n---\nChanges since v1:\nalso check the \"could not write index\" error output\n\n read-cache.c | 2 +-\n t/t3903-stash.sh | 18 ++++++------------\n 2 files changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex c0bb760ad..50e842bfa 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1456,7 +1456,7 @@ int repo_refresh_and_write_index(struct repository *repo,\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint fd, ret = 0;\n \n-\tfd = repo_hold_locked_index(repo, &lock_file, 0);\n+\tfd = repo_hold_locked_index(repo, &lock_file, gentle ? 0 : LOCK_REPORT_ON_ERROR);\n \tif (!gentle && fd < 0)\n \t\treturn -1;\n \tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex c58ccb136..0bb4648e3 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1672,11 +1672,9 @@ test_expect_success 'stash create reports a locked index' '\n \t\techo change >A.file &&\n \t\ttouch .git/index.lock &&\n \n-\t\tcat >expect <<-EOF &&\n-\t\terror: could not write index\n-\t\tEOF\n \t\ttest_must_fail git stash create 2>err &&\n-\t\ttest_cmp expect err\n+\t\ttest_grep \"error: could not write index\" err &&\n+\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n \t)\n '\n \n@@ -1689,11 +1687,9 @@ test_expect_success 'stash push reports a locked index' '\n \t\techo change >A.file &&\n \t\ttouch .git/index.lock &&\n \n-\t\tcat >expect <<-EOF &&\n-\t\terror: could not write index\n-\t\tEOF\n \t\ttest_must_fail git stash push 2>err &&\n-\t\ttest_cmp expect err\n+\t\ttest_grep \"error: could not write index\" err &&\n+\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n \t)\n '\n \n@@ -1707,11 +1703,9 @@ test_expect_success 'stash apply reports a locked index' '\n \t\tgit stash push &&\n \t\ttouch .git/index.lock &&\n \n-\t\tcat >expect <<-EOF &&\n-\t\terror: could not write index\n-\t\tEOF\n \t\ttest_must_fail git stash apply 2>err &&\n-\t\ttest_cmp expect err\n+\t\ttest_grep \"error: could not write index\" err &&\n+\t\ttest_grep \"error: Unable to create '.*index.lock'\" err\n \t)\n '\n \n-- \n2.50.0\n\n"},{"id":"521458","messageId":"xmqq8qkz5193.fsf@gitster.g","threadId":"63720","inReplyTo":"20250703074502.45593-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH v2] read-cache: report lock error when refreshing index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-07T18:01:12Z","receivedAt":"2025-07-07T18:01:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> In the repo_refresh_and_write_index of read-cache.c, we return -1 to\n> indicate that writing the index to disk failed.\n> However, callers do not use this information. Commands such as stash print\n> \"could not write index\"\n> and then exit, which does not help to discover the exact problem.\n>\n> We can let repo_hold_locked_index print the error message if the locking\n> failed.\n>\n> Signed-off-by: Han Young \n> ---\n> Changes since v1:\n> also check the \"could not write index\" error output\n>\n> read-cache.c | 2 +-\n> t/t3903-stash.sh | 18 ++++++------------\n> 2 files changed, 7 insertions(+), 13 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index c0bb760ad..50e842bfa 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1456,7 +1456,7 @@ int repo_refresh_and_write_index(struct repository *repo,\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \tint fd, ret = 0;\n> \n> -\tfd = repo_hold_locked_index(repo, &lock_file, 0);\n> +\tfd = repo_hold_locked_index(repo, &lock_file, gentle ? 0 : LOCK_REPORT_ON_ERROR);\n\nLet's wrap this line to stay under 80-columns, i.e.\n\n\tfd = repo_hold_locked_index(repo, &lock_file,\n\t\t\t\t gentle ? 0 : LOCK_REPORT_ON_ERROR);\n\nNo need to resend, as I've done so locally while applying.\n"},{"id":"521496","messageId":"CAG1j3zFz0RQ3mf=BRAQhCDWucOR0J9Z26Wmmz8G6+9dMbNO3Ag@mail.gmail.com","threadId":"63720","inReplyTo":"xmqq8qkz5193.fsf@gitster.g","subject":"Re: [External] Re: [PATCH v2] read-cache: report lock error when refreshing index","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-07-08T02:47:41Z","receivedAt":"2025-07-08T02:47:53Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Tue, Jul 8, 2025 at 2:01 AM Junio C Hamano wrote:\n> No need to resend, as I've done so locally while applying.\n\nThank you, I'll be careful next time.\n"}]}