[PATCH 0/3] make sure stash refreshes the index properly

[PATCH 0/3] make sure stash refreshes the index properly

[Thomas Gummerer]

Thanks Peff for spotting the bug!  Here's a series that fixes it.

> And before the third one, introduction of a new entry point that
> makes merge-recursive machinery inherit the already populated
> in-core index, happens, I think the right solution is to write the
> in-core index out---the write is not pointless.

Yup, I agree with that.  In fact there are some other places where we
just call 'refresh_cache()' as a replacement for 'git update-index
--refresh'.  At least the other one in 'do_apply_stash()' also seems
like a bug, as I assume the original intention (and behaviour) was
that the index is refreshed after 'stash apply -q' finishes.

I think in do_push_stash and do_create_stash we might be able to get
away without the write, but I wasn't 100% sure, so I made them write
the index after refreshing it as well, which is what the shell script
did.

The first patch is a small refactoring that makes the actual fix a bit
easier, while the second patch is a cleanup that I found while there.

Thomas Gummerer (3):
  factor out refresh_and_write_cache function
  merge: use refresh_and_write_cache
  stash: make sure to write refreshed cache

 builtin/am.c     | 16 ++--------------
 builtin/merge.c  | 15 ++++-----------
 builtin/stash.c  | 11 +++++++----
 cache.h          |  9 +++++++++
 read-cache.c     | 17 +++++++++++++++++
 t/t3903-stash.sh | 16 ++++++++++++++++
 6 files changed, 55 insertions(+), 29 deletions(-)

-- 
2.23.0.rc2.194.ge5444969c9

[PATCH 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer]

Getting the lock for the index, refreshing it and then writing it is a
pattern that happens more than once throughout the codebase.  Factor
out the refresh_and_write_cache function from builtin/am.c to
read-cache.c, so it can be re-used in other places in a subsequent
commit.

Note that we return different error codes for failing to refresh the
cache, and failing to write the index.  The current caller only cares
about failing to write the index.  However for other callers we're
going to convert in subsequent patches we will need this distinction.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/am.c | 16 ++--------------
 cache.h      |  9 +++++++++
 read-cache.c | 17 +++++++++++++++++
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1aea657a7f..e00410e4d7 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1071,19 +1071,6 @@ static const char *msgnum(const struct am_state *state)
 	return sb.buf;
 }
 
-/**
- * Refresh and write index.
- */
-static void refresh_and_write_cache(void)
-{
-	struct lock_file lock_file = LOCK_INIT;
-
-	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		die(_("unable to write index file"));
-}
-
 /**
  * Dies with a user-friendly message on how to proceed after resolving the
  * problem. This message can be overridden with state->resolvemsg.
@@ -1703,7 +1690,8 @@ static void am_run(struct am_state *state, int resume)
 
 	unlink(am_path(state, "dirtyindex"));
 
-	refresh_and_write_cache();
+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK) < 0)
+		die(_("failed to refresh cache"));
 
 	if (repo_index_has_changes(the_repository, NULL, &sb)) {
 		write_state_bool(state, "dirtyindex", 1);
diff --git a/cache.h b/cache.h
index b1da1ab08f..f72392f32b 100644
--- a/cache.h
+++ b/cache.h
@@ -414,6 +414,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) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), 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))
@@ -812,6 +813,14 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
 #define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
 #define REFRESH_PROGRESS	0x0040  /* show progress bar if stderr is tty */
 int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
+/*
+ * Refresh the index and write it to disk.
+ *
+ * Return 1 if refreshing the cache failed, -1 if writing the cache to
+ * disk failed, 0 on success.
+ */
+int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, const struct pathspec *, char *seen, const char *header_msg);
+
 struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
 
 void set_alternate_index_output(const char *);
diff --git a/read-cache.c b/read-cache.c
index 52ffa8a313..905d2ddd10 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1472,6 +1472,23 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,
 	printf(fmt, name);
 }
 
+int repo_refresh_and_write_index(struct  repository *repo,
+				 unsigned int refresh_flags,
+				 unsigned int write_flags,
+				 const struct pathspec *pathspec,
+				 char *seen, const char *header_msg)
+{
+	struct lock_file lock_file = LOCK_INIT;
+
+	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
+	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
+		return 1;
+	if (write_locked_index(repo->index, &lock_file, write_flags))
+		return error(_("unable to write index file"));
+	return 0;
+}
+
+
 int refresh_index(struct index_state *istate, unsigned int flags,
 		  const struct pathspec *pathspec,
 		  char *seen, const char *header_msg)
-- 
2.23.0.rc2.194.ge5444969c9

Re: [PATCH 1/3] factor out refresh_and_write_cache function

[Martin Ågren]

On Tue, 27 Aug 2019 at 12:14, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> Getting the lock for the index, refreshing it and then writing it is a
> pattern that happens more than once throughout the codebase.  Factor
> out the refresh_and_write_cache function from builtin/am.c to
> read-cache.c, so it can be re-used in other places in a subsequent
> commit.

> +/*
> + * Refresh the index and write it to disk.
> + *
> + * Return 1 if refreshing the cache failed, -1 if writing the cache to
> + * disk failed, 0 on success.
> + */

Thank you for documenting. :-) Should we say something about how this
doesn't explicitly print any error in case refreshing fails (that is, we
leave it to `refresh_index()`), but that we *do* explicitly print an
error if writing the index fails? That caught me off-guard as I looked
at how you convert the callers.

And do we actually want that asymmetry? Maybe we do.

Might be worth pointing out as you convert the callers how some (all?)
of them now emit different error messages from before, but that it
shouldn't matter(?) and it makes sense to unify those messages.

> +int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, const struct pathspec *, char *seen, const char *header_msg);

> +int repo_refresh_and_write_index(struct  repository *repo,
> +                                unsigned int refresh_flags,
> +                                unsigned int write_flags,
> +                                const struct pathspec *pathspec,
> +                                char *seen, const char *header_msg)
> +{
> +       struct lock_file lock_file = LOCK_INIT;
> +
> +       repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> +       if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
> +               return 1;
> +       if (write_locked_index(repo->index, &lock_file, write_flags))
> +               return error(_("unable to write index file"));
> +       return 0;
> +}

If `flags` doesn't contain `COMMIT_LOCK`, the lockfile will be closed
"gently", meaning we still need to either commit it, or roll it back. Or
let the exit handler roll it back, which is what would happen here, no?
We lose our handle on the stack and there's no way for anyone to say
"ok, now I'm done, commit it please" (or "roll it back").

In short, I think calling this function without providing `COMMIT_LOCK`
would be useless at best. We should probably let this function provide
`COMMIT_LOCK | write_flags` or `COMMIT_LOCK | extra_write_flags` or
whatever. Most callers would just provide "0". Hm?

Or, we could BUG if the COMMIT_LOCK bit isn't set, but that seems like a
less good choice to me. If we're so adamant about the bit being set --
which we should be, IMHO -- we might as well set it ourselves.



Martin

Re: [PATCH 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer]

On 08/28, Martin Ågren wrote:
> On Tue, 27 Aug 2019 at 12:14, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > Getting the lock for the index, refreshing it and then writing it is a
> > pattern that happens more than once throughout the codebase.  Factor
> > out the refresh_and_write_cache function from builtin/am.c to
> > read-cache.c, so it can be re-used in other places in a subsequent
> > commit.
> 
> > +/*
> > + * Refresh the index and write it to disk.
> > + *
> > + * Return 1 if refreshing the cache failed, -1 if writing the cache to
> > + * disk failed, 0 on success.
> > + */
> 
> Thank you for documenting. :-) Should we say something about how this
> doesn't explicitly print any error in case refreshing fails (that is, we
> leave it to `refresh_index()`), but that we *do* explicitly print an
> error if writing the index fails? That caught me off-guard as I looked
> at how you convert the callers.
> 
> And do we actually want that asymmetry? Maybe we do.

I think I needed the error for something while I went through a few
iterations of how to best structure this function, but I don't
remember for what exactly now.  I think it might actually be better to
just return -1 here, and let the caller distinguish and show the error
message if they need to.  That also avoids duplicating the error in
case the caller wants to die on error.

> Might be worth pointing out as you convert the callers how some (all?)
> of them now emit different error messages from before, but that it
> shouldn't matter(?) and it makes sense to unify those messages.

Yeah, I don't think changing the error message should matter, but
unifying them is not actually a goal of this series.  So with what you
pointed out above, I think I'll leave them as they are.

