{"thread":{"id":"51758","subject":"[PATCH 0/3] make sure stash refreshes the index properly","startedAt":"2019-08-27T10:14:20Z","lastAt":"2019-09-12T16:46:45Z","messageCount":29,"participants":["Thomas Gummerer","Martin Ågren","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"381337","messageId":"20190827101408.76757-1-t.gummerer@gmail.com","threadId":"51758","inReplyTo":null,"subject":"[PATCH 0/3] make sure stash refreshes the index properly","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-27T10:14:05Z","receivedAt":"2019-08-27T10:14:20Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Thanks Peff for spotting the bug! Here's a series that fixes it.\n\n> And before the third one, introduction of a new entry point that\n> makes merge-recursive machinery inherit the already populated\n> in-core index, happens, I think the right solution is to write the\n> in-core index out---the write is not pointless.\n\nYup, I agree with that. In fact there are some other places where we\njust call 'refresh_cache()' as a replacement for 'git update-index\n--refresh'. At least the other one in 'do_apply_stash()' also seems\nlike a bug, as I assume the original intention (and behaviour) was\nthat the index is refreshed after 'stash apply -q' finishes.\n\nI think in do_push_stash and do_create_stash we might be able to get\naway without the write, but I wasn't 100% sure, so I made them write\nthe index after refreshing it as well, which is what the shell script\ndid.\n\nThe first patch is a small refactoring that makes the actual fix a bit\neasier, while the second patch is a cleanup that I found while there.\n\nThomas Gummerer (3):\n factor out refresh_and_write_cache function\n merge: use refresh_and_write_cache\n stash: make sure to write refreshed cache\n\n builtin/am.c | 16 ++--------------\n builtin/merge.c | 15 ++++-----------\n builtin/stash.c | 11 +++++++----\n cache.h | 9 +++++++++\n read-cache.c | 17 +++++++++++++++++\n t/t3903-stash.sh | 16 ++++++++++++++++\n 6 files changed, 55 insertions(+), 29 deletions(-)\n\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381338","messageId":"20190827101408.76757-3-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190827101408.76757-1-t.gummerer@gmail.com","subject":"[PATCH 2/3] merge: use refresh_and_write_cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-27T10:14:07Z","receivedAt":"2019-08-27T10:14:23Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Use the 'refresh_and_write_cache()' convenience function introduced in\nthe last commit, instead of refreshing and writing the index manually\nin merge.c\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/merge.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e2ccbc44e2..b5e31ce283 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -691,11 +691,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \tstruct lock_file lock = LOCK_INIT;\n \tconst char *head_arg = \"HEAD\";\n \n-\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock,\n-\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n-\t\treturn error(_(\"Unable to write index.\"));\n+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)\n+\t\treturn -1;\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n@@ -860,13 +857,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n {\n \tstruct object_id result_tree, result_commit;\n \tstruct commit_list *parents, **pptr = &parents;\n-\tstruct lock_file lock = LOCK_INIT;\n \n-\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock,\n-\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n-\t\treturn error(_(\"Unable to write index.\"));\n+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)\n+\t\treturn -1;\n \n \twrite_tree_trivial(&result_tree);\n \tprintf(_(\"Wonderful.\\n\"));\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381339","messageId":"20190827101408.76757-4-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190827101408.76757-1-t.gummerer@gmail.com","subject":"[PATCH 3/3] stash: make sure to write refreshed cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-27T10:14:08Z","receivedAt":"2019-08-27T10:14:24Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When converting stash into C, calls to 'git update-index --refresh'\nwere replaced with the 'refresh_cache()' function. That is fine as\nlong as the index is only needed in-core, and not re-read from disk.\n\nHowever in many cases we do actually need the refreshed index to be\nwritten to disk, for example 'merge_recursive_generic()' discards the\nin-core index before re-reading it from disk, and in the case of 'apply\n--quiet', the 'refresh_cache()' we currently have is pointless without\nwriting the index to disk.\n\nAlways write the index after refreshing it to ensure there are no\nregressions in this compared to the scripted stash. In the future we\ncan consider avoiding the write where possible after making sure none\nof the subsequent calls actually need the refreshed cache, and it is\nnot expected to be refreshed after stash exits or it is written\nsomewhere else already.\n\nReported-by: Jeff King \nSigned-off-by: Thomas Gummerer \n---\n builtin/stash.c | 11 +++++++----\n t/t3903-stash.sh | 16 ++++++++++++++++\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex b5a301f24d..b36aada644 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -396,7 +396,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tconst struct object_id *bases[1];\n \n \tread_cache_preload(NULL);\n-\tif (refresh_cache(REFRESH_QUIET))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&c_tree, 0, NULL))\n@@ -485,7 +485,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t}\n \n \tif (quiet) {\n-\t\tif (refresh_cache(REFRESH_QUIET))\n+\t\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK))\n \t\t\twarning(\"could not refresh index\");\n \t} else {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1129,7 +1129,10 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \tprepare_fallback_ident(\"git stash\", \"git@stash\");\n \n \tread_cache_preload(NULL);\n-\trefresh_cache(REFRESH_QUIET);\n+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK) < 0) {\n+\t\tret = -1;\n+\t\tgoto done;\n+\t}\n \n \tif (get_oid(\"HEAD\", &info->b_commit)) {\n \t\tif (!quiet)\n@@ -1290,7 +1293,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\tfree(ps_matched);\n \t}\n \n-\tif (refresh_cache(REFRESH_QUIET)) {\n+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex b8e337893f..392954d6dd 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1241,4 +1241,20 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu\n \ttest_path_is_missing to-remove\n '\n \n+test_expect_success 'stash apply should succeed with unmodified file' '\n+\techo base >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\n+\t# now stash a modification\n+\techo modified >file &&\n+\tgit stash &&\n+\n+\t# make the file stat dirty\n+\tcp file other &&\n+\tmv other file &&\n+\n+\tgit stash apply\n+'\n+\n test_done\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381340","messageId":"20190827101408.76757-2-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190827101408.76757-1-t.gummerer@gmail.com","subject":"[PATCH 1/3] factor out refresh_and_write_cache function","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-27T10:14:06Z","receivedAt":"2019-08-27T10:14:24Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Getting the lock for the index, refreshing it and then writing it is a\npattern that happens more than once throughout the codebase. Factor\nout the refresh_and_write_cache function from builtin/am.c to\nread-cache.c, so it can be re-used in other places in a subsequent\ncommit.\n\nNote that we return different error codes for failing to refresh the\ncache, and failing to write the index. The current caller only cares\nabout failing to write the index. However for other callers we're\ngoing to convert in subsequent patches we will need this distinction.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/am.c | 16 ++--------------\n cache.h | 9 +++++++++\n read-cache.c | 17 +++++++++++++++++\n 3 files changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1aea657a7f..e00410e4d7 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1071,19 +1071,6 @@ static const char *msgnum(const struct am_state *state)\n \treturn sb.buf;\n }\n \n-/**\n- * Refresh and write index.\n- */\n-static void refresh_and_write_cache(void)\n-{\n-\tstruct lock_file lock_file = LOCK_INIT;\n-\n-\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\tdie(_(\"unable to write index file\"));\n-}\n-\n /**\n * Dies with a user-friendly message on how to proceed after resolving the\n * problem. This message can be overridden with state->resolvemsg.\n@@ -1703,7 +1690,8 @@ static void am_run(struct am_state *state, int resume)\n \n \tunlink(am_path(state, \"dirtyindex\"));\n \n-\trefresh_and_write_cache();\n+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK) < 0)\n+\t\tdie(_(\"failed to refresh cache\"));\n \n \tif (repo_index_has_changes(the_repository, NULL, &sb)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\ndiff --git a/cache.h b/cache.h\nindex b1da1ab08f..f72392f32b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -414,6 +414,7 @@ extern struct index_state the_index;\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n+#define refresh_and_write_cache(refresh_flags, write_flags) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))\n@@ -812,6 +813,14 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st\n #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n #define REFRESH_PROGRESS\t0x0040 /* show progress bar if stderr is tty */\n int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n+/*\n+ * Refresh the index and write it to disk.\n+ *\n+ * Return 1 if refreshing the cache failed, -1 if writing the cache to\n+ * disk failed, 0 on success.\n+ */\n+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);\n+\n struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);\n \n void set_alternate_index_output(const char *);\ndiff --git a/read-cache.c b/read-cache.c\nindex 52ffa8a313..905d2ddd10 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1472,6 +1472,23 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,\n \tprintf(fmt, name);\n }\n \n+int repo_refresh_and_write_index(struct repository *repo,\n+\t\t\t\t unsigned int refresh_flags,\n+\t\t\t\t unsigned int write_flags,\n+\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t char *seen, const char *header_msg)\n+{\n+\tstruct lock_file lock_file = LOCK_INIT;\n+\n+\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n+\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n+\t\treturn 1;\n+\tif (write_locked_index(repo->index, &lock_file, write_flags))\n+\t\treturn error(_(\"unable to write index file\"));\n+\treturn 0;\n+}\n+\n+\n int refresh_index(struct index_state *istate, unsigned int flags,\n \t\t const struct pathspec *pathspec,\n \t\t char *seen, const char *header_msg)\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381453","messageId":"CAN0heSptSEa6tcRZ3DVZjr7L=A2n7=U9fbnfYOvW0bBJ-M3WKQ@mail.gmail.com","threadId":"51758","inReplyTo":"20190827101408.76757-2-t.gummerer@gmail.com","subject":"Re: [PATCH 1/3] factor out refresh_and_write_cache function","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-28T15:49:56Z","receivedAt":"2019-08-28T15:50:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 27 Aug 2019 at 12:14, Thomas Gummerer wrote:\n>\n> Getting the lock for the index, refreshing it and then writing it is a\n> pattern that happens more than once throughout the codebase. Factor\n> out the refresh_and_write_cache function from builtin/am.c to\n> read-cache.c, so it can be re-used in other places in a subsequent\n> commit.\n\n> +/*\n> + * Refresh the index and write it to disk.\n> + *\n> + * Return 1 if refreshing the cache failed, -1 if writing the cache to\n> + * disk failed, 0 on success.\n> + */\n\nThank you for documenting. :-) Should we say something about how this\ndoesn't explicitly print any error in case refreshing fails (that is, we\nleave it to `refresh_index()`), but that we *do* explicitly print an\nerror if writing the index fails? That caught me off-guard as I looked\nat how you convert the callers.\n\nAnd do we actually want that asymmetry? Maybe we do.\n\nMight be worth pointing out as you convert the callers how some (all?)\nof them now emit different error messages from before, but that it\nshouldn't matter(?) and it makes sense to unify those messages.\n\n> +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);\n\n> +int repo_refresh_and_write_index(struct repository *repo,\n> + unsigned int refresh_flags,\n> + unsigned int write_flags,\n> + const struct pathspec *pathspec,\n> + char *seen, const char *header_msg)\n> +{\n> + struct lock_file lock_file = LOCK_INIT;\n> +\n> + repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n> + if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n> + return 1;\n> + if (write_locked_index(repo->index, &lock_file, write_flags))\n> + return error(_(\"unable to write index file\"));\n> + return 0;\n> +}\n\nIf `flags` doesn't contain `COMMIT_LOCK`, the lockfile will be closed\n\"gently\", meaning we still need to either commit it, or roll it back. Or\nlet the exit handler roll it back, which is what would happen here, no?\nWe lose our handle on the stack and there's no way for anyone to say\n\"ok, now I'm done, commit it please\" (or \"roll it back\").\n\nIn short, I think calling this function without providing `COMMIT_LOCK`\nwould be useless at best. We should probably let this function provide\n`COMMIT_LOCK | write_flags` or `COMMIT_LOCK | extra_write_flags` or\nwhatever. Most callers would just provide \"0\". Hm?\n\nOr, we could BUG if the COMMIT_LOCK bit isn't set, but that seems like a\nless good choice to me. If we're so adamant about the bit being set --\nwhich we should be, IMHO -- we might as well set it ourselves.\n\n\n\nMartin\n"},{"id":"381455","messageId":"CAN0heSrs42hL7gmqMuugGLNOV8Vd9gxPcUiLA5oTXnhPEM-9qw@mail.gmail.com","threadId":"51758","inReplyTo":"20190827101408.76757-3-t.gummerer@gmail.com","subject":"Re: [PATCH 2/3] merge: use refresh_and_write_cache","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-28T15:52:25Z","receivedAt":"2019-08-28T15:52:39Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 27 Aug 2019 at 12:15, Thomas Gummerer wrote:\n\n> struct lock_file lock = LOCK_INIT;\n> const char *head_arg = \"HEAD\";\n>\n> - hold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n> - refresh_cache(REFRESH_QUIET);\n> - if (write_locked_index(&the_index, &lock,\n> - COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> - return error(_(\"Unable to write index.\"));\n> + if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)\n> + return -1;\n\nI wondered why you didn't drop the `struct lock_file`, but it turns out\nwe still need it further down.\n\n> if (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n> int clean, x;\n\nWhat you could do, I guess, is to move its declaration to around here.\nProbably not worth a re-roll.\n\n> @@ -860,13 +857,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n> {\n> struct object_id result_tree, result_commit;\n> struct commit_list *parents, **pptr = &parents;\n> - struct lock_file lock = LOCK_INIT;\n>\n> - hold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n> - refresh_cache(REFRESH_QUIET);\n> - if (write_locked_index(&the_index, &lock,\n> - COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> - return error(_(\"Unable to write index.\"));\n> + if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)\n> + return -1;\n\nHere you do drop the `struct lock_file` entirely, ok.\n\n\n\nMartin\n"},{"id":"381527","messageId":"20190829175910.GB48344@cat","threadId":"51758","inReplyTo":"CAN0heSptSEa6tcRZ3DVZjr7L=A2n7=U9fbnfYOvW0bBJ-M3WKQ@mail.gmail.com","subject":"Re: [PATCH 1/3] factor out refresh_and_write_cache function","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-29T17:59:10Z","receivedAt":"2019-08-29T17:59:15Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 08/28, Martin Ågren wrote:\n> On Tue, 27 Aug 2019 at 12:14, Thomas Gummerer wrote:\n> >\n> > Getting the lock for the index, refreshing it and then writing it is a\n> > pattern that happens more than once throughout the codebase. Factor\n> > out the refresh_and_write_cache function from builtin/am.c to\n> > read-cache.c, so it can be re-used in other places in a subsequent\n> > commit.\n> \n> > +/*\n> > + * Refresh the index and write it to disk.\n> > + *\n> > + * Return 1 if refreshing the cache failed, -1 if writing the cache to\n> > + * disk failed, 0 on success.\n> > + */\n> \n> Thank you for documenting. :-) Should we say something about how this\n> doesn't explicitly print any error in case refreshing fails (that is, we\n> leave it to `refresh_index()`), but that we *do* explicitly print an\n> error if writing the index fails? That caught me off-guard as I looked\n> at how you convert the callers.\n> \n> And do we actually want that asymmetry? Maybe we do.\n\nI think I needed the error for something while I went through a few\niterations of how to best structure this function, but I don't\nremember for what exactly now. I think it might actually be better to\njust return -1 here, and let the caller distinguish and show the error\nmessage if they need to. That also avoids duplicating the error in\ncase the caller wants to die on error.\n\n> Might be worth pointing out as you convert the callers how some (all?)\n> of them now emit different error messages from before, but that it\n> shouldn't matter(?) and it makes sense to unify those messages.\n\nYeah, I don't think changing the error message should matter, but\nunifying them is not actually a goal of this series. So with what you\npointed out above, I think I'll leave them as they are.\n\n> > +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);\n> \n> > +int repo_refresh_and_write_index(struct repository *repo,\n> > + unsigned int refresh_flags,\n> > + unsigned int write_flags,\n> > + const struct pathspec *pathspec,\n> > + char *seen, const char *header_msg)\n> > +{\n> > + struct lock_file lock_file = LOCK_INIT;\n> > +\n> > + repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n> > + if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n> > + return 1;\n> > + if (write_locked_index(repo->index, &lock_file, write_flags))\n> > + return error(_(\"unable to write index file\"));\n> > + return 0;\n> > +}\n> \n> If `flags` doesn't contain `COMMIT_LOCK`, the lockfile will be closed\n> \"gently\", meaning we still need to either commit it, or roll it back. Or\n> let the exit handler roll it back, which is what would happen here, no?\n> We lose our handle on the stack and there's no way for anyone to say\n> \"ok, now I'm done, commit it please\" (or \"roll it back\").\n> \n> In short, I think calling this function without providing `COMMIT_LOCK`\n> would be useless at best. We should probably let this function provide\n> `COMMIT_LOCK | write_flags` or `COMMIT_LOCK | extra_write_flags` or\n> whatever. Most callers would just provide \"0\". Hm?\n> \n> Or, we could BUG if the COMMIT_LOCK bit isn't set, but that seems like a\n> less good choice to me. If we're so adamant about the bit being set --\n> which we should be, IMHO -- we might as well set it ourselves.\n\nYeah, you're right, making this function use `COMMIT_LOCK | write_flags`\nwould probably be the best option. I'll change that, and document it\nas well.\n\nThanks for your review!\n\n> \n> \n> Martin\n"},{"id":"381528","messageId":"20190829180005.GC48344@cat","threadId":"51758","inReplyTo":"CAN0heSrs42hL7gmqMuugGLNOV8Vd9gxPcUiLA5oTXnhPEM-9qw@mail.gmail.com","subject":"Re: [PATCH 2/3] merge: use refresh_and_write_cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-29T18:00:05Z","receivedAt":"2019-08-29T18:00:11Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 08/28, Martin Ågren wrote:\n> On Tue, 27 Aug 2019 at 12:15, Thomas Gummerer wrote:\n> \n> > struct lock_file lock = LOCK_INIT;\n> > const char *head_arg = \"HEAD\";\n> >\n> > - hold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n> > - refresh_cache(REFRESH_QUIET);\n> > - if (write_locked_index(&the_index, &lock,\n> > - COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> > - return error(_(\"Unable to write index.\"));\n> > + if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)\n> > + return -1;\n> \n> I wondered why you didn't drop the `struct lock_file`, but it turns out\n> we still need it further down.\n> \n> > if (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n> > int clean, x;\n> \n> What you could do, I guess, is to move its declaration to around here.\n> Probably not worth a re-roll.\n\nI'll re-roll anyway for the things you spotted in the first patch, so\nI'll drop it down here while I'm at it, thanks!\n\n> > @@ -860,13 +857,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n> > {\n> > struct object_id result_tree, result_commit;\n> > struct commit_list *parents, **pptr = &parents;\n> > - struct lock_file lock = LOCK_INIT;\n> >\n> > - hold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n> > - refresh_cache(REFRESH_QUIET);\n> > - if (write_locked_index(&the_index, &lock,\n> > - COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> > - return error(_(\"Unable to write index.\"));\n> > + if (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)\n> > + return -1;\n> \n> Here you do drop the `struct lock_file` entirely, ok.\n> \n> \n> \n> Martin\n"},{"id":"381530","messageId":"20190829182748.43802-3-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190829182748.43802-1-t.gummerer@gmail.com","subject":"[PATCH v2 2/3] merge: use refresh_and_write_cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-29T18:27:47Z","receivedAt":"2019-08-29T18:28:12Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Use the 'refresh_and_write_cache()' convenience function introduced in\nthe last commit, instead of refreshing and writing the index manually\nin merge.c\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/merge.c | 13 +++----------\n 1 file changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e2ccbc44e2..0148d938c9 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -688,16 +688,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t struct commit_list *remoteheads,\n \t\t\t struct commit *head)\n {\n-\tstruct lock_file lock = LOCK_INIT;\n \tconst char *head_arg = \"HEAD\";\n \n-\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock,\n-\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n+\t\tstruct lock_file lock = LOCK_INIT;\n \t\tint clean, x;\n \t\tstruct commit *result;\n \t\tstruct commit_list *reversed = NULL;\n@@ -860,12 +857,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n {\n \tstruct object_id result_tree, result_commit;\n \tstruct commit_list *parents, **pptr = &parents;\n-\tstruct lock_file lock = LOCK_INIT;\n \n-\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock,\n-\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \twrite_tree_trivial(&result_tree);\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381531","messageId":"20190829182748.43802-4-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190829182748.43802-1-t.gummerer@gmail.com","subject":"[PATCH v2 3/3] stash: make sure to write refreshed cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-29T18:27:48Z","receivedAt":"2019-08-29T18:28:14Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When converting stash into C, calls to 'git update-index --refresh'\nwere replaced with the 'refresh_cache()' function. That is fine as\nlong as the index is only needed in-core, and not re-read from disk.\n\nHowever in many cases we do actually need the refreshed index to be\nwritten to disk, for example 'merge_recursive_generic()' discards the\nin-core index before re-reading it from disk, and in the case of 'apply\n--quiet', the 'refresh_cache()' we currently have is pointless without\nwriting the index to disk.\n\nAlways write the index after refreshing it to ensure there are no\nregressions in this compared to the scripted stash. In the future we\ncan consider avoiding the write where possible after making sure none\nof the subsequent calls actually need the refreshed cache, and it is\nnot expected to be refreshed after stash exits or it is written\nsomewhere else already.\n\nReported-by: Jeff King \nSigned-off-by: Thomas Gummerer \n---\n builtin/stash.c | 11 +++++++----\n t/t3903-stash.sh | 16 ++++++++++++++++\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex b5a301f24d..da1260ca8e 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -396,7 +396,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tconst struct object_id *bases[1];\n \n \tread_cache_preload(NULL);\n-\tif (refresh_cache(REFRESH_QUIET))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&c_tree, 0, NULL))\n@@ -485,7 +485,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t}\n \n \tif (quiet) {\n-\t\tif (refresh_cache(REFRESH_QUIET))\n+\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0))\n \t\t\twarning(\"could not refresh index\");\n \t} else {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1129,7 +1129,10 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \tprepare_fallback_ident(\"git stash\", \"git@stash\");\n \n \tread_cache_preload(NULL);\n-\trefresh_cache(REFRESH_QUIET);\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0) < 0) {\n+\t\tret = -1;\n+\t\tgoto done;\n+\t}\n \n \tif (get_oid(\"HEAD\", &info->b_commit)) {\n \t\tif (!quiet)\n@@ -1290,7 +1293,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\tfree(ps_matched);\n \t}\n \n-\tif (refresh_cache(REFRESH_QUIET)) {\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex b8e337893f..392954d6dd 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1241,4 +1241,20 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu\n \ttest_path_is_missing to-remove\n '\n \n+test_expect_success 'stash apply should succeed with unmodified file' '\n+\techo base >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\n+\t# now stash a modification\n+\techo modified >file &&\n+\tgit stash &&\n+\n+\t# make the file stat dirty\n+\tcp file other &&\n+\tmv other file &&\n+\n+\tgit stash apply\n+'\n+\n test_done\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381532","messageId":"20190829182748.43802-1-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190827101408.76757-1-t.gummerer@gmail.com","subject":"[PATCH v2 0/3] make sure stash refreshes the index properly","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-29T18:27:45Z","receivedAt":"2019-08-29T18:28:23Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Thanks Martin for the review of the last round!\n\nChanges compared to the previous round:\n- always pass COMMIT_LOCK to write_locked_index\n- don't write the error message in repo_refresh_and_write_index, but\n let the caller handle that. This means that we no longer change any\n error messages. Potential cleanups in that area can come later.\n- Drop the lock variable to the scope it needs.\n\nRange diff below:\n\n1: 7249a3cf4e ! 1: 1f25fe227c factor out refresh_and_write_cache function\n @@ builtin/am.c: static void am_run(struct am_state *state, int resume)\n \tunlink(am_path(state, \"dirtyindex\"));\n \n -\trefresh_and_write_cache();\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK) < 0)\n -+\t\tdie(_(\"failed to refresh cache\"));\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)\n ++\t\tdie(_(\"unable to write index file\"));\n \n \tif (repo_index_has_changes(the_repository, NULL, &sb)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\n @@ cache.h: void fill_stat_cache_info(struct index_state *istate, struct cache_entr\n +/*\n + * Refresh the index and write it to disk.\n + *\n ++ * 'refresh_flags' is passed directly to 'refresh_index()', while\n ++ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so\n ++ * the lockfile is always either committed or rolled back.\n ++ *\n + * Return 1 if refreshing the cache failed, -1 if writing the cache to\n + * disk failed, 0 on success.\n + */\n @@ read-cache.c: static void show_file(const char * fmt, const char * name, int in_\n +\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n +\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n +\t\treturn 1;\n -+\tif (write_locked_index(repo->index, &lock_file, write_flags))\n -+\t\treturn error(_(\"unable to write index file\"));\n ++\tif (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n ++\t\treturn -1;\n +\treturn 0;\n +}\n +\n2: de5b8c1529 ! 2: 148a65d649 merge: use refresh_and_write_cache\n @@ Commit message\n \n ## builtin/merge.c ##\n @@ builtin/merge.c: static int try_merge_strategy(const char *strategy, struct commit_list *common,\n - \tstruct lock_file lock = LOCK_INIT;\n + \t\t\t struct commit_list *remoteheads,\n + \t\t\t struct commit *head)\n + {\n +-\tstruct lock_file lock = LOCK_INIT;\n \tconst char *head_arg = \"HEAD\";\n \n -\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n -\trefresh_cache(REFRESH_QUIET);\n -\tif (write_locked_index(&the_index, &lock,\n -\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n --\t\treturn error(_(\"Unable to write index.\"));\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)\n -+\t\treturn -1;\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)\n + \t\treturn error(_(\"Unable to write index.\"));\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n ++\t\tstruct lock_file lock = LOCK_INIT;\n \t\tint clean, x;\n + \t\tstruct commit *result;\n + \t\tstruct commit_list *reversed = NULL;\n @@ builtin/merge.c: static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n {\n \tstruct object_id result_tree, result_commit;\n @@ builtin/merge.c: static int merge_trivial(struct commit *head, struct commit_lis\n -\trefresh_cache(REFRESH_QUIET);\n -\tif (write_locked_index(&the_index, &lock,\n -\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n --\t\treturn error(_(\"Unable to write index.\"));\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK | SKIP_IF_UNCHANGED) < 0)\n -+\t\treturn -1;\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)\n + \t\treturn error(_(\"Unable to write index.\"));\n \n \twrite_tree_trivial(&result_tree);\n - \tprintf(_(\"Wonderful.\\n\"));\n3: d9efda0f2a ! 3: e0f6815192 stash: make sure to write refreshed cache\n @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info\n \n \tread_cache_preload(NULL);\n -\tif (refresh_cache(REFRESH_QUIET))\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK))\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, 0))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&c_tree, 0, NULL))\n @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info\n \n \tif (quiet) {\n -\t\tif (refresh_cache(REFRESH_QUIET))\n -+\t\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK))\n ++\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0))\n \t\t\twarning(\"could not refresh index\");\n \t} else {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n @@ builtin/stash.c: static int do_create_stash(const struct pathspec *ps, struct st\n \n \tread_cache_preload(NULL);\n -\trefresh_cache(REFRESH_QUIET);\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK) < 0) {\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, 0) < 0) {\n +\t\tret = -1;\n +\t\tgoto done;\n +\t}\n @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char\n \t}\n \n -\tif (refresh_cache(REFRESH_QUIET)) {\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, COMMIT_LOCK)) {\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, 0)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\n\nThomas Gummerer (3):\n factor out refresh_and_write_cache function\n merge: use refresh_and_write_cache\n stash: make sure to write refreshed cache\n\n builtin/am.c | 16 ++--------------\n builtin/merge.c | 17 +++++------------\n builtin/stash.c | 11 +++++++----\n cache.h | 13 +++++++++++++\n read-cache.c | 17 +++++++++++++++++\n t/t3903-stash.sh | 16 ++++++++++++++++\n 6 files changed, 60 insertions(+), 30 deletions(-)\n\n-- \n2.23.0.rc2.194.ge5444969c9\n"},{"id":"381533","messageId":"20190829182748.43802-2-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190829182748.43802-1-t.gummerer@gmail.com","subject":"[PATCH v2 1/3] factor out refresh_and_write_cache function","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-29T18:27:46Z","receivedAt":"2019-08-29T18:28:25Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Getting the lock for the index, refreshing it and then writing it is a\npattern that happens more than once throughout the codebase. Factor\nout the refresh_and_write_cache function from builtin/am.c to\nread-cache.c, so it can be re-used in other places in a subsequent\ncommit.\n\nNote that we return different error codes for failing to refresh the\ncache, and failing to write the index. The current caller only cares\nabout failing to write the index. However for other callers we're\ngoing to convert in subsequent patches we will need this distinction.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/am.c | 16 ++--------------\n cache.h | 13 +++++++++++++\n read-cache.c | 17 +++++++++++++++++\n 3 files changed, 32 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1aea657a7f..ddedd2b9d4 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1071,19 +1071,6 @@ static const char *msgnum(const struct am_state *state)\n \treturn sb.buf;\n }\n \n-/**\n- * Refresh and write index.\n- */\n-static void refresh_and_write_cache(void)\n-{\n-\tstruct lock_file lock_file = LOCK_INIT;\n-\n-\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\tdie(_(\"unable to write index file\"));\n-}\n-\n /**\n * Dies with a user-friendly message on how to proceed after resolving the\n * problem. This message can be overridden with state->resolvemsg.\n@@ -1703,7 +1690,8 @@ static void am_run(struct am_state *state, int resume)\n \n \tunlink(am_path(state, \"dirtyindex\"));\n \n-\trefresh_and_write_cache();\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)\n+\t\tdie(_(\"unable to write index file\"));\n \n \tif (repo_index_has_changes(the_repository, NULL, &sb)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\ndiff --git a/cache.h b/cache.h\nindex b1da1ab08f..987d289e8f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -414,6 +414,7 @@ extern struct index_state the_index;\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n+#define refresh_and_write_cache(refresh_flags, write_flags) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))\n@@ -812,6 +813,18 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st\n #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n #define REFRESH_PROGRESS\t0x0040 /* show progress bar if stderr is tty */\n int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n+/*\n+ * Refresh the index and write it to disk.\n+ *\n+ * 'refresh_flags' is passed directly to 'refresh_index()', while\n+ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so\n+ * the lockfile is always either committed or rolled back.\n+ *\n+ * Return 1 if refreshing the cache failed, -1 if writing the cache to\n+ * disk failed, 0 on success.\n+ */\n+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);\n+\n struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);\n \n void set_alternate_index_output(const char *);\ndiff --git a/read-cache.c b/read-cache.c\nindex 52ffa8a313..72662df077 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1472,6 +1472,23 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,\n \tprintf(fmt, name);\n }\n \n+int repo_refresh_and_write_index(struct repository *repo,\n+\t\t\t\t unsigned int refresh_flags,\n+\t\t\t\t unsigned int write_flags,\n+\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t char *seen, const char *header_msg)\n+{\n+\tstruct lock_file lock_file = LOCK_INIT;\n+\n+\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n+\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n+\t\treturn 1;\n+\tif (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+\n int refresh_index(struct index_state *istate, unsigned int flags,\n \t\t const struct pathspec *pathspec,\n \t\t char *seen, const char *header_msg)\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381581","messageId":"CAN0heSqZOG6NMJE4=RReKzG3eD_w1mh8EcYaAQWN6WBY3WuZ1Q@mail.gmail.com","threadId":"51758","inReplyTo":"20190829182748.43802-2-t.gummerer@gmail.com","subject":"Re: [PATCH v2 1/3] factor out refresh_and_write_cache function","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-30T15:07:29Z","receivedAt":"2019-08-30T15:07:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 29 Aug 2019 at 20:28, Thomas Gummerer wrote:\n> +int repo_refresh_and_write_index(struct repository *repo,\n> + unsigned int refresh_flags,\n> + unsigned int write_flags,\n> + const struct pathspec *pathspec,\n> + char *seen, const char *header_msg)\n> +{\n> + struct lock_file lock_file = LOCK_INIT;\n> +\n> + repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n> + if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n> + return 1;\n> + if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n> + return -1;\n> + return 0;\n> +}\n\nAFAIU, the_repository->index == &the_index so this patch is a noop on\nthe converted user as far as that aspect is concerned.\n\nThere's a difference in behavior that I'm not sure about: We used\nto ignore the return value of `refresh_cache()`, i.e. we didn't care\nwhether it had any errors. I have no idea whether that's safe to do --\nespecially as we go on to write the index. So I don't know whether this\npatch fixes a bug by introducing the early return. Or if it *introduces*\na bug by bailing too aggressively. Do you know more?\n\n(This conversion provides REFRESH_QUIET, which seems to suppress certain\nerrors, but not all.)\n\nIn any case, that early return introduces a bug with the lockfile, that\nmuch I know. We need to roll back the lockfile before doing the early\nreturn. I should have seen that already in your previous version.. :-(\n\nThe above makes me think that once this new function is in good shape,\nthe commit introducing it could sell it as \"this is hard to get right --\nlet's implement it correctly once and for all\". ;-)\n\nMartin\n"},{"id":"381593","messageId":"xmqq8srazipr.fsf@gitster-ct.c.googlers.com","threadId":"51758","inReplyTo":"CAN0heSqZOG6NMJE4=RReKzG3eD_w1mh8EcYaAQWN6WBY3WuZ1Q@mail.gmail.com","subject":"Re: [PATCH v2 1/3] factor out refresh_and_write_cache function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-30T17:06:08Z","receivedAt":"2019-08-30T17:06:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> There's a difference in behavior that I'm not sure about: We used\n> to ignore the return value of `refresh_cache()`, i.e. we didn't care\n> whether it had any errors. I have no idea whether that's safe to do --\n> especially as we go on to write the index. So I don't know whether this\n> patch fixes a bug by introducing the early return. Or if it *introduces*\n> a bug by bailing too aggressively. Do you know more?\n\nOne common reason why refresh_cache() fails is because the index is\nunmerged (i.e. has one or more higher-stage entries). After an\nattempt to refresh, this would not wrote out the index in such a\ncase, which might even be more correct thing to do than the original\nin the original context of \"git am\" implementation. The next thing\nthat happens after the caller calls this function is to ask\nrepo_index_has_changes(), and we'd say \"the index is dirty\" whether\nthe index is written back or not from such a state.\n\n> The above makes me think that once this new function is in good shape,\n> the commit introducing it could sell it as \"this is hard to get right --\n> let's implement it correctly once and for all\". ;-)\n\nYes, that is a more severe issue.\n"},{"id":"381659","messageId":"20190902171539.GB77876@cat","threadId":"51758","inReplyTo":"xmqq8srazipr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/3] factor out refresh_and_write_cache function","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-02T17:15:39Z","receivedAt":"2019-09-02T17:15:44Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 08/30, Junio C Hamano wrote:\n> Martin Ågren writes:\n> \n> > There's a difference in behavior that I'm not sure about: We used\n> > to ignore the return value of `refresh_cache()`, i.e. we didn't care\n> > whether it had any errors. I have no idea whether that's safe to do --\n> > especially as we go on to write the index. So I don't know whether this\n> > patch fixes a bug by introducing the early return. Or if it *introduces*\n> > a bug by bailing too aggressively. Do you know more?\n> \n> One common reason why refresh_cache() fails is because the index is\n> unmerged (i.e. has one or more higher-stage entries). After an\n> attempt to refresh, this would not wrote out the index in such a\n> case, which might even be more correct thing to do than the original\n> in the original context of \"git am\" implementation. The next thing\n> that happens after the caller calls this function is to ask\n> repo_index_has_changes(), and we'd say \"the index is dirty\" whether\n> the index is written back or not from such a state.\n\nLooking at the other callsites, we seem to do something similar\neverywhere, and usually fail if the index has unmerged entries. So\nthe refreshed index would only not be written out in the case where\nthere's unmerged entries, and we fail later, which I think is okay.\n\n> > The above makes me think that once this new function is in good shape,\n> > the commit introducing it could sell it as \"this is hard to get right --\n> > let's implement it correctly once and for all\". ;-)\n> \n> Yes, that is a more severe issue.\n\nWith this do you mean what you quoted above, or that the lockfile is\nnot rolled back? I agree that the lockfile not being rolled back if\n'refresh_cache()' fails is indeed the bigger issue, and I'll fix that\nin v3. I can also add something like the above to the commit message,\njust wanted to make sure I'm not missing something subtle in what you\nquoted above.\n"},{"id":"381737","messageId":"xmqqsgpdtgwh.fsf@gitster-ct.c.googlers.com","threadId":"51758","inReplyTo":"20190902171539.GB77876@cat","subject":"Re: [PATCH v2 1/3] factor out refresh_and_write_cache function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-03T17:43:10Z","receivedAt":"2019-09-03T17:43:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> On 08/30, Junio C Hamano wrote:\n>> Martin Ågren writes:\n>> ...\n>> > The above makes me think that once this new function is in good shape,\n>> > the commit introducing it could sell it as \"this is hard to get right --\n>> > let's implement it correctly once and for all\". ;-)\n>> \n>> Yes, that is a more severe issue.\n>\n> With this do you mean what you quoted above, or that the lockfile is\n> not rolled back? I agree that the lockfile not being rolled back if\n> 'refresh_cache()' fails is indeed the bigger issue, and I'll fix that\n> in v3. I can also add something like the above to the commit message,\n> just wanted to make sure I'm not missing something subtle in what you\n> quoted above.\n\nYou didn't miss anything, other than that I trimmed my quote too\nmuch and ended up confusing you.\n\nThanks.\n\n"},{"id":"381759","messageId":"20190903191041.10470-1-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190829182748.43802-1-t.gummerer@gmail.com","subject":"[PATCH v3 0/3] make sure stash refreshes the index properly","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-03T19:10:38Z","receivedAt":"2019-09-03T19:10:49Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Thanks Martin and Junio for the comments on the previous round.\n\nChanges compared to the previous round:\n- Document that when failing to refresh the index, the result won't be\n written to disk.\n- Rollback the lock file if refreshing the index fails, so we don't\n end up with a lock file that can't be rolled back or committed after\n the function returns\n- Some small tweaks in the commit message and documentation of the\n function.\n\nRange-diff below:\n\n1: 1f25fe227c ! 1: 7cc9f5fff4 factor out refresh_and_write_cache function\n @@ Commit message\n factor out refresh_and_write_cache function\n \n Getting the lock for the index, refreshing it and then writing it is a\n - pattern that happens more than once throughout the codebase. Factor\n - out the refresh_and_write_cache function from builtin/am.c to\n - read-cache.c, so it can be re-used in other places in a subsequent\n - commit.\n + pattern that happens more than once throughout the codebase, and isn't\n + trivial to get right. Factor out the refresh_and_write_cache function\n + from builtin/am.c to read-cache.c, so it can be re-used in other\n + places in a subsequent commit.\n \n Note that we return different error codes for failing to refresh the\n cache, and failing to write the index. The current caller only cares\n @@ cache.h: void fill_stat_cache_info(struct index_state *istate, struct cache_entr\n + * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so\n + * the lockfile is always either committed or rolled back.\n + *\n -+ * Return 1 if refreshing the cache failed, -1 if writing the cache to\n -+ * disk failed, 0 on success.\n ++ * Return 1 if refreshing the index returns an error, -1 if writing\n ++ * the index to disk fails, 0 on success.\n ++ *\n ++ * Note that if refreshing the index returns an error, we don't write\n ++ * the result to disk.\n + */\n +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);\n +\n @@ read-cache.c: static void show_file(const char * fmt, const char * name, int in_\n +\tstruct lock_file lock_file = LOCK_INIT;\n +\n +\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n -+\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n ++\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {\n ++\t\trollback_lock_file(&lock_file);\n +\t\treturn 1;\n ++\t}\n +\tif (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n +\t\treturn -1;\n +\treturn 0;\n2: 148a65d649 = 2: 0367d938b1 merge: use refresh_and_write_cache\n3: e0f6815192 = 3: 8ed3df9fec stash: make sure to write refreshed cache\n\nThomas Gummerer (3):\n factor out refresh_and_write_cache function\n merge: use refresh_and_write_cache\n stash: make sure to write refreshed cache\n\n builtin/am.c | 16 ++--------------\n builtin/merge.c | 13 +++----------\n builtin/stash.c | 11 +++++++----\n cache.h | 16 ++++++++++++++++\n read-cache.c | 19 +++++++++++++++++++\n t/t3903-stash.sh | 16 ++++++++++++++++\n 6 files changed, 63 insertions(+), 28 deletions(-)\n\n-- \n2.23.0.rc2.194.ge5444969c9\n"},{"id":"381760","messageId":"20190903191041.10470-2-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190903191041.10470-1-t.gummerer@gmail.com","subject":"[PATCH v3 1/3] factor out refresh_and_write_cache function","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-03T19:10:39Z","receivedAt":"2019-09-03T19:10:50Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Getting the lock for the index, refreshing it and then writing it is a\npattern that happens more than once throughout the codebase, and isn't\ntrivial to get right. Factor out the refresh_and_write_cache function\nfrom builtin/am.c to read-cache.c, so it can be re-used in other\nplaces in a subsequent commit.\n\nNote that we return different error codes for failing to refresh the\ncache, and failing to write the index. The current caller only cares\nabout failing to write the index. However for other callers we're\ngoing to convert in subsequent patches we will need this distinction.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/am.c | 16 ++--------------\n cache.h | 16 ++++++++++++++++\n read-cache.c | 19 +++++++++++++++++++\n 3 files changed, 37 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1aea657a7f..ddedd2b9d4 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1071,19 +1071,6 @@ static const char *msgnum(const struct am_state *state)\n \treturn sb.buf;\n }\n \n-/**\n- * Refresh and write index.\n- */\n-static void refresh_and_write_cache(void)\n-{\n-\tstruct lock_file lock_file = LOCK_INIT;\n-\n-\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\tdie(_(\"unable to write index file\"));\n-}\n-\n /**\n * Dies with a user-friendly message on how to proceed after resolving the\n * problem. This message can be overridden with state->resolvemsg.\n@@ -1703,7 +1690,8 @@ static void am_run(struct am_state *state, int resume)\n \n \tunlink(am_path(state, \"dirtyindex\"));\n \n-\trefresh_and_write_cache();\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)\n+\t\tdie(_(\"unable to write index file\"));\n \n \tif (repo_index_has_changes(the_repository, NULL, &sb)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\ndiff --git a/cache.h b/cache.h\nindex b1da1ab08f..2b14768bea 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -414,6 +414,7 @@ extern struct index_state the_index;\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n+#define refresh_and_write_cache(refresh_flags, write_flags) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))\n@@ -812,6 +813,21 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st\n #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n #define REFRESH_PROGRESS\t0x0040 /* show progress bar if stderr is tty */\n int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n+/*\n+ * Refresh the index and write it to disk.\n+ *\n+ * 'refresh_flags' is passed directly to 'refresh_index()', while\n+ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so\n+ * the lockfile is always either committed or rolled back.\n+ *\n+ * Return 1 if refreshing the index returns an error, -1 if writing\n+ * the index to disk fails, 0 on success.\n+ *\n+ * Note that if refreshing the index returns an error, we don't write\n+ * the result to disk.\n+ */\n+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);\n+\n struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);\n \n void set_alternate_index_output(const char *);\ndiff --git a/read-cache.c b/read-cache.c\nindex 52ffa8a313..2ad96677ae 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1472,6 +1472,25 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,\n \tprintf(fmt, name);\n }\n \n+int repo_refresh_and_write_index(struct repository *repo,\n+\t\t\t\t unsigned int refresh_flags,\n+\t\t\t\t unsigned int write_flags,\n+\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t char *seen, const char *header_msg)\n+{\n+\tstruct lock_file lock_file = LOCK_INIT;\n+\n+\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n+\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {\n+\t\trollback_lock_file(&lock_file);\n+\t\treturn 1;\n+\t}\n+\tif (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+\n int refresh_index(struct index_state *istate, unsigned int flags,\n \t\t const struct pathspec *pathspec,\n \t\t char *seen, const char *header_msg)\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381761","messageId":"20190903191041.10470-3-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190903191041.10470-1-t.gummerer@gmail.com","subject":"[PATCH v3 2/3] merge: use refresh_and_write_cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-03T19:10:40Z","receivedAt":"2019-09-03T19:10:51Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Use the 'refresh_and_write_cache()' convenience function introduced in\nthe last commit, instead of refreshing and writing the index manually\nin merge.c\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/merge.c | 13 +++----------\n 1 file changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e2ccbc44e2..0148d938c9 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -688,16 +688,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t struct commit_list *remoteheads,\n \t\t\t struct commit *head)\n {\n-\tstruct lock_file lock = LOCK_INIT;\n \tconst char *head_arg = \"HEAD\";\n \n-\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock,\n-\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n+\t\tstruct lock_file lock = LOCK_INIT;\n \t\tint clean, x;\n \t\tstruct commit *result;\n \t\tstruct commit_list *reversed = NULL;\n@@ -860,12 +857,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n {\n \tstruct object_id result_tree, result_commit;\n \tstruct commit_list *parents, **pptr = &parents;\n-\tstruct lock_file lock = LOCK_INIT;\n \n-\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock,\n-\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \twrite_tree_trivial(&result_tree);\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381762","messageId":"20190903191041.10470-4-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190903191041.10470-1-t.gummerer@gmail.com","subject":"[PATCH v3 3/3] stash: make sure to write refreshed cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-03T19:10:41Z","receivedAt":"2019-09-03T19:10:53Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When converting stash into C, calls to 'git update-index --refresh'\nwere replaced with the 'refresh_cache()' function. That is fine as\nlong as the index is only needed in-core, and not re-read from disk.\n\nHowever in many cases we do actually need the refreshed index to be\nwritten to disk, for example 'merge_recursive_generic()' discards the\nin-core index before re-reading it from disk, and in the case of 'apply\n--quiet', the 'refresh_cache()' we currently have is pointless without\nwriting the index to disk.\n\nAlways write the index after refreshing it to ensure there are no\nregressions in this compared to the scripted stash. In the future we\ncan consider avoiding the write where possible after making sure none\nof the subsequent calls actually need the refreshed cache, and it is\nnot expected to be refreshed after stash exits or it is written\nsomewhere else already.\n\nReported-by: Jeff King \nSigned-off-by: Thomas Gummerer \n---\n builtin/stash.c | 11 +++++++----\n t/t3903-stash.sh | 16 ++++++++++++++++\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex b5a301f24d..da1260ca8e 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -396,7 +396,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tconst struct object_id *bases[1];\n \n \tread_cache_preload(NULL);\n-\tif (refresh_cache(REFRESH_QUIET))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&c_tree, 0, NULL))\n@@ -485,7 +485,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t}\n \n \tif (quiet) {\n-\t\tif (refresh_cache(REFRESH_QUIET))\n+\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0))\n \t\t\twarning(\"could not refresh index\");\n \t} else {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1129,7 +1129,10 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \tprepare_fallback_ident(\"git stash\", \"git@stash\");\n \n \tread_cache_preload(NULL);\n-\trefresh_cache(REFRESH_QUIET);\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0) < 0) {\n+\t\tret = -1;\n+\t\tgoto done;\n+\t}\n \n \tif (get_oid(\"HEAD\", &info->b_commit)) {\n \t\tif (!quiet)\n@@ -1290,7 +1293,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\tfree(ps_matched);\n \t}\n \n-\tif (refresh_cache(REFRESH_QUIET)) {\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex b8e337893f..392954d6dd 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1241,4 +1241,20 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu\n \ttest_path_is_missing to-remove\n '\n \n+test_expect_success 'stash apply should succeed with unmodified file' '\n+\techo base >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\n+\t# now stash a modification\n+\techo modified >file &&\n+\tgit stash &&\n+\n+\t# make the file stat dirty\n+\tcp file other &&\n+\tmv other file &&\n+\n+\tgit stash apply\n+'\n+\n test_done\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"381919","messageId":"xmqqwoemo131.fsf@gitster-ct.c.googlers.com","threadId":"51758","inReplyTo":"20190903191041.10470-2-t.gummerer@gmail.com","subject":"Re: [PATCH v3 1/3] factor out refresh_and_write_cache function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T22:00:34Z","receivedAt":"2019-09-05T22:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> Getting the lock for the index, refreshing it and then writing it is a\n> pattern that happens more than once throughout the codebase, and isn't\n> trivial to get right. Factor out the refresh_and_write_cache function\n> from builtin/am.c to read-cache.c, so it can be re-used in other\n> places in a subsequent commit.\n>\n> Note that we return different error codes for failing to refresh the\n> cache, and failing to write the index. The current caller only cares\n> about failing to write the index. However for other callers we're\n> going to convert in subsequent patches we will need this distinction.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n> builtin/am.c | 16 ++--------------\n> cache.h | 16 ++++++++++++++++\n> read-cache.c | 19 +++++++++++++++++++\n> 3 files changed, 37 insertions(+), 14 deletions(-)\n\nI think this goes in the right direction, but obviously conflicts\nwith what Dscho wants to do in the builtin-add-i series, and needs\nto be reconciled by working better together.\n\nFor now, I'll eject builtin-add-i and queue this for a few days to\ngive it a bit more exposure, but after that requeue builtin-add-i\nand discard these three patches. By that time, hopefully you two\nwould have a rerolled version of this one and builtin-add-i that\nagree what kind of refresh-and-write-index behaviour they both want.\n\nThe differences I see that need reconciling are:\n\n - builtin-add-i seems to allow 'gentle' and allow returning an\n error when we cannot open the index for writing by passing false\n to 'gentle'; this feature is not used yet, though.\n\n - This version allows to pass pathspec, seen and header_msg, while\n the one in builtin-add-i cannot limit the part of the index\n getting refreshed with pathspec. It wouldn't be a brain surgery\n to use this version and adjust the caller (there only is one) in\n the builtin-add-i topic.\n\n - This version does not write the index back when refresh_index()\n returns non-zero, but the one in builtin-add-i ignores the\n returned value. I think, as a performance measure, it probably\n is a better idea to write it back, even when the function returns\n non-zero (the local variable's name is has_errors, but having an\n entry in the index that does not get refreshed is *not* an error;\n e.g. an unmerged entry is a normal thing in the index, and as\n long as we refreshed other entries while having an unmerged and\n unrefreshable entry, we are making progress that is worth writing\n out).\n\nThanks.\n\n> +int repo_refresh_and_write_index(struct repository *repo,\n> +\t\t\t\t unsigned int refresh_flags,\n> +\t\t\t\t unsigned int write_flags,\n> +\t\t\t\t const struct pathspec *pathspec,\n> +\t\t\t\t char *seen, const char *header_msg)\n> +{\n> +\tstruct lock_file lock_file = LOCK_INIT;\n> +\n> +\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n> +\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {\n> +\t\trollback_lock_file(&lock_file);\n> +\t\treturn 1;\n> +\t}\n> +\tif (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n> +\t\treturn -1;\n> +\treturn 0;\n> +}\n> +\n> +\n> int refresh_index(struct index_state *istate, unsigned int flags,\n> \t\t const struct pathspec *pathspec,\n> \t\t char *seen, const char *header_msg)\n"},{"id":"381957","messageId":"20190906141812.GA128436@cat","threadId":"51758","inReplyTo":"xmqqwoemo131.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/3] factor out refresh_and_write_cache function","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-06T14:18:12Z","receivedAt":"2019-09-06T14:18:19Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/05, Junio C Hamano wrote:\n> Thomas Gummerer writes:\n> \n> > Getting the lock for the index, refreshing it and then writing it is a\n> > pattern that happens more than once throughout the codebase, and isn't\n> > trivial to get right. Factor out the refresh_and_write_cache function\n> > from builtin/am.c to read-cache.c, so it can be re-used in other\n> > places in a subsequent commit.\n> >\n> > Note that we return different error codes for failing to refresh the\n> > cache, and failing to write the index. The current caller only cares\n> > about failing to write the index. However for other callers we're\n> > going to convert in subsequent patches we will need this distinction.\n> >\n> > Signed-off-by: Thomas Gummerer \n> > ---\n> > builtin/am.c | 16 ++--------------\n> > cache.h | 16 ++++++++++++++++\n> > read-cache.c | 19 +++++++++++++++++++\n> > 3 files changed, 37 insertions(+), 14 deletions(-)\n> \n> I think this goes in the right direction, but obviously conflicts\n> with what Dscho wants to do in the builtin-add-i series, and needs\n> to be reconciled by working better together.\n\nOops, I didn't realize there was another series in flight that also\nintroduces 'repo_refresh_and_write_index'. Probably should have done\na test merge of this with pu.\n\n> For now, I'll eject builtin-add-i and queue this for a few days to\n> give it a bit more exposure, but after that requeue builtin-add-i\n> and discard these three patches. By that time, hopefully you two\n> would have a rerolled version of this one and builtin-add-i that\n> agree what kind of refresh-and-write-index behaviour they both want.\n>\n> The differences I see that need reconciling are:\n\nThanks for writing these down.\n\n> - builtin-add-i seems to allow 'gentle' and allow returning an\n> error when we cannot open the index for writing by passing false\n> to 'gentle'; this feature is not used yet, though.\n\nRight, and if gentle is set to false, it avoids writing the index,\nwhich seems fine from my perspective.\n\n> - This version allows to pass pathspec, seen and header_msg, while\n> the one in builtin-add-i cannot limit the part of the index\n> getting refreshed with pathspec. It wouldn't be a brain surgery\n> to use this version and adjust the caller (there only is one) in\n> the builtin-add-i topic.\n\n'pathspec', 'seen' and 'header_msg' are not used in my version either,\nI just implemented it for completeness and compatibility. So I'd be\nfine to do without them.\n\n> - This version does not write the index back when refresh_index()\n> returns non-zero, but the one in builtin-add-i ignores the\n> returned value. I think, as a performance measure, it probably\n> is a better idea to write it back, even when the function returns\n> non-zero (the local variable's name is has_errors, but having an\n> entry in the index that does not get refreshed is *not* an error;\n> e.g. an unmerged entry is a normal thing in the index, and as\n> long as we refreshed other entries while having an unmerged and\n> unrefreshable entry, we are making progress that is worth writing\n> out).\n\nI'm happy with writing the index back even if there are errors.\nHowever I think we still need the option to get the return code from\n'refresh_index()', as some callers where I'm using\n'refresh_and_write_index()' in this series behave differently\ndepending on its return code.\n\nThere's two more differences between the versions:\n\n - The version in my series allows passing in write_flags to be passed\n to write_locked_index, which is required to convert the callers in\n builtin/merge.c.\n\n - Dscho's version also calls 'repo_read_index_preload()', which I\n don't do in mine. Some callers don't need to do that, so I think it\n would be nice to keep that outside of the\n 'repo_refresh_and_write_index()' function.\n\nI can think of a few ways forward here:\n\n - I incorporate features that are needed for the builtin-add-i series\n here, and that is rebased on top of this series.\n\n - We drop the first two patches of this series, so we only fix the\n problems in 'git stash' for now. Later we can have a refactoring\n series that uses repo_refresh_and_write_index in the places we\n converted here, once the dust of the builtin-add-i series settled.\n\n - I rebase this on top of builtin-add-i.\n\nI'm happy with either of the first two, but less so with the last\noption. I was hoping this series could potentially go to maint as it\nwas a bugfix, which we obviously can't do with that option.\n\nDscho, what do you think? :)\n\n> Thanks.\n> \n> > +int repo_refresh_and_write_index(struct repository *repo,\n> > +\t\t\t\t unsigned int refresh_flags,\n> > +\t\t\t\t unsigned int write_flags,\n> > +\t\t\t\t const struct pathspec *pathspec,\n> > +\t\t\t\t char *seen, const char *header_msg)\n> > +{\n> > +\tstruct lock_file lock_file = LOCK_INIT;\n> > +\n> > +\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n> > +\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {\n> > +\t\trollback_lock_file(&lock_file);\n> > +\t\treturn 1;\n> > +\t}\n> > +\tif (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n> > +\t\treturn -1;\n> > +\treturn 0;\n> > +}\n> > +\n> > +\n> > int refresh_index(struct index_state *istate, unsigned int flags,\n> > \t\t const struct pathspec *pathspec,\n> > \t\t char *seen, const char *header_msg)\n"},{"id":"382156","messageId":"nycvar.QRO.7.76.6.1909111155540.5377@tvgsbejvaqbjf.bet","threadId":"51758","inReplyTo":"20190906141812.GA128436@cat","subject":"Re: [PATCH v3 1/3] factor out refresh_and_write_cache function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-11T10:57:05Z","receivedAt":"2019-09-11T10:57:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Fri, 6 Sep 2019, Thomas Gummerer wrote:\n\n> On 09/05, Junio C Hamano wrote:\n> > Thomas Gummerer writes:\n> >\n> > > Getting the lock for the index, refreshing it and then writing it is a\n> > > pattern that happens more than once throughout the codebase, and isn't\n> > > trivial to get right. Factor out the refresh_and_write_cache function\n> > > from builtin/am.c to read-cache.c, so it can be re-used in other\n> > > places in a subsequent commit.\n> > >\n> > > Note that we return different error codes for failing to refresh the\n> > > cache, and failing to write the index. The current caller only cares\n> > > about failing to write the index. However for other callers we're\n> > > going to convert in subsequent patches we will need this distinction.\n> > >\n> > > Signed-off-by: Thomas Gummerer \n> > > ---\n> > > builtin/am.c | 16 ++--------------\n> > > cache.h | 16 ++++++++++++++++\n> > > read-cache.c | 19 +++++++++++++++++++\n> > > 3 files changed, 37 insertions(+), 14 deletions(-)\n> >\n> > I think this goes in the right direction, but obviously conflicts\n> > with what Dscho wants to do in the builtin-add-i series, and needs\n> > to be reconciled by working better together.\n>\n> Oops, I didn't realize there was another series in flight that also\n> introduces 'repo_refresh_and_write_index'. Probably should have done\n> a test merge of this with pu.\n\nYep, our patches clash. I would not mind placing my patch series on top\nof yours, provided that you can make a few changes that I need ;-)\n\n> > For now, I'll eject builtin-add-i and queue this for a few days to\n> > give it a bit more exposure, but after that requeue builtin-add-i\n> > and discard these three patches. By that time, hopefully you two\n> > would have a rerolled version of this one and builtin-add-i that\n> > agree what kind of refresh-and-write-index behaviour they both want.\n> >\n> > The differences I see that need reconciling are:\n>\n> Thanks for writing these down.\n>\n> > - builtin-add-i seems to allow 'gentle' and allow returning an\n> > error when we cannot open the index for writing by passing false\n> > to 'gentle'; this feature is not used yet, though.\n>\n> Right, and if gentle is set to false, it avoids writing the index,\n> which seems fine from my perspective.\n\nThis also suggests that it would make sense to avoid\n`LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a\nlibrary function, not just a helper function for a one-shot built-in\n(don't you like how this idea \"it is okay to use exit() to clean up\nafter us, we don't care\" comes back to bite us?).\n\n> > - This version allows to pass pathspec, seen and header_msg, while\n> > the one in builtin-add-i cannot limit the part of the index\n> > getting refreshed with pathspec. It wouldn't be a brain surgery\n> > to use this version and adjust the caller (there only is one) in\n> > the builtin-add-i topic.\n>\n> 'pathspec', 'seen' and 'header_msg' are not used in my version either,\n> I just implemented it for completeness and compatibility. So I'd be\n> fine to do without them.\n\nOh, why not keep them? I'd rather keep them and adjust the caller in\n`builtin-add-i`.\n\n> > - This version does not write the index back when refresh_index()\n> > returns non-zero, but the one in builtin-add-i ignores the\n> > returned value. I think, as a performance measure, it probably\n> > is a better idea to write it back, even when the function returns\n> > non-zero (the local variable's name is has_errors, but having an\n> > entry in the index that does not get refreshed is *not* an error;\n> > e.g. an unmerged entry is a normal thing in the index, and as\n> > long as we refreshed other entries while having an unmerged and\n> > unrefreshable entry, we are making progress that is worth writing\n> > out).\n>\n> I'm happy with writing the index back even if there are errors.\n> However I think we still need the option to get the return code from\n> 'refresh_index()', as some callers where I'm using\n> 'refresh_and_write_index()' in this series behave differently\n> depending on its return code.\n>\n> There's two more differences between the versions:\n>\n> - The version in my series allows passing in write_flags to be passed\n> to write_locked_index, which is required to convert the callers in\n> builtin/merge.c.\n\nI can always pass in 0 as `write_flags`.\n\n> - Dscho's version also calls 'repo_read_index_preload()', which I\n> don't do in mine. Some callers don't need to do that, so I think it\n> would be nice to keep that outside of the\n> 'repo_refresh_and_write_index()' function.\n\nAgreed.\n\n> I can think of a few ways forward here:\n>\n> - I incorporate features that are needed for the builtin-add-i series\n> here, and that is rebased on top of this series.\n\nI'd prefer this way forward. The `builtin-add-i` patch series is\nevolving more slowly than yours.\n\n> - We drop the first two patches of this series, so we only fix the\n> problems in 'git stash' for now. Later we can have a refactoring\n> series that uses repo_refresh_and_write_index in the places we\n> converted here, once the dust of the builtin-add-i series settled.\n>\n> - I rebase this on top of builtin-add-i.\n>\n> I'm happy with either of the first two, but less so with the last\n> option. I was hoping this series could potentially go to maint as it\n> was a bugfix, which we obviously can't do with that option.\n>\n> Dscho, what do you think? :)\n\nSee above ;-)\n\nThank you!\nDscho\n\n>\n> > Thanks.\n> >\n> > > +int repo_refresh_and_write_index(struct repository *repo,\n> > > +\t\t\t\t unsigned int refresh_flags,\n> > > +\t\t\t\t unsigned int write_flags,\n> > > +\t\t\t\t const struct pathspec *pathspec,\n> > > +\t\t\t\t char *seen, const char *header_msg)\n> > > +{\n> > > +\tstruct lock_file lock_file = LOCK_INIT;\n> > > +\n> > > +\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n> > > +\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {\n> > > +\t\trollback_lock_file(&lock_file);\n> > > +\t\treturn 1;\n> > > +\t}\n> > > +\tif (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n> > > +\t\treturn -1;\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +\n> > > int refresh_index(struct index_state *istate, unsigned int flags,\n> > > \t\t const struct pathspec *pathspec,\n> > > \t\t char *seen, const char *header_msg)\n>\n"},{"id":"382170","messageId":"20190911175201.GA11444@cat","threadId":"51758","inReplyTo":"nycvar.QRO.7.76.6.1909111155540.5377@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 1/3] factor out refresh_and_write_cache function","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-11T17:52:01Z","receivedAt":"2019-09-11T17:52:08Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/11, Johannes Schindelin wrote:\n> Hi Thomas,\n> \n> On Fri, 6 Sep 2019, Thomas Gummerer wrote:\n> > Oops, I didn't realize there was another series in flight that also\n> > introduces 'repo_refresh_and_write_index'. Probably should have done\n> > a test merge of this with pu.\n> \n> Yep, our patches clash. I would not mind placing my patch series on top\n> of yours, provided that you can make a few changes that I need ;-)\n\nSounds good. Looking ahead further I don't mind these changes at all!\n\n> > Right, and if gentle is set to false, it avoids writing the index,\n> > which seems fine from my perspective.\n> \n> This also suggests that it would make sense to avoid\n> `LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a\n> library function, not just a helper function for a one-shot built-in\n> (don't you like how this idea \"it is okay to use exit() to clean up\n> after us, we don't care\" comes back to bite us?).\n\nYup, returning an error for this definitely makes sense, especially\nfor future proofing.\n\n> > > - This version allows to pass pathspec, seen and header_msg, while\n> > > the one in builtin-add-i cannot limit the part of the index\n> > > getting refreshed with pathspec. It wouldn't be a brain surgery\n> > > to use this version and adjust the caller (there only is one) in\n> > > the builtin-add-i topic.\n> >\n> > 'pathspec', 'seen' and 'header_msg' are not used in my version either,\n> > I just implemented it for completeness and compatibility. So I'd be\n> > fine to do without them.\n> \n> Oh, why not keep them? I'd rather keep them and adjust the caller in\n> `builtin-add-i`.\n\nGreat, I'm happy to keep them.\n\n> > There's two more differences between the versions:\n> >\n> > - The version in my series allows passing in write_flags to be passed\n> > to write_locked_index, which is required to convert the callers in\n> > builtin/merge.c.\n> \n> I can always pass in 0 as `write_flags`.\n> \n> > - Dscho's version also calls 'repo_read_index_preload()', which I\n> > don't do in mine. Some callers don't need to do that, so I think it\n> > would be nice to keep that outside of the\n> > 'repo_refresh_and_write_index()' function.\n> \n> Agreed.\n> \n> > I can think of a few ways forward here:\n> >\n> > - I incorporate features that are needed for the builtin-add-i series\n> > here, and that is rebased on top of this series.\n> \n> I'd prefer this way forward. The `builtin-add-i` patch series is\n> evolving more slowly than yours.\n\nGreat! I'll send an updated version of my series soon. Thanks!\n"},{"id":"382175","messageId":"20190911182027.41284-1-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190903191041.10470-1-t.gummerer@gmail.com","subject":"[PATCH v4 0/3] make sure stash refreshes the index properly","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-11T18:20:24Z","receivedAt":"2019-09-11T18:21:16Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Compared to the previous round this round introduces a gentle flag for\nrefresh_and_write_{index,cache}, which should make this function\nsuitable for use in the Dscho's builtin-add-i series. The latter will have to be \n\nI have also pushed this to https://github.com/tgummerer/git tg/stash-refresh-index\n\nRange-diff below:\n\n1: 7cc9f5fff4 ! 1: 2a7bebb20f factor out refresh_and_write_cache function\n @@ Commit message\n about failing to write the index. However for other callers we're\n going to convert in subsequent patches we will need this distinction.\n \n + Helped-by: Martin Ågren \n + Helped-by: Johannes Schindelin \n Signed-off-by: Thomas Gummerer \n \n ## builtin/am.c ##\n @@ builtin/am.c: static void am_run(struct am_state *state, int resume)\n \tunlink(am_path(state, \"dirtyindex\"));\n \n -\trefresh_and_write_cache();\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, 0) < 0)\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0)\n +\t\tdie(_(\"unable to write index file\"));\n \n \tif (repo_index_has_changes(the_repository, NULL, &sb)) {\n @@ cache.h: extern struct index_state the_index;\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n -+#define refresh_and_write_cache(refresh_flags, write_flags) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), NULL, NULL, NULL)\n ++#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)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))\n @@ cache.h: void fill_stat_cache_info(struct index_state *istate, struct cache_entr\n + * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so\n + * the lockfile is always either committed or rolled back.\n + *\n ++ * If 'gentle' is passed, errors locking the index are ignored.\n ++ *\n + * Return 1 if refreshing the index returns an error, -1 if writing\n + * the index to disk fails, 0 on success.\n + *\n -+ * Note that if refreshing the index returns an error, we don't write\n -+ * the result to disk.\n ++ * Note that if refreshing the index returns an error, we still write\n ++ * out the result (unless locking failed).\n + */\n -+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);\n ++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);\n +\n struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);\n \n @@ read-cache.c: static void show_file(const char * fmt, const char * name, int in_\n \tprintf(fmt, name);\n }\n \n -+int repo_refresh_and_write_index(struct repository *repo,\n ++int repo_refresh_and_write_index(struct repository *repo,\n +\t\t\t\t unsigned int refresh_flags,\n +\t\t\t\t unsigned int write_flags,\n ++\t\t\t\t int gentle,\n +\t\t\t\t const struct pathspec *pathspec,\n +\t\t\t\t char *seen, const char *header_msg)\n +{\n +\tstruct lock_file lock_file = LOCK_INIT;\n ++\tint fd, ret = 0;\n +\n -+\trepo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);\n -+\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {\n -+\t\trollback_lock_file(&lock_file);\n -+\t\treturn 1;\n -+\t}\n -+\tif (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n ++\tfd = repo_hold_locked_index(repo, &lock_file, 0);\n ++\tif (!gentle && fd < 0)\n +\t\treturn -1;\n -+\treturn 0;\n ++\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n ++\t\tret = 1;\n ++\tif (0 <= fd && write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n ++\t\tret = -1;\n ++\trollback_lock_file(&lock_file);\n ++\treturn ret;\n +}\n +\n +\n2: 0367d938b1 ! 2: 555c982eae merge: use refresh_and_write_cache\n @@ builtin/merge.c: static int try_merge_strategy(const char *strategy, struct comm\n -\trefresh_cache(REFRESH_QUIET);\n -\tif (write_locked_index(&the_index, &lock,\n -\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n @@ builtin/merge.c: static int merge_trivial(struct commit *head, struct commit_lis\n -\trefresh_cache(REFRESH_QUIET);\n -\tif (write_locked_index(&the_index, &lock,\n -\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED) < 0)\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \twrite_tree_trivial(&result_tree);\n3: 8ed3df9fec ! 3: cf74fe6053 stash: make sure to write refreshed cache\n @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info\n \n \tread_cache_preload(NULL);\n -\tif (refresh_cache(REFRESH_QUIET))\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, 0))\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&c_tree, 0, NULL))\n @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info\n \n \tif (quiet) {\n -\t\tif (refresh_cache(REFRESH_QUIET))\n -+\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0))\n ++\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n \t\t\twarning(\"could not refresh index\");\n \t} else {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n @@ builtin/stash.c: static int do_create_stash(const struct pathspec *ps, struct st\n \n \tread_cache_preload(NULL);\n -\trefresh_cache(REFRESH_QUIET);\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, 0) < 0) {\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0) {\n +\t\tret = -1;\n +\t\tgoto done;\n +\t}\n @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char\n \t}\n \n -\tif (refresh_cache(REFRESH_QUIET)) {\n -+\tif (refresh_and_write_cache(REFRESH_QUIET, 0)) {\n ++\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\n\nThomas Gummerer (3):\n factor out refresh_and_write_cache function\n merge: use refresh_and_write_cache\n stash: make sure to write refreshed cache\n\n builtin/am.c | 16 ++--------------\n builtin/merge.c | 13 +++----------\n builtin/stash.c | 11 +++++++----\n cache.h | 18 ++++++++++++++++++\n read-cache.c | 21 +++++++++++++++++++++\n t/t3903-stash.sh | 16 ++++++++++++++++\n 6 files changed, 67 insertions(+), 28 deletions(-)\n\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"382176","messageId":"20190911182027.41284-2-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190911182027.41284-1-t.gummerer@gmail.com","subject":"[PATCH v4 1/3] factor out refresh_and_write_cache function","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-11T18:20:25Z","receivedAt":"2019-09-11T18:21:18Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Getting the lock for the index, refreshing it and then writing it is a\npattern that happens more than once throughout the codebase, and isn't\ntrivial to get right. Factor out the refresh_and_write_cache function\nfrom builtin/am.c to read-cache.c, so it can be re-used in other\nplaces in a subsequent commit.\n\nNote that we return different error codes for failing to refresh the\ncache, and failing to write the index. The current caller only cares\nabout failing to write the index. However for other callers we're\ngoing to convert in subsequent patches we will need this distinction.\n\nHelped-by: Martin Ågren \nHelped-by: Johannes Schindelin \nSigned-off-by: Thomas Gummerer \n---\n builtin/am.c | 16 ++--------------\n cache.h | 18 ++++++++++++++++++\n read-cache.c | 21 +++++++++++++++++++++\n 3 files changed, 41 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1aea657a7f..92e0e70069 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1071,19 +1071,6 @@ static const char *msgnum(const struct am_state *state)\n \treturn sb.buf;\n }\n \n-/**\n- * Refresh and write index.\n- */\n-static void refresh_and_write_cache(void)\n-{\n-\tstruct lock_file lock_file = LOCK_INIT;\n-\n-\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\tdie(_(\"unable to write index file\"));\n-}\n-\n /**\n * Dies with a user-friendly message on how to proceed after resolving the\n * problem. This message can be overridden with state->resolvemsg.\n@@ -1703,7 +1690,8 @@ static void am_run(struct am_state *state, int resume)\n \n \tunlink(am_path(state, \"dirtyindex\"));\n \n-\trefresh_and_write_cache();\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0)\n+\t\tdie(_(\"unable to write index file\"));\n \n \tif (repo_index_has_changes(the_repository, NULL, &sb)) {\n \t\twrite_state_bool(state, \"dirtyindex\", 1);\ndiff --git a/cache.h b/cache.h\nindex b1da1ab08f..68a54f50ac 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -414,6 +414,7 @@ extern struct index_state the_index;\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n+#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)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))\n@@ -812,6 +813,23 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st\n #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n #define REFRESH_PROGRESS\t0x0040 /* show progress bar if stderr is tty */\n int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n+/*\n+ * Refresh the index and write it to disk.\n+ *\n+ * 'refresh_flags' is passed directly to 'refresh_index()', while\n+ * 'COMMIT_LOCK | write_flags' is passed to 'write_locked_index()', so\n+ * the lockfile is always either committed or rolled back.\n+ *\n+ * If 'gentle' is passed, errors locking the index are ignored.\n+ *\n+ * Return 1 if refreshing the index returns an error, -1 if writing\n+ * the index to disk fails, 0 on success.\n+ *\n+ * Note that if refreshing the index returns an error, we still write\n+ * out the index (unless locking fails).\n+ */\n+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);\n+\n struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);\n \n void set_alternate_index_output(const char *);\ndiff --git a/read-cache.c b/read-cache.c\nindex 52ffa8a313..7e646e06c2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1472,6 +1472,27 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,\n \tprintf(fmt, name);\n }\n \n+int repo_refresh_and_write_index(struct repository *repo,\n+\t\t\t\t unsigned int refresh_flags,\n+\t\t\t\t unsigned int write_flags,\n+\t\t\t\t int gentle,\n+\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t char *seen, const char *header_msg)\n+{\n+\tstruct lock_file lock_file = LOCK_INIT;\n+\tint fd, ret = 0;\n+\n+\tfd = repo_hold_locked_index(repo, &lock_file, 0);\n+\tif (!gentle && fd < 0)\n+\t\treturn -1;\n+\tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n+\t\tret = 1;\n+\tif (0 <= fd && write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))\n+\t\tret = -1;\n+\treturn ret;\n+}\n+\n+\n int refresh_index(struct index_state *istate, unsigned int flags,\n \t\t const struct pathspec *pathspec,\n \t\t char *seen, const char *header_msg)\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"382177","messageId":"20190911182027.41284-3-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190911182027.41284-1-t.gummerer@gmail.com","subject":"[PATCH v4 2/3] merge: use refresh_and_write_cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-11T18:20:26Z","receivedAt":"2019-09-11T18:21:18Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Use the 'refresh_and_write_cache()' convenience function introduced in\nthe last commit, instead of refreshing and writing the index manually\nin merge.c\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/merge.c | 13 +++----------\n 1 file changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e2ccbc44e2..83e42fcb10 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -688,16 +688,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t struct commit_list *remoteheads,\n \t\t\t struct commit *head)\n {\n-\tstruct lock_file lock = LOCK_INIT;\n \tconst char *head_arg = \"HEAD\";\n \n-\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock,\n-\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n+\t\tstruct lock_file lock = LOCK_INIT;\n \t\tint clean, x;\n \t\tstruct commit *result;\n \t\tstruct commit_list *reversed = NULL;\n@@ -860,12 +857,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n {\n \tstruct object_id result_tree, result_commit;\n \tstruct commit_list *parents, **pptr = &parents;\n-\tstruct lock_file lock = LOCK_INIT;\n \n-\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n-\trefresh_cache(REFRESH_QUIET);\n-\tif (write_locked_index(&the_index, &lock,\n-\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \twrite_tree_trivial(&result_tree);\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"382178","messageId":"20190911182027.41284-4-t.gummerer@gmail.com","threadId":"51758","inReplyTo":"20190911182027.41284-1-t.gummerer@gmail.com","subject":"[PATCH v4 3/3] stash: make sure to write refreshed cache","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-09-11T18:20:27Z","receivedAt":"2019-09-11T18:21:19Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When converting stash into C, calls to 'git update-index --refresh'\nwere replaced with the 'refresh_cache()' function. That is fine as\nlong as the index is only needed in-core, and not re-read from disk.\n\nHowever in many cases we do actually need the refreshed index to be\nwritten to disk, for example 'merge_recursive_generic()' discards the\nin-core index before re-reading it from disk, and in the case of 'apply\n--quiet', the 'refresh_cache()' we currently have is pointless without\nwriting the index to disk.\n\nAlways write the index after refreshing it to ensure there are no\nregressions in this compared to the scripted stash. In the future we\ncan consider avoiding the write where possible after making sure none\nof the subsequent calls actually need the refreshed cache, and it is\nnot expected to be refreshed after stash exits or it is written\nsomewhere else already.\n\nReported-by: Jeff King \nSigned-off-by: Thomas Gummerer \n---\n builtin/stash.c | 11 +++++++----\n t/t3903-stash.sh | 16 ++++++++++++++++\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex b5a301f24d..ab30d1e920 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -396,7 +396,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tconst struct object_id *bases[1];\n \n \tread_cache_preload(NULL);\n-\tif (refresh_cache(REFRESH_QUIET))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&c_tree, 0, NULL))\n@@ -485,7 +485,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t}\n \n \tif (quiet) {\n-\t\tif (refresh_cache(REFRESH_QUIET))\n+\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n \t\t\twarning(\"could not refresh index\");\n \t} else {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1129,7 +1129,10 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \tprepare_fallback_ident(\"git stash\", \"git@stash\");\n \n \tread_cache_preload(NULL);\n-\trefresh_cache(REFRESH_QUIET);\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0) {\n+\t\tret = -1;\n+\t\tgoto done;\n+\t}\n \n \tif (get_oid(\"HEAD\", &info->b_commit)) {\n \t\tif (!quiet)\n@@ -1290,7 +1293,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\tfree(ps_matched);\n \t}\n \n-\tif (refresh_cache(REFRESH_QUIET)) {\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex b8e337893f..392954d6dd 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1241,4 +1241,20 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu\n \ttest_path_is_missing to-remove\n '\n \n+test_expect_success 'stash apply should succeed with unmodified file' '\n+\techo base >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\n+\t# now stash a modification\n+\techo modified >file &&\n+\tgit stash &&\n+\n+\t# make the file stat dirty\n+\tcp file other &&\n+\tmv other file &&\n+\n+\tgit stash apply\n+'\n+\n test_done\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"382228","messageId":"xmqqh85hh382.fsf@gitster-ct.c.googlers.com","threadId":"51758","inReplyTo":"20190911175201.GA11444@cat","subject":"Re: [PATCH v3 1/3] factor out refresh_and_write_cache function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-12T16:46:37Z","receivedAt":"2019-09-12T16:46:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> On 09/11, Johannes Schindelin wrote:\n>> Hi Thomas,\n>> \n>> On Fri, 6 Sep 2019, Thomas Gummerer wrote:\n>> > Oops, I didn't realize there was another series in flight that also\n>> > introduces 'repo_refresh_and_write_index'. Probably should have done\n>> > a test merge of this with pu.\n>> \n>> Yep, our patches clash. I would not mind placing my patch series on top\n>> of yours, provided that you can make a few changes that I need ;-)\n>\n> Sounds good. Looking ahead further I don't mind these changes at all!\n>\n>> > Right, and if gentle is set to false, it avoids writing the index,\n>> > which seems fine from my perspective.\n>> \n>> This also suggests that it would make sense to avoid\n>> `LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a\n>> library function, not just a helper function for a one-shot built-in\n>> (don't you like how this idea \"it is okay to use exit() to clean up\n>> after us, we don't care\" comes back to bite us?).\n>\n> Yup, returning an error for this definitely makes sense, especially\n> for future proofing.\n>\n>> > > - This version allows to pass pathspec, seen and header_msg, while\n>> > > the one in builtin-add-i cannot limit the part of the index\n>> > > getting refreshed with pathspec. It wouldn't be a brain surgery\n>> > > to use this version and adjust the caller (there only is one) in\n>> > > the builtin-add-i topic.\n>> >\n>> > 'pathspec', 'seen' and 'header_msg' are not used in my version either,\n>> > I just implemented it for completeness and compatibility. So I'd be\n>> > fine to do without them.\n>> \n>> Oh, why not keep them? I'd rather keep them and adjust the caller in\n>> `builtin-add-i`.\n>\n> Great, I'm happy to keep them.\n>\n>> > There's two more differences between the versions:\n>> >\n>> > - The version in my series allows passing in write_flags to be passed\n>> > to write_locked_index, which is required to convert the callers in\n>> > builtin/merge.c.\n>> \n>> I can always pass in 0 as `write_flags`.\n>> \n>> > - Dscho's version also calls 'repo_read_index_preload()', which I\n>> > don't do in mine. Some callers don't need to do that, so I think it\n>> > would be nice to keep that outside of the\n>> > 'repo_refresh_and_write_index()' function.\n>> \n>> Agreed.\n>> \n>> > I can think of a few ways forward here:\n>> >\n>> > - I incorporate features that are needed for the builtin-add-i series\n>> > here, and that is rebased on top of this series.\n>> \n>> I'd prefer this way forward. The `builtin-add-i` patch series is\n>> evolving more slowly than yours.\n>\n> Great! I'll send an updated version of my series soon. Thanks!\n\nI just read the conclusion you two reached (after being down and\noffline for two days) and found the reasoning totally sensible.\n\nThanks, both of you, for working well together.\n"}]}