{"thread":{"id":"60846","subject":"git bug","startedAt":"2024-02-04T23:42:16Z","lastAt":"2024-02-06T05:34:45Z","messageCount":8,"participants":["moti sd","Patrick Steinhardt","Rubén Justo","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"487907","messageId":"CAPvDF0P7_s-iy_V7FNSHtXXc9v5E3Fm=AdgduDDsd0rM-zNg-g@mail.gmail.com","threadId":"60846","inReplyTo":null,"subject":"git bug","fromName":"moti sd","fromEmail":"motisd8@gmail.com","sentAt":"2024-02-04T23:42:03Z","receivedAt":"2024-02-04T23:42:16Z","isPatch":false,"sender":{"key":"motisd8@gmail.com","avatar":null},"body":"Dear Git Team,\n\nI am writing to report a potential bug encountered while using the \"git\nstash\" command. The issue was observed during a troubleshooting. Please\nfind the details below:\n\n*Problem Description:*\n\nThe \"git stash\" command is not providing any feedback or indication of\nexecution, even though modifications are present and the stash operation is\nattempted.\n*Steps to Reproduce:*\n\nMake modifications to a file.\nExecute \"git stash\" to stash changes.\n*Expected Behavior:*\n\nThe \"git stash\" command should provide feedback on the stash operation,\nconfirming whether it was successful or if there were any errors.\n*Additional Information:*\nThe issue was initially observed when executing \"git commit -a --amend\" and\npressing Ctrl+Z to exit. Subsequent attempts to use \"git stash\" resulted in\nan error.\nTesting with a provided ZIP file containing a problematic repository\nconfirmed the issue's recurrence.\n*Investigation and Findings:*\n\nUpon investigation, it was discovered that the use of Ctrl+Z after \"git\ncommit -a --amend\" might be the root cause.\nDeleting a lock file left behind by the incomplete git command resolved the\nissue, indicating a potential bug in how \"git stash\" handles incomplete\ncommands.\n*Recommendation:*\n\nThe \"git stash\" command should provide meaningful feedback or error\nmessages to users, even in cases where the operation is incomplete or\nencounters issues.\n*Steps to Reproduce the Bug:*\n\nExecute \"git commit -a --amend.\"\nPress Ctrl+Z to exit the command.\nAttempt \"git stash.\"\n*Attachment:*\nA ZIP file containing a problematic repository is attached for your\nreference.\n*Impact:*\n\nThe bug affects users' ability to use the \"git stash\" command effectively,\nleading to potential confusion and hindering workflow.\nResolution:\n\n\nI appreciate your attention to this matter, and I believe addressing this\nbug will enhance the overall user experience with Git. If further\ninformation or testing is required, please feel free to reach out.\n\n\nBest regards,\nMoti\n\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\nI executed \"git commit -a --amend,\" where I pressed Ctrl+Z to exit. Later, when trying again, an error occurred.\n\nWhat did you expect to happen? (Expected behavior)\nI intended to execute the \"git stash\" command, but it is not functioning as expected. I wish to receive an error message to better understand the issue and address it accordingly. The command is silent, whereas during a commit, error messages are displayed, providing helpful feedback for resolution.\nWhat happened instead? (Actual behavior)\nDue to .git/lock file, git stash does not behave normally, and the error message does not show up\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.38.1.windows.1\ncpu: x86_64\nbuilt from commit: b85c8f604d375d4d773a36842964e8a7ec056aae\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Windows 10.0 22631 \ncompiler info: gnuc: 12.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): <unset>\n\n\n[Enabled Hooks]\n"},{"id":"487919","messageId":"ZcCEbaId0HL4Briz@tanuki","threadId":"60846","inReplyTo":"CAPvDF0P7_s-iy_V7FNSHtXXc9v5E3Fm=AdgduDDsd0rM-zNg-g@mail.gmail.com","subject":"Re: git bug","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-05T06:47:09Z","receivedAt":"2024-02-05T06:47:15Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 05, 2024 at 12:42:03AM +0100, moti sd wrote:\n> Dear Git Team,\n> \n> I am writing to report a potential bug encountered while using the \"git\n> stash\" command. The issue was observed during a troubleshooting. Please\n> find the details below:\n> \n> *Problem Description:*\n> \n> The \"git stash\" command is not providing any feedback or indication of\n> execution, even though modifications are present and the stash operation is\n> attempted.\n> *Steps to Reproduce:*\n> \n> Make modifications to a file.\n> Execute \"git stash\" to stash changes.\n> *Expected Behavior:*\n> \n> The \"git stash\" command should provide feedback on the stash operation,\n> confirming whether it was successful or if there were any errors.\n> *Additional Information:*\n> The issue was initially observed when executing \"git commit -a --amend\" and\n> pressing Ctrl+Z to exit. Subsequent attempts to use \"git stash\" resulted in\n> an error.\n> Testing with a provided ZIP file containing a problematic repository\n> confirmed the issue's recurrence.\n> *Investigation and Findings:*\n> \n> Upon investigation, it was discovered that the use of Ctrl+Z after \"git\n> commit -a --amend\" might be the root cause.\n> Deleting a lock file left behind by the incomplete git command resolved the\n> issue, indicating a potential bug in how \"git stash\" handles incomplete\n> commands.\n> *Recommendation:*\n\nThis is indeed it. The problem is that the index will be locked while\ngit-commit(1) is active, and thus git-stash(1) cannot write to it at the\nsame time. So this is working as designed. What's not though is the fact\nthat we don't print an error message.\n\nI'll send a patch in reply to this message to fix this bug. Thanks for\nyour report!\n\nPatrick\n"},{"id":"487920","messageId":"2cd44b01dc29b099a07658499481a6847c46562d.1707116449.git.ps@pks.im","threadId":"60846","inReplyTo":"CAPvDF0P7_s-iy_V7FNSHtXXc9v5E3Fm=AdgduDDsd0rM-zNg-g@mail.gmail.com","subject":"[PATCH] builtin/stash: report failure to write to index","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-05T07:01:04Z","receivedAt":"2024-02-05T07:01:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-stash(1) command needs to write to the index for many of its\noperations. When the index is locked by a concurrent writer it will thus\nfail to operate, which is expected. What is not expected though is that\nwe do not print any error message at all in this case. The user can thus\neasily miss the fact that the command didn't do what they expected it to\ndo and would be left wondering why that is.\n\nFix this bug and report failures to write to the index. Add tests for\nthe subcommands which hit the respective code paths.\n\nNote that the chosen error message (\"Cannot write to the index\") does\nnot match our guidelines as it starts with a capitalized letter. This is\nintentional though and matches the style of all the other messages used\nin git-stash(1).\n\nReported-by: moti sd <motisd8@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/stash.c  |  6 +++---\n t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex b2813c614c..9df072b459 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \trepo_read_index_preload(the_repository, NULL, 0);\n \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n \t\t\t\t\t NULL, NULL, NULL))\n-\t\treturn -1;\n+\t\treturn error(_(\"Cannot write to the index\"));\n \n \tif (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,\n \t\t\t\tNULL))\n@@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \trepo_read_index_preload(the_repository, NULL, 0);\n \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n \t\t\t\t\t NULL, NULL, NULL) < 0) {\n-\t\tret = -1;\n+\t\tret = error(_(\"Cannot write to the index\"));\n \t\tgoto done;\n \t}\n \n@@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \n \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n \t\t\t\t\t NULL, NULL, NULL)) {\n-\t\tret = -1;\n+\t\tret = error(_(\"Cannot write to the index\"));\n \t\tgoto done;\n \t}\n \ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 3319240515..770881e537 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1516,4 +1516,56 @@ test_expect_success 'restore untracked files even when we hit conflicts' '\n \t)\n '\n \n+test_expect_success 'stash create reports a locked index' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A A.file &&\n+\t\techo change >A.file &&\n+\t\ttouch .git/index.lock &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\terror: Cannot write to the index\n+\t\tEOF\n+\t\ttest_must_fail git stash create 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'stash push reports a locked index' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A A.file &&\n+\t\techo change >A.file &&\n+\t\ttouch .git/index.lock &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\terror: Cannot write to the index\n+\t\tEOF\n+\t\ttest_must_fail git stash push 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'stash apply reports a locked index' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A A.file &&\n+\t\techo change >A.file &&\n+\t\tgit stash push &&\n+\t\ttouch .git/index.lock &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\terror: Cannot write to the index\n+\t\tEOF\n+\t\ttest_must_fail git stash apply 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_done\n-- \n2.43.GIT\n\n"},{"id":"487958","messageId":"f07d33e4-5595-43e3-838e-fd89c5621485@gmail.com","threadId":"60846","inReplyTo":"2cd44b01dc29b099a07658499481a6847c46562d.1707116449.git.ps@pks.im","subject":"Re: [PATCH] builtin/stash: report failure to write to index","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-05T22:22:01Z","receivedAt":"2024-02-05T22:22:05Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 05-feb-2024 08:01:04, Patrick Steinhardt wrote:\n> The git-stash(1) command needs to write to the index for many of its\n> operations. When the index is locked by a concurrent writer it will thus\n> fail to operate, which is expected. What is not expected though is that\n> we do not print any error message at all in this case. The user can thus\n> easily miss the fact that the command didn't do what they expected it to\n> do and would be left wondering why that is.\n> \n> Fix this bug and report failures to write to the index. Add tests for\n> the subcommands which hit the respective code paths.\n> \n> Note that the chosen error message (\"Cannot write to the index\") does\n> not match our guidelines as it starts with a capitalized letter. This is\n> intentional though and matches the style of all the other messages used\n> in git-stash(1).\n\nYour message made me curious, so I ran:\n\n   $ git grep -E '(die|error)\\(' builtin/stash.c\n   \n   builtin/stash.c:168:               die(_(\"'%s' is not a stash-like commit\"), revision);\n   builtin/stash.c:214:               return error(_(\"%s is not a valid reference\"), revision);\n   builtin/stash.c:261:               return error(_(\"git stash clear with arguments is \"\n   builtin/stash.c:303:               return error(_(\"unable to write new index file\"));\n   builtin/stash.c:487:                                       die(\"Failed to move %s to %s\\n\",\n   builtin/stash.c:523:               die(_(\"Unable to write index.\"));\n   builtin/stash.c:544:               return error(_(\"cannot apply a stash in the middle of a merge\"));\n   builtin/stash.c:555:                               return error(_(\"could not generate diff %s^!.\"),\n   builtin/stash.c:562:                               return error(_(\"conflicts in index. \"\n   builtin/stash.c:569:                               return error(_(\"could not save index tree\"));\n   builtin/stash.c:610:               ret = error(_(\"could not write index\"));\n   builtin/stash.c:630:               ret = error(_(\"could not restore untracked files from stash\"));\n   builtin/stash.c:702:               return error(_(\"%s: Could not drop stash entry\"),\n   builtin/stash.c:721:               return error(_(\"'%s' is not a stash reference\"), info->revision.buf);\n   builtin/stash.c:872:                       die(_(\"failed to parse tree\"));\n   builtin/stash.c:883:               die(_(\"failed to unpack trees\"));\n   builtin/stash.c:1547:              if (report_path_error(ps_matched, ps)) {\n   builtin/stash.c:1763:                      die(\"subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'\",\n   builtin/stash.c:1773:                      die(_(\"options '%s' and '%s' cannot be used together\"), \"--pathspec-from-file\", \"--patch\");\n   builtin/stash.c:1776:                      die(_(\"options '%s' and '%s' cannot be used together\"), \"--pathspec-from-file\", \"--staged\");\n   builtin/stash.c:1779:                      die(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n   builtin/stash.c:1785:              die(_(\"the option '%s' requires '%s'\"), \"--pathspec-file-nul\", \"--pathspec-from-file\");\n\nCertainly, there are some error messages in builtin/stash.c that do not\nfollow the notes in Documentation/CodingGuideLines, but it does not seem\nto be \"the style of all other messages\" in git-stash.\n\nHowever, your message is clear ...  What error messages are you\nconsidering?\n\n> \n> Reported-by: moti sd <motisd8@gmail.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/stash.c  |  6 +++---\n>  t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 55 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index b2813c614c..9df072b459 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>  \trepo_read_index_preload(the_repository, NULL, 0);\n>  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n>  \t\t\t\t\t NULL, NULL, NULL))\n> -\t\treturn -1;\n> +\t\treturn error(_(\"Cannot write to the index\"));\n>  \n>  \tif (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,\n>  \t\t\t\tNULL))\n> @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n>  \trepo_read_index_preload(the_repository, NULL, 0);\n>  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n>  \t\t\t\t\t NULL, NULL, NULL) < 0) {\n> -\t\tret = -1;\n> +\t\tret = error(_(\"Cannot write to the index\"));\n>  \t\tgoto done;\n>  \t}\n>  \n> @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>  \n>  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n>  \t\t\t\t\t NULL, NULL, NULL)) {\n> -\t\tret = -1;\n> +\t\tret = error(_(\"Cannot write to the index\"));\n>  \t\tgoto done;\n>  \t}\n>  \n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 3319240515..770881e537 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1516,4 +1516,56 @@ test_expect_success 'restore untracked files even when we hit conflicts' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'stash create reports a locked index' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit A A.file &&\n> +\t\techo change >A.file &&\n> +\t\ttouch .git/index.lock &&\n> +\n> +\t\tcat >expect <<-EOF &&\n> +\t\terror: Cannot write to the index\n> +\t\tEOF\n> +\t\ttest_must_fail git stash create 2>err &&\n> +\t\ttest_cmp expect err\n> +\t)\n> +'\n> +\n> +test_expect_success 'stash push reports a locked index' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit A A.file &&\n> +\t\techo change >A.file &&\n> +\t\ttouch .git/index.lock &&\n> +\n> +\t\tcat >expect <<-EOF &&\n> +\t\terror: Cannot write to the index\n> +\t\tEOF\n> +\t\ttest_must_fail git stash push 2>err &&\n> +\t\ttest_cmp expect err\n> +\t)\n> +'\n> +\n> +test_expect_success 'stash apply reports a locked index' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit A A.file &&\n> +\t\techo change >A.file &&\n> +\t\tgit stash push &&\n> +\t\ttouch .git/index.lock &&\n> +\n> +\t\tcat >expect <<-EOF &&\n> +\t\terror: Cannot write to the index\n> +\t\tEOF\n> +\t\ttest_must_fail git stash apply 2>err &&\n> +\t\ttest_cmp expect err\n> +\t)\n> +'\n> +\n>  test_done\n> -- \n> 2.43.GIT\n> \n\n\n"},{"id":"487987","messageId":"xmqqo7cul1fi.fsf@gitster.g","threadId":"60846","inReplyTo":"2cd44b01dc29b099a07658499481a6847c46562d.1707116449.git.ps@pks.im","subject":"Re: [PATCH] builtin/stash: report failure to write to index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T03:24:49Z","receivedAt":"2024-02-06T03:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The git-stash(1) command needs to write to the index for many of its\n> operations. When the index is locked by a concurrent writer it will thus\n> fail to operate, which is expected. What is not expected though is that\n> we do not print any error message at all in this case. The user can thus\n> easily miss the fact that the command didn't do what they expected it to\n> do and would be left wondering why that is.\n\nHopefully, they know they notice the exit status of the command, or\ndo we throw the error away and exit(0) from the program?\n\nIn any case, telling the users what did (and did not) happen is a\ngood idea.\n\n> Fix this bug and report failures to write to the index. Add tests for\n> the subcommands which hit the respective code paths.\n>\n> Note that the chosen error message (\"Cannot write to the index\") does\n> not match our guidelines as it starts with a capitalized letter. This is\n> intentional though and matches the style of all the other messages used\n> in git-stash(1).\n\nStyle may be OK, but I wonder if they should say different things,\nto hint what failed.  For example:\n\n> @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>  \trepo_read_index_preload(the_repository, NULL, 0);\n>  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n>  \t\t\t\t\t NULL, NULL, NULL))\n> -\t\treturn -1;\n> +\t\treturn error(_(\"Cannot write to the index\"));\n>\n>  \tif (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,\n>  \t\t\t\tNULL))\n\nThis failure and message comes before anything interesting happens.\nWe attempted to refresh the current index and failed to write out\nthe result, meaning that whatever index we had on disk did not get\noverwritten.  Is this new message enough to tell the user that we\ndidn't touch the working tree or the index, which would happen if\neven some part of \"stash apply\" happened?  Or is it obvious that we\ndid not do anything?\n\n> @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n>  \trepo_read_index_preload(the_repository, NULL, 0);\n>  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n>  \t\t\t\t\t NULL, NULL, NULL) < 0) {\n> -\t\tret = -1;\n> +\t\tret = error(_(\"Cannot write to the index\"));\n>  \t\tgoto done;\n>  \t}\n\nDitto.\n\n> @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>  \n>  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n>  \t\t\t\t\t NULL, NULL, NULL)) {\n> -\t\tret = -1;\n> +\t\tret = error(_(\"Cannot write to the index\"));\n>  \t\tgoto done;\n>  \t}\n>  \n\nDitto.\n\nThanks.\n"},{"id":"487991","messageId":"ZcG-XVZxSWKvrzqL@tanuki","threadId":"60846","inReplyTo":"f07d33e4-5595-43e3-838e-fd89c5621485@gmail.com","subject":"Re: [PATCH] builtin/stash: report failure to write to index","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-06T05:06:37Z","receivedAt":"2024-02-06T05:06:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 05, 2024 at 11:22:01PM +0100, Rubén Justo wrote:\n> On 05-feb-2024 08:01:04, Patrick Steinhardt wrote:\n> > The git-stash(1) command needs to write to the index for many of its\n> > operations. When the index is locked by a concurrent writer it will thus\n> > fail to operate, which is expected. What is not expected though is that\n> > we do not print any error message at all in this case. The user can thus\n> > easily miss the fact that the command didn't do what they expected it to\n> > do and would be left wondering why that is.\n> > \n> > Fix this bug and report failures to write to the index. Add tests for\n> > the subcommands which hit the respective code paths.\n> > \n> > Note that the chosen error message (\"Cannot write to the index\") does\n> > not match our guidelines as it starts with a capitalized letter. This is\n> > intentional though and matches the style of all the other messages used\n> > in git-stash(1).\n> \n> Your message made me curious, so I ran:\n> \n>    $ git grep -E '(die|error)\\(' builtin/stash.c\n>    \n>    builtin/stash.c:168:               die(_(\"'%s' is not a stash-like commit\"), revision);\n>    builtin/stash.c:214:               return error(_(\"%s is not a valid reference\"), revision);\n>    builtin/stash.c:261:               return error(_(\"git stash clear with arguments is \"\n>    builtin/stash.c:303:               return error(_(\"unable to write new index file\"));\n>    builtin/stash.c:487:                                       die(\"Failed to move %s to %s\\n\",\n>    builtin/stash.c:523:               die(_(\"Unable to write index.\"));\n>    builtin/stash.c:544:               return error(_(\"cannot apply a stash in the middle of a merge\"));\n>    builtin/stash.c:555:                               return error(_(\"could not generate diff %s^!.\"),\n>    builtin/stash.c:562:                               return error(_(\"conflicts in index. \"\n>    builtin/stash.c:569:                               return error(_(\"could not save index tree\"));\n>    builtin/stash.c:610:               ret = error(_(\"could not write index\"));\n>    builtin/stash.c:630:               ret = error(_(\"could not restore untracked files from stash\"));\n>    builtin/stash.c:702:               return error(_(\"%s: Could not drop stash entry\"),\n>    builtin/stash.c:721:               return error(_(\"'%s' is not a stash reference\"), info->revision.buf);\n>    builtin/stash.c:872:                       die(_(\"failed to parse tree\"));\n>    builtin/stash.c:883:               die(_(\"failed to unpack trees\"));\n>    builtin/stash.c:1547:              if (report_path_error(ps_matched, ps)) {\n>    builtin/stash.c:1763:                      die(\"subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'\",\n>    builtin/stash.c:1773:                      die(_(\"options '%s' and '%s' cannot be used together\"), \"--pathspec-from-file\", \"--patch\");\n>    builtin/stash.c:1776:                      die(_(\"options '%s' and '%s' cannot be used together\"), \"--pathspec-from-file\", \"--staged\");\n>    builtin/stash.c:1779:                      die(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n>    builtin/stash.c:1785:              die(_(\"the option '%s' requires '%s'\"), \"--pathspec-file-nul\", \"--pathspec-from-file\");\n> \n> Certainly, there are some error messages in builtin/stash.c that do not\n> follow the notes in Documentation/CodingGuideLines, but it does not seem\n> to be \"the style of all other messages\" in git-stash.\n> \n> However, your message is clear ...  What error messages are you\n> considering?\n\nThat's because many of the code paths don't use either `error()` or\n`die()`, but use `fprintf_ln()` instead:\n\n$ git grep 'fprintf_ln(stderr' -- builtin/stash.c\nbuiltin/stash.c:\t\tfprintf_ln(stderr, _(\"Too many revisions specified:%s\"),\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"No stash entries found.\"));\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"Index was not unstashed.\"));\nbuiltin/stash.c:\t\tfprintf_ln(stderr, _(\"No branch name specified\"));\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"Cannot update %s with %s\"),\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"\\\"git stash store\\\" requires one \"\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"Cannot update %s with %s\"),\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"No staged changes\"));\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"No changes selected\"));\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"You do not have \"\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"Cannot save the current \"\nbuiltin/stash.c:\t\t\t\tfprintf_ln(stderr, _(\"Cannot save \"\nbuiltin/stash.c:\t\t\t\tfprintf_ln(stderr, _(\"Cannot save the current \"\nbuiltin/stash.c:\t\t\t\tfprintf_ln(stderr, _(\"Cannot save the current \"\nbuiltin/stash.c:\t\t\t\tfprintf_ln(stderr, _(\"Cannot save the current \"\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"Cannot record \"\nbuiltin/stash.c:\t\tfprintf_ln(stderr, _(\"Can't use --patch and --include-untracked\"\nbuiltin/stash.c:\t\tfprintf_ln(stderr, _(\"Can't use --staged and --include-untracked\"\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"Did you forget to 'git add'?\"));\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"Cannot initialize stash\"));\nbuiltin/stash.c:\t\t\tfprintf_ln(stderr, _(\"Cannot save the current status\"));\nbuiltin/stash.c:\t\t\t\tfprintf_ln(stderr, _(\"Cannot remove \"\n\nOverall, \"builtin/stash.c\" is a wild mixture of error styles. Some are\nlikely user-facing, where it might be a good idea to use `fprinf_ln()`\ninstead of `error()`. But some of them are closer to what we do in this\npatch and likely should be converted to use `error()`, too.\n\nI dunno, I find this to be confusing. But I spotted that there's an\ninstance already where we say `error(\"cannot write index\")`, so I'll\njust use that.\n\nPatrick\n\n> > \n> > Reported-by: moti sd <motisd8@gmail.com>\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  builtin/stash.c  |  6 +++---\n> >  t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++\n> >  2 files changed, 55 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/builtin/stash.c b/builtin/stash.c\n> > index b2813c614c..9df072b459 100644\n> > --- a/builtin/stash.c\n> > +++ b/builtin/stash.c\n> > @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> >  \trepo_read_index_preload(the_repository, NULL, 0);\n> >  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n> >  \t\t\t\t\t NULL, NULL, NULL))\n> > -\t\treturn -1;\n> > +\t\treturn error(_(\"Cannot write to the index\"));\n> >  \n> >  \tif (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,\n> >  \t\t\t\tNULL))\n> > @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n> >  \trepo_read_index_preload(the_repository, NULL, 0);\n> >  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n> >  \t\t\t\t\t NULL, NULL, NULL) < 0) {\n> > -\t\tret = -1;\n> > +\t\tret = error(_(\"Cannot write to the index\"));\n> >  \t\tgoto done;\n> >  \t}\n> >  \n> > @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n> >  \n> >  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n> >  \t\t\t\t\t NULL, NULL, NULL)) {\n> > -\t\tret = -1;\n> > +\t\tret = error(_(\"Cannot write to the index\"));\n> >  \t\tgoto done;\n> >  \t}\n> >  \n> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> > index 3319240515..770881e537 100755\n> > --- a/t/t3903-stash.sh\n> > +++ b/t/t3903-stash.sh\n> > @@ -1516,4 +1516,56 @@ test_expect_success 'restore untracked files even when we hit conflicts' '\n> >  \t)\n> >  '\n> >  \n> > +test_expect_success 'stash create reports a locked index' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\ttest_commit A A.file &&\n> > +\t\techo change >A.file &&\n> > +\t\ttouch .git/index.lock &&\n> > +\n> > +\t\tcat >expect <<-EOF &&\n> > +\t\terror: Cannot write to the index\n> > +\t\tEOF\n> > +\t\ttest_must_fail git stash create 2>err &&\n> > +\t\ttest_cmp expect err\n> > +\t)\n> > +'\n> > +\n> > +test_expect_success 'stash push reports a locked index' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\ttest_commit A A.file &&\n> > +\t\techo change >A.file &&\n> > +\t\ttouch .git/index.lock &&\n> > +\n> > +\t\tcat >expect <<-EOF &&\n> > +\t\terror: Cannot write to the index\n> > +\t\tEOF\n> > +\t\ttest_must_fail git stash push 2>err &&\n> > +\t\ttest_cmp expect err\n> > +\t)\n> > +'\n> > +\n> > +test_expect_success 'stash apply reports a locked index' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\ttest_commit A A.file &&\n> > +\t\techo change >A.file &&\n> > +\t\tgit stash push &&\n> > +\t\ttouch .git/index.lock &&\n> > +\n> > +\t\tcat >expect <<-EOF &&\n> > +\t\terror: Cannot write to the index\n> > +\t\tEOF\n> > +\t\ttest_must_fail git stash apply 2>err &&\n> > +\t\ttest_cmp expect err\n> > +\t)\n> > +'\n> > +\n> >  test_done\n> > -- \n> > 2.43.GIT\n> > \n> \n> \n"},{"id":"488021","messageId":"ZcHBmGAuOBEUBcKU@tanuki","threadId":"60846","inReplyTo":"xmqqo7cul1fi.fsf@gitster.g","subject":"Re: [PATCH] builtin/stash: report failure to write to index","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-06T05:20:24Z","receivedAt":"2024-02-06T05:20:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 05, 2024 at 07:24:49PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > The git-stash(1) command needs to write to the index for many of its\n> > operations. When the index is locked by a concurrent writer it will thus\n> > fail to operate, which is expected. What is not expected though is that\n> > we do not print any error message at all in this case. The user can thus\n> > easily miss the fact that the command didn't do what they expected it to\n> > do and would be left wondering why that is.\n> \n> Hopefully, they know they notice the exit status of the command, or\n> do we throw the error away and exit(0) from the program?\n\nWe do return a proper exit code as demonstrated by the tests. But if you\ninteractively use those commands in the shell then you're quite likely\nto not notice error codes at all -- my shell certainly doesn't highlight\nfailed commands in any special way.\n\n> In any case, telling the users what did (and did not) happen is a\n> good idea.\n> \n> > Fix this bug and report failures to write to the index. Add tests for\n> > the subcommands which hit the respective code paths.\n> >\n> > Note that the chosen error message (\"Cannot write to the index\") does\n> > not match our guidelines as it starts with a capitalized letter. This is\n> > intentional though and matches the style of all the other messages used\n> > in git-stash(1).\n> \n> Style may be OK, but I wonder if they should say different things,\n> to hint what failed.  For example:\n> \n> > @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> >  \trepo_read_index_preload(the_repository, NULL, 0);\n> >  \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n> >  \t\t\t\t\t NULL, NULL, NULL))\n> > -\t\treturn -1;\n> > +\t\treturn error(_(\"Cannot write to the index\"));\n> >\n> >  \tif (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,\n> >  \t\t\t\tNULL))\n> \n> This failure and message comes before anything interesting happens.\n> We attempted to refresh the current index and failed to write out\n> the result, meaning that whatever index we had on disk did not get\n> overwritten.  Is this new message enough to tell the user that we\n> didn't touch the working tree or the index, which would happen if\n> even some part of \"stash apply\" happened?  Or is it obvious that we\n> did not do anything?\n\nAs a user, my expectation is that if a command failed, it didn't do\nanything. If it did something before failing and wasn't able to clean it\nup, then it is the responsibility of the command to tell me that it\nmight have screwed up and left behind some partially-applied changes.\n\nIt could certainly be that my expectation is way off. But personally, I\ndon't think it's useful to say \"We didn't do anything\" in every case\nwhere we failed without doing anything -- I'd rather feel that it is\nquite spammy.\n\nBut anyway, I know that my UX skills are severely lacking. So in case\nyou or anybody else has a specific suggestion for how to make it better\nthen I'm certainly happy to adapt.\n\nPatrick\n"},{"id":"488023","messageId":"cb098cf88cbfcbf7c4872f8887856629b909cb91.1707197653.git.ps@pks.im","threadId":"60846","inReplyTo":"CAPvDF0P7_s-iy_V7FNSHtXXc9v5E3Fm=AdgduDDsd0rM-zNg-g@mail.gmail.com","subject":"[PATCH v2] builtin/stash: report failure to write to index","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-06T05:34:41Z","receivedAt":"2024-02-06T05:34:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-stash(1) command needs to write to the index for many of its\noperations. When the index is locked by a concurrent writer it will thus\nfail to operate, which is expected. What is not expected though is that\nwe do not print any error message at all in this case. The user can thus\neasily miss the fact that the command didn't do what they expected it to\ndo and would be left wondering why that is.\n\nFix this bug and report failures to write to the index. Add tests for\nthe subcommands which hit the respective code paths.\n\nWhile at it, unify error messages when writing to the index fails. The\nchosen error message is already used in \"builtin/stash.c\".\n\nReported-by: moti sd <motisd8@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nRange-diff against v1:\n1:  f114688eac < -:  ---------- drop dot\n2:  b13a5a10ac ! 1:  cb098cf88c builtin/stash: report failure to write to index\n    @@ Commit message\n         Fix this bug and report failures to write to the index. Add tests for\n         the subcommands which hit the respective code paths.\n     \n    -    Note that the chosen error message (\"Cannot write to the index\") does\n    -    not match our guidelines as it starts with a capitalized letter. This is\n    -    intentional though and matches the style of all the other messages used\n    -    in git-stash(1).\n    +    While at it, unify error messages when writing to the index fails. The\n    +    chosen error message is already used in \"builtin/stash.c\".\n     \n         Reported-by: moti sd <motisd8@gmail.com>\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n      ## builtin/stash.c ##\n    +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)\n    + \trepo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);\n    + \tif (write_locked_index(&the_index, &lock,\n    + \t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n    +-\t\tdie(_(\"Unable to write index.\"));\n    ++\t\tdie(_(\"could not write index\"));\n    + }\n    + \n    + static int do_apply_stash(const char *prefix, struct stash_info *info,\n     @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info *info,\n      \trepo_read_index_preload(the_repository, NULL, 0);\n      \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n      \t\t\t\t\t NULL, NULL, NULL))\n     -\t\treturn -1;\n    -+\t\treturn error(_(\"Cannot write to the index\"));\n    ++\t\treturn error(_(\"could not write index\"));\n      \n      \tif (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,\n      \t\t\t\tNULL))\n    @@ builtin/stash.c: static int do_create_stash(const struct pathspec *ps, struct st\n      \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n      \t\t\t\t\t NULL, NULL, NULL) < 0) {\n     -\t\tret = -1;\n    -+\t\tret = error(_(\"Cannot write to the index\"));\n    ++\t\tret = error(_(\"could not write index\"));\n      \t\tgoto done;\n      \t}\n      \n    @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char\n      \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n      \t\t\t\t\t NULL, NULL, NULL)) {\n     -\t\tret = -1;\n    -+\t\tret = error(_(\"Cannot write to the index\"));\n    ++\t\tret = error(_(\"could not write index\"));\n      \t\tgoto done;\n      \t}\n      \n    @@ t/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit\n     +\t\ttouch .git/index.lock &&\n     +\n     +\t\tcat >expect <<-EOF &&\n    -+\t\terror: Cannot write to the index\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/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit\n     +\t\ttouch .git/index.lock &&\n     +\n     +\t\tcat >expect <<-EOF &&\n    -+\t\terror: Cannot write to the index\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/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit\n     +\t\ttouch .git/index.lock &&\n     +\n     +\t\tcat >expect <<-EOF &&\n    -+\t\terror: Cannot write to the index\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\n builtin/stash.c  |  8 ++++----\n t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 26489b76c0..d65cd20ee6 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -520,7 +520,7 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)\n \trepo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);\n \tif (write_locked_index(&the_index, &lock,\n \t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n-\t\tdie(_(\"Unable to write index.\"));\n+\t\tdie(_(\"could not write index\"));\n }\n \n static int do_apply_stash(const char *prefix, struct stash_info *info,\n@@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \trepo_read_index_preload(the_repository, NULL, 0);\n \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n \t\t\t\t\t NULL, NULL, NULL))\n-\t\treturn -1;\n+\t\treturn error(_(\"could not write index\"));\n \n \tif (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,\n \t\t\t\tNULL))\n@@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \trepo_read_index_preload(the_repository, NULL, 0);\n \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n \t\t\t\t\t NULL, NULL, NULL) < 0) {\n-\t\tret = -1;\n+\t\tret = error(_(\"could not write index\"));\n \t\tgoto done;\n \t}\n \n@@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \n \tif (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,\n \t\t\t\t\t NULL, NULL, NULL)) {\n-\t\tret = -1;\n+\t\tret = error(_(\"could not write index\"));\n \t\tgoto done;\n \t}\n \ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 3319240515..00db82fb24 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1516,4 +1516,56 @@ test_expect_success 'restore untracked files even when we hit conflicts' '\n \t)\n '\n \n+test_expect_success 'stash create reports a locked index' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A A.file &&\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)\n+'\n+\n+test_expect_success 'stash push reports a locked index' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A A.file &&\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)\n+'\n+\n+test_expect_success 'stash apply reports a locked index' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A A.file &&\n+\t\techo change >A.file &&\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)\n+'\n+\n test_done\n-- \n2.43.GIT\n\n"}]}