> > +int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, const struct pathspec *, char *seen, const char *header_msg);
> 
> > +int repo_refresh_and_write_index(struct  repository *repo,
> > +                                unsigned int refresh_flags,
> > +                                unsigned int write_flags,
> > +                                const struct pathspec *pathspec,
> > +                                char *seen, const char *header_msg)
> > +{
> > +       struct lock_file lock_file = LOCK_INIT;
> > +
> > +       repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> > +       if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
> > +               return 1;
> > +       if (write_locked_index(repo->index, &lock_file, write_flags))
> > +               return error(_("unable to write index file"));
> > +       return 0;
> > +}
> 
> If `flags` doesn't contain `COMMIT_LOCK`, the lockfile will be closed
> "gently", meaning we still need to either commit it, or roll it back. Or
> let the exit handler roll it back, which is what would happen here, no?
> We lose our handle on the stack and there's no way for anyone to say
> "ok, now I'm done, commit it please" (or "roll it back").
> 
> In short, I think calling this function without providing `COMMIT_LOCK`
> would be useless at best. We should probably let this function provide
> `COMMIT_LOCK | write_flags` or `COMMIT_LOCK | extra_write_flags` or
> whatever. Most callers would just provide "0". Hm?
> 
> Or, we could BUG if the COMMIT_LOCK bit isn't set, but that seems like a
> less good choice to me. If we're so adamant about the bit being set --
> which we should be, IMHO -- we might as well set it ourselves.

Yeah, you're right, making this function use `COMMIT_LOCK | write_flags`
would probably be the best option.  I'll change that, and document it
as well.

Thanks for your review!

> 
> 
> Martin

[PATCH 2/3] merge: use refresh_and_write_cache

[Thomas Gummerer]

Use the 'refresh_and_write_cache()' convenience function introduced in
the last commit, instead of refreshing and writing the index manually
in merge.c

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/merge.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index e2ccbc44e2..b5e31ce283 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -691,11 +691,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 	struct lock_file lock = LOCK_INIT;
 	const char *head_arg = "HEAD";
 
-	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock,
-			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
-		return error(_("Unable to write index."));
+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)
+		return -1;
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
 		int clean, x;
@@ -860,13 +857,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 {
 	struct object_id result_tree, result_commit;
 	struct commit_list *parents, **pptr = &parents;
-	struct lock_file lock = LOCK_INIT;
 
-	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock,
-			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
-		return error(_("Unable to write index."));
+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)
+		return -1;
 
 	write_tree_trivial(&result_tree);
 	printf(_("Wonderful.\n"));
-- 
2.23.0.rc2.194.ge5444969c9

Re: [PATCH 2/3] merge: use refresh_and_write_cache

[Martin Ågren]

On Tue, 27 Aug 2019 at 12:15, Thomas Gummerer <t.gummerer@gmail.com> wrote:

>         struct lock_file lock = LOCK_INIT;
>         const char *head_arg = "HEAD";
>
> -       hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
> -       refresh_cache(REFRESH_QUIET);
> -       if (write_locked_index(&the_index, &lock,
> -                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
> -               return error(_("Unable to write index."));
> +       if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)
> +               return -1;

I wondered why you didn't drop the `struct lock_file`, but it turns out
we still need it further down.

>         if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
>                 int clean, x;

What you could do, I guess, is to move its declaration to around here.
Probably not worth a re-roll.

> @@ -860,13 +857,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
>  {
>         struct object_id result_tree, result_commit;
>         struct commit_list *parents, **pptr = &parents;
> -       struct lock_file lock = LOCK_INIT;
>
> -       hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
> -       refresh_cache(REFRESH_QUIET);
> -       if (write_locked_index(&the_index, &lock,
> -                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
> -               return error(_("Unable to write index."));
> +       if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)
> +               return -1;

Here you do drop the `struct lock_file` entirely, ok.



Martin

Re: [PATCH 2/3] merge: use refresh_and_write_cache

[Thomas Gummerer]

On 08/28, Martin Ågren wrote:
> On Tue, 27 Aug 2019 at 12:15, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> 
> >         struct lock_file lock = LOCK_INIT;
> >         const char *head_arg = "HEAD";
> >
> > -       hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
> > -       refresh_cache(REFRESH_QUIET);
> > -       if (write_locked_index(&the_index, &lock,
> > -                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
> > -               return error(_("Unable to write index."));
> > +       if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)
> > +               return -1;
> 
> I wondered why you didn't drop the `struct lock_file`, but it turns out
> we still need it further down.
> 
> >         if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
> >                 int clean, x;
> 
> What you could do, I guess, is to move its declaration to around here.
> Probably not worth a re-roll.

I'll re-roll anyway for the things you spotted in the first patch, so
I'll drop it down here while I'm at it, thanks!

> > @@ -860,13 +857,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
> >  {
> >         struct object_id result_tree, result_commit;
> >         struct commit_list *parents, **pptr = &parents;
> > -       struct lock_file lock = LOCK_INIT;
> >
> > -       hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
> > -       refresh_cache(REFRESH_QUIET);
> > -       if (write_locked_index(&the_index, &lock,
> > -                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
> > -               return error(_("Unable to write index."));
> > +       if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)
> > +               return -1;
> 
> Here you do drop the `struct lock_file` entirely, ok.
> 
> 
> 
> Martin

[PATCH 3/3] stash: make sure to write refreshed cache

[Thomas Gummerer]

When converting stash into C, calls to 'git update-index --refresh'
were replaced with the 'refresh_cache()' function.  That is fine as
long as the index is only needed in-core, and not re-read from disk.

However in many cases we do actually need the refreshed index to be
written to disk, for example 'merge_recursive_generic()' discards the
in-core index before re-reading it from disk, and in the case of 'apply
--quiet', the 'refresh_cache()' we currently have is pointless without
writing the index to disk.

Always write the index after refreshing it to ensure there are no
regressions in this compared to the scripted stash.  In the future we
can consider avoiding the write where possible after making sure none
of the subsequent calls actually need the refreshed cache, and it is
not expected to be refreshed after stash exits or it is written
somewhere else already.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash.c  | 11 +++++++----
 t/t3903-stash.sh | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index b5a301f24d..b36aada644 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -396,7 +396,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_cache(REFRESH_QUIET))
+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK))
 		return -1;
 
 	if (write_cache_as_tree(&c_tree, 0, NULL))
@@ -485,7 +485,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 	}
 
 	if (quiet) {
-		if (refresh_cache(REFRESH_QUIET))
+		if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK))
 			warning("could not refresh index");
 	} else {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -1129,7 +1129,10 @@ 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);
-	refresh_cache(REFRESH_QUIET);
+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK) < 0) {
+		ret = -1;
+		goto done;
+	}
 
 	if (get_oid("HEAD", &info->b_commit)) {
 		if (!quiet)
@@ -1290,7 +1293,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		free(ps_matched);
 	}
 
-	if (refresh_cache(REFRESH_QUIET)) {
+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK)) {
 		ret = -1;
 		goto done;
 	}
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b8e337893f..392954d6dd 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1241,4 +1241,20 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash apply should succeed with unmodified file' '
+	echo base >file &&
+	git add file &&
+	git commit -m base &&
+
+	# now stash a modification
+	echo modified >file &&
+	git stash &&
+
+	# make the file stat dirty
+	cp file other &&
+	mv other file &&
+
+	git stash apply
+'
+
 test_done
-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v2 0/3] make sure stash refreshes the index properly

[Thomas Gummerer]

Thanks Martin for the review of the last round!

Changes compared to the previous round:
- always pass COMMIT_LOCK to write_locked_index
- don't write the error message in repo_refresh_and_write_index, but
  let the caller handle that.  This means that we no longer change any
  error messages.  Potential cleanups in that area can come later.
- Drop the lock variable to the scope it needs.

Range diff below:

