| Message ID | 20190827101408.76757-2-t.gummerer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make sure stash refreshes the index properly | expand |
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
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
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)
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(-)