* [PATCH] t7800: fix racy "difftool --dir-diff syncs worktree" test
@ 2025-12-31 22:19 Paul Tarjan via GitGitGadget
2026-01-01 14:49 ` Johannes Schindelin
2026-01-01 18:27 ` [PATCH v2] " Paul Tarjan via GitGitGadget
0 siblings, 2 replies; 11+ messages in thread
From: Paul Tarjan via GitGitGadget @ 2025-12-31 22:19 UTC (permalink / raw)
To: git; +Cc: Paul Tarjan, Paul Tarjan
From: Paul Tarjan <github@paulisageek.com>
The "difftool --dir-diff syncs worktree without unstaged change" test
fails intermittently, particularly on Windows CI.
The test modifies a file in difftool's temp directory via an extcmd
script and expects the change to be synced back to the worktree. The
sync-back detection relies on git's change detection mechanisms.
The root cause is that the original file content and the replacement
content have identical sizes:
- Original: "main\ntest\na\n" = 12 bytes
- New: "new content\n" = 12 bytes
When difftool creates the temporary index (wtindex), the cache entries
have sd_size = 0 (zero-initialized via make_cache_entry with no
refresh). Git's ie_modified() is designed to handle this by calling
ce_modified_check_fs() for content hashing when sd_size is 0.
However, Windows has known filesystem issues that may cause this to
fail intermittently:
- UNRELIABLE_FSTAT: Windows fstat() on open files may not return the
same information as lstat() after close (config.mak.uname:506)
- NTFS timestamp issues: The racy-git documentation notes that NTFS
is "still broken" regarding timestamp granularity between in-core
and on-disk representations (Documentation/technical/racy-git.adoc)
- Attribute caching: Windows GetFileAttributesExW may cache results
Fix this by changing the replacement content to "modified content\n"
(17 bytes), ensuring the change is detected at the earliest size
comparison in match_stat_data(), bypassing any platform-specific edge
cases in the more complex code paths.
Note: Other tests with same-size file patterns (t0010-racy-git.sh,
t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable
because they use normal Git index operations with proper racy git
detection. The difftool case is unique due to its ephemeral wtindex
created via make_cache_entry() without full stat refresh.
Signed-off-by: Paul Tarjan <github@paulisageek.com>
---
t7800: fix racy "difftool --dir-diff syncs worktree" test
In
https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416
this test failed for me on an unrelated commit. I had Claude look into
it and it thought that this could be a racy git problem. I'm skeptical
but a) I don't know the source well enough and b) the fix is low risk so
I thought I'd send it to you folks. Everything below is the AI generated
explanation.
The "difftool --dir-diff syncs worktree without unstaged change" test
fails intermittently, particularly on Windows CI.
The test modifies a file in difftool's temp directory via an extcmd
script and expects the change to be synced back to the worktree. The
sync-back detection relies on git's change detection mechanisms.
The root cause is that the original file content and the replacement
content have identical sizes:
* Original: "main\ntest\na\n" = 12 bytes
* New: "new content\n" = 12 bytes
When difftool creates the temporary index (wtindex), the cache entries
have sd_size = 0 (zero-initialized via make_cache_entry with no
refresh). Git's ie_modified() is designed to handle this by calling
ce_modified_check_fs() for content hashing when sd_size is 0.
However, Windows has known filesystem issues that may cause this to fail
intermittently:
* UNRELIABLE_FSTAT: Windows fstat() on open files may not return the
same information as lstat() after close (config.mak.uname:506)
* NTFS timestamp issues: The racy-git documentation notes that NTFS is
"still broken" regarding timestamp granularity between in-core and
on-disk representations (Documentation/technical/racy-git.adoc)
* Attribute caching: Windows GetFileAttributesExW may cache results
Fix this by changing the replacement content to "modified content\n" (17
bytes), ensuring the change is detected at the earliest size comparison
in match_stat_data(), bypassing any platform-specific edge cases in the
more complex code paths.
Note: Other tests with same-size file patterns (t0010-racy-git.sh,
t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable
because they use normal Git index operations with proper racy git
detection. The difftool case is unique due to its ephemeral wtindex
created via make_cache_entry() without full stat refresh.
Signed-off-by: Paul Tarjan github@paulisageek.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2149%2Fptarjan%2Fclaude%2Ffix-difftool-test-DDxDC-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2149/ptarjan/claude/fix-difftool-test-DDxDC-v1
Pull-Request: https://github.com/git/git/pull/2149
t/t7800-difftool.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bf0f67378d..8a91ff3603 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -647,21 +647,21 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch
'
write_script modify-right-file <<\EOF
-echo "new content" >"$2/file"
+echo "modified content" >"$2/file"
EOF
run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '
test_when_finished git reset --hard &&
echo "orig content" >file &&
git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch &&
- echo "new content" >expect &&
+ echo "modified content" >expect &&
test_cmp expect file
'
run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '
test_when_finished git reset --hard &&
git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch &&
- echo "new content" >expect &&
+ echo "modified content" >expect &&
test_cmp expect file
'
base-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe
--
gitgitgadget
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] t7800: fix racy "difftool --dir-diff syncs worktree" test 2025-12-31 22:19 [PATCH] t7800: fix racy "difftool --dir-diff syncs worktree" test Paul Tarjan via GitGitGadget @ 2026-01-01 14:49 ` Johannes Schindelin 2026-01-01 18:27 ` [PATCH v2] " Paul Tarjan via GitGitGadget 1 sibling, 0 replies; 11+ messages in thread From: Johannes Schindelin @ 2026-01-01 14:49 UTC (permalink / raw) To: Paul Tarjan via GitGitGadget; +Cc: git, Paul Tarjan Hi Paul, On Thu, 1 Jan 2026, Paul Tarjan via GitGitGadget wrote: > From: Paul Tarjan <github@paulisageek.com> > > The "difftool --dir-diff syncs worktree without unstaged change" test > fails intermittently, particularly on Windows CI. > > The test modifies a file in difftool's temp directory via an extcmd > script and expects the change to be synced back to the worktree. The > sync-back detection relies on git's change detection mechanisms. > > The root cause is that the original file content and the replacement > content have identical sizes: > > - Original: "main\ntest\na\n" = 12 bytes > - New: "new content\n" = 12 bytes > > When difftool creates the temporary index (wtindex), the cache entries > have sd_size = 0 (zero-initialized via make_cache_entry with no > refresh). Git's ie_modified() is designed to handle this by calling > ce_modified_check_fs() for content hashing when sd_size is 0. > > However, Windows has known filesystem issues that may cause this to > fail intermittently: > > - UNRELIABLE_FSTAT: Windows fstat() on open files may not return the > same information as lstat() after close (config.mak.uname:506) > > - NTFS timestamp issues: The racy-git documentation notes that NTFS > is "still broken" regarding timestamp granularity between in-core > and on-disk representations (Documentation/technical/racy-git.adoc) > > - Attribute caching: Windows GetFileAttributesExW may cache results > > Fix this by changing the replacement content to "modified content\n" > (17 bytes), ensuring the change is detected at the earliest size > comparison in match_stat_data(), bypassing any platform-specific edge > cases in the more complex code paths. > > Note: Other tests with same-size file patterns (t0010-racy-git.sh, > t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable > because they use normal Git index operations with proper racy git > detection. The difftool case is unique due to its ephemeral wtindex > created via make_cache_entry() without full stat refresh. > > Signed-off-by: Paul Tarjan <github@paulisageek.com> > --- Nice! This test case indeed is flaky, and the analysis looks sound. If anything, I would add that Git relies on the inode to change when nothing else is different (file size, mtime, etc), but on Windows, there are no inodes. For what it's worth, this issue is actually a real-world problem, not just a side effect observed exclusively in test scenarios, see e.g. https://github.com/git-for-windows/git/issues/5132 for a bug report about Git's being challenged with changes that aren't reflected by file size/mtime differences. Side note: There is _something_ similar to inodes for NTFS (called `nFileIndexHigh`/`nFileIndexLow`), but it has no equivalent with FAT filesystems and is therefore not really a solution in general. See https://github.com/git-for-windows/msys2-runtime/pull/17 for an excellent demonstration of the consequences of trying to emulate inodes for FAT filesystems. Another side note: Having said all that about "no solution in general", there _is_ a ticket in Git for Windows to try to address this: https://github.com/git-for-windows/git/issues/3707. The major challenge with _that_ is that users sometimes have to access the same Git worktrees using different Git implementations (e.g. Git for Windows and an Ubuntu Git via the Windows Subsystem for Linux), and if the stat information between those implementations does not match, the Git index will be considered eternally "dirty". All this is to say: Thank you for working on this flake and addressing it. Feel free to add a Reviewed-by: trailer with my ident, if you want. Thanks! Johannes > t7800: fix racy "difftool --dir-diff syncs worktree" test > > In > https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 > this test failed for me on an unrelated commit. I had Claude look into > it and it thought that this could be a racy git problem. I'm skeptical > but a) I don't know the source well enough and b) the fix is low risk so > I thought I'd send it to you folks. Everything below is the AI generated > explanation. > > The "difftool --dir-diff syncs worktree without unstaged change" test > fails intermittently, particularly on Windows CI. > > The test modifies a file in difftool's temp directory via an extcmd > script and expects the change to be synced back to the worktree. The > sync-back detection relies on git's change detection mechanisms. > > The root cause is that the original file content and the replacement > content have identical sizes: > > * Original: "main\ntest\na\n" = 12 bytes > * New: "new content\n" = 12 bytes > > When difftool creates the temporary index (wtindex), the cache entries > have sd_size = 0 (zero-initialized via make_cache_entry with no > refresh). Git's ie_modified() is designed to handle this by calling > ce_modified_check_fs() for content hashing when sd_size is 0. > > However, Windows has known filesystem issues that may cause this to fail > intermittently: > > * UNRELIABLE_FSTAT: Windows fstat() on open files may not return the > same information as lstat() after close (config.mak.uname:506) > > * NTFS timestamp issues: The racy-git documentation notes that NTFS is > "still broken" regarding timestamp granularity between in-core and > on-disk representations (Documentation/technical/racy-git.adoc) > > * Attribute caching: Windows GetFileAttributesExW may cache results > > Fix this by changing the replacement content to "modified content\n" (17 > bytes), ensuring the change is detected at the earliest size comparison > in match_stat_data(), bypassing any platform-specific edge cases in the > more complex code paths. > > Note: Other tests with same-size file patterns (t0010-racy-git.sh, > t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable > because they use normal Git index operations with proper racy git > detection. The difftool case is unique due to its ephemeral wtindex > created via make_cache_entry() without full stat refresh. > > Signed-off-by: Paul Tarjan github@paulisageek.com > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2149%2Fptarjan%2Fclaude%2Ffix-difftool-test-DDxDC-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2149/ptarjan/claude/fix-difftool-test-DDxDC-v1 > Pull-Request: https://github.com/git/git/pull/2149 > > t/t7800-difftool.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index bf0f67378d..8a91ff3603 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -647,21 +647,21 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch > ' > > write_script modify-right-file <<\EOF > -echo "new content" >"$2/file" > +echo "modified content" >"$2/file" > EOF > > run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' ' > test_when_finished git reset --hard && > echo "orig content" >file && > git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && > - echo "new content" >expect && > + echo "modified content" >expect && > test_cmp expect file > ' > > run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' ' > test_when_finished git reset --hard && > git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && > - echo "new content" >expect && > + echo "modified content" >expect && > test_cmp expect file > ' > > > base-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe > -- > gitgitgadget > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] t7800: fix racy "difftool --dir-diff syncs worktree" test 2025-12-31 22:19 [PATCH] t7800: fix racy "difftool --dir-diff syncs worktree" test Paul Tarjan via GitGitGadget 2026-01-01 14:49 ` Johannes Schindelin @ 2026-01-01 18:27 ` Paul Tarjan via GitGitGadget 2026-01-01 22:49 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: Paul Tarjan via GitGitGadget @ 2026-01-01 18:27 UTC (permalink / raw) To: git; +Cc: Paul Tarjan, Paul Tarjan From: Paul Tarjan <github@paulisageek.com> The "difftool --dir-diff syncs worktree without unstaged change" test fails intermittently, particularly on Windows CI. The test modifies a file in difftool's temp directory via an extcmd script and expects the change to be synced back to the worktree. The sync-back detection relies on git's change detection mechanisms. The root cause is that the original file content and the replacement content have identical sizes: - Original: "main\ntest\na\n" = 12 bytes - New: "new content\n" = 12 bytes When difftool creates the temporary index (wtindex), the cache entries have sd_size = 0 (zero-initialized via make_cache_entry with no refresh). Git's ie_modified() is designed to handle this by calling ce_modified_check_fs() for content hashing when sd_size is 0. However, Windows has known filesystem issues that may cause this to fail intermittently: - UNRELIABLE_FSTAT: Windows fstat() on open files may not return the same information as lstat() after close (config.mak.uname:506) - NTFS timestamp issues: The racy-git documentation notes that NTFS is "still broken" regarding timestamp granularity between in-core and on-disk representations (Documentation/technical/racy-git.adoc) - Attribute caching: Windows GetFileAttributesExW may cache results Fix this by changing the replacement content to "modified content\n" (17 bytes), ensuring the change is detected at the earliest size comparison in match_stat_data(), bypassing any platform-specific edge cases in the more complex code paths. Note: Other tests with same-size file patterns (t0010-racy-git.sh, t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable because they use normal Git index operations with proper racy git detection. The difftool case is unique due to its ephemeral wtindex created via make_cache_entry() without full stat refresh. Signed-off-by: Paul Tarjan <github@paulisageek.com> --- t7800: fix racy "difftool --dir-diff syncs worktree" test In https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 this test failed for me on an unrelated commit. I had Claude look into it and it thought that this could be a racy git problem. I'm skeptical but a) I don't know the source well enough and b) the fix is low risk so I thought I'd send it to you folks. Everything below is the AI generated explanation. The "difftool --dir-diff syncs worktree without unstaged change" test fails intermittently, particularly on Windows CI. The test modifies a file in difftool's temp directory via an extcmd script and expects the change to be synced back to the worktree. The sync-back detection relies on git's change detection mechanisms. The root cause is that the original file content and the replacement content have identical sizes: * Original: "main\ntest\na\n" = 12 bytes * New: "new content\n" = 12 bytes When difftool creates the temporary index (wtindex), the cache entries have sd_size = 0 (zero-initialized via make_cache_entry with no refresh). Git's ie_modified() is designed to handle this by calling ce_modified_check_fs() for content hashing when sd_size is 0. However, Windows has known filesystem issues that may cause this to fail intermittently: * UNRELIABLE_FSTAT: Windows fstat() on open files may not return the same information as lstat() after close (config.mak.uname:506) * NTFS timestamp issues: The racy-git documentation notes that NTFS is "still broken" regarding timestamp granularity between in-core and on-disk representations (Documentation/technical/racy-git.adoc) * Attribute caching: Windows GetFileAttributesExW may cache results Fix this by changing the replacement content to "modified content\n" (17 bytes), ensuring the change is detected at the earliest size comparison in match_stat_data(), bypassing any platform-specific edge cases in the more complex code paths. Note: Other tests with same-size file patterns (t0010-racy-git.sh, t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable because they use normal Git index operations with proper racy git detection. The difftool case is unique due to its ephemeral wtindex created via make_cache_entry() without full stat refresh. Signed-off-by: Paul Tarjan github@paulisageek.com Reviewed-by: Johannes Schindelin Johannes.Schindelin@gmx.de Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2149%2Fptarjan%2Fclaude%2Ffix-difftool-test-DDxDC-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2149/ptarjan/claude/fix-difftool-test-DDxDC-v2 Pull-Request: https://github.com/git/git/pull/2149 Range-diff vs v1: 1: dd5b774451 = 1: 98bc88f336 t7800: fix racy "difftool --dir-diff syncs worktree" test t/t7800-difftool.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index bf0f67378d..8a91ff3603 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -647,21 +647,21 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch ' write_script modify-right-file <<\EOF -echo "new content" >"$2/file" +echo "modified content" >"$2/file" EOF run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' ' test_when_finished git reset --hard && echo "orig content" >file && git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && - echo "new content" >expect && + echo "modified content" >expect && test_cmp expect file ' run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' ' test_when_finished git reset --hard && git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && - echo "new content" >expect && + echo "modified content" >expect && test_cmp expect file ' base-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe -- gitgitgadget ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] t7800: fix racy "difftool --dir-diff syncs worktree" test 2026-01-01 18:27 ` [PATCH v2] " Paul Tarjan via GitGitGadget @ 2026-01-01 22:49 ` Junio C Hamano 2026-01-03 9:39 ` Phillip Wood 2026-01-03 20:40 ` [PATCH v3] " Paul Tarjan via GitGitGadget 2 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2026-01-01 22:49 UTC (permalink / raw) To: Paul Tarjan via GitGitGadget; +Cc: git, Paul Tarjan "Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes: > Range-diff vs v1: > > 1: dd5b774451 = 1: 98bc88f336 t7800: fix racy "difftool --dir-diff syncs worktree" test In other words, absolutely no change? But that is fine. It is clear that these changes would work around the problem with racily clean index entries. Are there things we could do to help "difftool"? For example, would it help to add a new option to "git update-index" so that scripts can say "I updated this file in the working tree, mark it as potentially racily dirty", and use it in "difftool", perhaps, or something? Queued. Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] t7800: fix racy "difftool --dir-diff syncs worktree" test 2026-01-01 18:27 ` [PATCH v2] " Paul Tarjan via GitGitGadget 2026-01-01 22:49 ` Junio C Hamano @ 2026-01-03 9:39 ` Phillip Wood 2026-01-03 16:30 ` Paul Tarjan 2026-01-03 20:40 ` [PATCH v3] " Paul Tarjan via GitGitGadget 2 siblings, 1 reply; 11+ messages in thread From: Phillip Wood @ 2026-01-03 9:39 UTC (permalink / raw) To: Paul Tarjan via GitGitGadget, git; +Cc: Paul Tarjan Hi Paul On 01/01/2026 18:27, Paul Tarjan via GitGitGadget wrote: > From: Paul Tarjan <github@paulisageek.com> > > The "difftool --dir-diff syncs worktree without unstaged change" test > fails intermittently, particularly on Windows CI. Thanks for working on this. I've seen it fail a lot in Windows CI runs - does it fail on other platforms as well? > The test modifies a file in difftool's temp directory via an extcmd > script and expects the change to be synced back to the worktree. The > sync-back detection relies on git's change detection mechanisms. > > The root cause is that the original file content and the replacement > content have identical sizes: > > - Original: "main\ntest\na\n" = 12 bytes > - New: "new content\n" = 12 bytes > > When difftool creates the temporary index (wtindex), the cache entries > have sd_size = 0 (zero-initialized via make_cache_entry with no > refresh). Git's ie_modified() is designed to handle this by calling > ce_modified_check_fs() for content hashing when sd_size is 0. > > However, Windows has known filesystem issues that may cause this to > fail intermittently: > > - UNRELIABLE_FSTAT: Windows fstat() on open files may not return the > same information as lstat() after close (config.mak.uname:506) As I understand it the test is flaky because the file is updated without changing any of the stat fields that git looks at. How does that relate to fstat() returning different data to lstat()? Also doesn't UNRELIABLE_FSTAT exist so that we can work around the problem? > - NTFS timestamp issues: The racy-git documentation notes that NTFS > is "still broken" regarding timestamp granularity between in-core > and on-disk representations (Documentation/technical/racy-git.adoc) That comment is specifically talking about linux so how does it relate to a test that is flaky on Windows? > - Attribute caching: Windows GetFileAttributesExW may cache results When git refreshes the index it calls lstat() on each path in the index. GitFileAttributesExW() provides an API like readir() which returns paths in an arbitary order and it also resolves symbolic links so I'm having a hard time understating where it is called by git. (There was a post [1] on reddit recently about using GitFileAttributesExW in this context) [1] https://www.reddit.com/r/rust/comments/1prkzqg/writing_the_fastest_implementation_of_git_status/ > Fix this by changing the replacement content to "modified content\n" > (17 bytes), ensuring the change is detected at the earliest size > comparison in match_stat_data(), bypassing any platform-specific edge > cases in the more complex code paths. This stops the test from being flaky but it is a real bug. If the user is modifying the files interactively then they're unlikely to be able to update the file fast enough to be affected but if anyone is scripting like the test does then they might be affected. Thanks Phillip > Note: Other tests with same-size file patterns (t0010-racy-git.sh, > t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable > because they use normal Git index operations with proper racy git > detection. The difftool case is unique due to its ephemeral wtindex > created via make_cache_entry() without full stat refresh. > > Signed-off-by: Paul Tarjan <github@paulisageek.com> > --- > t7800: fix racy "difftool --dir-diff syncs worktree" test > > In > https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 > this test failed for me on an unrelated commit. I had Claude look into > it and it thought that this could be a racy git problem. I'm skeptical > but a) I don't know the source well enough and b) the fix is low risk so > I thought I'd send it to you folks. Everything below is the AI generated > explanation. > > The "difftool --dir-diff syncs worktree without unstaged change" test > fails intermittently, particularly on Windows CI. > > The test modifies a file in difftool's temp directory via an extcmd > script and expects the change to be synced back to the worktree. The > sync-back detection relies on git's change detection mechanisms. > > The root cause is that the original file content and the replacement > content have identical sizes: > > * Original: "main\ntest\na\n" = 12 bytes > * New: "new content\n" = 12 bytes > > When difftool creates the temporary index (wtindex), the cache entries > have sd_size = 0 (zero-initialized via make_cache_entry with no > refresh). Git's ie_modified() is designed to handle this by calling > ce_modified_check_fs() for content hashing when sd_size is 0. > > However, Windows has known filesystem issues that may cause this to fail > intermittently: > > * UNRELIABLE_FSTAT: Windows fstat() on open files may not return the > same information as lstat() after close (config.mak.uname:506) > > * NTFS timestamp issues: The racy-git documentation notes that NTFS is > "still broken" regarding timestamp granularity between in-core and > on-disk representations (Documentation/technical/racy-git.adoc) > > * Attribute caching: Windows GetFileAttributesExW may cache results > > Fix this by changing the replacement content to "modified content\n" (17 > bytes), ensuring the change is detected at the earliest size comparison > in match_stat_data(), bypassing any platform-specific edge cases in the > more complex code paths. > > Note: Other tests with same-size file patterns (t0010-racy-git.sh, > t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable > because they use normal Git index operations with proper racy git > detection. The difftool case is unique due to its ephemeral wtindex > created via make_cache_entry() without full stat refresh. > > Signed-off-by: Paul Tarjan github@paulisageek.com Reviewed-by: Johannes > Schindelin Johannes.Schindelin@gmx.de > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2149%2Fptarjan%2Fclaude%2Ffix-difftool-test-DDxDC-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2149/ptarjan/claude/fix-difftool-test-DDxDC-v2 > Pull-Request: https://github.com/git/git/pull/2149 > > Range-diff vs v1: > > 1: dd5b774451 = 1: 98bc88f336 t7800: fix racy "difftool --dir-diff syncs worktree" test > > > t/t7800-difftool.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index bf0f67378d..8a91ff3603 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -647,21 +647,21 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch > ' > > write_script modify-right-file <<\EOF > -echo "new content" >"$2/file" > +echo "modified content" >"$2/file" > EOF > > run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' ' > test_when_finished git reset --hard && > echo "orig content" >file && > git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && > - echo "new content" >expect && > + echo "modified content" >expect && > test_cmp expect file > ' > > run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' ' > test_when_finished git reset --hard && > git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && > - echo "new content" >expect && > + echo "modified content" >expect && > test_cmp expect file > ' > > > base-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] t7800: fix racy "difftool --dir-diff syncs worktree" test 2026-01-03 9:39 ` Phillip Wood @ 2026-01-03 16:30 ` Paul Tarjan 2026-01-03 20:29 ` Johannes Schindelin 0 siblings, 1 reply; 11+ messages in thread From: Paul Tarjan @ 2026-01-03 16:30 UTC (permalink / raw) To: phillip.wood; +Cc: Paul Tarjan via GitGitGadget, git, Paul Tarjan I've updated the commit and PR summary for your comments. Should I re-run /submit to send a no-op patch or leave it as is until code changes are needed? On Fri, Jan 2, 2026 at 11:39 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Paul > > On 01/01/2026 18:27, Paul Tarjan via GitGitGadget wrote: > > From: Paul Tarjan <github@paulisageek.com> > > > > The "difftool --dir-diff syncs worktree without unstaged change" test > > fails intermittently, particularly on Windows CI. > > Thanks for working on this. I've seen it fail a lot in Windows CI runs - > does it fail on other platforms as well? I did a cursory grep through the Github actions and didn't find any other failures for this. The fact that you have seen it too means this is more widespread. I was merely reacting to the fact that it failed on my unrelated diff. My guess is this will start failing more once my fsmonitor for linux merges in since it will be yet another platform to fail on. > > > The test modifies a file in difftool's temp directory via an extcmd > > script and expects the change to be synced back to the worktree. The > > sync-back detection relies on git's change detection mechanisms. > > > > The root cause is that the original file content and the replacement > > content have identical sizes: > > > > - Original: "main\ntest\na\n" = 12 bytes > > - New: "new content\n" = 12 bytes > > > > When difftool creates the temporary index (wtindex), the cache entries > > have sd_size = 0 (zero-initialized via make_cache_entry with no > > refresh). Git's ie_modified() is designed to handle this by calling > > ce_modified_check_fs() for content hashing when sd_size is 0. > > > However, Windows has known filesystem issues that may cause this to > > fail intermittently: > > > > - UNRELIABLE_FSTAT: Windows fstat() on open files may not return the > > same information as lstat() after close (config.mak.uname:506) > > As I understand it the test is flaky because the file is updated without > changing any of the stat fields that git looks at. How does that relate > to fstat() returning different data to lstat()? Also doesn't > UNRELIABLE_FSTAT exist so that we can work around the problem? You're right, the UNRELIABLE_FSTAT reference was misapplied here. That flag addresses a different issue (fstat vs lstat discrepancies on open files). The actual problem is simpler: when file size and mtime both match, stat-based detection fails entirely. I'll remove this from the commit message. > > > - NTFS timestamp issues: The racy-git documentation notes that NTFS > > is "still broken" regarding timestamp granularity between in-core > > and on-disk representations (Documentation/technical/racy-git.adoc) > > That comment is specifically talking about linux so how does it relate > to a test that is flaky on Windows? You're right - I conflated unrelated documentation. Looking at CI history, the failure was only observed on Windows (win test (8)). The root cause is Windows-specific: Git relies on inode changes as a fallback when other stat fields match, but Windows lacks inodes. Johannes linked git-for-windows#5132 showing this affects real users, not just tests. > > > - Attribute caching: Windows GetFileAttributesExW may cache results > > When git refreshes the index it calls lstat() on each path in the index. > GitFileAttributesExW() provides an API like readir() which returns paths > in an arbitary order and it also resolves symbolic links so I'm having a > hard time understating where it is called by git. (There was a post [1] > on reddit recently about using GitFileAttributesExW in this context) > > [1] > https://www.reddit.com/r/rust/comments/1prkzqg/writing_the_fastest_implementation_of_git_status/ This was speculation on my part that doesn't hold up. The actual mechanism is straightforward: changed_files() in difftool runs update-index --really-refresh and diff-files against a temporary index. When size and mtime match, no change is detected. The Windows API details aren't relevant. I'll remove this from the explanation. > > > Fix this by changing the replacement content to "modified content\n" > > (17 bytes), ensuring the change is detected at the earliest size > > comparison in match_stat_data(), bypassing any platform-specific edge > > cases in the more complex code paths. > > This stops the test from being flaky but it is a real bug. If the user > is modifying the files interactively then they're unlikely to be able to > update the file fast enough to be affected but if anyone is scripting > like the test does then they might be affected. Agreed completely. This fix was to make the lives of git developers easier, not its users. The fix addresses the symptom, not the cause. The difftool creates its wtindex via make_cache_entry() and the subsequent refresh/diff-files path doesn't trigger content comparison when stat data matches. Anyone scripting difftool with modifications that preserve file size could hit this silently. > > Thanks > > Phillip > > > Note: Other tests with same-size file patterns (t0010-racy-git.sh, > > t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable > > because they use normal Git index operations with proper racy git > > detection. The difftool case is unique due to its ephemeral wtindex > > created via make_cache_entry() without full stat refresh. > > > > Signed-off-by: Paul Tarjan <github@paulisageek.com> > > --- > > t7800: fix racy "difftool --dir-diff syncs worktree" test > > > > In > > https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 > > this test failed for me on an unrelated commit. I had Claude look into > > it and it thought that this could be a racy git problem. I'm skeptical > > but a) I don't know the source well enough and b) the fix is low risk so > > I thought I'd send it to you folks. Everything below is the AI generated > > explanation. > > > > The "difftool --dir-diff syncs worktree without unstaged change" test > > fails intermittently, particularly on Windows CI. > > > > The test modifies a file in difftool's temp directory via an extcmd > > script and expects the change to be synced back to the worktree. The > > sync-back detection relies on git's change detection mechanisms. > > > > The root cause is that the original file content and the replacement > > content have identical sizes: > > > > * Original: "main\ntest\na\n" = 12 bytes > > * New: "new content\n" = 12 bytes > > > > When difftool creates the temporary index (wtindex), the cache entries > > have sd_size = 0 (zero-initialized via make_cache_entry with no > > refresh). Git's ie_modified() is designed to handle this by calling > > ce_modified_check_fs() for content hashing when sd_size is 0. > > > > However, Windows has known filesystem issues that may cause this to fail > > intermittently: > > > > * UNRELIABLE_FSTAT: Windows fstat() on open files may not return the > > same information as lstat() after close (config.mak.uname:506) > > > > * NTFS timestamp issues: The racy-git documentation notes that NTFS is > > "still broken" regarding timestamp granularity between in-core and > > on-disk representations (Documentation/technical/racy-git.adoc) > > > > * Attribute caching: Windows GetFileAttributesExW may cache results > > > > Fix this by changing the replacement content to "modified content\n" (17 > > bytes), ensuring the change is detected at the earliest size comparison > > in match_stat_data(), bypassing any platform-specific edge cases in the > > more complex code paths. > > > > Note: Other tests with same-size file patterns (t0010-racy-git.sh, > > t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable > > because they use normal Git index operations with proper racy git > > detection. The difftool case is unique due to its ephemeral wtindex > > created via make_cache_entry() without full stat refresh. > > > > Signed-off-by: Paul Tarjan github@paulisageek.com Reviewed-by: Johannes > > Schindelin Johannes.Schindelin@gmx.de > > > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2149%2Fptarjan%2Fclaude%2Ffix-difftool-test-DDxDC-v2 > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2149/ptarjan/claude/fix-difftool-test-DDxDC-v2 > > Pull-Request: https://github.com/git/git/pull/2149 > > > > Range-diff vs v1: > > > > 1: dd5b774451 = 1: 98bc88f336 t7800: fix racy "difftool --dir-diff syncs worktree" test > > > > > > t/t7800-difftool.sh | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > > index bf0f67378d..8a91ff3603 100755 > > --- a/t/t7800-difftool.sh > > +++ b/t/t7800-difftool.sh > > @@ -647,21 +647,21 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch > > ' > > > > write_script modify-right-file <<\EOF > > -echo "new content" >"$2/file" > > +echo "modified content" >"$2/file" > > EOF > > > > run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' ' > > test_when_finished git reset --hard && > > echo "orig content" >file && > > git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && > > - echo "new content" >expect && > > + echo "modified content" >expect && > > test_cmp expect file > > ' > > > > run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' ' > > test_when_finished git reset --hard && > > git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && > > - echo "new content" >expect && > > + echo "modified content" >expect && > > test_cmp expect file > > ' > > > > > > base-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] t7800: fix racy "difftool --dir-diff syncs worktree" test 2026-01-03 16:30 ` Paul Tarjan @ 2026-01-03 20:29 ` Johannes Schindelin 2026-01-04 2:27 ` Junio C Hamano 0 siblings, 1 reply; 11+ messages in thread From: Johannes Schindelin @ 2026-01-03 20:29 UTC (permalink / raw) To: Paul Tarjan; +Cc: phillip.wood, Paul Tarjan via GitGitGadget, git, Paul Tarjan Hi Paul, On Sat, 3 Jan 2026, Paul Tarjan wrote: > I've updated the commit and PR summary for your comments. Should I > re-run /submit to send a no-op patch or leave it as is until code > changes are needed? I believe that the change you intended for v2 (adding the "Reviewed-by" trailer) accidentally made it to the cover letter only, not to the commit message where it wants to live. Also, I would like to suggest to replace the non-URL "git-for-windows/git#5132" with the actual URL: https://github.com/git-for-windows/git/issues/5132. Remember: Commit messages are not usually read on GitHub (and some very vocal Git contributors actually refuse to use GitHub for their contributions). Ciao, Johannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] t7800: fix racy "difftool --dir-diff syncs worktree" test 2026-01-03 20:29 ` Johannes Schindelin @ 2026-01-04 2:27 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2026-01-04 2:27 UTC (permalink / raw) To: Johannes Schindelin Cc: Paul Tarjan, phillip.wood, Paul Tarjan via GitGitGadget, git, Paul Tarjan Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Paul, > > On Sat, 3 Jan 2026, Paul Tarjan wrote: > >> I've updated the commit and PR summary for your comments. Should I >> re-run /submit to send a no-op patch or leave it as is until code >> changes are needed? > > I believe that the change you intended for v2 (adding the "Reviewed-by" > trailer) accidentally made it to the cover letter only, not to the commit > message where it wants to live. > > Also, I would like to suggest to replace the non-URL > "git-for-windows/git#5132" with the actual URL: > https://github.com/git-for-windows/git/issues/5132. Remember: Commit > messages are not usually read on GitHub (and some very vocal Git > contributors actually refuse to use GitHub for their contributions). Both good suggestions. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] t7800: fix racy "difftool --dir-diff syncs worktree" test 2026-01-01 18:27 ` [PATCH v2] " Paul Tarjan via GitGitGadget 2026-01-01 22:49 ` Junio C Hamano 2026-01-03 9:39 ` Phillip Wood @ 2026-01-03 20:40 ` Paul Tarjan via GitGitGadget 2026-01-04 2:34 ` Junio C Hamano 2026-01-05 10:33 ` Phillip Wood 2 siblings, 2 replies; 11+ messages in thread From: Paul Tarjan via GitGitGadget @ 2026-01-03 20:40 UTC (permalink / raw) To: git; +Cc: Phillip Wood, Paul Tarjan, Paul Tarjan, Paul Tarjan From: Paul Tarjan <github@paulisageek.com> The "difftool --dir-diff syncs worktree without unstaged change" test fails intermittently on Windows CI, as seen at: https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 The root cause is that the original file content and the replacement content have identical sizes: - Original: "main\ntest\na\n" = 12 bytes - New: "new content\n" = 12 bytes When difftool's sync-back mechanism checks for changes, it compares stat data between the temporary index and the modified files. If the modification happens within the same timestamp granularity window and file size stays the same, the change goes undetected. On Windows, this is more likely to manifest because Git relies on inode changes as a fallback when other stat fields match, but Windows filesystems lack inodes. This is a real bug that could affect users scripting difftool similarly, as seen at: https://github.com/git-for-windows/git/issues/5132 Fix the test by changing the replacement content to "modified content" (17 bytes), ensuring the size difference is detected regardless of timestamp resolution or platform-specific stat behavior. Note: This fixes the test flakiness but not the underlying issue in difftool's change detection. Other tests with same-size file patterns (t0010-racy-git.sh, t2200-add-update.sh) are not affected because they use normal index operations with proper racy-git detection. Signed-off-by: Paul Tarjan <github@paulisageek.com> Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> --- t7800: fix racy "difftool --dir-diff syncs worktree" test The "difftool --dir-diff syncs worktree without unstaged change" test fails intermittently on Windows CI, as seen at: https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 The root cause is that the original file content and the replacement content have identical sizes: * Original: "main\ntest\na\n" = 12 bytes * New: "new content\n" = 12 bytes When difftool's sync-back mechanism checks for changes, it compares stat data between the temporary index and the modified files. If the modification happens within the same timestamp granularity window and file size stays the same, the change goes undetected. On Windows, this is more likely to manifest because Git relies on inode changes as a fallback when other stat fields match, but Windows filesystems lack inodes. This is a real bug that could affect users scripting difftool similarly (see https://github.com/git-for-windows/git/issues/5132 for a related real-world report). Fix the test by changing the replacement content to "modified content" (17 bytes), ensuring the size difference is detected regardless of timestamp resolution or platform-specific stat behavior. Note: This fixes the test flakiness but not the underlying issue in difftool's change detection. Other tests with same-size file patterns (t0010-racy-git.sh, t2200-add-update.sh) are not affected because they use normal index operations with proper racy-git detection. Changes since v2 * Added Reviewed-by to the commit message * Updated URL to be a full link to github * Reduced speculation from commit message Changes since v1 * Added Reviewed-by Signed-off-by: Paul Tarjan github@paulisageek.com Reviewed-by: Johannes Schindelin Johannes.Schindelin@gmx.de Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2149%2Fptarjan%2Fclaude%2Ffix-difftool-test-DDxDC-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2149/ptarjan/claude/fix-difftool-test-DDxDC-v3 Pull-Request: https://github.com/git/git/pull/2149 Range-diff vs v2: 1: 98bc88f336 ! 1: 3e43dcc7fd t7800: fix racy "difftool --dir-diff syncs worktree" test @@ Commit message t7800: fix racy "difftool --dir-diff syncs worktree" test The "difftool --dir-diff syncs worktree without unstaged change" test - fails intermittently, particularly on Windows CI. + fails intermittently on Windows CI, as seen at: - The test modifies a file in difftool's temp directory via an extcmd - script and expects the change to be synced back to the worktree. The - sync-back detection relies on git's change detection mechanisms. + https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 The root cause is that the original file content and the replacement content have identical sizes: @@ Commit message - Original: "main\ntest\na\n" = 12 bytes - New: "new content\n" = 12 bytes - When difftool creates the temporary index (wtindex), the cache entries - have sd_size = 0 (zero-initialized via make_cache_entry with no - refresh). Git's ie_modified() is designed to handle this by calling - ce_modified_check_fs() for content hashing when sd_size is 0. + When difftool's sync-back mechanism checks for changes, it compares + stat data between the temporary index and the modified files. If the + modification happens within the same timestamp granularity window and + file size stays the same, the change goes undetected. - However, Windows has known filesystem issues that may cause this to - fail intermittently: + On Windows, this is more likely to manifest because Git relies on + inode changes as a fallback when other stat fields match, but Windows + filesystems lack inodes. This is a real bug that could affect users + scripting difftool similarly, as seen at: - - UNRELIABLE_FSTAT: Windows fstat() on open files may not return the - same information as lstat() after close (config.mak.uname:506) + https://github.com/git-for-windows/git/issues/5132 - - NTFS timestamp issues: The racy-git documentation notes that NTFS - is "still broken" regarding timestamp granularity between in-core - and on-disk representations (Documentation/technical/racy-git.adoc) + Fix the test by changing the replacement content to "modified content" + (17 bytes), ensuring the size difference is detected regardless of + timestamp resolution or platform-specific stat behavior. - - Attribute caching: Windows GetFileAttributesExW may cache results - - Fix this by changing the replacement content to "modified content\n" - (17 bytes), ensuring the change is detected at the earliest size - comparison in match_stat_data(), bypassing any platform-specific edge - cases in the more complex code paths. - - Note: Other tests with same-size file patterns (t0010-racy-git.sh, - t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable - because they use normal Git index operations with proper racy git - detection. The difftool case is unique due to its ephemeral wtindex - created via make_cache_entry() without full stat refresh. + Note: This fixes the test flakiness but not the underlying issue in + difftool's change detection. Other tests with same-size file patterns + (t0010-racy-git.sh, t2200-add-update.sh) are not affected because they + use normal index operations with proper racy-git detection. Signed-off-by: Paul Tarjan <github@paulisageek.com> + Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> ## t/t7800-difftool.sh ## @@ t/t7800-difftool.sh: test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch t/t7800-difftool.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index bf0f67378d..8a91ff3603 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -647,21 +647,21 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch ' write_script modify-right-file <<\EOF -echo "new content" >"$2/file" +echo "modified content" >"$2/file" EOF run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' ' test_when_finished git reset --hard && echo "orig content" >file && git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && - echo "new content" >expect && + echo "modified content" >expect && test_cmp expect file ' run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' ' test_when_finished git reset --hard && git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && - echo "new content" >expect && + echo "modified content" >expect && test_cmp expect file ' base-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe -- gitgitgadget ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] t7800: fix racy "difftool --dir-diff syncs worktree" test 2026-01-03 20:40 ` [PATCH v3] " Paul Tarjan via GitGitGadget @ 2026-01-04 2:34 ` Junio C Hamano 2026-01-05 10:33 ` Phillip Wood 1 sibling, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2026-01-04 2:34 UTC (permalink / raw) To: Paul Tarjan via GitGitGadget; +Cc: git, Phillip Wood, Paul Tarjan, Paul Tarjan "Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes: > On Windows, this is more likely to manifest because Git relies on > inode changes as a fallback when other stat fields match, but Windows > filesystems lack inodes. "inode" -> "inode number" (or "ino" / "inum")? I suspect the same issue would bite users on network file systems, so while avoiding problematic scenarios in tests may improve the pass rate of the tests, we may want to fix the underlying issue with the difftool command for real in the longer term. Regardless, will queue this updated patch. Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] t7800: fix racy "difftool --dir-diff syncs worktree" test 2026-01-03 20:40 ` [PATCH v3] " Paul Tarjan via GitGitGadget 2026-01-04 2:34 ` Junio C Hamano @ 2026-01-05 10:33 ` Phillip Wood 1 sibling, 0 replies; 11+ messages in thread From: Phillip Wood @ 2026-01-05 10:33 UTC (permalink / raw) To: Paul Tarjan via GitGitGadget, git; +Cc: Paul Tarjan, Paul Tarjan Hi Paul Thanks for rewording the commit message. This looks good - it will be nice to have one less flaky test to worry about when the CI fails Phillip On 03/01/2026 20:40, Paul Tarjan via GitGitGadget wrote: > From: Paul Tarjan <github@paulisageek.com> > > The "difftool --dir-diff syncs worktree without unstaged change" test > fails intermittently on Windows CI, as seen at: > > https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 > > The root cause is that the original file content and the replacement > content have identical sizes: > > - Original: "main\ntest\na\n" = 12 bytes > - New: "new content\n" = 12 bytes > > When difftool's sync-back mechanism checks for changes, it compares > stat data between the temporary index and the modified files. If the > modification happens within the same timestamp granularity window and > file size stays the same, the change goes undetected. > > On Windows, this is more likely to manifest because Git relies on > inode changes as a fallback when other stat fields match, but Windows > filesystems lack inodes. This is a real bug that could affect users > scripting difftool similarly, as seen at: > > https://github.com/git-for-windows/git/issues/5132 > > Fix the test by changing the replacement content to "modified content" > (17 bytes), ensuring the size difference is detected regardless of > timestamp resolution or platform-specific stat behavior. > > Note: This fixes the test flakiness but not the underlying issue in > difftool's change detection. Other tests with same-size file patterns > (t0010-racy-git.sh, t2200-add-update.sh) are not affected because they > use normal index operations with proper racy-git detection. > > Signed-off-by: Paul Tarjan <github@paulisageek.com> > Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > --- > t7800: fix racy "difftool --dir-diff syncs worktree" test > > The "difftool --dir-diff syncs worktree without unstaged change" test > fails intermittently on Windows CI, as seen at: > > https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 > > The root cause is that the original file content and the replacement > content have identical sizes: > > * Original: "main\ntest\na\n" = 12 bytes > * New: "new content\n" = 12 bytes > > When difftool's sync-back mechanism checks for changes, it compares stat > data between the temporary index and the modified files. If the > modification happens within the same timestamp granularity window and > file size stays the same, the change goes undetected. > > On Windows, this is more likely to manifest because Git relies on inode > changes as a fallback when other stat fields match, but Windows > filesystems lack inodes. This is a real bug that could affect users > scripting difftool similarly (see > https://github.com/git-for-windows/git/issues/5132 for a related > real-world report). > > Fix the test by changing the replacement content to "modified content" > (17 bytes), ensuring the size difference is detected regardless of > timestamp resolution or platform-specific stat behavior. > > Note: This fixes the test flakiness but not the underlying issue in > difftool's change detection. Other tests with same-size file patterns > (t0010-racy-git.sh, t2200-add-update.sh) are not affected because they > use normal index operations with proper racy-git detection. > > Changes since v2 > > * Added Reviewed-by to the commit message > * Updated URL to be a full link to github > * Reduced speculation from commit message > > Changes since v1 > > * Added Reviewed-by > > Signed-off-by: Paul Tarjan github@paulisageek.com Reviewed-by: Johannes > Schindelin Johannes.Schindelin@gmx.de > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2149%2Fptarjan%2Fclaude%2Ffix-difftool-test-DDxDC-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2149/ptarjan/claude/fix-difftool-test-DDxDC-v3 > Pull-Request: https://github.com/git/git/pull/2149 > > Range-diff vs v2: > > 1: 98bc88f336 ! 1: 3e43dcc7fd t7800: fix racy "difftool --dir-diff syncs worktree" test > @@ Commit message > t7800: fix racy "difftool --dir-diff syncs worktree" test > > The "difftool --dir-diff syncs worktree without unstaged change" test > - fails intermittently, particularly on Windows CI. > + fails intermittently on Windows CI, as seen at: > > - The test modifies a file in difftool's temp directory via an extcmd > - script and expects the change to be synced back to the worktree. The > - sync-back detection relies on git's change detection mechanisms. > + https://github.com/git/git/actions/runs/20624095002/job/59231745784#step:5:416 > > The root cause is that the original file content and the replacement > content have identical sizes: > @@ Commit message > - Original: "main\ntest\na\n" = 12 bytes > - New: "new content\n" = 12 bytes > > - When difftool creates the temporary index (wtindex), the cache entries > - have sd_size = 0 (zero-initialized via make_cache_entry with no > - refresh). Git's ie_modified() is designed to handle this by calling > - ce_modified_check_fs() for content hashing when sd_size is 0. > + When difftool's sync-back mechanism checks for changes, it compares > + stat data between the temporary index and the modified files. If the > + modification happens within the same timestamp granularity window and > + file size stays the same, the change goes undetected. > > - However, Windows has known filesystem issues that may cause this to > - fail intermittently: > + On Windows, this is more likely to manifest because Git relies on > + inode changes as a fallback when other stat fields match, but Windows > + filesystems lack inodes. This is a real bug that could affect users > + scripting difftool similarly, as seen at: > > - - UNRELIABLE_FSTAT: Windows fstat() on open files may not return the > - same information as lstat() after close (config.mak.uname:506) > + https://github.com/git-for-windows/git/issues/5132 > > - - NTFS timestamp issues: The racy-git documentation notes that NTFS > - is "still broken" regarding timestamp granularity between in-core > - and on-disk representations (Documentation/technical/racy-git.adoc) > + Fix the test by changing the replacement content to "modified content" > + (17 bytes), ensuring the size difference is detected regardless of > + timestamp resolution or platform-specific stat behavior. > > - - Attribute caching: Windows GetFileAttributesExW may cache results > - > - Fix this by changing the replacement content to "modified content\n" > - (17 bytes), ensuring the change is detected at the earliest size > - comparison in match_stat_data(), bypassing any platform-specific edge > - cases in the more complex code paths. > - > - Note: Other tests with same-size file patterns (t0010-racy-git.sh, > - t2200-add-update.sh, t1701-racy-split-index.sh) are not vulnerable > - because they use normal Git index operations with proper racy git > - detection. The difftool case is unique due to its ephemeral wtindex > - created via make_cache_entry() without full stat refresh. > + Note: This fixes the test flakiness but not the underlying issue in > + difftool's change detection. Other tests with same-size file patterns > + (t0010-racy-git.sh, t2200-add-update.sh) are not affected because they > + use normal index operations with proper racy-git detection. > > Signed-off-by: Paul Tarjan <github@paulisageek.com> > + Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > ## t/t7800-difftool.sh ## > @@ t/t7800-difftool.sh: test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch > > > t/t7800-difftool.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index bf0f67378d..8a91ff3603 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -647,21 +647,21 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch > ' > > write_script modify-right-file <<\EOF > -echo "new content" >"$2/file" > +echo "modified content" >"$2/file" > EOF > > run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' ' > test_when_finished git reset --hard && > echo "orig content" >file && > git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && > - echo "new content" >expect && > + echo "modified content" >expect && > test_cmp expect file > ' > > run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' ' > test_when_finished git reset --hard && > git difftool -d $symlinks --extcmd "$PWD/modify-right-file" branch && > - echo "new content" >expect && > + echo "modified content" >expect && > test_cmp expect file > ' > > > base-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-01-05 10:33 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-12-31 22:19 [PATCH] t7800: fix racy "difftool --dir-diff syncs worktree" test Paul Tarjan via GitGitGadget 2026-01-01 14:49 ` Johannes Schindelin 2026-01-01 18:27 ` [PATCH v2] " Paul Tarjan via GitGitGadget 2026-01-01 22:49 ` Junio C Hamano 2026-01-03 9:39 ` Phillip Wood 2026-01-03 16:30 ` Paul Tarjan 2026-01-03 20:29 ` Johannes Schindelin 2026-01-04 2:27 ` Junio C Hamano 2026-01-03 20:40 ` [PATCH v3] " Paul Tarjan via GitGitGadget 2026-01-04 2:34 ` Junio C Hamano 2026-01-05 10:33 ` Phillip Wood
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).