1:  7249a3cf4e ! 1:  1f25fe227c factor out refresh_and_write_cache function
    @@ builtin/am.c: static void am_run(struct am_state *state, int resume)
      	unlink(am_path(state, "dirtyindex"));
      
     -	refresh_and_write_cache();
    -+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK) < 0)
    -+		die(_("failed to refresh cache"));
    ++	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)
    ++		die(_("unable to write index file"));
      
      	if (repo_index_has_changes(the_repository, NULL, &sb)) {
      		write_state_bool(state, "dirtyindex", 1);
    @@ cache.h: void fill_stat_cache_info(struct index_state *istate, struct cache_entr
     +/*
     + * Refresh the index and write it to disk.
     + *
    ++ * 'refresh_flags' is passed directly to 'refresh_index()', while
    ++ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so
    ++ * the lockfile is always either committed or rolled back.
    ++ *
     + * Return 1 if refreshing the cache failed, -1 if writing the cache to
     + * disk failed, 0 on success.
     + */
    @@ read-cache.c: static void show_file(const char * fmt, const char * name, int in_
     +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
     +	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
     +		return 1;
    -+	if (write_locked_index(repo->index, &lock_file, write_flags))
    -+		return error(_("unable to write index file"));
    ++	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
    ++		return -1;
     +	return 0;
     +}
     +
2:  de5b8c1529 ! 2:  148a65d649 merge: use refresh_and_write_cache
    @@ Commit message
     
      ## builtin/merge.c ##
     @@ builtin/merge.c: static int try_merge_strategy(const char *strategy, struct commit_list *common,
    - 	struct lock_file lock = LOCK_INIT;
    + 			      struct commit_list *remoteheads,
    + 			      struct commit *head)
    + {
    +-	struct lock_file lock = LOCK_INIT;
      	const char *head_arg = "HEAD";
      
     -	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
     -	refresh_cache(REFRESH_QUIET);
     -	if (write_locked_index(&the_index, &lock,
     -			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
    --		return error(_("Unable to write index."));
    -+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)
    -+		return -1;
    ++	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)
    + 		return error(_("Unable to write index."));
      
      	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
    ++		struct lock_file lock = LOCK_INIT;
      		int clean, x;
    + 		struct commit *result;
    + 		struct commit_list *reversed = NULL;
     @@ builtin/merge.c: static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
      {
      	struct object_id result_tree, result_commit;
    @@ builtin/merge.c: static int merge_trivial(struct commit *head, struct commit_lis
     -	refresh_cache(REFRESH_QUIET);
     -	if (write_locked_index(&the_index, &lock,
     -			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
    --		return error(_("Unable to write index."));
    -+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)
    -+		return -1;
    ++	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)
    + 		return error(_("Unable to write index."));
      
      	write_tree_trivial(&result_tree);
    - 	printf(_("Wonderful.\n"));
3:  d9efda0f2a ! 3:  e0f6815192 stash: make sure to write refreshed cache
    @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info
      
      	read_cache_preload(NULL);
     -	if (refresh_cache(REFRESH_QUIET))
    -+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK))
    ++	if (refresh_and_write_cache(REFRESH_QUIET, 0))
      		return -1;
      
      	if (write_cache_as_tree(&c_tree, 0, NULL))
    @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info
      
      	if (quiet) {
     -		if (refresh_cache(REFRESH_QUIET))
    -+		if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK))
    ++		if (refresh_and_write_cache(REFRESH_QUIET, 0))
      			warning("could not refresh index");
      	} else {
      		struct child_process cp = CHILD_PROCESS_INIT;
    @@ builtin/stash.c: static int do_create_stash(const struct pathspec *ps, struct st
      
      	read_cache_preload(NULL);
     -	refresh_cache(REFRESH_QUIET);
    -+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK) < 0) {
    ++	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0) {
     +		ret = -1;
     +		goto done;
     +	}
    @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char
      	}
      
     -	if (refresh_cache(REFRESH_QUIET)) {
    -+	if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK)) {
    ++	if (refresh_and_write_cache(REFRESH_QUIET, 0)) {
      		ret = -1;
      		goto done;
      	}

Thomas Gummerer (3):
  factor out refresh_and_write_cache function
  merge: use refresh_and_write_cache
  stash: make sure to write refreshed cache

 builtin/am.c     | 16 ++--------------
 builtin/merge.c  | 17 +++++------------
 builtin/stash.c  | 11 +++++++----
 cache.h          | 13 +++++++++++++
 read-cache.c     | 17 +++++++++++++++++
 t/t3903-stash.sh | 16 ++++++++++++++++
 6 files changed, 60 insertions(+), 30 deletions(-)

-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v2 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer]

Getting the lock for the index, refreshing it and then writing it is a
pattern that happens more than once throughout the codebase.  Factor
out the refresh_and_write_cache function from builtin/am.c to
read-cache.c, so it can be re-used in other places in a subsequent
commit.

Note that we return different error codes for failing to refresh the
cache, and failing to write the index.  The current caller only cares
about failing to write the index.  However for other callers we're
going to convert in subsequent patches we will need this distinction.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/am.c | 16 ++--------------
 cache.h      | 13 +++++++++++++
 read-cache.c | 17 +++++++++++++++++
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1aea657a7f..ddedd2b9d4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1071,19 +1071,6 @@ static const char *msgnum(const struct am_state *state)
 	return sb.buf;
 }
 
-/**
- * Refresh and write index.
- */
-static void refresh_and_write_cache(void)
-{
-	struct lock_file lock_file = LOCK_INIT;
-
-	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		die(_("unable to write index file"));
-}
-
 /**
  * Dies with a user-friendly message on how to proceed after resolving the
  * problem. This message can be overridden with state->resolvemsg.
@@ -1703,7 +1690,8 @@ static void am_run(struct am_state *state, int resume)
 
 	unlink(am_path(state, "dirtyindex"));
 
-	refresh_and_write_cache();
+	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)
+		die(_("unable to write index file"));
 
 	if (repo_index_has_changes(the_repository, NULL, &sb)) {
 		write_state_bool(state, "dirtyindex", 1);
diff --git a/cache.h b/cache.h
index b1da1ab08f..987d289e8f 100644
--- a/cache.h
+++ b/cache.h
@@ -414,6 +414,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) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), 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))
@@ -812,6 +813,18 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
 #define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
 #define REFRESH_PROGRESS	0x0040  /* show progress bar if stderr is tty */
 int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
+/*
+ * Refresh the index and write it to disk.
+ *
+ * 'refresh_flags' is passed directly to 'refresh_index()', while
+ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so
+ * the lockfile is always either committed or rolled back.
+ *
+ * Return 1 if refreshing the cache failed, -1 if writing the cache to
+ * disk failed, 0 on success.
+ */
+int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, const struct pathspec *, char *seen, const char *header_msg);
+
 struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
 
 void set_alternate_index_output(const char *);
diff --git a/read-cache.c b/read-cache.c
index 52ffa8a313..72662df077 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1472,6 +1472,23 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,
 	printf(fmt, name);
 }
 
+int repo_refresh_and_write_index(struct  repository *repo,
+				 unsigned int refresh_flags,
+				 unsigned int write_flags,
+				 const struct pathspec *pathspec,
+				 char *seen, const char *header_msg)
+{
+	struct lock_file lock_file = LOCK_INIT;
+
+	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
+	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
+		return 1;
+	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
+		return -1;
+	return 0;
+}
+
+
 int refresh_index(struct index_state *istate, unsigned int flags,
 		  const struct pathspec *pathspec,
 		  char *seen, const char *header_msg)
-- 
2.23.0.rc2.194.ge5444969c9

Re: [PATCH v2 1/3] factor out refresh_and_write_cache function

[Martin Ågren]

On Thu, 29 Aug 2019 at 20:28, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> +int repo_refresh_and_write_index(struct  repository *repo,
> +                                unsigned int refresh_flags,
> +                                unsigned int write_flags,
> +                                const struct pathspec *pathspec,
> +                                char *seen, const char *header_msg)
> +{
> +       struct lock_file lock_file = LOCK_INIT;
> +
> +       repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> +       if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
> +               return 1;
> +       if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> +               return -1;
> +       return 0;
> +}

AFAIU, the_repository->index == &the_index so this patch is a noop on
the converted user as far as that aspect is concerned.

There's a difference in behavior that I'm not sure about: We used
to ignore the return value of `refresh_cache()`, i.e. we didn't care
whether it had any errors. I have no idea whether that's safe to do --
especially as we go on to write the index. So I don't know whether this
patch fixes a bug by introducing the early return. Or if it *introduces*
a bug by bailing too aggressively. Do you know more?

(This conversion provides REFRESH_QUIET, which seems to suppress certain
errors, but not all.)

