{"thread":{"id":"57615","subject":"[PATCH] mv: refresh stat info for moved entry","startedAt":"2022-03-25T01:57:06Z","lastAt":"2022-03-29T19:05:34Z","messageCount":10,"participants":["Victoria Dye via GitGitGadget","Derrick Stolee","Victoria Dye","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"452253","messageId":"pull.1187.git.1648173419533.gitgitgadget@gmail.com","threadId":"57615","inReplyTo":null,"subject":"[PATCH] mv: refresh stat info for moved entry","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-25T01:56:59Z","receivedAt":"2022-03-25T01:57:06Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nAdd 'refresh_cache_entry()' after moving the index entry in\n'rename_index_entry_at()'. Internally, 'git mv' uses\n'rename_index_entry_at()' to move the source index entry to the destination,\noverwriting the old entry if '-f' is specified. However, it does not refresh\nthe stat information on destination index entry, making its 'CE_UPTODATE'\nflag out-of-date until the index is refreshed (e.g., by 'git status').\n\nSome commands, such as 'git reset', assume the 'CE_UPTODATE' information\nthey read from the index is accurate, and use that information to determine\nwhether the operation can be done successfully or not. In order to ensure\nthe index is correct for commands such as these, explicitly refresh the\ndestination index entry in 'git mv' before exiting.\n\nReported-by: Maximilian Reichel <reichemn@icloud.com>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n    mv: refresh stat info for moved entry\n    \n    This patch fixes a bug [1] encountered when executing 'git reset --merge\n    HEAD' immediately after 'git mv -f' overwrites an existing index entry.\n    Because the 'CE_UPTODATE' flag is not refreshed on the destination entry\n    (and therefore incorrectly appeared to not be \"up-to-date\"), 'git reset\n    --merge HEAD' fails when it should otherwise succeed.\n    \n    To avoid exiting 'git mv' with a stale index that may affect subsequent\n    commands, 'rename_index_entry_at()' (used internally by 'git mv') is\n    updated to refresh the destination index entry's stat information after\n    the move is complete.\n    \n    [1]\n    https://lore.kernel.org/git/84FF8F9A-3A9A-4F2A-8D8E-5D50F2F06203@icloud.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1187%2Fvdye%2Freset%2Fmerge-inconsistency-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1187/vdye/reset/merge-inconsistency-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1187\n\n read-cache.c  |  1 +\n t/t7001-mv.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 1ad56d02e1d..2c5ccc48d6c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -148,6 +148,7 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n \tuntracked_cache_remove_from_index(istate, old_entry->name);\n \tremove_index_entry_at(istate, nr);\n \tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n+\trefresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n }\n \n void fill_stat_data(struct stat_data *sd, struct stat *st)\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 963356ba5f9..ab8607678e7 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -4,6 +4,17 @@ test_description='git mv in subdirs'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff-data.sh\n \n+test_expect_success 'mv -f refreshes updated index entry' '\n+\techo test >bar &&\n+\tgit add bar &&\n+\tgit commit -m test &&\n+\n+\techo foo >foo &&\n+\tgit add foo &&\n+\tgit mv -f foo bar &&\n+\tgit reset --merge HEAD\n+'\n+\n test_expect_success 'prepare reference tree' '\n \tmkdir path0 path1 &&\n \tCOPYING_test_data >path0/COPYING &&\n\nbase-commit: a68dfadae5e95c7f255cf38c9efdcbc2e36d1931\n-- \ngitgitgadget\n"},{"id":"452267","messageId":"30d20d10-da33-ae41-7887-d73279e14e92@github.com","threadId":"57615","inReplyTo":"pull.1187.git.1648173419533.gitgitgadget@gmail.com","subject":"Re: [PATCH] mv: refresh stat info for moved entry","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-25T14:31:10Z","receivedAt":"2022-03-25T14:31:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/24/2022 9:56 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye <vdye@github.com>\n> \n> Add 'refresh_cache_entry()' after moving the index entry in\n> 'rename_index_entry_at()'. Internally, 'git mv' uses\n> 'rename_index_entry_at()' to move the source index entry to the destination,\n> overwriting the old entry if '-f' is specified. However, it does not refresh\n> the stat information on destination index entry, making its 'CE_UPTODATE'\n> flag out-of-date until the index is refreshed (e.g., by 'git status').\n> \n> Some commands, such as 'git reset', assume the 'CE_UPTODATE' information\n> they read from the index is accurate, and use that information to determine\n> whether the operation can be done successfully or not. In order to ensure\n> the index is correct for commands such as these, explicitly refresh the\n> destination index entry in 'git mv' before exiting.\n\nGood find. Thanks for the fix!\n\n> Reported-by: Maximilian Reichel <reichemn@icloud.com>\n\nThanks for the report, Maximilian!\n\n> @@ -148,6 +148,7 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n>  \tuntracked_cache_remove_from_index(istate, old_entry->name);\n>  \tremove_index_entry_at(istate, nr);\n>  \tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n> +\trefresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n\nIt certainly seems reasonable to add this line. I was unfamiliar\nwith this method, and it is used only twice: when creating a new\ncache entry in make_cache_entry() and in merge-recursive.c's\nadd_cache_info(). So, it is currently acting in the case of a\nnewly-inserted cache entry in its existing cases, and here in\n'git mv' that's essentially what we are doing (deleting the old\nand adding a new would be more appropriate than just moving the\nold one).\n\nI took a brief detour thinking about performance, but this is\nrun only once per command-line argument, so any new overhead\nis minimal.\n  \n> +test_expect_success 'mv -f refreshes updated index entry' '\n> +\techo test >bar &&\n> +\tgit add bar &&\n> +\tgit commit -m test &&\n> +\n> +\techo foo >foo &&\n> +\tgit add foo &&\n> +\tgit mv -f foo bar &&\n> +\tgit reset --merge HEAD\n\nIs there any post-condition on the index that we want to check here?\n\nThat is, is there anything that we could notice via 'git status' or\nsimilar that would break before this patch (assuming we put a\ntest_might_fail in front of the 'git reset --merge HEAD' line)?\n\nThanks,\n-Stolee\n"},{"id":"452391","messageId":"d03a34e6-d6a7-6ddb-5784-57078e32ab89@github.com","threadId":"57615","inReplyTo":"30d20d10-da33-ae41-7887-d73279e14e92@github.com","subject":"Re: [PATCH] mv: refresh stat info for moved entry","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-25T22:37:41Z","receivedAt":"2022-03-25T22:37:46Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee wrote:\n> On 3/24/2022 9:56 PM, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye <vdye@github.com>\n>>\n>> Add 'refresh_cache_entry()' after moving the index entry in\n>> 'rename_index_entry_at()'. Internally, 'git mv' uses\n>> 'rename_index_entry_at()' to move the source index entry to the destination,\n>> overwriting the old entry if '-f' is specified. However, it does not refresh\n>> the stat information on destination index entry, making its 'CE_UPTODATE'\n>> flag out-of-date until the index is refreshed (e.g., by 'git status').\n>>\n>> Some commands, such as 'git reset', assume the 'CE_UPTODATE' information\n>> they read from the index is accurate, and use that information to determine\n>> whether the operation can be done successfully or not. In order to ensure\n>> the index is correct for commands such as these, explicitly refresh the\n>> destination index entry in 'git mv' before exiting.\n> \n> Good find. Thanks for the fix!\n> \n>> Reported-by: Maximilian Reichel <reichemn@icloud.com>\n> \n> Thanks for the report, Maximilian!\n> \n>> @@ -148,6 +148,7 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n>>  \tuntracked_cache_remove_from_index(istate, old_entry->name);\n>>  \tremove_index_entry_at(istate, nr);\n>>  \tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n>> +\trefresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n> \n> It certainly seems reasonable to add this line. I was unfamiliar\n> with this method, and it is used only twice: when creating a new\n> cache entry in make_cache_entry() and in merge-recursive.c's\n> add_cache_info(). So, it is currently acting in the case of a\n> newly-inserted cache entry in its existing cases, and here in\n> 'git mv' that's essentially what we are doing (deleting the old\n> and adding a new would be more appropriate than just moving the\n> old one).\n> \n> I took a brief detour thinking about performance, but this is\n> run only once per command-line argument, so any new overhead\n> is minimal.\n>   \n>> +test_expect_success 'mv -f refreshes updated index entry' '\n>> +\techo test >bar &&\n>> +\tgit add bar &&\n>> +\tgit commit -m test &&\n>> +\n>> +\techo foo >foo &&\n>> +\tgit add foo &&\n>> +\tgit mv -f foo bar &&\n>> +\tgit reset --merge HEAD\n> \n> Is there any post-condition on the index that we want to check here?\n> \n> That is, is there anything that we could notice via 'git status' or\n> similar that would break before this patch (assuming we put a\n> test_might_fail in front of the 'git reset --merge HEAD' line)?\n> \n\nWhile looking into this, I realized 1) that I wasn't actually refreshing the\ncache entry because 'refresh_cache_entry' doesn't work in-place (will fix in\nthe next version) and 2) the test was only passing because of a race\ncondition that (as of right now) I can't quite figure out. \n\nIf I add a 'sleep 1' after the 'git add', the test behaves as I expect:\nfails when 'mv' doesn't refresh the entry, passes when it does. However,\nwhen the sleep *isn't* there and I'm testing 'git mv' without the refresh,\nmost of the time the test *doesn't* fail (sometimes it does, but not\nreliably). I'm going to try finding the root cause of that before sending\nV2, in case I'm missing something else that should be fixed.\n\nBut to answer your question, yes - 'git diff-files' will produce an empty\nresult if the cache is reset properly, and will be non-empty if it is not.\nI'll include that in the re-roll as well. \n\n> Thanks,\n> -Stolee\n\n"},{"id":"452393","messageId":"xmqqk0ch8vc3.fsf@gitster.g","threadId":"57615","inReplyTo":"pull.1187.git.1648173419533.gitgitgadget@gmail.com","subject":"Re: [PATCH] mv: refresh stat info for moved entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-25T23:29:32Z","receivedAt":"2022-03-25T23:29:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/read-cache.c b/read-cache.c\n> index 1ad56d02e1d..2c5ccc48d6c 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -148,6 +148,7 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n>  \tuntracked_cache_remove_from_index(istate, old_entry->name);\n>  \tremove_index_entry_at(istate, nr);\n>  \tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n> +\trefresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n>  }\n\nThis is a bit unforunate.\n\nIf we are renaming \"foo\" to \"bar\", I wonder if we can grab the\ncached stat info before calling remove_index_entry_at() for \"foo\"\nand copy it to the new entry we create for \"bar\".  After all, we\nwould be making a corresponding change to rename from \"foo\" to \"bar\"\nat the filesystem level, too, no?\n\nWell, we are already doing that by calling copy_cache_entry().  So\nwhy do we further need to refresh the cache entry in the first\nplace?  There is something fishy going on, isn't it?\n\nPuzzling...\n\nIn any case, refresh_cache_entry() either returns ce or create a\nnewly allocated entry, so you'd want to first refresh and then the\nadd the cache entry returned from the function to the index, I\nthink.\n\nThanks.\n"},{"id":"452403","messageId":"703439d9-bf4e-4e71-754b-60dabc07301a@github.com","threadId":"57615","inReplyTo":"xmqqk0ch8vc3.fsf@gitster.g","subject":"Re: [PATCH] mv: refresh stat info for moved entry","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-26T01:23:55Z","receivedAt":"2022-03-26T01:24:00Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> diff --git a/read-cache.c b/read-cache.c\n>> index 1ad56d02e1d..2c5ccc48d6c 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -148,6 +148,7 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n>>  \tuntracked_cache_remove_from_index(istate, old_entry->name);\n>>  \tremove_index_entry_at(istate, nr);\n>>  \tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n>> +\trefresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n>>  }\n> \n> This is a bit unforunate.\n> \n> If we are renaming \"foo\" to \"bar\", I wonder if we can grab the\n> cached stat info before calling remove_index_entry_at() for \"foo\"\n> and copy it to the new entry we create for \"bar\".  After all, we\n> would be making a corresponding change to rename from \"foo\" to \"bar\"\n> at the filesystem level, too, no?\n> \n> Well, we are already doing that by calling copy_cache_entry().  So\n> why do we further need to refresh the cache entry in the first\n> place?  There is something fishy going on, isn't it?\n> \n\nAfter some more debugging, I found that the exact trigger for the \"not\nuptodate\" error is a mismatch between the 'ctime' logged in the index entry\nand the 'ctime' found on disk. Because we're copying stat information over\ndirectly from the original index entry in 'git mv', the 'ctime' does *not*\nreflect the time of file renaming, leading to the error later.\n\nI think this explains all of the behavior we've seen so far:\n\n1. Any index-refreshing operation run after 'git mv' would prevent the 'git\n   reset --merge' error, since the ctime would be updated.\n2. The error doesn't happen when the file is created within ~1 second of the\n   'git mv' (noticed in the test earlier today [1]), since the 'ctime' would\n   be seen as \"matching\" between the index and on-disk.\n3. Adding 'refresh_cache_entry()' (and assigning the return value properly,\n   unlike in V1 of this patch) avoids the error for the same reason as #1.\n\nSo, based on that, I think a \"refresh\" at the end of\n'rename_index_entry_at()' is still needed, but only the stat info needs to\nbe updated. If that sounds reasonable, I'll send V2 with that change and\nupdate the test to more appropriately test this scenario. \n\nThanks!\n\n[1] https://lore.kernel.org/git/d03a34e6-d6a7-6ddb-5784-57078e32ab89@github.com/\n\n> Puzzling...\n> \n> In any case, refresh_cache_entry() either returns ce or create a\n> newly allocated entry, so you'd want to first refresh and then the\n> add the cache entry returned from the function to the index, I\n> think.\n> \n> Thanks.\n\n"},{"id":"452540","messageId":"pull.1187.v2.git.1648516027925.gitgitgadget@gmail.com","threadId":"57615","inReplyTo":"pull.1187.git.1648173419533.gitgitgadget@gmail.com","subject":"[PATCH v2] mv: refresh stat info for moved entry","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-29T01:07:07Z","receivedAt":"2022-03-29T01:07:13Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nUpdate the stat info of the moved index entry in 'rename_index_entry_at()'\nif the entry is up-to-date with the index. Internally, 'git mv' uses\n'rename_index_entry_at()' to move the source index entry to the destination.\nHowever, it directly copies the stat info of the original cache entry, which\nwill not reflect the 'ctime' of the file renaming operation that happened as\npart of the move. If a file is otherwise up-to-date with the index, that\ndifference in 'ctime' will make the entry appear out-of-date until the next\nindex-refreshing operation (e.g., 'git status').\n\nSome commands, such as 'git reset', use the cached stat information to\ndetermine whether a file is up-to-date; if this information is incorrect,\nthe command will fail when it should pass. In order to ensure a moved entry\nis evaluated as 'up-to-date' when appropriate, refresh the destination index\nentry's stat info in 'git mv' if and only if the file is up-to-date.\n\nNote that the test added in 't7001-mv.sh' requires a \"sleep 1\" to ensure the\n'ctime' of the file creation will be definitively older than the 'ctime' of\nthe renamed file in 'git mv'.\n\nReported-by: Maximilian Reichel <reichemn@icloud.com>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n    mv: refresh stat info for moved entry\n    \n    This patch fixes a bug [1] encountered when executing 'git reset --merge\n    HEAD' immediately after 'git mv'. Because the stat info of the original\n    entry is copied to the new one, including all cached stat information,\n    the 'ctime' isn't updated corresponding to the rename. If the index\n    entry is otherwise up-to-date with the contents on-disk, the incorrect\n    'ctime' makes subsequent operations (e.g., 'git reset --merge') identify\n    the index entry as out-of-date, failing when they should succeed.\n    \n    Note, however, that we use 'refresh_cache_entry()' to refresh the stat\n    information rather than 'fill_stat_cache_info()' directly because the\n    stat information should only be updated if the index entry is up-to-date\n    with the file on-disk. If we ignored this distinction, the stat info\n    would match the state of unstaged changes on-disk, not the entry in the\n    index as it should.\n    \n    To avoid exiting 'git mv' with a stale index that may affect subsequent\n    commands, 'rename_index_entry_at()' (used internally by 'git mv') is\n    updated to refresh the destination index entry's stat information after\n    the move is complete.\n    \n    [1]\n    https://lore.kernel.org/git/84FF8F9A-3A9A-4F2A-8D8E-5D50F2F06203@icloud.com/\n    \n    \n    Changes since V1\n    ================\n    \n     * Determined the exact cause of the failure (the mismatched 'ctime'),\n       as well as the reasoning for why the stat information cannot always\n       be updated; revised the implementation accordingly.\n     * Fixed usage of 'refresh_cache_entry()'; because it does not update\n       cache entries in-place, insert its return value into the index (if\n       valid), and discard the no-longer-needed 'new_entry' cache entry.\n     * Updated the test for the bug report scenario to wait long enough such\n       that the 'ctime' of the 'mv' is distinct from the original file\n       creation time.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1187%2Fvdye%2Freset%2Fmerge-inconsistency-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1187/vdye/reset/merge-inconsistency-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1187\n\nRange-diff vs v1:\n\n 1:  fe9f5f0d8ec ! 1:  bde58070eda mv: refresh stat info for moved entry\n     @@ Metadata\n       ## Commit message ##\n          mv: refresh stat info for moved entry\n      \n     -    Add 'refresh_cache_entry()' after moving the index entry in\n     -    'rename_index_entry_at()'. Internally, 'git mv' uses\n     -    'rename_index_entry_at()' to move the source index entry to the destination,\n     -    overwriting the old entry if '-f' is specified. However, it does not refresh\n     -    the stat information on destination index entry, making its 'CE_UPTODATE'\n     -    flag out-of-date until the index is refreshed (e.g., by 'git status').\n     +    Update the stat info of the moved index entry in 'rename_index_entry_at()'\n     +    if the entry is up-to-date with the index. Internally, 'git mv' uses\n     +    'rename_index_entry_at()' to move the source index entry to the destination.\n     +    However, it directly copies the stat info of the original cache entry, which\n     +    will not reflect the 'ctime' of the file renaming operation that happened as\n     +    part of the move. If a file is otherwise up-to-date with the index, that\n     +    difference in 'ctime' will make the entry appear out-of-date until the next\n     +    index-refreshing operation (e.g., 'git status').\n      \n     -    Some commands, such as 'git reset', assume the 'CE_UPTODATE' information\n     -    they read from the index is accurate, and use that information to determine\n     -    whether the operation can be done successfully or not. In order to ensure\n     -    the index is correct for commands such as these, explicitly refresh the\n     -    destination index entry in 'git mv' before exiting.\n     +    Some commands, such as 'git reset', use the cached stat information to\n     +    determine whether a file is up-to-date; if this information is incorrect,\n     +    the command will fail when it should pass. In order to ensure a moved entry\n     +    is evaluated as 'up-to-date' when appropriate, refresh the destination index\n     +    entry's stat info in 'git mv' if and only if the file is up-to-date.\n     +\n     +    Note that the test added in 't7001-mv.sh' requires a \"sleep 1\" to ensure the\n     +    'ctime' of the file creation will be definitively older than the 'ctime' of\n     +    the renamed file in 'git mv'.\n      \n          Reported-by: Maximilian Reichel <reichemn@icloud.com>\n          Signed-off-by: Victoria Dye <vdye@github.com>\n      \n       ## read-cache.c ##\n     +@@ read-cache.c: static void replace_index_entry(struct index_state *istate, int nr, struct cache\n     + \n     + void rename_index_entry_at(struct index_state *istate, int nr, const char *new_name)\n     + {\n     +-\tstruct cache_entry *old_entry = istate->cache[nr], *new_entry;\n     ++\tstruct cache_entry *old_entry = istate->cache[nr], *new_entry, *refreshed;\n     + \tint namelen = strlen(new_name);\n     + \n     + \tnew_entry = make_empty_cache_entry(istate, namelen);\n      @@ read-cache.c: void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n     + \tcache_tree_invalidate_path(istate, old_entry->name);\n       \tuntracked_cache_remove_from_index(istate, old_entry->name);\n       \tremove_index_entry_at(istate, nr);\n     - \tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n     -+\trefresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n     +-\tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n     ++\n     ++\t/*\n     ++\t * Refresh the new index entry. Using 'refresh_cache_entry' ensures\n     ++\t * we only update stat info if the entry is otherwise up-to-date (i.e.,\n     ++\t * the contents/mode haven't changed). This ensures that we reflect the\n     ++\t * 'ctime' of the rename in the index without (incorrectly) updating\n     ++\t * the cached stat info to reflect unstaged changes on disk.\n     ++\t */\n     ++\trefreshed = refresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n     ++\tif (refreshed && refreshed != new_entry) {\n     ++\t\tadd_index_entry(istate, refreshed, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n     ++\t\tdiscard_cache_entry(new_entry);\n     ++\t} else\n     ++\t\tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n       }\n       \n       void fill_stat_data(struct stat_data *sd, struct stat *st)\n     @@ t/t7001-mv.sh: test_description='git mv in subdirs'\n      +\n      +\techo foo >foo &&\n      +\tgit add foo &&\n     ++\n     ++\t# Wait one second to ensure ctime of rename will differ from original\n     ++\t# file creation ctime.\n     ++\tsleep 1 &&\n      +\tgit mv -f foo bar &&\n     -+\tgit reset --merge HEAD\n     ++\tgit reset --merge HEAD &&\n     ++\n     ++\t# Verify the index has been reset\n     ++\tgit diff-files >out &&\n     ++\ttest_must_be_empty out\n      +'\n      +\n       test_expect_success 'prepare reference tree' '\n\n\n read-cache.c  | 17 +++++++++++++++--\n t/t7001-mv.sh | 19 +++++++++++++++++++\n 2 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 3e0e7d41837..4df97e185e9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -134,7 +134,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n \n void rename_index_entry_at(struct index_state *istate, int nr, const char *new_name)\n {\n-\tstruct cache_entry *old_entry = istate->cache[nr], *new_entry;\n+\tstruct cache_entry *old_entry = istate->cache[nr], *new_entry, *refreshed;\n \tint namelen = strlen(new_name);\n \n \tnew_entry = make_empty_cache_entry(istate, namelen);\n@@ -147,7 +147,20 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n \tcache_tree_invalidate_path(istate, old_entry->name);\n \tuntracked_cache_remove_from_index(istate, old_entry->name);\n \tremove_index_entry_at(istate, nr);\n-\tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n+\n+\t/*\n+\t * Refresh the new index entry. Using 'refresh_cache_entry' ensures\n+\t * we only update stat info if the entry is otherwise up-to-date (i.e.,\n+\t * the contents/mode haven't changed). This ensures that we reflect the\n+\t * 'ctime' of the rename in the index without (incorrectly) updating\n+\t * the cached stat info to reflect unstaged changes on disk.\n+\t */\n+\trefreshed = refresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n+\tif (refreshed && refreshed != new_entry) {\n+\t\tadd_index_entry(istate, refreshed, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n+\t\tdiscard_cache_entry(new_entry);\n+\t} else\n+\t\tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n }\n \n void fill_stat_data(struct stat_data *sd, struct stat *st)\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 963356ba5f9..a402908142d 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -4,6 +4,25 @@ test_description='git mv in subdirs'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff-data.sh\n \n+test_expect_success 'mv -f refreshes updated index entry' '\n+\techo test >bar &&\n+\tgit add bar &&\n+\tgit commit -m test &&\n+\n+\techo foo >foo &&\n+\tgit add foo &&\n+\n+\t# Wait one second to ensure ctime of rename will differ from original\n+\t# file creation ctime.\n+\tsleep 1 &&\n+\tgit mv -f foo bar &&\n+\tgit reset --merge HEAD &&\n+\n+\t# Verify the index has been reset\n+\tgit diff-files >out &&\n+\ttest_must_be_empty out\n+'\n+\n test_expect_success 'prepare reference tree' '\n \tmkdir path0 path1 &&\n \tCOPYING_test_data >path0/COPYING &&\n\nbase-commit: abf474a5dd901f28013c52155411a48fd4c09922\n-- \ngitgitgadget\n"},{"id":"452560","messageId":"5ca04e86-6c61-3d4e-88a0-a3c827e19e13@github.com","threadId":"57615","inReplyTo":"pull.1187.v2.git.1648516027925.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] mv: refresh stat info for moved entry","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-29T13:19:29Z","receivedAt":"2022-03-29T13:19:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/28/2022 9:07 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye <vdye@github.com>\n> \n> Update the stat info of the moved index entry in 'rename_index_entry_at()'\n> if the entry is up-to-date with the index. Internally, 'git mv' uses\n> 'rename_index_entry_at()' to move the source index entry to the destination.\n> However, it directly copies the stat info of the original cache entry, which\n> will not reflect the 'ctime' of the file renaming operation that happened as\n> part of the move. If a file is otherwise up-to-date with the index, that\n> difference in 'ctime' will make the entry appear out-of-date until the next\n> index-refreshing operation (e.g., 'git status').\n> \n> Some commands, such as 'git reset', use the cached stat information to\n> determine whether a file is up-to-date; if this information is incorrect,\n> the command will fail when it should pass. In order to ensure a moved entry\n> is evaluated as 'up-to-date' when appropriate, refresh the destination index\n> entry's stat info in 'git mv' if and only if the file is up-to-date.\n> \n> Note that the test added in 't7001-mv.sh' requires a \"sleep 1\" to ensure the\n> 'ctime' of the file creation will be definitively older than the 'ctime' of\n> the renamed file in 'git mv'.\n\nUnfortunate that this is necessary, but it seems to be the only way\nto handle this because of the interaction with the system clock and\nthe filesystem. There are several sleeps like this in\nt1701-racy-split-index.sh, for example.\n\n>  void rename_index_entry_at(struct index_state *istate, int nr, const char *new_name)\n>  {\n> -\tstruct cache_entry *old_entry = istate->cache[nr], *new_entry;\n> +\tstruct cache_entry *old_entry = istate->cache[nr], *new_entry, *refreshed;\n>  \tint namelen = strlen(new_name);\n>  \n>  \tnew_entry = make_empty_cache_entry(istate, namelen);\n> @@ -147,7 +147,20 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n>  \tcache_tree_invalidate_path(istate, old_entry->name);\n>  \tuntracked_cache_remove_from_index(istate, old_entry->name);\n>  \tremove_index_entry_at(istate, nr);\n> -\tadd_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n> +\n> +\t/*\n> +\t * Refresh the new index entry. Using 'refresh_cache_entry' ensures\n> +\t * we only update stat info if the entry is otherwise up-to-date (i.e.,\n> +\t * the contents/mode haven't changed). This ensures that we reflect the\n> +\t * 'ctime' of the rename in the index without (incorrectly) updating\n> +\t * the cached stat info to reflect unstaged changes on disk.\n> +\t */\n> +\trefreshed = refresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n> +\tif (refreshed && refreshed != new_entry) {\n> +\t\tadd_index_entry(istate, refreshed, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n> +\t\tdiscard_cache_entry(new_entry);\n\nI'm glad you're checking that both refreshed is non-NULL and not equal\nto new_entry. Both are possible results from refresh_cache_entry().\n\n> +test_expect_success 'mv -f refreshes updated index entry' '\n> +\techo test >bar &&\n> +\tgit add bar &&\n> +\tgit commit -m test &&\n> +\n> +\techo foo >foo &&\n> +\tgit add foo &&\n> +\n> +\t# Wait one second to ensure ctime of rename will differ from original\n> +\t# file creation ctime.\n> +\tsleep 1 &&\n> +\tgit mv -f foo bar &&\n> +\tgit reset --merge HEAD &&\n> +\n> +\t# Verify the index has been reset\n> +\tgit diff-files >out &&\n> +\ttest_must_be_empty out\n> +'\n> +\n\nNew test looks good.\n\nThanks,\n-Stolee\n"},{"id":"452583","messageId":"xmqqa6d8wvwt.fsf@gitster.g","threadId":"57615","inReplyTo":"5ca04e86-6c61-3d4e-88a0-a3c827e19e13@github.com","subject":"Re: [PATCH v2] mv: refresh stat info for moved entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-29T16:44:34Z","receivedAt":"2022-03-29T16:44:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>> Note that the test added in 't7001-mv.sh' requires a \"sleep 1\" to ensure the\n>> 'ctime' of the file creation will be definitively older than the 'ctime' of\n>> the renamed file in 'git mv'.\n>\n> Unfortunate that this is necessary, but it seems to be the only way\n> to handle this because of the interaction with the system clock and\n> the filesystem. There are several sleeps like this in\n> t1701-racy-split-index.sh, for example.\n\nDoes \"test-tool chmtime\" to tweak the filesystem timestamp help?  I\ndidn't look at the specific step that uses sleep to work around.\n\n>> +\n>> +\t/*\n>> +\t * Refresh the new index entry. Using 'refresh_cache_entry' ensures\n>> +\t * we only update stat info if the entry is otherwise up-to-date (i.e.,\n>> +\t * the contents/mode haven't changed). This ensures that we reflect the\n>> +\t * 'ctime' of the rename in the index without (incorrectly) updating\n>> +\t * the cached stat info to reflect unstaged changes on disk.\n>> +\t */\n>> +\trefreshed = refresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);\n>> +\tif (refreshed && refreshed != new_entry) {\n>> +\t\tadd_index_entry(istate, refreshed, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n>> +\t\tdiscard_cache_entry(new_entry);\n>\n> I'm glad you're checking that both refreshed is non-NULL and not equal\n> to new_entry. Both are possible results from refresh_cache_entry().\n\n;-)\n"},{"id":"452598","messageId":"d017b0c6-c06b-e8cd-55a1-6972a9ae7c52@github.com","threadId":"57615","inReplyTo":"xmqqa6d8wvwt.fsf@gitster.g","subject":"Re: [PATCH v2] mv: refresh stat info for moved entry","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-29T18:54:57Z","receivedAt":"2022-03-29T18:55:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/29/2022 12:44 PM, Junio C Hamano wrote:\n> Derrick Stolee <derrickstolee@github.com> writes:\n> \n>>> Note that the test added in 't7001-mv.sh' requires a \"sleep 1\" to ensure the\n>>> 'ctime' of the file creation will be definitively older than the 'ctime' of\n>>> the renamed file in 'git mv'.\n>>\n>> Unfortunate that this is necessary, but it seems to be the only way\n>> to handle this because of the interaction with the system clock and\n>> the filesystem. There are several sleeps like this in\n>> t1701-racy-split-index.sh, for example.\n> \n> Does \"test-tool chmtime\" to tweak the filesystem timestamp help?  I\n> didn't look at the specific step that uses sleep to work around.\n\nThe issue here is related to ctime, which is not modified by that\nhelper. It also uses utime() which does not seem to have a way to\nmodify ctime (which makes sense).\n\nThanks,\n-Stolee\n"},{"id":"452599","messageId":"xmqqee2ktw96.fsf@gitster.g","threadId":"57615","inReplyTo":"d017b0c6-c06b-e8cd-55a1-6972a9ae7c52@github.com","subject":"Re: [PATCH v2] mv: refresh stat info for moved entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-29T19:05:25Z","receivedAt":"2022-03-29T19:05:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>> Does \"test-tool chmtime\" to tweak the filesystem timestamp help?  I\n>> didn't look at the specific step that uses sleep to work around.\n>\n> The issue here is related to ctime, which is not modified by that\n> helper. It also uses utime() which does not seem to have a way to\n> modify ctime (which makes sense).\n\nAhh, silly me.  You're right.\n"}]}