| Message ID | 2cd44b01dc29b099a07658499481a6847c46562d.1707116449.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/stash: report failure to write to index | expand |
On 05-feb-2024 08:01:04, Patrick Steinhardt wrote: > The git-stash(1) command needs to write to the index for many of its > operations. When the index is locked by a concurrent writer it will thus > fail to operate, which is expected. What is not expected though is that > we do not print any error message at all in this case. The user can thus > easily miss the fact that the command didn't do what they expected it to > do and would be left wondering why that is. > > Fix this bug and report failures to write to the index. Add tests for > the subcommands which hit the respective code paths. > > Note that the chosen error message ("Cannot write to the index") does > not match our guidelines as it starts with a capitalized letter. This is > intentional though and matches the style of all the other messages used > in git-stash(1). Your message made me curious, so I ran: $ git grep -E '(die|error)\(' builtin/stash.c builtin/stash.c:168: die(_("'%s' is not a stash-like commit"), revision); builtin/stash.c:214: return error(_("%s is not a valid reference"), revision); builtin/stash.c:261: return error(_("git stash clear with arguments is " builtin/stash.c:303: return error(_("unable to write new index file")); builtin/stash.c:487: die("Failed to move %s to %s\n", builtin/stash.c:523: die(_("Unable to write index.")); builtin/stash.c:544: return error(_("cannot apply a stash in the middle of a merge")); builtin/stash.c:555: return error(_("could not generate diff %s^!."), builtin/stash.c:562: return error(_("conflicts in index. " builtin/stash.c:569: return error(_("could not save index tree")); builtin/stash.c:610: ret = error(_("could not write index")); builtin/stash.c:630: ret = error(_("could not restore untracked files from stash")); builtin/stash.c:702: return error(_("%s: Could not drop stash entry"), builtin/stash.c:721: return error(_("'%s' is not a stash reference"), info->revision.buf); builtin/stash.c:872: die(_("failed to parse tree")); builtin/stash.c:883: die(_("failed to unpack trees")); builtin/stash.c:1547: if (report_path_error(ps_matched, ps)) { builtin/stash.c:1763: die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'", builtin/stash.c:1773: die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch"); builtin/stash.c:1776: die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--staged"); builtin/stash.c:1779: die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file"); builtin/stash.c:1785: die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file"); Certainly, there are some error messages in builtin/stash.c that do not follow the notes in Documentation/CodingGuideLines, but it does not seem to be "the style of all other messages" in git-stash. However, your message is clear ... What error messages are you considering? > > Reported-by: moti sd <motisd8@gmail.com> > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/stash.c | 6 +++--- > t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/builtin/stash.c b/builtin/stash.c > index b2813c614c..9df072b459 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > repo_read_index_preload(the_repository, NULL, 0); > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > NULL, NULL, NULL)) > - return -1; > + return error(_("Cannot write to the index")); > > if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0, > NULL)) > @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b > repo_read_index_preload(the_repository, NULL, 0); > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > NULL, NULL, NULL) < 0) { > - ret = -1; > + ret = error(_("Cannot write to the index")); > goto done; > } > > @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > NULL, NULL, NULL)) { > - ret = -1; > + ret = error(_("Cannot write to the index")); > goto done; > } > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 3319240515..770881e537 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1516,4 +1516,56 @@ test_expect_success 'restore untracked files even when we hit conflicts' ' > ) > ' > > +test_expect_success 'stash create reports a locked index' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + test_commit A A.file && > + echo change >A.file && > + touch .git/index.lock && > + > + cat >expect <<-EOF && > + error: Cannot write to the index > + EOF > + test_must_fail git stash create 2>err && > + test_cmp expect err > + ) > +' > + > +test_expect_success 'stash push reports a locked index' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + test_commit A A.file && > + echo change >A.file && > + touch .git/index.lock && > + > + cat >expect <<-EOF && > + error: Cannot write to the index > + EOF > + test_must_fail git stash push 2>err && > + test_cmp expect err > + ) > +' > + > +test_expect_success 'stash apply reports a locked index' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + test_commit A A.file && > + echo change >A.file && > + git stash push && > + touch .git/index.lock && > + > + cat >expect <<-EOF && > + error: Cannot write to the index > + EOF > + test_must_fail git stash apply 2>err && > + test_cmp expect err > + ) > +' > + > test_done > -- > 2.43.GIT >
Patrick Steinhardt <ps@pks.im> writes: > The git-stash(1) command needs to write to the index for many of its > operations. When the index is locked by a concurrent writer it will thus > fail to operate, which is expected. What is not expected though is that > we do not print any error message at all in this case. The user can thus > easily miss the fact that the command didn't do what they expected it to > do and would be left wondering why that is. Hopefully, they know they notice the exit status of the command, or do we throw the error away and exit(0) from the program? In any case, telling the users what did (and did not) happen is a good idea. > Fix this bug and report failures to write to the index. Add tests for > the subcommands which hit the respective code paths. > > Note that the chosen error message ("Cannot write to the index") does > not match our guidelines as it starts with a capitalized letter. This is > intentional though and matches the style of all the other messages used > in git-stash(1). Style may be OK, but I wonder if they should say different things, to hint what failed. For example: > @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > repo_read_index_preload(the_repository, NULL, 0); > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > NULL, NULL, NULL)) > - return -1; > + return error(_("Cannot write to the index")); > > if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0, > NULL)) This failure and message comes before anything interesting happens. We attempted to refresh the current index and failed to write out the result, meaning that whatever index we had on disk did not get overwritten. Is this new message enough to tell the user that we didn't touch the working tree or the index, which would happen if even some part of "stash apply" happened? Or is it obvious that we did not do anything? > @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b > repo_read_index_preload(the_repository, NULL, 0); > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > NULL, NULL, NULL) < 0) { > - ret = -1; > + ret = error(_("Cannot write to the index")); > goto done; > } Ditto. > @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > NULL, NULL, NULL)) { > - ret = -1; > + ret = error(_("Cannot write to the index")); > goto done; > } > Ditto. Thanks.
On Mon, Feb 05, 2024 at 11:22:01PM +0100, Rubén Justo wrote: > On 05-feb-2024 08:01:04, Patrick Steinhardt wrote: > > The git-stash(1) command needs to write to the index for many of its > > operations. When the index is locked by a concurrent writer it will thus > > fail to operate, which is expected. What is not expected though is that > > we do not print any error message at all in this case. The user can thus > > easily miss the fact that the command didn't do what they expected it to > > do and would be left wondering why that is. > > > > Fix this bug and report failures to write to the index. Add tests for > > the subcommands which hit the respective code paths. > > > > Note that the chosen error message ("Cannot write to the index") does > > not match our guidelines as it starts with a capitalized letter. This is > > intentional though and matches the style of all the other messages used > > in git-stash(1). > > Your message made me curious, so I ran: > > $ git grep -E '(die|error)\(' builtin/stash.c > > builtin/stash.c:168: die(_("'%s' is not a stash-like commit"), revision); > builtin/stash.c:214: return error(_("%s is not a valid reference"), revision); > builtin/stash.c:261: return error(_("git stash clear with arguments is " > builtin/stash.c:303: return error(_("unable to write new index file")); > builtin/stash.c:487: die("Failed to move %s to %s\n", > builtin/stash.c:523: die(_("Unable to write index.")); > builtin/stash.c:544: return error(_("cannot apply a stash in the middle of a merge")); > builtin/stash.c:555: return error(_("could not generate diff %s^!."), > builtin/stash.c:562: return error(_("conflicts in index. " > builtin/stash.c:569: return error(_("could not save index tree")); > builtin/stash.c:610: ret = error(_("could not write index")); > builtin/stash.c:630: ret = error(_("could not restore untracked files from stash")); > builtin/stash.c:702: return error(_("%s: Could not drop stash entry"), > builtin/stash.c:721: return error(_("'%s' is not a stash reference"), info->revision.buf); > builtin/stash.c:872: die(_("failed to parse tree")); > builtin/stash.c:883: die(_("failed to unpack trees")); > builtin/stash.c:1547: if (report_path_error(ps_matched, ps)) { > builtin/stash.c:1763: die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'", > builtin/stash.c:1773: die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch"); > builtin/stash.c:1776: die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--staged"); > builtin/stash.c:1779: die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file"); > builtin/stash.c:1785: die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file"); > > Certainly, there are some error messages in builtin/stash.c that do not > follow the notes in Documentation/CodingGuideLines, but it does not seem > to be "the style of all other messages" in git-stash. > > However, your message is clear ... What error messages are you > considering? That's because many of the code paths don't use either `error()` or `die()`, but use `fprintf_ln()` instead: $ git grep 'fprintf_ln(stderr' -- builtin/stash.c builtin/stash.c: fprintf_ln(stderr, _("Too many revisions specified:%s"), builtin/stash.c: fprintf_ln(stderr, _("No stash entries found.")); builtin/stash.c: fprintf_ln(stderr, _("Index was not unstashed.")); builtin/stash.c: fprintf_ln(stderr, _("No branch name specified")); builtin/stash.c: fprintf_ln(stderr, _("Cannot update %s with %s"), builtin/stash.c: fprintf_ln(stderr, _("\"git stash store\" requires one " builtin/stash.c: fprintf_ln(stderr, _("Cannot update %s with %s"), builtin/stash.c: fprintf_ln(stderr, _("No staged changes")); builtin/stash.c: fprintf_ln(stderr, _("No changes selected")); builtin/stash.c: fprintf_ln(stderr, _("You do not have " builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current " builtin/stash.c: fprintf_ln(stderr, _("Cannot save " builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current " builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current " builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current " builtin/stash.c: fprintf_ln(stderr, _("Cannot record " builtin/stash.c: fprintf_ln(stderr, _("Can't use --patch and --include-untracked" builtin/stash.c: fprintf_ln(stderr, _("Can't use --staged and --include-untracked" builtin/stash.c: fprintf_ln(stderr, _("Did you forget to 'git add'?")); builtin/stash.c: fprintf_ln(stderr, _("Cannot initialize stash")); builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current status")); builtin/stash.c: fprintf_ln(stderr, _("Cannot remove " Overall, "builtin/stash.c" is a wild mixture of error styles. Some are likely user-facing, where it might be a good idea to use `fprinf_ln()` instead of `error()`. But some of them are closer to what we do in this patch and likely should be converted to use `error()`, too. I dunno, I find this to be confusing. But I spotted that there's an instance already where we say `error("cannot write index")`, so I'll just use that. Patrick > > > > Reported-by: moti sd <motisd8@gmail.com> > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > builtin/stash.c | 6 +++--- > > t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 55 insertions(+), 3 deletions(-) > > > > diff --git a/builtin/stash.c b/builtin/stash.c > > index b2813c614c..9df072b459 100644 > > --- a/builtin/stash.c > > +++ b/builtin/stash.c > > @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > > repo_read_index_preload(the_repository, NULL, 0); > > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > > NULL, NULL, NULL)) > > - return -1; > > + return error(_("Cannot write to the index")); > > > > if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0, > > NULL)) > > @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b > > repo_read_index_preload(the_repository, NULL, 0); > > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > > NULL, NULL, NULL) < 0) { > > - ret = -1; > > + ret = error(_("Cannot write to the index")); > > goto done; > > } > > > > @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > > > > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > > NULL, NULL, NULL)) { > > - ret = -1; > > + ret = error(_("Cannot write to the index")); > > goto done; > > } > > > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > > index 3319240515..770881e537 100755 > > --- a/t/t3903-stash.sh > > +++ b/t/t3903-stash.sh > > @@ -1516,4 +1516,56 @@ test_expect_success 'restore untracked files even when we hit conflicts' ' > > ) > > ' > > > > +test_expect_success 'stash create reports a locked index' ' > > + test_when_finished "rm -rf repo" && > > + git init repo && > > + ( > > + cd repo && > > + test_commit A A.file && > > + echo change >A.file && > > + touch .git/index.lock && > > + > > + cat >expect <<-EOF && > > + error: Cannot write to the index > > + EOF > > + test_must_fail git stash create 2>err && > > + test_cmp expect err > > + ) > > +' > > + > > +test_expect_success 'stash push reports a locked index' ' > > + test_when_finished "rm -rf repo" && > > + git init repo && > > + ( > > + cd repo && > > + test_commit A A.file && > > + echo change >A.file && > > + touch .git/index.lock && > > + > > + cat >expect <<-EOF && > > + error: Cannot write to the index > > + EOF > > + test_must_fail git stash push 2>err && > > + test_cmp expect err > > + ) > > +' > > + > > +test_expect_success 'stash apply reports a locked index' ' > > + test_when_finished "rm -rf repo" && > > + git init repo && > > + ( > > + cd repo && > > + test_commit A A.file && > > + echo change >A.file && > > + git stash push && > > + touch .git/index.lock && > > + > > + cat >expect <<-EOF && > > + error: Cannot write to the index > > + EOF > > + test_must_fail git stash apply 2>err && > > + test_cmp expect err > > + ) > > +' > > + > > test_done > > -- > > 2.43.GIT > > > >
On Mon, Feb 05, 2024 at 07:24:49PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > The git-stash(1) command needs to write to the index for many of its > > operations. When the index is locked by a concurrent writer it will thus > > fail to operate, which is expected. What is not expected though is that > > we do not print any error message at all in this case. The user can thus > > easily miss the fact that the command didn't do what they expected it to > > do and would be left wondering why that is. > > Hopefully, they know they notice the exit status of the command, or > do we throw the error away and exit(0) from the program? We do return a proper exit code as demonstrated by the tests. But if you interactively use those commands in the shell then you're quite likely to not notice error codes at all -- my shell certainly doesn't highlight failed commands in any special way. > In any case, telling the users what did (and did not) happen is a > good idea. > > > Fix this bug and report failures to write to the index. Add tests for > > the subcommands which hit the respective code paths. > > > > Note that the chosen error message ("Cannot write to the index") does > > not match our guidelines as it starts with a capitalized letter. This is > > intentional though and matches the style of all the other messages used > > in git-stash(1). > > Style may be OK, but I wonder if they should say different things, > to hint what failed. For example: > > > @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > > repo_read_index_preload(the_repository, NULL, 0); > > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, > > NULL, NULL, NULL)) > > - return -1; > > + return error(_("Cannot write to the index")); > > > > if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0, > > NULL)) > > This failure and message comes before anything interesting happens. > We attempted to refresh the current index and failed to write out > the result, meaning that whatever index we had on disk did not get > overwritten. Is this new message enough to tell the user that we > didn't touch the working tree or the index, which would happen if > even some part of "stash apply" happened? Or is it obvious that we > did not do anything? As a user, my expectation is that if a command failed, it didn't do anything. If it did something before failing and wasn't able to clean it up, then it is the responsibility of the command to tell me that it might have screwed up and left behind some partially-applied changes. It could certainly be that my expectation is way off. But personally, I don't think it's useful to say "We didn't do anything" in every case where we failed without doing anything -- I'd rather feel that it is quite spammy. But anyway, I know that my UX skills are severely lacking. So in case you or anybody else has a specific suggestion for how to make it better then I'm certainly happy to adapt. Patrick
diff --git a/builtin/stash.c b/builtin/stash.c index b2813c614c..9df072b459 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, repo_read_index_preload(the_repository, NULL, 0); if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, NULL, NULL, NULL)) - return -1; + return error(_("Cannot write to the index")); if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0, NULL)) @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b repo_read_index_preload(the_repository, NULL, 0); if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, NULL, NULL, NULL) < 0) { - ret = -1; + ret = error(_("Cannot write to the index")); goto done; } @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0, NULL, NULL, NULL)) { - ret = -1; + ret = error(_("Cannot write to the index")); goto done; } diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 3319240515..770881e537 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1516,4 +1516,56 @@ test_expect_success 'restore untracked files even when we hit conflicts' ' ) ' +test_expect_success 'stash create reports a locked index' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit A A.file && + echo change >A.file && + touch .git/index.lock && + + cat >expect <<-EOF && + error: Cannot write to the index + EOF + test_must_fail git stash create 2>err && + test_cmp expect err + ) +' + +test_expect_success 'stash push reports a locked index' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit A A.file && + echo change >A.file && + touch .git/index.lock && + + cat >expect <<-EOF && + error: Cannot write to the index + EOF + test_must_fail git stash push 2>err && + test_cmp expect err + ) +' + +test_expect_success 'stash apply reports a locked index' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit A A.file && + echo change >A.file && + git stash push && + touch .git/index.lock && + + cat >expect <<-EOF && + error: Cannot write to the index + EOF + test_must_fail git stash apply 2>err && + test_cmp expect err + ) +' + test_done
The git-stash(1) command needs to write to the index for many of its operations. When the index is locked by a concurrent writer it will thus fail to operate, which is expected. What is not expected though is that we do not print any error message at all in this case. The user can thus easily miss the fact that the command didn't do what they expected it to do and would be left wondering why that is. Fix this bug and report failures to write to the index. Add tests for the subcommands which hit the respective code paths. Note that the chosen error message ("Cannot write to the index") does not match our guidelines as it starts with a capitalized letter. This is intentional though and matches the style of all the other messages used in git-stash(1). Reported-by: moti sd <motisd8@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/stash.c | 6 +++--- t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-)