Re: [PATCH] stash: show error message when lockfile is present

Taylor Blau [off-list ref] writes:

I wonder if there are callers of repo_refresh_and_write_index() that
don't want any errors reported. Not having thought about it too hard
(much less looked through any of these callers), I would expect that
having the choice to either error() or die() is something worth keeping.
But I do not know if there are callers which want neither.
...

quoted

 add-interactive.c | 4 ++--
 add-patch.c       | 4 ++--
 builtin/am.c      | 2 +-
 builtin/merge.c   | 4 ++--
 builtin/stash.c   | 6 +++---
 cache.h           | 4 ++--
 read-cache.c      | 3 ++-
 7 files changed, 14 insertions(+), 13 deletions(-)

I think most of the changes in this patch, other than the ones to
builtin/stash.c, are unwanted, and I suspect what you wondered above
may be the same thing.  Take for example this hunk:

diff --git a/builtin/stash.c b/builtin/stash.c
index a0ccc8654d..977fcc4e40 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c

@@ -501,7 +501,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 	const struct object_id *bases[1];
 
 	read_cache_preload(NULL);
-	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
+	if (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0))
 		return -1;
 
 	if (write_cache_as_tree(&c_tree, 0, NULL))

Telling the function to be quiet and at the same time be noisy on
only one particular kind of error sounds somewhat strange.  I do not
think of any reason why we should believe that failing to lock will
be the only special kind of failure to be of interest to the users.

I would think the "fix" should look more like this:

 	read_cache_preload(NULL);
	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
- 		return -1;
+ 		return error(_("failed to refresh the index"));

That is, tell the function that the caller will do the error
reporting (i.e. "QUIET") and do so.

Thanks.