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

From

Junio C Hamano <gitster@pobox.com>

Date

Nov 8, 2021, 07:10 UTC

Message-ID

<xmqqlf1zunqe.fsf@gitster.g>

In-Reply-To

<YYiXw41upJfPS7l0@nand.local>

Taylor Blau <me@ttaylorr.com> 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.
> ...
>>  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(-)

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.