git bug

git bug

[moti sd]

[-- Attachment #1.1: Type: text/plain, Size: 1985 bytes --]

Dear Git Team,

I am writing to report a potential bug encountered while using the "git
stash" command. The issue was observed during a troubleshooting. Please
find the details below:

*Problem Description:*

The "git stash" command is not providing any feedback or indication of
execution, even though modifications are present and the stash operation is
attempted.
*Steps to Reproduce:*

Make modifications to a file.
Execute "git stash" to stash changes.
*Expected Behavior:*

The "git stash" command should provide feedback on the stash operation,
confirming whether it was successful or if there were any errors.
*Additional Information:*
The issue was initially observed when executing "git commit -a --amend" and
pressing Ctrl+Z to exit. Subsequent attempts to use "git stash" resulted in
an error.
Testing with a provided ZIP file containing a problematic repository
confirmed the issue's recurrence.
*Investigation and Findings:*

Upon investigation, it was discovered that the use of Ctrl+Z after "git
commit -a --amend" might be the root cause.
Deleting a lock file left behind by the incomplete git command resolved the
issue, indicating a potential bug in how "git stash" handles incomplete
commands.
*Recommendation:*

The "git stash" command should provide meaningful feedback or error
messages to users, even in cases where the operation is incomplete or
encounters issues.
*Steps to Reproduce the Bug:*

Execute "git commit -a --amend."
Press Ctrl+Z to exit the command.
Attempt "git stash."
*Attachment:*
A ZIP file containing a problematic repository is attached for your
reference.
*Impact:*

The bug affects users' ability to use the "git stash" command effectively,
leading to potential confusion and hindering workflow.
Resolution:


I appreciate your attention to this matter, and I believe addressing this
bug will enhance the overall user experience with Git. If further
information or testing is required, please feel free to reach out.


Best regards,
Moti

[-- Attachment #1.2: Type: text/html, Size: 2332 bytes --]

[-- Attachment #2: git-bugreport-2024-02-05-0015.txt --]
[-- Type: text/plain, Size: 1262 bytes --]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
I executed "git commit -a --amend," where I pressed Ctrl+Z to exit. Later, when trying again, an error occurred.

What did you expect to happen? (Expected behavior)
I 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.
What happened instead? (Actual behavior)
Due to .git/lock file, git stash does not behave normally, and the error message does not show up

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.38.1.windows.1
cpu: x86_64
built from commit: b85c8f604d375d4d773a36842964e8a7ec056aae
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 22631 
compiler info: gnuc: 12.2
libc info: no libc information available
$SHELL (typically, interactive shell): <unset>


[Enabled Hooks]

[-- Attachment #3: git-diagnostics-2024-02-05-0015.zip --]
[-- Type: application/x-zip-compressed, Size: 844 bytes --]

[-- Attachment #4: 02-stash-amend-wd.no-quiere-stashear.zip --]
[-- Type: application/x-zip-compressed, Size: 28391 bytes --]

Re: git bug

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

On Mon, Feb 05, 2024 at 12:42:03AM +0100, moti sd wrote:
> Dear Git Team,
> 
> I am writing to report a potential bug encountered while using the "git
> stash" command. The issue was observed during a troubleshooting. Please
> find the details below:
> 
> *Problem Description:*
> 
> The "git stash" command is not providing any feedback or indication of
> execution, even though modifications are present and the stash operation is
> attempted.
> *Steps to Reproduce:*
> 
> Make modifications to a file.
> Execute "git stash" to stash changes.
> *Expected Behavior:*
> 
> The "git stash" command should provide feedback on the stash operation,
> confirming whether it was successful or if there were any errors.
> *Additional Information:*
> The issue was initially observed when executing "git commit -a --amend" and
> pressing Ctrl+Z to exit. Subsequent attempts to use "git stash" resulted in
> an error.
> Testing with a provided ZIP file containing a problematic repository
> confirmed the issue's recurrence.
> *Investigation and Findings:*
> 
> Upon investigation, it was discovered that the use of Ctrl+Z after "git
> commit -a --amend" might be the root cause.
> Deleting a lock file left behind by the incomplete git command resolved the
> issue, indicating a potential bug in how "git stash" handles incomplete
> commands.
> *Recommendation:*

This is indeed it. The problem is that the index will be locked while
git-commit(1) is active, and thus git-stash(1) cannot write to it at the
same time. So this is working as designed. What's not though is the fact
that we don't print an error message.

I'll send a patch in reply to this message to fix this bug. Thanks for
your report!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] builtin/stash: report failure to write to index

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

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(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] builtin/stash: report failure to write to index

[Rubén Justo]

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
> 

Re: [PATCH] builtin/stash: report failure to write to index

[Junio C Hamano]

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.

Re: [PATCH] builtin/stash: report failure to write to index

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 9056 bytes --]

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
> > 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] builtin/stash: report failure to write to index

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] builtin/stash: report failure to write to index

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 7465 bytes --]

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.

