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
