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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

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

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.