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

[Taylor Blau <me@ttaylorr.com>]

I forgot to mention in my earlier email, but in 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))

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