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.

On Sun, Nov 07, 2021 at 11:10:17PM -0800, Junio C Hamano wrote:

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; this is a much better approach. It makes Birk's change much more
direct, and does not require plumbing a new parameter through many
function calls.

Doing something like this (with appropriate tests) would look good to
me.

Thanks,
Taylor

On Sun, Nov 07 2021, Junio C Hamano wrote:

We shouldn't be doing that because we won't get an error that's as
meaningful as what we'll get from unable_to_lock_message().

I think the patch as-is is taking the right approach. It would be nice
to see a re-indentation of the argument list, and perhaps we should
provide another macro name for this one caller, but those are all nits.

The "quiet" here is orthagonal, it's to disable the chatty output from
read-cache.c.