In any case, that early return introduces a bug with the lockfile, that
much I know. We need to roll back the lockfile before doing the early
return. I should have seen that already in your previous version.. :-(

The above makes me think that once this new function is in good shape,
the commit introducing it could sell it as "this is hard to get right --
let's implement it correctly once and for all". ;-)

Martin

Re: [PATCH v2 1/3] factor out refresh_and_write_cache function

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> There's a difference in behavior that I'm not sure about: We used
> to ignore the return value of `refresh_cache()`, i.e. we didn't care
> whether it had any errors. I have no idea whether that's safe to do --
> especially as we go on to write the index. So I don't know whether this
> patch fixes a bug by introducing the early return. Or if it *introduces*
> a bug by bailing too aggressively. Do you know more?

One common reason why refresh_cache() fails is because the index is
unmerged (i.e. has one or more higher-stage entries).  After an
attempt to refresh, this would not wrote out the index in such a
case, which might even be more correct thing to do than the original
in the original context of "git am" implementation.  The next thing
that happens after the caller calls this function is to ask
repo_index_has_changes(), and we'd say "the index is dirty" whether
the index is written back or not from such a state.

> The above makes me think that once this new function is in good shape,
> the commit introducing it could sell it as "this is hard to get right --
> let's implement it correctly once and for all". ;-)

Yes, that is a more severe issue.

Re: [PATCH v2 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer]

On 08/30, Junio C Hamano wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
> 
> > There's a difference in behavior that I'm not sure about: We used
> > to ignore the return value of `refresh_cache()`, i.e. we didn't care
> > whether it had any errors. I have no idea whether that's safe to do --
> > especially as we go on to write the index. So I don't know whether this
> > patch fixes a bug by introducing the early return. Or if it *introduces*
> > a bug by bailing too aggressively. Do you know more?
> 
> One common reason why refresh_cache() fails is because the index is
> unmerged (i.e. has one or more higher-stage entries).  After an
> attempt to refresh, this would not wrote out the index in such a
> case, which might even be more correct thing to do than the original
> in the original context of "git am" implementation.  The next thing
> that happens after the caller calls this function is to ask
> repo_index_has_changes(), and we'd say "the index is dirty" whether
> the index is written back or not from such a state.

Looking at the other callsites, we seem to do something similar
everywhere, and usually fail if the index has unmerged entries.  So
the refreshed index would only not be written out in the case where
there's unmerged entries, and we fail later, which I think is okay.

> > The above makes me think that once this new function is in good shape,
> > the commit introducing it could sell it as "this is hard to get right --
> > let's implement it correctly once and for all". ;-)
> 
> Yes, that is a more severe issue.

With this do you mean what you quoted above, or that the lockfile is
not rolled back?  I agree that the lockfile not being rolled back if
'refresh_cache()' fails is indeed the bigger issue, and I'll fix that
in v3.  I can also add something like the above to the commit message,
just wanted to make sure I'm not missing something subtle in what you
quoted above.

Re: [PATCH v2 1/3] factor out refresh_and_write_cache function

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> On 08/30, Junio C Hamano wrote:
>> Martin Ågren <martin.agren@gmail.com> writes:
>> ...
>> > The above makes me think that once this new function is in good shape,
>> > the commit introducing it could sell it as "this is hard to get right --
>> > let's implement it correctly once and for all". ;-)
>> 
>> Yes, that is a more severe issue.
>
> With this do you mean what you quoted above, or that the lockfile is
> not rolled back?  I agree that the lockfile not being rolled back if
> 'refresh_cache()' fails is indeed the bigger issue, and I'll fix that
> in v3.  I can also add something like the above to the commit message,
> just wanted to make sure I'm not missing something subtle in what you
> quoted above.

You didn't miss anything, other than that I trimmed my quote too
much and ended up confusing you.

Thanks.

[PATCH v2 2/3] merge: use refresh_and_write_cache

[Thomas Gummerer]

Use the 'refresh_and_write_cache()' convenience function introduced in
the last commit, instead of refreshing and writing the index manually
in merge.c

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/merge.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index e2ccbc44e2..0148d938c9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -688,16 +688,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			      struct commit_list *remoteheads,
 			      struct commit *head)
 {
-	struct lock_file lock = LOCK_INIT;
 	const char *head_arg = "HEAD";
 
-	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock,
-			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)
 		return error(_("Unable to write index."));
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
+		struct lock_file lock = LOCK_INIT;
 		int clean, x;
 		struct commit *result;
 		struct commit_list *reversed = NULL;
@@ -860,12 +857,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 {
 	struct object_id result_tree, result_commit;
 	struct commit_list *parents, **pptr = &parents;
-	struct lock_file lock = LOCK_INIT;
 
-	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock,
-			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)
 		return error(_("Unable to write index."));
 
 	write_tree_trivial(&result_tree);
-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v2 3/3] stash: make sure to write refreshed cache

[Thomas Gummerer]

When converting stash into C, calls to 'git update-index --refresh'
were replaced with the 'refresh_cache()' function.  That is fine as
long as the index is only needed in-core, and not re-read from disk.

However in many cases we do actually need the refreshed index to be
written to disk, for example 'merge_recursive_generic()' discards the
in-core index before re-reading it from disk, and in the case of 'apply
--quiet', the 'refresh_cache()' we currently have is pointless without
writing the index to disk.

Always write the index after refreshing it to ensure there are no
regressions in this compared to the scripted stash.  In the future we
can consider avoiding the write where possible after making sure none
of the subsequent calls actually need the refreshed cache, and it is
not expected to be refreshed after stash exits or it is written
somewhere else already.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash.c  | 11 +++++++----
 t/t3903-stash.sh | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index b5a301f24d..da1260ca8e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -396,7 +396,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_cache(REFRESH_QUIET))
+	if (refresh_and_write_cache(REFRESH_QUIET, 0))
 		return -1;
 
 	if (write_cache_as_tree(&c_tree, 0, NULL))
@@ -485,7 +485,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 	}
 
 	if (quiet) {
-		if (refresh_cache(REFRESH_QUIET))
+		if (refresh_and_write_cache(REFRESH_QUIET, 0))
 			warning("could not refresh index");
 	} else {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -1129,7 +1129,10 @@ 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);
-	refresh_cache(REFRESH_QUIET);
+	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0) {
+		ret = -1;
+		goto done;
+	}
 
 	if (get_oid("HEAD", &info->b_commit)) {
 		if (!quiet)
@@ -1290,7 +1293,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		free(ps_matched);
 	}
 
-	if (refresh_cache(REFRESH_QUIET)) {
+	if (refresh_and_write_cache(REFRESH_QUIET, 0)) {
 		ret = -1;
 		goto done;
 	}
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b8e337893f..392954d6dd 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1241,4 +1241,20 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash apply should succeed with unmodified file' '
+	echo base >file &&
+	git add file &&
+	git commit -m base &&
+
+	# now stash a modification
+	echo modified >file &&
+	git stash &&
+
+	# make the file stat dirty
+	cp file other &&
+	mv other file &&
+
+	git stash apply
+'
+
 test_done
-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v3 0/3] make sure stash refreshes the index properly

[Thomas Gummerer]

Thanks Martin and Junio for the comments on the previous round.

Changes compared to the previous round:
- Document that when failing to refresh the index, the result won't be
  written to disk.
- Rollback the lock file if refreshing the index fails, so we don't
  end up with a lock file that can't be rolled back or committed after
  the function returns
- Some small tweaks in the commit message and documentation of the
  function.

Range-diff below:

1:  1f25fe227c ! 1:  7cc9f5fff4 factor out refresh_and_write_cache function
    @@ Commit message
         factor out refresh_and_write_cache function
     
         Getting the lock for the index, refreshing it and then writing it is a
    -    pattern that happens more than once throughout the codebase.  Factor
    -    out the refresh_and_write_cache function from builtin/am.c to
    -    read-cache.c, so it can be re-used in other places in a subsequent
    -    commit.
    +    pattern that happens more than once throughout the codebase, and isn't
    +    trivial to get right.  Factor out the refresh_and_write_cache function
    +    from builtin/am.c to read-cache.c, so it can be re-used in other
    +    places in a subsequent commit.
     
         Note that we return different error codes for failing to refresh the
         cache, and failing to write the index.  The current caller only cares
    @@ cache.h: void fill_stat_cache_info(struct index_state *istate, struct cache_entr
     + * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so
     + * the lockfile is always either committed or rolled back.
     + *
    -+ * Return 1 if refreshing the cache failed, -1 if writing the cache to
    -+ * disk failed, 0 on success.
    ++ * Return 1 if refreshing the index returns an error, -1 if writing
    ++ * the index to disk fails, 0 on success.
    ++ *
    ++ * Note that if refreshing the index returns an error, we don't write
    ++ * the result to disk.
     + */
     +int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, const struct pathspec *, char *seen, const char *header_msg);
     +
    @@ read-cache.c: static void show_file(const char * fmt, const char * name, int in_
     +	struct lock_file lock_file = LOCK_INIT;
     +
     +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
    -+	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
    ++	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
    ++		rollback_lock_file(&lock_file);
     +		return 1;
    ++	}
     +	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
     +		return -1;
     +	return 0;
2:  148a65d649 = 2:  0367d938b1 merge: use refresh_and_write_cache
3:  e0f6815192 = 3:  8ed3df9fec stash: make sure to write refreshed cache

Thomas Gummerer (3):
  factor out refresh_and_write_cache function
  merge: use refresh_and_write_cache
  stash: make sure to write refreshed cache

 builtin/am.c     | 16 ++--------------
 builtin/merge.c  | 13 +++----------
 builtin/stash.c  | 11 +++++++----
 cache.h          | 16 ++++++++++++++++
 read-cache.c     | 19 +++++++++++++++++++
 t/t3903-stash.sh | 16 ++++++++++++++++
 6 files changed, 63 insertions(+), 28 deletions(-)

-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v3 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer]

Getting the lock for the index, refreshing it and then writing it is a
pattern that happens more than once throughout the codebase, and isn't
trivial to get right.  Factor out the refresh_and_write_cache function
from builtin/am.c to read-cache.c, so it can be re-used in other
places in a subsequent commit.

Note that we return different error codes for failing to refresh the
cache, and failing to write the index.  The current caller only cares
about failing to write the index.  However for other callers we're
going to convert in subsequent patches we will need this distinction.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/am.c | 16 ++--------------
 cache.h      | 16 ++++++++++++++++
 read-cache.c | 19 +++++++++++++++++++
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1aea657a7f..ddedd2b9d4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1071,19 +1071,6 @@ static const char *msgnum(const struct am_state *state)
 	return sb.buf;
 }
 
