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

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

[Birk Tjelmeland]

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.

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.

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(-)

diff --git a/add-interactive.c b/add-interactive.c
index 6498ae196f..e51d19e1b5 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -810,7 +810,7 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,
 				       COMMIT_LOCK) < 0)
 		res = -1;
 	else
-		res = repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0, 1,
+		res = repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0, 0, 1,
 						   NULL, NULL, NULL);
 
 	if (!res)
@@ -1150,7 +1150,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
 
 	if (discard_index(r->index) < 0 ||
 	    repo_read_index(r) < 0 ||
-	    repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1,
+	    repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 0, 1,
 					 NULL, NULL, NULL) < 0)
 		warning(_("could not refresh index"));
 
diff --git a/add-patch.c b/add-patch.c
index 8c41cdfe39..d8000f02d4 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1680,7 +1680,7 @@ static int patch_update_file(struct add_p_state *s,
 				error(_("'git apply' failed"));
 		}
 		if (repo_read_index(s->s.r) >= 0)
-			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
+			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0, 0,
 						     1, NULL, NULL, NULL);
 	}
 
@@ -1733,7 +1733,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0 ||
 	    (!s.mode->index_only &&
-	     repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1,
+	     repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 0, 1,
 					  NULL, NULL, NULL) < 0) ||
 	    parse_diff(&s, ps) < 0) {
 		add_p_state_clear(&s);
diff --git a/builtin/am.c b/builtin/am.c
index 8677ea2348..a0dead658c 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1750,7 +1750,7 @@ static void am_run(struct am_state *state, int resume)
 
 	unlink(am_path(state, "dirtyindex"));
 
-	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0)
+	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0, 0) < 0)
 		die(_("unable to write index file"));
 
 	if (repo_index_has_changes(the_repository, NULL, &sb)) {
diff --git a/builtin/merge.c b/builtin/merge.c
index ea3112e0c0..9d168ee965 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -714,7 +714,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 {
 	const char *head_arg = "HEAD";
 
-	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)
+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0, 0) < 0)
 		return error(_("Unable to write index."));
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree") ||
@@ -900,7 +900,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 	struct object_id result_tree, result_commit;
 	struct commit_list *parents, **pptr = &parents;
 
-	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)
+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0, 0) < 0)
 		return error(_("Unable to write index."));
 
 	write_tree_trivial(&result_tree);
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))
@@ -1277,7 +1277,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 	prepare_fallback_ident("git stash", "git@stash");
 
 	read_cache_preload(NULL);
-	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0) {
+	if (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0) < 0) {
 		ret = -1;
 		goto done;
 	}
@@ -1443,7 +1443,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		free(ps_matched);
 	}
 
-	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) {
+	if (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0)) {
 		ret = -1;
 		goto done;
 	}
diff --git a/cache.h b/cache.h
index eba12487b9..fee557f409 100644
--- a/cache.h
+++ b/cache.h
@@ -444,7 +444,7 @@ extern struct index_state the_index;
 #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))
 #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))
 #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)
-#define refresh_and_write_cache(refresh_flags, write_flags, gentle) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), (gentle), NULL, NULL, NULL)
+#define refresh_and_write_cache(refresh_flags, write_flags, lock_flags, gentle) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), (lock_flags), (gentle), NULL, NULL, NULL)
 #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))
 #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))
 #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))
@@ -933,7 +933,7 @@ int refresh_index(struct index_state *, unsigned int flags, const struct pathspe
  * Note that if refreshing the index returns an error, we still write
  * out the index (unless locking fails).
  */
-int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, int gentle, const struct pathspec *, char *seen, const char *header_msg);
+int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, int lock_flags, int gentle, const struct pathspec *, char *seen, const char *header_msg);
 
 struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
 
diff --git a/read-cache.c b/read-cache.c
index f398659662..01f2a2d470 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1540,6 +1540,7 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,
 int repo_refresh_and_write_index(struct repository *repo,
 				 unsigned int refresh_flags,
 				 unsigned int write_flags,
+				 int lock_flags,
 				 int gentle,
 				 const struct pathspec *pathspec,
 				 char *seen, const char *header_msg)
