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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 8, 2021, 03:21 UTC
Message-ID: <YYiXw41upJfPS7l0@nand.local>
In-Reply-To: <20211107213012.6978-1-git@birktj.no>

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.

Show 9 quoted lines

> 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.

Show 10 quoted lines

> 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