-/**
- * Refresh and write index.
- */
-static void refresh_and_write_cache(void)
-{
-	struct lock_file lock_file = LOCK_INIT;
-
-	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		die(_("unable to write index file"));
-}
-
 /**
  * Dies with a user-friendly message on how to proceed after resolving the
  * problem. This message can be overridden with state->resolvemsg.
@@ -1703,7 +1690,8 @@ static void am_run(struct am_state *state, int resume)
 
 	unlink(am_path(state, "dirtyindex"));
 
-	refresh_and_write_cache();
+	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)
+		die(_("unable to write index file"));
 
 	if (repo_index_has_changes(the_repository, NULL, &sb)) {
 		write_state_bool(state, "dirtyindex", 1);
diff --git a/cache.h b/cache.h
index b1da1ab08f..2b14768bea 100644
--- a/cache.h
+++ b/cache.h
@@ -414,6 +414,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) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), 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))
@@ -812,6 +813,21 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
 #define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
 #define REFRESH_PROGRESS	0x0040  /* show progress bar if stderr is tty */
 int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
+/*
+ * Refresh the index and write it to disk.
+ *
+ * 'refresh_flags' is passed directly to 'refresh_index()', while
+ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so
+ * the lockfile is always either committed or rolled back.
+ *
+ * Return 1 if refreshing the index returns an error, -1 if writing
+ * the index to disk fails, 0 on success.
+ *
+ * Note that if refreshing the index returns an error, we don't write
+ * the result to disk.
+ */
+int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, const struct pathspec *, char *seen, const char *header_msg);
+
 struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
 
 void set_alternate_index_output(const char *);
diff --git a/read-cache.c b/read-cache.c
index 52ffa8a313..2ad96677ae 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1472,6 +1472,25 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,
 	printf(fmt, name);
 }
 
+int repo_refresh_and_write_index(struct  repository *repo,
+				 unsigned int refresh_flags,
+				 unsigned int write_flags,
+				 const struct pathspec *pathspec,
+				 char *seen, const char *header_msg)
+{
+	struct lock_file lock_file = LOCK_INIT;
+
+	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
+	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
+		rollback_lock_file(&lock_file);
+		return 1;
+	}
+	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
+		return -1;
+	return 0;
+}
+
+
 int refresh_index(struct index_state *istate, unsigned int flags,
 		  const struct pathspec *pathspec,
 		  char *seen, const char *header_msg)
-- 
2.23.0.rc2.194.ge5444969c9

Re: [PATCH v3 1/3] factor out refresh_and_write_cache function

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> Getting the lock for the index, refreshing it and then writing it is a
> pattern that happens more than once throughout the codebase, and isn't
> trivial to get right.  Factor out the refresh_and_write_cache function
> from builtin/am.c to read-cache.c, so it can be re-used in other
> places in a subsequent commit.
>
> Note that we return different error codes for failing to refresh the
> cache, and failing to write the index.  The current caller only cares
> about failing to write the index.  However for other callers we're
> going to convert in subsequent patches we will need this distinction.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/am.c | 16 ++--------------
>  cache.h      | 16 ++++++++++++++++
>  read-cache.c | 19 +++++++++++++++++++
>  3 files changed, 37 insertions(+), 14 deletions(-)

I think this goes in the right direction, but obviously conflicts
with what Dscho wants to do in the builtin-add-i series, and needs
to be reconciled by working better together.

For now, I'll eject builtin-add-i and queue this for a few days to
give it a bit more exposure, but after that requeue builtin-add-i
and discard these three patches.  By that time, hopefully you two
would have a rerolled version of this one and builtin-add-i that
agree what kind of refresh-and-write-index behaviour they both want.

The differences I see that need reconciling are:

 - builtin-add-i seems to allow 'gentle' and allow returning an
   error when we cannot open the index for writing by passing false
   to 'gentle'; this feature is not used yet, though.

 - This version allows to pass pathspec, seen and header_msg, while
   the one in builtin-add-i cannot limit the part of the index
   getting refreshed with pathspec.  It wouldn't be a brain surgery
   to use this version and adjust the caller (there only is one) in
   the builtin-add-i topic.

 - This version does not write the index back when refresh_index()
   returns non-zero, but the one in builtin-add-i ignores the
   returned value.  I think, as a performance measure, it probably
   is a better idea to write it back, even when the function returns
   non-zero (the local variable's name is has_errors, but having an
   entry in the index that does not get refreshed is *not* an error;
   e.g. an unmerged entry is a normal thing in the index, and as
   long as we refreshed other entries while having an unmerged and
   unrefreshable entry, we are making progress that is worth writing
   out).

Thanks.

> +int repo_refresh_and_write_index(struct  repository *repo,
> +				 unsigned int refresh_flags,
> +				 unsigned int write_flags,
> +				 const struct pathspec *pathspec,
> +				 char *seen, const char *header_msg)
> +{
> +	struct lock_file lock_file = LOCK_INIT;
> +
> +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> +	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
> +		rollback_lock_file(&lock_file);
> +		return 1;
> +	}
> +	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> +		return -1;
> +	return 0;
> +}
> +
> +
>  int refresh_index(struct index_state *istate, unsigned int flags,
>  		  const struct pathspec *pathspec,
>  		  char *seen, const char *header_msg)

Re: [PATCH v3 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer]

On 09/05, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > Getting the lock for the index, refreshing it and then writing it is a
> > pattern that happens more than once throughout the codebase, and isn't
> > trivial to get right.  Factor out the refresh_and_write_cache function
> > from builtin/am.c to read-cache.c, so it can be re-used in other
> > places in a subsequent commit.
> >
> > Note that we return different error codes for failing to refresh the
> > cache, and failing to write the index.  The current caller only cares
> > about failing to write the index.  However for other callers we're
> > going to convert in subsequent patches we will need this distinction.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >  builtin/am.c | 16 ++--------------
> >  cache.h      | 16 ++++++++++++++++
> >  read-cache.c | 19 +++++++++++++++++++
> >  3 files changed, 37 insertions(+), 14 deletions(-)
> 
> I think this goes in the right direction, but obviously conflicts
> with what Dscho wants to do in the builtin-add-i series, and needs
> to be reconciled by working better together.

Oops, I didn't realize there was another series in flight that also
introduces 'repo_refresh_and_write_index'.  Probably should have done
a test merge of this with pu.

> For now, I'll eject builtin-add-i and queue this for a few days to
> give it a bit more exposure, but after that requeue builtin-add-i
> and discard these three patches.  By that time, hopefully you two
> would have a rerolled version of this one and builtin-add-i that
> agree what kind of refresh-and-write-index behaviour they both want.
>
> The differences I see that need reconciling are:

Thanks for writing these down.

>  - builtin-add-i seems to allow 'gentle' and allow returning an
>    error when we cannot open the index for writing by passing false
>    to 'gentle'; this feature is not used yet, though.

Right, and if gentle is set to false, it avoids writing the index,
which seems fine from my perspective.

>  - This version allows to pass pathspec, seen and header_msg, while
>    the one in builtin-add-i cannot limit the part of the index
>    getting refreshed with pathspec.  It wouldn't be a brain surgery
>    to use this version and adjust the caller (there only is one) in
>    the builtin-add-i topic.

'pathspec', 'seen' and 'header_msg' are not used in my version either,
I just implemented it for completeness and compatibility.  So I'd be
fine to do without them.

>  - This version does not write the index back when refresh_index()
>    returns non-zero, but the one in builtin-add-i ignores the
>    returned value.  I think, as a performance measure, it probably
>    is a better idea to write it back, even when the function returns
>    non-zero (the local variable's name is has_errors, but having an
>    entry in the index that does not get refreshed is *not* an error;
>    e.g. an unmerged entry is a normal thing in the index, and as
>    long as we refreshed other entries while having an unmerged and
>    unrefreshable entry, we are making progress that is worth writing
>    out).

I'm happy with writing the index back even if there are errors.
However I think we still need the option to get the return code from
'refresh_index()', as some callers where I'm using
'refresh_and_write_index()' in this series behave differently
depending on its return code.

There's two more differences between the versions:

 - The version in my series allows passing in write_flags to be passed
   to write_locked_index, which is required to convert the callers in
   builtin/merge.c.

 - Dscho's version also calls 'repo_read_index_preload()', which I
   don't do in mine.  Some callers don't need to do that, so I think it
   would be nice to keep that outside of the
   'repo_refresh_and_write_index()' function.

I can think of a few ways forward here:

 - I incorporate features that are needed for the builtin-add-i series
   here, and that is rebased on top of this series.

 - We drop the first two patches of this series, so we only fix the
   problems in 'git stash' for now.  Later we can have a refactoring
   series that uses repo_refresh_and_write_index in the places we
   converted here, once the dust of the builtin-add-i series settled.

 - I rebase this on top of builtin-add-i.

I'm happy with either of the first two, but less so with the last
option.  I was hoping this series could potentially go to maint as it
was a bugfix, which we obviously can't do with that option.

