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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 8, 2021, 04:15 UTC
Message-ID: <YYike53eJEjSX3er@nand.local>
In-Reply-To: <YYiXw41upJfPS7l0@nand.local>

I forgot to mention in my earlier email, but in this hunk:

Show 13 quoted lines

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

Not the fault of your patch, but this hunk is unlike the others in that it only checks the return value of refresh_and_write_cache() is non-zero, not non-negative.

Looking through refresh_and_write_cache(), we can return a non-zero value in any one of three cases:

  - We could not acquire the index.lock file with
    repo_hold_locked_index(), or

  - We failed to write the index (indicated by write_locked_index()
    failing), or

  - refresh_index() returned a non-*zero* value, which happens when it
    sets its `has_errors` variable to 1.

So because even non-zero positive return values from this function indicate an error, this is OK. In other words, the current implementation of refresh_and_write_cache() (and the functions that it calls) make it so that it doesn't matter if you check whether the return value is negative, or non-zero.

But at least for consistency with the other callers (not to mention saving future readers in this area the same thought process I just wrote down here) it may be worth changing this to:

    if (refresh_and_write_cache(...) < 0)
      return -1;

Thanks, Taylor