While at it, unify error messages when writing to the index fails. The
chosen error message is already used in "builtin/stash.c".

Reported-by: moti sd <motisd8@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Range-diff against v1:
1:  f114688eac < -:  ---------- drop dot
2:  b13a5a10ac ! 1:  cb098cf88c builtin/stash: report failure to write to index
    @@ Commit message
         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).
    +    While at it, unify error messages when writing to the index fails. The
    +    chosen error message is already used in "builtin/stash.c".
     
         Reported-by: moti sd <motisd8@gmail.com>
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## builtin/stash.c ##
    +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)
    + 	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
    + 	if (write_locked_index(&the_index, &lock,
    + 			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
    +-		die(_("Unable to write index."));
    ++		die(_("could not write index"));
    + }
    + 
    + static int do_apply_stash(const char *prefix, struct stash_info *info,
     @@ builtin/stash.c: 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"));
    ++		return error(_("could not write index"));
      
      	if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,
      				NULL))
    @@ builtin/stash.c: static int do_create_stash(const struct pathspec *ps, struct st
      	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"));
    ++		ret = error(_("could not write index"));
      		goto done;
      	}
      
    @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char
      	if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
      					 NULL, NULL, NULL)) {
     -		ret = -1;
    -+		ret = error(_("Cannot write to the index"));
    ++		ret = error(_("could not write index"));
      		goto done;
      	}
      
    @@ t/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit
     +		touch .git/index.lock &&
     +
     +		cat >expect <<-EOF &&
    -+		error: Cannot write to the index
    ++		error: could not write index
     +		EOF
     +		test_must_fail git stash create 2>err &&
     +		test_cmp expect err
    @@ t/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit
     +		touch .git/index.lock &&
     +
     +		cat >expect <<-EOF &&
    -+		error: Cannot write to the index
    ++		error: could not write index
     +		EOF
     +		test_must_fail git stash push 2>err &&
     +		test_cmp expect err
    @@ t/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit
     +		touch .git/index.lock &&
     +
     +		cat >expect <<-EOF &&
    -+		error: Cannot write to the index
    ++		error: could not write index
     +		EOF
     +		test_must_fail git stash apply 2>err &&
     +		test_cmp expect err

 builtin/stash.c  |  8 ++++----
 t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 26489b76c0..d65cd20ee6 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -520,7 +520,7 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)
 	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
 	if (write_locked_index(&the_index, &lock,
 			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
-		die(_("Unable to write index."));
+		die(_("could not write index"));
 }
 
 static int do_apply_stash(const char *prefix, struct stash_info *info,
@@ -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(_("could not write 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(_("could not write 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(_("could not write index"));
 		goto done;
 	}
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3319240515..00db82fb24 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: could not write 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: could not write 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: could not write index
+		EOF
+		test_must_fail git stash apply 2>err &&
+		test_cmp expect err
+	)
+'
+
 test_done
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]