Dscho, what do you think? :)

> Thanks.
> 
> > +int repo_refresh_and_write_index(struct  repository *repo,
> > +				 unsigned int refresh_flags,
> > +				 unsigned int write_flags,
> > +				 const struct pathspec *pathspec,
> > +				 char *seen, const char *header_msg)
> > +{
> > +	struct lock_file lock_file = LOCK_INIT;
> > +
> > +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> > +	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
> > +		rollback_lock_file(&lock_file);
> > +		return 1;
> > +	}
> > +	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> > +		return -1;
> > +	return 0;
> > +}
> > +
> > +
> >  int refresh_index(struct index_state *istate, unsigned int flags,
> >  		  const struct pathspec *pathspec,
> >  		  char *seen, const char *header_msg)

Re: [PATCH v3 1/3] factor out refresh_and_write_cache function

[Johannes Schindelin]

Hi Thomas,

On Fri, 6 Sep 2019, Thomas Gummerer wrote:

> On 09/05, Junio C Hamano wrote:
> > Thomas Gummerer <t.gummerer@gmail.com> writes:
> >
> > > Getting the lock for the index, refreshing it and then writing it is a
> > > pattern that happens more than once throughout the codebase, and isn't
> > > trivial to get right.  Factor out the refresh_and_write_cache function
> > > from builtin/am.c to read-cache.c, so it can be re-used in other
> > > places in a subsequent commit.
> > >
> > > Note that we return different error codes for failing to refresh the
> > > cache, and failing to write the index.  The current caller only cares
> > > about failing to write the index.  However for other callers we're
> > > going to convert in subsequent patches we will need this distinction.
> > >
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > >  builtin/am.c | 16 ++--------------
> > >  cache.h      | 16 ++++++++++++++++
> > >  read-cache.c | 19 +++++++++++++++++++
> > >  3 files changed, 37 insertions(+), 14 deletions(-)
> >
> > I think this goes in the right direction, but obviously conflicts
> > with what Dscho wants to do in the builtin-add-i series, and needs
> > to be reconciled by working better together.
>
> Oops, I didn't realize there was another series in flight that also
> introduces 'repo_refresh_and_write_index'.  Probably should have done
> a test merge of this with pu.

Yep, our patches clash. I would not mind placing my patch series on top
of yours, provided that you can make a few changes that I need ;-)

> > For now, I'll eject builtin-add-i and queue this for a few days to
> > give it a bit more exposure, but after that requeue builtin-add-i
> > and discard these three patches.  By that time, hopefully you two
> > would have a rerolled version of this one and builtin-add-i that
> > agree what kind of refresh-and-write-index behaviour they both want.
> >
> > The differences I see that need reconciling are:
>
> Thanks for writing these down.
>
> >  - builtin-add-i seems to allow 'gentle' and allow returning an
> >    error when we cannot open the index for writing by passing false
> >    to 'gentle'; this feature is not used yet, though.
>
> Right, and if gentle is set to false, it avoids writing the index,
> which seems fine from my perspective.

