On Sun, Nov 07, 2021 at 10:30:12PM +0100, Birk Tjelmeland wrote:
> Multiple git-stash commands silently fail when index.lock is present,
> including git stash push and git stash apply. This is somewhat confusing
> and a better behaviour would probably be to exit with a meaningful error
> message like most other git commands do.
We do terminate with non-zero exit code when trying to, for e.g., 'git
stash push' when $GIT_DIR/index.lock already exists. That is reflected
in your patch by not adding any new paths which we return, which makes
sense.
> This patch updates repo_refresh_and_write_index to accept another
> parameter lock_flags and updates some callsites of this function to call
> it with LOCK_REPORT_ON_ERROR resulting a suitable error message when the
> relevant git-stash commands used on a repo with an index.lock file.
>
> This patch only adds the described error message to git-stash commands,
> however the diff highlights other uses of repo_refresh_and_write_index
> which could also benefit from the changes. On the other hand these
> callsites already have some limited error messages.
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.
> Signed-off-by: Birk Tjelmeland <git@xxxxxxxxx>
> ---
> 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(-)
It looks like the sum-total of this patch are a few things:
- repo_refresh_and_write_index() gets a new lock_flags parameter which
is passed down to repo_hold_locked_index()
- refresh_and_write_cache() which is a thin wrapper around
repo_refresh_and_write_index() also learned the new parameter
- do_apply_stash(), do_create_stash(), do_push_stash() all pass
LOCK_REPORT_ON_ERROR via the new lock_flags parameter
That all makes sense to me. It results in us printing a helpful error
message when we couldn't acquire an exclusive lock on
$GIT_DIR/index.lock where before we would have silently failed and
exited non-zero (which is not exactly a *silent* failure, but it is
close).
This patch does not include any tests, which I think that you should add
in another revision before we consider queuing this.
Thanks,
Taylor
