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

[Taylor Blau <me@ttaylorr.com>]

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@birktj.no>
> ---
>  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