This also suggests that it would make sense to avoid
`LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a
library function, not just a helper function for a one-shot built-in
(don't you like how this idea "it is okay to use exit() to clean up
after us, we don't care" comes back to bite us?).

> >  - This version allows to pass pathspec, seen and header_msg, while
> >    the one in builtin-add-i cannot limit the part of the index
> >    getting refreshed with pathspec.  It wouldn't be a brain surgery
> >    to use this version and adjust the caller (there only is one) in
> >    the builtin-add-i topic.
>
> 'pathspec', 'seen' and 'header_msg' are not used in my version either,
> I just implemented it for completeness and compatibility.  So I'd be
> fine to do without them.

Oh, why not keep them? I'd rather keep them and adjust the caller in
`builtin-add-i`.

> >  - This version does not write the index back when refresh_index()
> >    returns non-zero, but the one in builtin-add-i ignores the
> >    returned value.  I think, as a performance measure, it probably
> >    is a better idea to write it back, even when the function returns
> >    non-zero (the local variable's name is has_errors, but having an
> >    entry in the index that does not get refreshed is *not* an error;
> >    e.g. an unmerged entry is a normal thing in the index, and as
> >    long as we refreshed other entries while having an unmerged and
> >    unrefreshable entry, we are making progress that is worth writing
> >    out).
>
> I'm happy with writing the index back even if there are errors.
> However I think we still need the option to get the return code from
> 'refresh_index()', as some callers where I'm using
> 'refresh_and_write_index()' in this series behave differently
> depending on its return code.
>
> There's two more differences between the versions:
>
>  - The version in my series allows passing in write_flags to be passed
>    to write_locked_index, which is required to convert the callers in
>    builtin/merge.c.

I can always pass in 0 as `write_flags`.

>  - Dscho's version also calls 'repo_read_index_preload()', which I
>    don't do in mine.  Some callers don't need to do that, so I think it
>    would be nice to keep that outside of the
>    'repo_refresh_and_write_index()' function.

Agreed.

> I can think of a few ways forward here:
>
>  - I incorporate features that are needed for the builtin-add-i series
>    here, and that is rebased on top of this series.

I'd prefer this way forward. The `builtin-add-i` patch series is
evolving more slowly than yours.

>  - We drop the first two patches of this series, so we only fix the
>    problems in 'git stash' for now.  Later we can have a refactoring
>    series that uses repo_refresh_and_write_index in the places we
>    converted here, once the dust of the builtin-add-i series settled.
>
>  - I rebase this on top of builtin-add-i.
>
> I'm happy with either of the first two, but less so with the last
> option.  I was hoping this series could potentially go to maint as it
> was a bugfix, which we obviously can't do with that option.
>
> Dscho, what do you think? :)

See above ;-)

Thank you!
Dscho

>
> > Thanks.
> >
> > > +int repo_refresh_and_write_index(struct  repository *repo,
> > > +				 unsigned int refresh_flags,
> > > +				 unsigned int write_flags,
> > > +				 const struct pathspec *pathspec,
> > > +				 char *seen, const char *header_msg)
> > > +{
> > > +	struct lock_file lock_file = LOCK_INIT;
> > > +
> > > +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> > > +	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
> > > +		rollback_lock_file(&lock_file);
> > > +		return 1;
> > > +	}
> > > +	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> > > +		return -1;
> > > +	return 0;
> > > +}
> > > +
> > > +
> > >  int refresh_index(struct index_state *istate, unsigned int flags,
> > >  		  const struct pathspec *pathspec,
> > >  		  char *seen, const char *header_msg)
>

Re: [PATCH v3 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer]

On 09/11, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Fri, 6 Sep 2019, Thomas Gummerer wrote:
> > Oops, I didn't realize there was another series in flight that also
> > introduces 'repo_refresh_and_write_index'.  Probably should have done
> > a test merge of this with pu.
> 
> Yep, our patches clash. I would not mind placing my patch series on top
> of yours, provided that you can make a few changes that I need ;-)

Sounds good.  Looking ahead further I don't mind these changes at all!

> > Right, and if gentle is set to false, it avoids writing the index,
> > which seems fine from my perspective.
> 
> This also suggests that it would make sense to avoid
> `LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a
> library function, not just a helper function for a one-shot built-in
> (don't you like how this idea "it is okay to use exit() to clean up
> after us, we don't care" comes back to bite us?).

Yup, returning an error for this definitely makes sense, especially
for future proofing.

> > >  - This version allows to pass pathspec, seen and header_msg, while
> > >    the one in builtin-add-i cannot limit the part of the index
> > >    getting refreshed with pathspec.  It wouldn't be a brain surgery
> > >    to use this version and adjust the caller (there only is one) in
> > >    the builtin-add-i topic.
> >
> > 'pathspec', 'seen' and 'header_msg' are not used in my version either,
> > I just implemented it for completeness and compatibility.  So I'd be
> > fine to do without them.
> 
> Oh, why not keep them? I'd rather keep them and adjust the caller in
> `builtin-add-i`.

Great, I'm happy to keep them.

> > There's two more differences between the versions:
> >
> >  - The version in my series allows passing in write_flags to be passed
> >    to write_locked_index, which is required to convert the callers in
> >    builtin/merge.c.
> 
> I can always pass in 0 as `write_flags`.
> 
> >  - Dscho's version also calls 'repo_read_index_preload()', which I
> >    don't do in mine.  Some callers don't need to do that, so I think it
> >    would be nice to keep that outside of the
> >    'repo_refresh_and_write_index()' function.
> 
> Agreed.
> 
> > I can think of a few ways forward here:
> >
> >  - I incorporate features that are needed for the builtin-add-i series
> >    here, and that is rebased on top of this series.
> 
> I'd prefer this way forward. The `builtin-add-i` patch series is
> evolving more slowly than yours.

Great!  I'll send an updated version of my series soon.  Thanks!

Re: [PATCH v3 1/3] factor out refresh_and_write_cache function

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> On 09/11, Johannes Schindelin wrote:
>> Hi Thomas,
>> 
>> On Fri, 6 Sep 2019, Thomas Gummerer wrote:
>> > Oops, I didn't realize there was another series in flight that also
>> > introduces 'repo_refresh_and_write_index'.  Probably should have done
>> > a test merge of this with pu.
>> 
>> Yep, our patches clash. I would not mind placing my patch series on top
>> of yours, provided that you can make a few changes that I need ;-)
>
> Sounds good.  Looking ahead further I don't mind these changes at all!
>
>> > Right, and if gentle is set to false, it avoids writing the index,
>> > which seems fine from my perspective.
>> 
>> This also suggests that it would make sense to avoid
>> `LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a
>> library function, not just a helper function for a one-shot built-in
>> (don't you like how this idea "it is okay to use exit() to clean up
>> after us, we don't care" comes back to bite us?).
>
> Yup, returning an error for this definitely makes sense, especially
> for future proofing.
>
>> > >  - This version allows to pass pathspec, seen and header_msg, while
>> > >    the one in builtin-add-i cannot limit the part of the index
>> > >    getting refreshed with pathspec.  It wouldn't be a brain surgery
>> > >    to use this version and adjust the caller (there only is one) in
>> > >    the builtin-add-i topic.
>> >
>> > 'pathspec', 'seen' and 'header_msg' are not used in my version either,
>> > I just implemented it for completeness and compatibility.  So I'd be
>> > fine to do without them.
>> 
>> Oh, why not keep them? I'd rather keep them and adjust the caller in
>> `builtin-add-i`.
>
> Great, I'm happy to keep them.
>
>> > There's two more differences between the versions:
>> >
>> >  - The version in my series allows passing in write_flags to be passed
>> >    to write_locked_index, which is required to convert the callers in
>> >    builtin/merge.c.
>> 
>> I can always pass in 0 as `write_flags`.
>> 
>> >  - Dscho's version also calls 'repo_read_index_preload()', which I
>> >    don't do in mine.  Some callers don't need to do that, so I think it
>> >    would be nice to keep that outside of the
>> >    'repo_refresh_and_write_index()' function.
>> 
>> Agreed.
>> 
>> > I can think of a few ways forward here:
>> >
>> >  - I incorporate features that are needed for the builtin-add-i series
>> >    here, and that is rebased on top of this series.
>> 
>> I'd prefer this way forward. The `builtin-add-i` patch series is
>> evolving more slowly than yours.
>
> Great!  I'll send an updated version of my series soon.  Thanks!

I just read the conclusion you two reached (after being down and
offline for two days) and found the reasoning totally sensible.

Thanks, both of you, for working well together.

[PATCH v3 2/3] merge: use refresh_and_write_cache

[Thomas Gummerer]

Use the 'refresh_and_write_cache()' convenience function introduced in
the last commit, instead of refreshing and writing the index manually
in merge.c

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/merge.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index e2ccbc44e2..0148d938c9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -688,16 +688,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			      struct commit_list *remoteheads,
 			      struct commit *head)
 {
-	struct lock_file lock = LOCK_INIT;
 	const char *head_arg = "HEAD";
 
-	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock,
-			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)
 		return error(_("Unable to write index."));
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
+		struct lock_file lock = LOCK_INIT;
 		int clean, x;
 		struct commit *result;
 		struct commit_list *reversed = NULL;
@@ -860,12 +857,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 {
 	struct object_id result_tree, result_commit;
 	struct commit_list *parents, **pptr = &parents;
-	struct lock_file lock = LOCK_INIT;
 
-	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock,
-			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)
 		return error(_("Unable to write index."));
 
 	write_tree_trivial(&result_tree);
-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v3 3/3] stash: make sure to write refreshed cache

[Thomas Gummerer]

When converting stash into C, calls to 'git update-index --refresh'
were replaced with the 'refresh_cache()' function.  That is fine as
long as the index is only needed in-core, and not re-read from disk.

However in many cases we do actually need the refreshed index to be
written to disk, for example 'merge_recursive_generic()' discards the
in-core index before re-reading it from disk, and in the case of 'apply
--quiet', the 'refresh_cache()' we currently have is pointless without
writing the index to disk.

Always write the index after refreshing it to ensure there are no
regressions in this compared to the scripted stash.  In the future we
can consider avoiding the write where possible after making sure none
of the subsequent calls actually need the refreshed cache, and it is
not expected to be refreshed after stash exits or it is written
somewhere else already.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash.c  | 11 +++++++----
 t/t3903-stash.sh | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index b5a301f24d..da1260ca8e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -396,7 +396,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_cache(REFRESH_QUIET))
+	if (refresh_and_write_cache(REFRESH_QUIET, 0))
 		return -1;
 
 	if (write_cache_as_tree(&c_tree, 0, NULL))
@@ -485,7 +485,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 	}
 
 	if (quiet) {
-		if (refresh_cache(REFRESH_QUIET))
+		if (refresh_and_write_cache(REFRESH_QUIET, 0))
 			warning("could not refresh index");
 	} else {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -1129,7 +1129,10 @@ 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);
-	refresh_cache(REFRESH_QUIET);
+	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0) {
+		ret = -1;
+		goto done;
+	}
 
 	if (get_oid("HEAD", &info->b_commit)) {
 		if (!quiet)
@@ -1290,7 +1293,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		free(ps_matched);
 	}
 
-	if (refresh_cache(REFRESH_QUIET)) {
+	if (refresh_and_write_cache(REFRESH_QUIET, 0)) {
 		ret = -1;
 		goto done;
 	}
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b8e337893f..392954d6dd 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1241,4 +1241,20 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash apply should succeed with unmodified file' '
+	echo base >file &&
+	git add file &&
+	git commit -m base &&
+
+	# now stash a modification
+	echo modified >file &&
+	git stash &&
+
+	# make the file stat dirty
+	cp file other &&
+	mv other file &&
+
+	git stash apply
+'
+
 test_done
-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v4 0/3] make sure stash refreshes the index properly

[Thomas Gummerer]

Compared to the previous round this round introduces a gentle flag for
refresh_and_write_{index,cache}, which should make this function
suitable for use in the Dscho's builtin-add-i series.  The latter will have to be 

I have also pushed this to https://github.com/tgummerer/git tg/stash-refresh-index

Range-diff below:

1:  7cc9f5fff4 ! 1:  2a7bebb20f factor out refresh_and_write_cache function
    @@ Commit message
         about failing to write the index.  However for other callers we're
         going to convert in subsequent patches we will need this distinction.
     
    +    Helped-by: Martin Ågren <martin.agren@gmail.com>
    +    Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
         Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
     
      ## builtin/am.c ##
    @@ builtin/am.c: static void am_run(struct am_state *state, int resume)
      	unlink(am_path(state, "dirtyindex"));
      
     -	refresh_and_write_cache();
    -+	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)
    ++	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0)
     +		die(_("unable to write index file"));
      
      	if (repo_index_has_changes(the_repository, NULL, &sb)) {
    @@ cache.h: 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) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_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 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))
    @@ cache.h: void fill_stat_cache_info(struct index_state *istate, struct cache_entr
     + * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so
     + * the lockfile is always either committed or rolled back.
     + *
    ++ * If 'gentle' is passed, errors locking the index are ignored.
    ++ *
     + * Return 1 if refreshing the index returns an error, -1 if writing
     + * the index to disk fails, 0 on success.
     + *
    -+ * Note that if refreshing the index returns an error, we don't write
    -+ * the result to disk.
    ++ * Note that if refreshing the index returns an error, we still write
    ++ * out the result (unless locking failed).
     + */
    -+int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, 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 gentle, const struct pathspec *, char *seen, const char *header_msg);
     +
      struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
      
    @@ read-cache.c: static void show_file(const char * fmt, const char * name, int in_
      	printf(fmt, name);
      }
      
    -+int repo_refresh_and_write_index(struct  repository *repo,
    ++int repo_refresh_and_write_index(struct repository *repo,
     +				 unsigned int refresh_flags,
     +				 unsigned int write_flags,
    ++				 int gentle,
     +				 const struct pathspec *pathspec,
     +				 char *seen, const char *header_msg)
     +{
     +	struct lock_file lock_file = LOCK_INIT;
    ++	int fd, ret = 0;
     +
    -+	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
    -+	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
    -+		rollback_lock_file(&lock_file);
    -+		return 1;
    -+	}
    -+	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
    ++	fd = repo_hold_locked_index(repo, &lock_file, 0);
    ++	if (!gentle && fd < 0)
     +		return -1;
    -+	return 0;
    ++	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
    ++		ret = 1;
    ++	if (0 <= fd && write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
    ++		ret = -1;
    ++	rollback_lock_file(&lock_file);
    ++	return ret;
     +}
     +
     +
2:  0367d938b1 ! 2:  555c982eae merge: use refresh_and_write_cache
    @@ builtin/merge.c: static int try_merge_strategy(const char *strategy, struct comm
     -	refresh_cache(REFRESH_QUIET);
     -	if (write_locked_index(&the_index, &lock,
     -			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
    -+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)
    ++	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)
      		return error(_("Unable to write index."));
      
      	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
    @@ builtin/merge.c: static int merge_trivial(struct commit *head, struct commit_lis
     -	refresh_cache(REFRESH_QUIET);
     -	if (write_locked_index(&the_index, &lock,
     -			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
    -+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)
    ++	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)
      		return error(_("Unable to write index."));
      
      	write_tree_trivial(&result_tree);
3:  8ed3df9fec ! 3:  cf74fe6053 stash: make sure to write refreshed cache
    @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info
      
      	read_cache_preload(NULL);
     -	if (refresh_cache(REFRESH_QUIET))
    -+	if (refresh_and_write_cache(REFRESH_QUIET, 0))
    ++	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
      		return -1;
      
      	if (write_cache_as_tree(&c_tree, 0, NULL))
    @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info
      
      	if (quiet) {
     -		if (refresh_cache(REFRESH_QUIET))
    -+		if (refresh_and_write_cache(REFRESH_QUIET, 0))
    ++		if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
      			warning("could not refresh index");
      	} else {
      		struct child_process cp = CHILD_PROCESS_INIT;
    @@ builtin/stash.c: static int do_create_stash(const struct pathspec *ps, struct st
      
      	read_cache_preload(NULL);
     -	refresh_cache(REFRESH_QUIET);
    -+	if (refresh_and_write_cache(REFRESH_QUIET, 0) < 0) {
    ++	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0) {
     +		ret = -1;
     +		goto done;
     +	}
    @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char
      	}
      
     -	if (refresh_cache(REFRESH_QUIET)) {
    -+	if (refresh_and_write_cache(REFRESH_QUIET, 0)) {
    ++	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) {
      		ret = -1;
      		goto done;
      	}

Thomas Gummerer (3):
  factor out refresh_and_write_cache function
  merge: use refresh_and_write_cache
  stash: make sure to write refreshed cache

 builtin/am.c     | 16 ++--------------
 builtin/merge.c  | 13 +++----------
 builtin/stash.c  | 11 +++++++----
 cache.h          | 18 ++++++++++++++++++
 read-cache.c     | 21 +++++++++++++++++++++
 t/t3903-stash.sh | 16 ++++++++++++++++
 6 files changed, 67 insertions(+), 28 deletions(-)

-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v4 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer]

Getting the lock for the index, refreshing it and then writing it is a
pattern that happens more than once throughout the codebase, and isn't
trivial to get right.  Factor out the refresh_and_write_cache function
from builtin/am.c to read-cache.c, so it can be re-used in other
places in a subsequent commit.

Note that we return different error codes for failing to refresh the
cache, and failing to write the index.  The current caller only cares
about failing to write the index.  However for other callers we're
going to convert in subsequent patches we will need this distinction.

Helped-by: Martin Ågren <martin.agren@gmail.com>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/am.c | 16 ++--------------
 cache.h      | 18 ++++++++++++++++++
 read-cache.c | 21 +++++++++++++++++++++
 3 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1aea657a7f..92e0e70069 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1071,19 +1071,6 @@ static const char *msgnum(const struct am_state *state)
 	return sb.buf;
 }
 
-/**
- * Refresh and write index.
- */
-static void refresh_and_write_cache(void)
-{
-	struct lock_file lock_file = LOCK_INIT;
-
-	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		die(_("unable to write index file"));
-}
-
 /**
  * Dies with a user-friendly message on how to proceed after resolving the
  * problem. This message can be overridden with state->resolvemsg.
@@ -1703,7 +1690,8 @@ static void am_run(struct am_state *state, int resume)
 
 	unlink(am_path(state, "dirtyindex"));
 
-	refresh_and_write_cache();
+	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0)
+		die(_("unable to write index file"));
 
 	if (repo_index_has_changes(the_repository, NULL, &sb)) {
 		write_state_bool(state, "dirtyindex", 1);
diff --git a/cache.h b/cache.h
index b1da1ab08f..68a54f50ac 100644
--- a/cache.h
+++ b/cache.h
@@ -414,6 +414,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 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))
@@ -812,6 +813,23 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
 #define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
 #define REFRESH_PROGRESS	0x0040  /* show progress bar if stderr is tty */
 int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
+/*
+ * Refresh the index and write it to disk.
+ *
+ * 'refresh_flags' is passed directly to 'refresh_index()', while
+ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so
+ * the lockfile is always either committed or rolled back.
+ *
+ * If 'gentle' is passed, errors locking the index are ignored.
+ *
+ * Return 1 if refreshing the index returns an error, -1 if writing
+ * the index to disk fails, 0 on success.
+ *
+ * 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);
+
 struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
 
 void set_alternate_index_output(const char *);
diff --git a/read-cache.c b/read-cache.c
index 52ffa8a313..7e646e06c2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1472,6 +1472,27 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,
 	printf(fmt, name);
 }
 
+int repo_refresh_and_write_index(struct repository *repo,
+				 unsigned int refresh_flags,
+				 unsigned int write_flags,
+				 int gentle,
+				 const struct pathspec *pathspec,
+				 char *seen, const char *header_msg)
+{
+	struct lock_file lock_file = LOCK_INIT;
+	int fd, ret = 0;
+
+	fd = repo_hold_locked_index(repo, &lock_file, 0);
+	if (!gentle && fd < 0)
+		return -1;
+	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
+		ret = 1;
+	if (0 <= fd && write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
+		ret = -1;
+	return ret;
+}
+
+
 int refresh_index(struct index_state *istate, unsigned int flags,
 		  const struct pathspec *pathspec,
 		  char *seen, const char *header_msg)
-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v4 2/3] merge: use refresh_and_write_cache

[Thomas Gummerer]

Use the 'refresh_and_write_cache()' convenience function introduced in
the last commit, instead of refreshing and writing the index manually
in merge.c

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/merge.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index e2ccbc44e2..83e42fcb10 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -688,16 +688,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			      struct commit_list *remoteheads,
 			      struct commit *head)
 {
-	struct lock_file lock = LOCK_INIT;
 	const char *head_arg = "HEAD";
 
-	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock,
-			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)
 		return error(_("Unable to write index."));
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
+		struct lock_file lock = LOCK_INIT;
 		int clean, x;
 		struct commit *result;
 		struct commit_list *reversed = NULL;
@@ -860,12 +857,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 {
 	struct object_id result_tree, result_commit;
 	struct commit_list *parents, **pptr = &parents;
-	struct lock_file lock = LOCK_INIT;
 
-	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, &lock,
-			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)
 		return error(_("Unable to write index."));
 
 	write_tree_trivial(&result_tree);
-- 
2.23.0.rc2.194.ge5444969c9

[PATCH v4 3/3] stash: make sure to write refreshed cache

[Thomas Gummerer]

When converting stash into C, calls to 'git update-index --refresh'
were replaced with the 'refresh_cache()' function.  That is fine as
long as the index is only needed in-core, and not re-read from disk.

However in many cases we do actually need the refreshed index to be
written to disk, for example 'merge_recursive_generic()' discards the
in-core index before re-reading it from disk, and in the case of 'apply
--quiet', the 'refresh_cache()' we currently have is pointless without
writing the index to disk.

Always write the index after refreshing it to ensure there are no
regressions in this compared to the scripted stash.  In the future we
can consider avoiding the write where possible after making sure none
of the subsequent calls actually need the refreshed cache, and it is
not expected to be refreshed after stash exits or it is written
somewhere else already.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash.c  | 11 +++++++----
 t/t3903-stash.sh | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index b5a301f24d..ab30d1e920 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -396,7 +396,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_cache(REFRESH_QUIET))
+	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
 		return -1;
 
 	if (write_cache_as_tree(&c_tree, 0, NULL))
@@ -485,7 +485,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 	}
 
 	if (quiet) {
-		if (refresh_cache(REFRESH_QUIET))
+		if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))
 			warning("could not refresh index");
 	} else {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -1129,7 +1129,10 @@ 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);
-	refresh_cache(REFRESH_QUIET);
+	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0) {
+		ret = -1;
+		goto done;
+	}
 
 	if (get_oid("HEAD", &info->b_commit)) {
 		if (!quiet)
@@ -1290,7 +1293,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		free(ps_matched);
 	}
 
-	if (refresh_cache(REFRESH_QUIET)) {
+	if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) {
 		ret = -1;
 		goto done;
 	}
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b8e337893f..392954d6dd 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1241,4 +1241,20 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash apply should succeed with unmodified file' '
+	echo base >file &&
+	git add file &&
+	git commit -m base &&
+
+	# now stash a modification
+	echo modified >file &&
+	git stash &&
+
+	# make the file stat dirty
+	cp file other &&
+	mv other file &&
+
+	git stash apply
+'
+
 test_done
-- 
2.23.0.rc2.194.ge5444969c9