[1/3] factor out refresh_and_write_cache function
diff mbox series

Message ID 20190827101408.76757-2-t.gummerer@gmail.com
State New
Headers show
Series
  • make sure stash refreshes the index properly
Related show

Commit Message

Thomas Gummerer Aug. 27, 2019, 10:14 a.m. UTC
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(-)

Comments

Martin Ågren Aug. 28, 2019, 3:49 p.m. UTC | #1
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
Thomas Gummerer Aug. 29, 2019, 5:59 p.m. UTC | #2
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
diff mbox series

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)