@@ -1547,7 +1548,7 @@ int repo_refresh_and_write_index(struct repository *repo,
 	struct lock_file lock_file = LOCK_INIT;
 	int fd, ret = 0;
 
-	fd = repo_hold_locked_index(repo, &lock_file, 0);
+	fd = repo_hold_locked_index(repo, &lock_file, lock_flags);
 	if (!gentle && fd < 0)
 		return -1;
 	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
-- 
2.33.1

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

[Taylor Blau]

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

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

[Taylor Blau]

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

I think most of the changes in this patch, other than the ones to
builtin/stash.c, are unwanted, and I suspect what you wondered above
may be the same thing.  Take for example 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))

Telling the function to be quiet and at the same time be noisy on
only one particular kind of error sounds somewhat strange.  I do not
think of any reason why we should believe that failing to lock will
be the only special kind of failure to be of interest to the users.

I would think the "fix" should look more like this:

 	read_cache_preload(NULL);
	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
- 		return -1;
+ 		return error(_("failed to refresh the index"));

That is, tell the function that the caller will do the error
reporting (i.e. "QUIET") and do so.

Thanks.

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

[Taylor Blau]

On Sun, Nov 07, 2021 at 11:10:17PM -0800, Junio C Hamano wrote:
> I would think the "fix" should look more like this:
>
>  	read_cache_preload(NULL);
> 	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
> - 		return -1;
> + 		return error(_("failed to refresh the index"));
>
> That is, tell the function that the caller will do the error
> reporting (i.e. "QUIET") and do so.

Thanks; this is a much better approach. It makes Birk's change much more
direct, and does not require plumbing a new parameter through many
function calls.

Doing something like this (with appropriate tests) would look good to
me.

Thanks,
Taylor

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

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 07 2021, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
>
>> 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.
>> ...
>>>  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(-)
>
> I think most of the changes in this patch, other than the ones to
> builtin/stash.c, are unwanted, and I suspect what you wondered above
> may be the same thing.  Take for example 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))
>
> Telling the function to be quiet and at the same time be noisy on
> only one particular kind of error sounds somewhat strange.  I do not
> think of any reason why we should believe that failing to lock will
> be the only special kind of failure to be of interest to the users.
>
> I would think the "fix" should look more like this:
>
>  	read_cache_preload(NULL);
> 	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
> - 		return -1;
> + 		return error(_("failed to refresh the index"));
>
> That is, tell the function that the caller will do the error
> reporting (i.e. "QUIET") and do so.
>
> Thanks.

We shouldn't be doing that because we won't get an error that's as
meaningful as what we'll get from unable_to_lock_message().

I think the patch as-is is taking the right approach. It would be nice
to see a re-indentation of the argument list, and perhaps we should
provide another macro name for this one caller, but those are all nits.

The "quiet" here is orthagonal, it's to disable the chatty output from
read-cache.c.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> The "quiet" here is orthagonal, it's to disable the chatty output from
> read-cache.c.

It shouldn't be.  read-cache.c may want to report errors and
progresses, and among the errors it may not include lock-related
things.  If the change were to (1) extend the flag word to make it
more uniform, so that it does not special case ONLY the lock-related
errors, and to (2) figure out how to grab richer diagnoses out of
the lower level API functions, and to (3) reduce the messages issued
by the lower-level API functions and have it callers' responsibility
to give the messages, that would have been palatable, but the patch
as posted goes totally the opposite direction on all these points.

Yuck.

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

[Birk Tjelmeland]

> I would think the "fix" should look more like this:
>
>  	read_cache_preload(NULL);
> 	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
> - 		return -1;
> + 		return error(_("failed to refresh the index"));

This is how the other callsites of refresh_and_write_cache (the ones
outside of stash.c) solve this problem, however I agree with Ævar that
this error is not as useful as the one given by
LOCK_REPORT_ON_ERROR (which is why I wrote the patch this way
to begin with, although I do see that it is not exactly the most elegant
solution to the problem)

> If the change were to (1) extend the flag word to make it more uniform,
> so that it does not special case ONLY the lock-related errors

Would adding a REFRESH_REPORT_ERRORS flag which could then be
used with repo_refresh_and_write_index be a more acceptable solution? 
Then various functions in read-cache.c could check for this flag and
output errors but not progress when it is present.

Regards,
Birk