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

[Patrick Steinhardt <ps@pks.im>]

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