Re: [PATCH] stash: show error message when lockfile is present
From: Taylor Blau <hidden>
Date: 2021-11-08 04:16:00
I forgot to mention in my earlier email, but in this hunk:
quoted hunk ↗ jump to 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