Reduce explicit sleep calls in t7063 untracked cache tests
cover Reduce explicit sleep calls in t7063 untracked cache tests
As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a second, in order to avoid the untracked cache content being invalidated by an mtime race condition.
Even though it's only 9 seconds of sleeping that can be straightforwardly replaced, it seems worth fixing if possible.
Replace sleep calls with backdating of filesystem changes, but first fix the test-tool chmtime functionality to work for directories in Windows.
I do have a question to the list here: Do mingw.c changes need to be upstreamed somewhere? I don't understand the exact relationship between this file and the MinGW project.
Tao Klerks (2):
t/helper/test-chmtime: update mingw to support chmtime on directories
t7063: mtime-mangling instead of delays in untracked cache testing
compat/mingw.c | 33 ++++++++++++++++++++++++++----- t/t7063-status-untracked-cache.sh | 29 ++++++++++++++++----------- 2 files changed, 45 insertions(+), 17 deletions(-)
base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1166
Signature · 1 line
gitgitgadget
PATCH 1/2 t/helper/test-chmtime: update mingw to support chmtime on directories
From: Tao Klerks <[email protected]>
The mingw_utime implementation in mingw.c does not support directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped by Jeff Hostetler, in "t/helper/test-chmtime: skip directories on Windows" in the "Builtin FSMonitor Part 2" work.
It would make sense to backdate file and folder changes in untracked cache tests, to avoid needing to insert explicit delays/pauses in the tests.
Add support for directory date manipulation in mingw_utime by calling CreateFileW() explicitly to create the directory handle, and CloseHandle() to close it.
--- compat/mingw.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-)
compat/mingw.c +28 -5
@@ -964,6 +964,8 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)int fh, rc;DWORD attrs;wchar_t wfilename[MAX_PATH ]; +HANDLE osfilehandle; +if (xutftowcs_path (wfilename, file_name) <0 )return -1 ;
@@ -975,9 +977,26 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)SetFileAttributesW (wfilename, attrs & ~FILE_ATTRIBUTE_READONLY ); } -if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { - rc =-1 ; - goto revert_attrs; +if (attrs &FILE_ATTRIBUTE_DIRECTORY ) { + fh =0 ; + osfilehandle =CreateFileW (wfilename, +0x100 /*FILE_WRITE_ATTRIBUTES*/ , +0 /*FileShare.None*/ , +NULL , +OPEN_EXISTING , +FILE_FLAG_BACKUP_SEMANTICS , +NULL ); +if (osfilehandle ==INVALID_HANDLE_VALUE ) { + errno =err_win_to_posix (GetLastError ()); + rc =-1 ; + goto revert_attrs; + } + }else { +if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { + rc =-1 ; + goto revert_attrs; + } + osfilehandle = (HANDLE )_get_osfhandle (fh); }if (times) {
@@ -987,12 +1006,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)GetSystemTimeAsFileTime (&mft); aft = mft; } -if (!SetFileTime ((HANDLE )_get_osfhandle (fh),NULL , &aft, &mft)) { +if (!SetFileTime (osfilehandle,NULL , &aft, &mft)) { errno =EINVAL ; rc =-1 ; }else rc =0 ; -close (fh); + +if (fh) +close (fh); +else if (osfilehandle) +CloseHandle (osfilehandle); revert_attrs: if (attrs != INVALID_FILE_ATTRIBUTES &&
Signature · 1 line
gitgitgadget
PATCH 2/2 t7063: mtime-mangling instead of delays in untracked cache testing
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
The untracked cache test uses an avoid_racy function to deal with an mtime-resolution challenge in testing: If an untracked cache entry's mtime falls in the same second as the mtime of the index the untracked cache was stored in, then it cannot be trusted.
Explicitly delaying tests is a simple effective strategy to avoid these issues, but should be avoided where possible.
Switch from a delay-based strategy to instead backdating all file changes using test-tool chmtime, where that is an option, to shave 9 seconds off the test run time.
Don't update test cases that delay for other reasons, for now at least (4 seconds).
--- t/t7063-status-untracked-cache.sh | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
t/t7063-status-untracked-cache.sh +17 -12
@@ -24,6 +24,14 @@ sync_mtime () { find . -type d -exec ls -ld {} + >/dev/null } +chmmtime_worktree_root () { + # chmtime doesnt handle relative paths on windows, so need + # to "hardcode" a reference to the worktree folder name. + cd .. && + test-tool chmtime $1 worktree && + cd worktree +} + avoid_racy() { sleep 1 }
@@ -90,6 +98,9 @@ test_expect_success 'setup' ' cd worktree && mkdir done dtwo dthree && touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && + chmmtime_worktree_root =-300 && git add one two done/one && : >.git/info/exclude && git update-index --untracked-cache &&
@@ -142,7 +153,6 @@ two EOF test_expect_success 'status first time (empty cache)' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -166,7 +176,6 @@ test_expect_success 'untracked cache after first status' ' ' test_expect_success 'status second time (fully populated cache)' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -190,8 +199,8 @@ test_expect_success 'untracked cache after second status' ' ' test_expect_success 'modify in root directory, one dir invalidation' ' - avoid_racy && : >four && + test-tool chmtime =-240 four && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -241,7 +250,6 @@ EOF ' test_expect_success 'new .gitignore invalidates recursively' ' - avoid_racy && echo four >.gitignore && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \
@@ -292,7 +300,6 @@ EOF ' test_expect_success 'new info/exclude invalidates everything' ' - avoid_racy && echo three >>.git/info/exclude && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \
@@ -520,14 +527,14 @@ test_expect_success 'create/modify files, some of which are gitignored' ' echo three >done/three && # three is gitignored echo four >done/four && # four is gitignored at a higher level echo five >done/five && # five is not gitignored - echo test >base && #we need to ensure that the root dir is touched - rm base && + test-tool chmtime =-180 done/two done/three done/four done/five done && + # we need to ensure that the root dir is touched (in the past); + chmmtime_worktree_root =-180 && sync_mtime ' test_expect_success 'test sparse status with untracked cache' ' : >../trace.output && - avoid_racy && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual && iuc status --porcelain >../status.iuc &&
@@ -570,7 +577,6 @@ EOF ' test_expect_success 'test sparse status again with untracked cache' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
@@ -597,11 +603,11 @@ EOF test_expect_success 'set up for test of subdir and sparse checkouts' ' mkdir done/sub && mkdir done/sub/sub && - echo "sub" > done/sub/sub/file + echo "sub" > done/sub/sub/file && + test-tool chmtime =-120 done/sub/sub/file done/sub/sub done/sub done ' test_expect_success 'test sparse status with untracked cache and subdir' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
@@ -651,7 +657,6 @@ EOF ' test_expect_success 'test sparse status again with untracked cache and subdir' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
Signature · 1 line
gitgitgadget
Quoted 10 lines · Tao Klerks via GitGitGadget
As noted in a recent proposed patch to
t/t7519-status-fsmonitor.sh, a number of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a second, in order to avoid the untracked cache content being invalidated by an mtime race condition.Even though it's only 9 seconds of sleeping that can be straightforwardly replaced, it seems worth fixing if possible.
Replace sleep calls with backdating of filesystem changes, but first fix the test-tool chmtime functionality to work for directories in Windows.
Thanks, it will be nice to have that test run a bit faster in wallclock terms.
Quoted 3 lines
I do have a question to the list here: Do
mingw.cchanges need to be upstreamed somewhere? I don't understand the exact relationship between this file and the MinGW project.
No, it's 100% ours. We just put (somewhat inconsistently in some cases) our own code that is only needed for "compat" with some "platforms" into compat/*.
Well, somewhat/mostly, then "contrib/" is supposed to be third-party imports, but some code in both is 100% ours, some is out-of-tree, some is effectively perma-forked and has no "upstream" anymore etc.
Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
In reply to Tao Klerks via GitGitGadgetQuoted 26 lines · Tao Klerks via GitGitGadget
From: Tao Klerks <[email protected]>
The
mingw_utimeimplementation inmingw.cdoes not support directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped by Jeff Hostetler, in "t/helper/test-chmtime: skip directories on Windows" in the "Builtin FSMonitor Part 2" work.It would make sense to backdate file and folder changes in untracked cache tests, to avoid needing to insert explicit delays/pauses in the tests.
Add support for directory date manipulation in
mingw_utimeby callingCreateFileW()explicitly to create the directory handle, andCloseHandle()to close it.Signed-off-by Tao Klerks <[email protected]>--- compat/mingw.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-)compat/mingw.c +0
-0
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 967
Quoted 4 lines
compat/mingw.c +1
-0@@ -964,6 +964,8 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)int fh, rc;DWORD attrs;wchar_t wfilename[MAX_PATH ]; +HANDLE osfilehandle;
I'd be more comfortable initializing this variable to INVALID_HANDLE_VALUE.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 981
Quoted 13 lines
compat/mingw.c +3
-3+if (xutftowcs_path (wfilename, file_name) <0 )return -1 ;@@ -975,9 +977,26 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)SetFileAttributesW (wfilename, attrs & ~FILE_ATTRIBUTE_READONLY ); } -if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { - rc =-1 ; - goto revert_attrs; +if (attrs &FILE_ATTRIBUTE_DIRECTORY ) { + fh =0 ;
and here initializing fh = -1.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 984
Quoted 3 lines
compat/mingw.c +3
-0+ osfilehandle = CreateFileW(wfilename, + 0x100 /*FILE_WRITE_ATTRIBUTES*/, + 0 /*FileShare.None*/,
Is there a reason that you're not using the #define's here? I assume you ran into a header file problem or something, but there are other symbols used nearby, so I'm not sure.
Quoted 24 lines
compat/mingw.c +16
-1+NULL , +OPEN_EXISTING , +FILE_FLAG_BACKUP_SEMANTICS , + NULL); +if (osfilehandle ==INVALID_HANDLE_VALUE ) { + errno =err_win_to_posix (GetLastError ()); + rc =-1 ; + goto revert_attrs; + } + }else { +if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { + rc =-1 ; + goto revert_attrs; + } + osfilehandle = (HANDLE )_get_osfhandle (fh); }if (times) {@@ -987,12 +1006,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)GetSystemTimeAsFileTime (&mft); aft = mft; } -if (!SetFileTime ((HANDLE )_get_osfhandle (fh),NULL , &aft, &mft)) { +if (!SetFileTime (osfilehandle,NULL , &aft, &mft)) {
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 1018
Quoted 10 lines
compat/mingw.c +5
-1errno =EINVAL ; rc =-1 ; }else rc =0 ; -close (fh); + +if (fh) +close (fh); +else if (osfilehandle) +CloseHandle (osfilehandle);
And then this becomes:
if (fh != -1) close(fh) else if (osfilehandle != INVALID_HANDLE_VALUE) Closehandle(osfilehandle);
Which I think makes it more clear that we'll properly close the handle.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times)
Quoted 4 lines
compat/mingw.c +0
-0revert_attrs: if (attrs != INVALID_FILE_ATTRIBUTES &&
An alternative solution would be to replace the _wopen() with a call to CreateFileW() without the backup flag for non-directories and just convert the whole routine to use HANDLE's rather fd's.
Thanks
Jeff
Quoted 24 lines · Tao Klerks via GitGitGadget
As noted in a recent proposed patch to
t/t7519-status-fsmonitor.sh, a number of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a second, in order to avoid the untracked cache content being invalidated by an mtime race condition.Even though it's only 9 seconds of sleeping that can be straightforwardly replaced, it seems worth fixing if possible.
Replace sleep calls with backdating of filesystem changes, but first fix the test-tool chmtime functionality to work for directories in Windows.
I do have a question to the list here: Do
mingw.cchanges need to be upstreamed somewhere? I don't understand the exact relationship between this file and the MinGW project.Tao Klerks (2):
t/helper/test-chmtime: update mingw to support chmtime on directories
t7063: mtime-mangling instead of delays in untracked cache testingcompat/mingw.c | 33 ++++++++++++++++++++++++++----- t/t7063-status-untracked-cache.sh | 29 ++++++++++++++++----------- 2 files changed, 45 insertions(+), 17 deletions(-)
Quoted 5 lines
base-commit:
4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1166
Thanks for working on this. It would be nice to be able to drop that commit (read: hack) from my fsmonitor series.
Jeff
Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
In reply to Tao Klerks via GitGitGadgetQuoted 7 lines · Tao Klerks via GitGitGadget
From: Tao Klerks <[email protected]>
The
mingw_utimeimplementation inmingw.cdoes not support directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped by Jeff Hostetler, in "t/helper/test-chmtime: skip directories on Windows" in the "Builtin FSMonitor Part 2" work.
I was expecting that this will be applicable _before_ FSMonitor Part 2 and later. This mention would probably belong to the comment after three-dashes?
Quoted 7 lines
It would make sense to backdate file and folder changes in untracked cache tests, to avoid needing to insert explicit delays/pauses in the tests.
Add support for directory date manipulation in
mingw_utimeby callingCreateFileW()explicitly to create the directory handle, andCloseHandle()to close it.
OK.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 967
Quoted 5 lines
compat/mingw.c +1
-0@@ -964,6 +964,8 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)int fh, rc;DWORD attrs;wchar_t wfilename[MAX_PATH ]; +HANDLE osfilehandle;
The variable is not initialized here, but that is perfectly OK. We'll set it in both branches for directories and files.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 980
Quoted 11 lines
compat/mingw.c +1
-3if (xutftowcs_path (wfilename, file_name) <0 )return -1 ;@@ -975,9 +977,26 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)SetFileAttributesW (wfilename, attrs & ~FILE_ATTRIBUTE_READONLY ); } -if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { - rc =-1 ; - goto revert_attrs; +if (attrs &FILE_ATTRIBUTE_DIRECTORY ) {
Excellent. We can reuse existing attrs without any extra calls to introduce the new codepath to deal with directories.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 981
Quoted 1 line
compat/mingw.c +1
-0+ fh =0 ;
This should be
fh = -1;
instead. More on this later.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 993
Quoted 12 lines
compat/mingw.c +12
-0+ osfilehandle =CreateFileW (wfilename, +0x100 /*FILE_WRITE_ATTRIBUTES*/ , +0 /*FileShare.None*/ , +NULL , +OPEN_EXISTING , +FILE_FLAG_BACKUP_SEMANTICS , +NULL ); +if (osfilehandle ==INVALID_HANDLE_VALUE ) { + errno =err_win_to_posix (GetLastError ()); + rc =-1 ; + goto revert_attrs; + }
Upon an error, we'll jump to revert_attrs but otherwise, we will have a valid osfilehandle (which presumably is not NULL).
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 999
Quoted 6 lines
compat/mingw.c +6
-0+ }else { +if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { + rc =-1 ; + goto revert_attrs; + } + osfilehandle = (HANDLE )_get_osfhandle (fh);
This side is the same as before. This code assumes that, as long as _wopen() gives us a usable fh, _get_osfhandle(fh) would always give us a valid handle. This assumption should be safe, as the original code has been relying on it anyway.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 1009
Quoted 6 lines
compat/mingw.c +1
-1@@ -987,12 +1006,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)GetSystemTimeAsFileTime (&mft); aft = mft; } -if (!SetFileTime ((HANDLE )_get_osfhandle (fh),NULL , &aft, &mft)) { +if (!SetFileTime (osfilehandle,NULL , &aft, &mft)) {
And we use the osfilehandle we obtained, either from the code that is identical to the original for files or from the new code for directories. Good.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 1018
Quoted 10 lines
compat/mingw.c +5
-1errno =EINVAL ; rc =-1 ; }else rc =0 ; -close (fh); + +if (fh) +close (fh); +else if (osfilehandle) +CloseHandle (osfilehandle);
In the context of "git" process, I do not think we would ever close FD#0, so it may be safe to assume that _wopen() above will never yield FD#0, so this is not quite wrong per-se, but to be future-proof, it would be even safer to instead do:
if (0 <= fh) close(fh); else if (osfilehandle) CloseHandle(osfilehandle);
here. That is consistent with the error checking done where _wopen() was called to obtain it above, i.e.
if ((fh = _wopen(...)) < 0) ... error ...
Quoted 2 lines
compat/mingw.c +0
-0revert_attrs: if (attrs != INVALID_FILE_ATTRIBUTES &&
Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
In reply to Jeff HostetlerQuoted 4 lines · Jeff Hostetler
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 967
Quoted 1 line
compat/mingw.c +1
-0+HANDLE osfilehandle;I'd be more comfortable initializing this variable to
INVALID_HANDLE_VALUE.
Both directories/files branches assign, so it is not needed.
Quoted 4 lines
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 981
Quoted 2 lines
compat/mingw.c +2
-0+if (attrs &FILE_ATTRIBUTE_DIRECTORY ) { + fh =0 ;and here initializing fh = -1.
This does make it safer.
Quoted 6 lines
And then this becomes:
if (fh != -1) close(fh) else if (osfilehandle != INVALID_HANDLE_VALUE) Closehandle(osfilehandle);
Exactly.
Re: [PATCH 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
In reply to Tao Klerks via GitGitGadgetQuoted 7 lines · Tao Klerks via GitGitGadget
From: Tao Klerks <[email protected]>
The untracked cache test uses an
avoid_racyfunction to deal with an mtime-resolution challenge in testing: If an untracked cache entry's mtime falls in the same second as the mtime of the index the untracked cache was stored in, then it cannot be trusted.
Quoted 6 lines
Explicitly delaying tests is a simple effective strategy to avoid these issues, but should be avoided where possible.
Switch from a delay-based strategy to instead backdating all file changes using test-tool chmtime, where that is an option, to shave 9 seconds off the test run time.
OK.
Quoted 2 lines
Don't update test cases that delay for other reasons, for now at least (4 seconds).
Sad but one step at a time.
Quoted 1 line
+chmmtime_worktree_root () {
Not "chmtime_worktree_root"? Is doubled-m intended?
t/t7063-status-untracked-cache.sh sync_mtime () { · line 32
Quoted 5 lines
t/t7063-status-untracked-cache.sh +5
-0+ # chmtime doesnt handle relative paths on windows, so need + # to "hardcode" a reference to the worktree folder name. + cd .. && + test-tool chmtime $1 worktree && + cd worktree
An unsuspecting caller will be left in a directory it didn't expect to when "test-tool chmtime" fails, which is not nice.
( cd .. && test-tool chmtime "$1" worktree )
at least until the tool learns to do
test-tool chmtime "$1" ../worktree
or
test-tool -C.. chmtime "$1" worktree
Oh, isn't the last one already available?
t/t7063-status-untracked-cache.sh test_expect_success 'setup' ' · line 102
Quoted 11 lines
t/t7063-status-untracked-cache.sh +4
-0+} + avoid_racy() { sleep 1 }@@ -90,6 +98,9 @@ test_expect_success 'setup' ' cd worktree && mkdir done dtwo dthree && touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree &&
Not a huge deal, but it would have been very nice if "test-tool chmtime" had a "-R" option to recurse into directories.
Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
In reply to Jeff HostetlerQuoted 10 lines · Jeff Hostetler
Quoted 1 line · Tao Klerks via GitGitGadget
+ HANDLE osfilehandle;
I'd be more comfortable initializing this variable to
INVALID_HANDLE_VALUE.
Makes sense, thanks. (but less relevant after switch to CreateFileW)
Quoted 3 lines
Quoted 1 line
+ fh = 0;
and here initializing fh = -1.
Makes sense, thanks. (but overshadowed by switch to CreateFileW)
Quoted 7 lines
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 984
Quoted 3 lines
compat/mingw.c +3
-0+ osfilehandle = CreateFileW(wfilename, + 0x100 /*FILE_WRITE_ATTRIBUTES*/, + 0 /*FileShare.None*/,Is there a reason that you're not using the #define's here? I assume you ran into a header file problem or something, but there are other symbols used nearby, so I'm not sure.
I couldn't find these, and am a complete C APIs n00b - I have no idea whether or how to add them. I figured commenting on their meaning is the simplest safest thing to do locally?
Quoted 5 lines
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 1018
Quoted 4 lines
compat/mingw.c +4
-0+if (fh) +close (fh); +else if (osfilehandle) +CloseHandle (osfilehandle);
Quoted 8 lines
And then this becomes:
if (fh != -1) close(fh) else if (osfilehandle != INVALID_HANDLE_VALUE) Closehandle(osfilehandle);Which I think makes it more clear that we'll properly close the handle.
Perfect, thx.
Quoted 10 lines
Quoted 5 lines
revert_attrs:if (attrs != INVALID_FILE_ATTRIBUTES &&An alternative solution would be to replace the
_wopen()with a call toCreateFileW()without the backup flag for non-directories and just convert the whole routine to use HANDLE's rather fd's.
I was at first scared of making changes to things I don't fully understand, but looking at the existing use of GetFileAttributesW I don't think that makes much sense. This is cleaner/simpler even if it is a bigger change, and consistent with other things.
Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
In reply to Junio C HamanoQuoted 9 lines · Junio C Hamano
Quoted 5 lines · Tao Klerks via GitGitGadget
The
mingw_utimeimplementation inmingw.cdoes not support directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped by Jeff Hostetler, in "t/helper/test-chmtime: skip directories on Windows" in the "Builtin FSMonitor Part 2" work.
Quoted 4 lines
I was expecting that this will be applicable _before_ FSMonitor Part 2 and later. This mention would probably belong to the comment after three-dashes?
I've updated the text slightly in the next re-roll, but I didn't understand the bit about dashes... What is "the comment after three dashes"?
Quoted 8 lines
Quoted 1 line
+ fh = 0;
This should be
fh = -1;instead. More on this later.
Makes sense, but obviated by full switch to CreateFileW().
Quoted 15 lines
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 1018
Quoted 4 lines
compat/mingw.c +4
-0+if (fh) +close (fh); +else if (osfilehandle) +CloseHandle (osfilehandle);In the context of "git" process, I do not think we would ever close FD#0, so it may be safe to assume that
_wopen()above will never yield FD#0, so this is not quite wrong per-se, but to be future-proof, it would be even safer to instead do:if (0 <= fh) close(fh); else if (osfilehandle) CloseHandle(osfilehandle);
Quoted 6 lines
here. That is consistent with the error checking done where
_wopen()was called to obtain it above, i.e.if ((fh = _wopen(...)) < 0) ... error ...
Makes sense, but obviated by full switch to CreateFileW().
Re: [PATCH 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
In reply to Junio C HamanoQuoted 7 lines · Junio C Hamano
Quoted 1 line · Tao Klerks via GitGitGadget
+chmmtime_worktree_root () {
Not "
chmtime_worktree_root"? Is doubled-m intended?
Oops, typo, fixing.
Quoted 16 lines
t/t7063-status-untracked-cache.sh sync_mtime () { · line 32
Quoted 5 lines
t/t7063-status-untracked-cache.sh +5
-0+ # chmtime doesnt handle relative paths on windows, so need + # to "hardcode" a reference to the worktree folder name. + cd .. && + test-tool chmtime $1 worktree && + cd worktreeAn unsuspecting caller will be left in a directory it didn't expect to when "test-tool chmtime" fails, which is not nice.
( cd .. && test-tool chmtime "$1" worktree )at least until the tool learns to do
Quoted 7 lines
test-tool chmtime "$1" ../worktree
or
test-tool -C.. chmtime "$1" worktree
Oh, isn't the last one already available?
Ah, lovely, thank you!
v2 cover Reduce explicit sleep calls in t7063 untracked cache tests
In reply to Tao Klerks via GitGitGadgetAs noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a second, in order to avoid the untracked cache content being invalidated by an mtime race condition.
Even though it's only 9 seconds of sleeping that can be straightforwardly replaced, it seems worth fixing if possible.
Replace sleep calls with backdating of filesystem changes, but first fix the test-tool chmtime functionality to work for directories in Windows.
I do have a question to the list here: Do mingw.c changes need to be upstreamed somewhere? I don't understand the exact relationship between this file and the MinGW project.
Tao Klerks (2):
t/helper/test-chmtime: update mingw to support chmtime on directories
t7063: mtime-mangling instead of delays in untracked cache testing
compat/mingw.c | 21 +++++++++++++++++---- t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------ 2 files changed, 32 insertions(+), 16 deletions(-)
base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1166
Range-diff vs v1:
1: 76b6216281e ! 1: 7cdef0ad5fb t/helper/test-chmtime: update mingw to support chmtime on directories
@@ Commit message
The mingw_utime implementation in mingw.c does not support
directories. This means that "test-tool chmtime" fails on Windows when
targeting directories. This has previously been noted and sidestepped
- by Jeff Hostetler, in "t/helper/test-chmtime: skip directories
- on Windows" in the "Builtin FSMonitor Part 2" work.
+ temporarily by Jeff Hostetler, in "t/helper/test-chmtime: skip
+ directories on Windows" in the "Builtin FSMonitor Part 2" work, but
+ not yet fixed.
It would make sense to backdate file and folder changes in untracked
cache tests, to avoid needing to insert explicit delays/pauses in the
tests.
- Add support for directory date manipulation in mingw_utime by calling
- CreateFileW() explicitly to create the directory handle, and
- CloseHandle() to close it.
+ Add support for directory date manipulation in mingw_utime by
+ replacing the file-oriented _wopen() call with the
+ directory-supporting CreateFileW() windows API explicitly.
Signed-off-by: Tao Klerks <[email protected]>
## compat/mingw.c ##
-@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times)
- int fh, rc;
+@@ compat/mingw.c: static inline void time_t_to_filetime(time_t t, FILETIME *ft)
+ int mingw_utime (const char *file_name, const struct utimbuf *times)
+ {
+ FILETIME mft, aft;
+- int fh, rc;
++ int rc;
DWORD attrs;
wchar_t wfilename[MAX_PATH];
+ HANDLE osfilehandle;
@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti
}
- if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
-- rc = -1;
-- goto revert_attrs;
-+ if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
-+ fh = 0;
-+ osfilehandle = CreateFileW(wfilename,
-+ 0x100 /*FILE_WRITE_ATTRIBUTES*/,
-+ 0 /*FileShare.None*/,
-+ NULL,
-+ OPEN_EXISTING,
-+ FILE_FLAG_BACKUP_SEMANTICS,
-+ NULL);
-+ if (osfilehandle == INVALID_HANDLE_VALUE) {
-+ errno = err_win_to_posix(GetLastError());
-+ rc = -1;
-+ goto revert_attrs;
-+ }
-+ } else {
-+ if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
-+ rc = -1;
-+ goto revert_attrs;
-+ }
-+ osfilehandle = (HANDLE)_get_osfhandle(fh);
++ osfilehandle = CreateFileW(wfilename,
++ 0x100 /*FILE_WRITE_ATTRIBUTES*/,
++ 0 /*FileShare.None*/,
++ NULL,
++ OPEN_EXISTING,
++ attrs & FILE_ATTRIBUTE_DIRECTORY ?
++ FILE_FLAG_BACKUP_SEMANTICS : 0,
++ NULL);
++ if (osfilehandle == INVALID_HANDLE_VALUE) {
++ errno = err_win_to_posix(GetLastError());
+ rc = -1;
+ goto revert_attrs;
}
-
- if (times) {
@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times)
GetSystemTimeAsFileTime(&mft);
aft = mft;
@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti
rc = 0;
- close(fh);
+
-+ if (fh)
-+ close(fh);
-+ else if (osfilehandle)
++ if (osfilehandle != INVALID_HANDLE_VALUE)
+ CloseHandle(osfilehandle);
revert_attrs:
2: a1806c56333 ! 2: 3e3c9c7faac t7063: mtime-mangling instead of delays in untracked cache testing
@@ t/t7063-status-untracked-cache.sh: sync_mtime () {
find . -type d -exec ls -ld {} + >/dev/null
}
-+chmmtime_worktree_root () {
++chmtime_worktree_root () {
+ # chmtime doesnt handle relative paths on windows, so need
+ # to "hardcode" a reference to the worktree folder name.
-+ cd .. &&
-+ test-tool chmtime $1 worktree &&
-+ cd worktree
++ test-tool -C .. chmtime $1 worktree
+}
+
avoid_racy() {
@@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' '
touch one two three done/one dtwo/two dthree/three &&
+ test-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&
+ test-tool chmtime =-300 done dtwo dthree &&
-+ chmmtime_worktree_root =-300 &&
++ chmtime_worktree_root =-300 &&
git add one two done/one &&
: >.git/info/exclude &&
git update-index --untracked-cache &&
@@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som
- rm base &&
+ test-tool chmtime =-180 done/two done/three done/four done/five done &&
+ # we need to ensure that the root dir is touched (in the past);
-+ chmmtime_worktree_root =-180 &&
++ chmtime_worktree_root =-180 &&
sync_mtime
'Signature · 1 line
gitgitgadget
PATCH v2 1/2 t/helper/test-chmtime: update mingw to support chmtime on directories
From: Tao Klerks <[email protected]>
The mingw_utime implementation in mingw.c does not support directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped temporarily by Jeff Hostetler, in "t/helper/test-chmtime: skip directories on Windows" in the "Builtin FSMonitor Part 2" work, but not yet fixed.
It would make sense to backdate file and folder changes in untracked cache tests, to avoid needing to insert explicit delays/pauses in the tests.
Add support for directory date manipulation in mingw_utime by replacing the file-oriented _wopen() call with the directory-supporting CreateFileW() windows API explicitly.
--- compat/mingw.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
compat/mingw.c +17 -4
@@ -961,9 +961,11 @@ static inline void time_t_to_filetime(time_t t, FILETIME *ft)int mingw_utime (const char *file_name,const struct utimbuf *times) {FILETIME mft, aft; -int fh, rc; +int rc;DWORD attrs;wchar_t wfilename[MAX_PATH ]; +HANDLE osfilehandle; +if (xutftowcs_path (wfilename, file_name) <0 )return -1 ;
@@ -975,7 +977,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)SetFileAttributesW (wfilename, attrs & ~FILE_ATTRIBUTE_READONLY ); } -if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { + osfilehandle =CreateFileW (wfilename, +0x100 /*FILE_WRITE_ATTRIBUTES*/ , +0 /*FileShare.None*/ , +NULL , +OPEN_EXISTING , + attrs &FILE_ATTRIBUTE_DIRECTORY ? +FILE_FLAG_BACKUP_SEMANTICS :0 , +NULL ); +if (osfilehandle ==INVALID_HANDLE_VALUE ) { + errno =err_win_to_posix (GetLastError ()); rc =-1 ; goto revert_attrs; }
@@ -987,12 +998,14 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)GetSystemTimeAsFileTime (&mft); aft = mft; } -if (!SetFileTime ((HANDLE )_get_osfhandle (fh),NULL , &aft, &mft)) { +if (!SetFileTime (osfilehandle,NULL , &aft, &mft)) { errno =EINVAL ; rc =-1 ; }else rc =0 ; -close (fh); + +if (osfilehandle !=INVALID_HANDLE_VALUE ) +CloseHandle (osfilehandle); revert_attrs: if (attrs != INVALID_FILE_ATTRIBUTES &&
Signature · 1 line
gitgitgadget
PATCH v2 2/2 t7063: mtime-mangling instead of delays in untracked cache testing
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
The untracked cache test uses an avoid_racy function to deal with an mtime-resolution challenge in testing: If an untracked cache entry's mtime falls in the same second as the mtime of the index the untracked cache was stored in, then it cannot be trusted.
Explicitly delaying tests is a simple effective strategy to avoid these issues, but should be avoided where possible.
Switch from a delay-based strategy to instead backdating all file changes using test-tool chmtime, where that is an option, to shave 9 seconds off the test run time.
Don't update test cases that delay for other reasons, for now at least (4 seconds).
--- t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)
t/t7063-status-untracked-cache.sh +15 -12
@@ -24,6 +24,12 @@ sync_mtime () { find . -type d -exec ls -ld {} + >/dev/null } +chmtime_worktree_root () { + # chmtime doesnt handle relative paths on windows, so need + # to "hardcode" a reference to the worktree folder name. + test-tool -C .. chmtime $1 worktree +} + avoid_racy() { sleep 1 }
@@ -90,6 +96,9 @@ test_expect_success 'setup' ' cd worktree && mkdir done dtwo dthree && touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && + chmtime_worktree_root =-300 && git add one two done/one && : >.git/info/exclude && git update-index --untracked-cache &&
@@ -142,7 +151,6 @@ two EOF test_expect_success 'status first time (empty cache)' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -166,7 +174,6 @@ test_expect_success 'untracked cache after first status' ' ' test_expect_success 'status second time (fully populated cache)' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -190,8 +197,8 @@ test_expect_success 'untracked cache after second status' ' ' test_expect_success 'modify in root directory, one dir invalidation' ' - avoid_racy && : >four && + test-tool chmtime =-240 four && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -241,7 +248,6 @@ EOF ' test_expect_success 'new .gitignore invalidates recursively' ' - avoid_racy && echo four >.gitignore && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \
@@ -292,7 +298,6 @@ EOF ' test_expect_success 'new info/exclude invalidates everything' ' - avoid_racy && echo three >>.git/info/exclude && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \
@@ -520,14 +525,14 @@ test_expect_success 'create/modify files, some of which are gitignored' ' echo three >done/three && # three is gitignored echo four >done/four && # four is gitignored at a higher level echo five >done/five && # five is not gitignored - echo test >base && #we need to ensure that the root dir is touched - rm base && + test-tool chmtime =-180 done/two done/three done/four done/five done && + # we need to ensure that the root dir is touched (in the past); + chmtime_worktree_root =-180 && sync_mtime ' test_expect_success 'test sparse status with untracked cache' ' : >../trace.output && - avoid_racy && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual && iuc status --porcelain >../status.iuc &&
@@ -570,7 +575,6 @@ EOF ' test_expect_success 'test sparse status again with untracked cache' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
@@ -597,11 +601,11 @@ EOF test_expect_success 'set up for test of subdir and sparse checkouts' ' mkdir done/sub && mkdir done/sub/sub && - echo "sub" > done/sub/sub/file + echo "sub" > done/sub/sub/file && + test-tool chmtime =-120 done/sub/sub/file done/sub/sub done/sub done ' test_expect_success 'test sparse status with untracked cache and subdir' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
@@ -651,7 +655,6 @@ EOF ' test_expect_success 'test sparse status again with untracked cache and subdir' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
Signature · 1 line
gitgitgadget
Quoted 3 lines
I do have a question to the list here: Do
mingw.cchanges need to be upstreamed somewhere? I don't understand the exact relationship between this file and the MinGW project.
(sorry, failed to update the cover letter to remove the now-answered question :( )
Quoted 160 lines · Tao Klerks via GitGitGadget
As noted in a recent proposed patch to
t/t7519-status-fsmonitor.sh, a number of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a second, in order to avoid the untracked cache content being invalidated by an mtime race condition.Even though it's only 9 seconds of sleeping that can be straightforwardly replaced, it seems worth fixing if possible.
Replace sleep calls with backdating of filesystem changes, but first fix the test-tool chmtime functionality to work for directories in Windows.
I do have a question to the list here: Do
mingw.cchanges need to be upstreamed somewhere? I don't understand the exact relationship between this file and the MinGW project.Tao Klerks (2):
t/helper/test-chmtime: update mingw to support chmtime on directories
t7063: mtime-mangling instead of delays in untracked cache testingcompat/mingw.c | 21 +++++++++++++++++---- t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------ 2 files changed, 32 insertions(+), 16 deletions(-)base-commit:
4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1166Range-diff vs v1:
1:
76b6216281e! 1:7cdef0ad5fbt/helper/test-chmtime: update mingw to support chmtime on directories
@@ Commit message
Themingw_utimeimplementation inmingw.cdoes not support
directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped
- by Jeff Hostetler, in "t/helper/test-chmtime: skip directories
- on Windows" in the "Builtin FSMonitor Part 2" work.
+ temporarily by Jeff Hostetler, in "t/helper/test-chmtime: skip
+ directories on Windows" in the "Builtin FSMonitor Part 2" work, but
+ not yet fixed.It would make sense to backdate file and folder changes in untracked cache tests, to avoid needing to insert explicit delays/pauses in the tests.
- Add support for directory date manipulation in
mingw_utimeby calling
-CreateFileW()explicitly to create the directory handle, and
-CloseHandle()to close it.
+ Add support for directory date manipulation inmingw_utimeby
+ replacing the file-oriented_wopen()call with the
+ directory-supportingCreateFileW()windows API explicitly.Signed-off-by Tao Klerks <[email protected]>##
compat/mingw.c##
-@@compat/mingw.c: intmingw_utime(const char*file_name, const struct utimbuf *times)
- int fh, rc;
+@@compat/mingw.c: static inline voidtime_t_to_filetime(time_t t, FILETIME *ft)
+ intmingw_utime(const char*file_name, const struct utimbuf *times)
+ {
+ FILETIME mft, aft;
+- int fh, rc;
++ int rc;
DWORD attrs;
wchar_twfilename[MAX_PATH];
+ HANDLE osfilehandle;
@@compat/mingw.c: intmingw_utime(const char*file_name, const struct utimbuf *ti
}- if ((fh =
_wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
-- rc = -1;
-- gotorevert_attrs;
-+ if (attrs &FILE_ATTRIBUTE_DIRECTORY) {
-+ fh = 0;
-+ osfilehandle = CreateFileW(wfilename,
-+0x100/*FILE_WRITE_ATTRIBUTES*/,
-+ 0 /*FileShare.None*/,
-+ NULL,
-+OPEN_EXISTING,
-+FILE_FLAG_BACKUP_SEMANTICS,
-+ NULL);
-+ if (osfilehandle ==INVALID_HANDLE_VALUE) {
-+ errno = err_win_to_posix(GetLastError());
-+ rc = -1;
-+ gotorevert_attrs;
-+ }
-+ } else {
-+ if ((fh =_wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
-+ rc = -1;
-+ gotorevert_attrs;
-+ }
-+ osfilehandle = (HANDLE)_get_osfhandle(fh);
++ osfilehandle = CreateFileW(wfilename,
++0x100/*FILE_WRITE_ATTRIBUTES*/,
++ 0 /*FileShare.None*/,
++ NULL,
++OPEN_EXISTING,
++ attrs &FILE_ATTRIBUTE_DIRECTORY?
++FILE_FLAG_BACKUP_SEMANTICS: 0,
++ NULL);
++ if (osfilehandle ==INVALID_HANDLE_VALUE) {
++ errno = err_win_to_posix(GetLastError());
+ rc = -1;
+ gotorevert_attrs;
}
-
- if (times) {
@@compat/mingw.c: intmingw_utime(const char*file_name, const struct utimbuf *times)
GetSystemTimeAsFileTime(&mft);
aft = mft;
@@compat/mingw.c: intmingw_utime(const char*file_name, const struct utimbuf *ti
rc = 0;
- close(fh);
+
-+ if (fh)
-+ close(fh);
-+ else if (osfilehandle)
++ if (osfilehandle !=INVALID_HANDLE_VALUE)
+ CloseHandle(osfilehandle);
revert_attrs:
2:a1806c56333! 2:3e3c9c7faact7063: mtime-mangling instead of delays in untracked cache testing@@ t/t7063-status-untracked-cache.sh: sync_mtime () { find . -type d -exec ls -ld {} + >/dev/null } -+chmmtime_worktree_root () { ++chmtime_worktree_root () { + # chmtime doesnt handle relative paths on windows, so need + # to "hardcode" a reference to the worktree folder name. -+ cd .. && -+ test-tool chmtime $1 worktree && -+ cd worktree ++ test-tool -C .. chmtime $1 worktree +} + avoid_racy() { @@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' ' touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && -+ chmmtime_worktree_root =-300 && ++ chmtime_worktree_root =-300 && git add one two done/one && : >.git/info/exclude && git update-index --untracked-cache && @@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som - rm base && + test-tool chmtime =-180 done/two done/three done/four done/five done && + # we need to ensure that the root dir is touched (in the past); -+ chmmtime_worktree_root =-180 && ++ chmtime_worktree_root =-180 && sync_mtime '--
gitgitgadget
Re: [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
In reply to Tao Klerks via GitGitGadgetQuoted 40 lines · Tao Klerks via GitGitGadget
From: Tao Klerks <[email protected]>
The
mingw_utimeimplementation inmingw.cdoes not support directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped temporarily by Jeff Hostetler, in "t/helper/test-chmtime: skip directories on Windows" in the "Builtin FSMonitor Part 2" work, but not yet fixed.It would make sense to backdate file and folder changes in untracked cache tests, to avoid needing to insert explicit delays/pauses in the tests.
Add support for directory date manipulation in
mingw_utimeby replacing the file-oriented_wopen()call with the directory-supportingCreateFileW()windows API explicitly.Signed-off-by Tao Klerks <[email protected]>--- compat/mingw.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)compat/mingw.c +3
-1@@ -961,9 +961,11 @@ static inline void time_t_to_filetime(time_t t, FILETIME *ft)int mingw_utime (const char *file_name,const struct utimbuf *times) {FILETIME mft, aft; -int fh, rc; +int rc;DWORD attrs;wchar_t wfilename[MAX_PATH ]; +HANDLE osfilehandle; +if (xutftowcs_path (wfilename, file_name) <0 )return -1 ;
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 981
Quoted 6 lines
compat/mingw.c +2
-1@@ -975,7 +977,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)SetFileAttributesW (wfilename, attrs & ~FILE_ATTRIBUTE_READONLY ); } -if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { + osfilehandle = CreateFileW(wfilename, + 0x100 /*FILE_WRITE_ATTRIBUTES*/,
https://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants
indicates that FILE_WRITE_ATTRIBUTES is defined in <WinNT.h> which we get from <windows.h> which was included by "win32.h", so it should already be present.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 986
Quoted 5 lines
compat/mingw.c +5
-0+0 /*FileShare.None*/ , +NULL , +OPEN_EXISTING , + attrs &FILE_ATTRIBUTE_DIRECTORY ? + FILE_FLAG_BACKUP_SEMANTICS : 0,
There is a weird error case here. If the GetFileAttributesW() call at the top fails, it returns INVALID_FILE_ATTRIBUTES (aka -1). So the (attrs & ...) here expression is questionable.
I'm not sure that there is a best way to handle the earlier failure (other than returning an error at the top), but we do try to limp along (for some reason).
So maybe make this something like:
(attrs != INVALID_FILE_ATTRIBUTES &&
(attrs & FILE_ATTRIBUTE_DIRECTORY)) ?
FILE_FLAG_BACKUP_SEMANTICS : 0
Quoted 24 lines
compat/mingw.c +7
-2+ NULL); +if (osfilehandle ==INVALID_HANDLE_VALUE ) { + errno =err_win_to_posix (GetLastError ()); rc =-1 ; goto revert_attrs; }@@ -987,12 +998,14 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)GetSystemTimeAsFileTime (&mft); aft = mft; } -if (!SetFileTime ((HANDLE )_get_osfhandle (fh),NULL , &aft, &mft)) { +if (!SetFileTime (osfilehandle,NULL , &aft, &mft)) { errno =EINVAL ; rc =-1 ; }else rc =0 ; -close (fh); + +if (osfilehandle !=INVALID_HANDLE_VALUE ) +CloseHandle (osfilehandle); revert_attrs: if (attrs != INVALID_FILE_ATTRIBUTES &&
Quoted 7 lines · Tao Klerks via GitGitGadget
As noted in a recent proposed patch to
t/t7519-status-fsmonitor.sh, a number of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a second, in order to avoid the untracked cache content being invalidated by an mtime race condition.Even though it's only 9 seconds of sleeping that can be straightforwardly replaced, it seems worth fixing if possible.
Absolutely. Thanks for an update.
Re: [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
In reply to Tao Klerks via GitGitGadgett/t7063-status-untracked-cache.sh sync_mtime () { · line 32
Quoted 6 lines · Tao Klerks via GitGitGadget
t/t7063-status-untracked-cache.sh +6
-0+chmtime_worktree_root () { + # chmtime doesnt handle relative paths on windows, so need + # to "hardcode" a reference to the worktree folder name. + test-tool -C .. chmtime $1 worktree +} +
Enclose $1 in a pair of double-quotes to help readers. They do not have to wonder if the caller is interested in (or has to worry about) triggering word splitting at $IFS if you did so.
t/t7063-status-untracked-cache.sh test_expect_success 'setup' ' · line 101
Quoted 10 lines
avoid_racy(){t/t7063-status-untracked-cache.sh +3
-0sleep 1 }@@ -90,6 +96,9 @@ test_expect_success 'setup' ' cd worktree && mkdir done dtwo dthree && touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && + chmtime_worktree_root =-300 &&
I am wondering if it is better to spelling it out like this:
test-tool -C.. chmtime =-300 worktree &&
instead of hiding the fact that "../worktree" is being touched behind a one-line helper. Being able to explicitly write "worktree" in the context that this particular code path uses the "worktree" directory is a big plus, but at the same time, bypassing the helper makes it unclear why we just don't chmtime "../worktree", and will strongly tempt future developers into breaking it, so, I dunno.
What's the reason why utime() works only on a path in the current directory and cannot take "../worktree" again? If we cannot solve that, I guess an extra helper with a big comment, like we see in this patch, would be the least bad solution.
Thanks.
Re: [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
In reply to Jeff HostetlerQuoted 7 lines · Jeff Hostetler
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 981
Quoted 4 lines · Tao Klerks via GitGitGadget
compat/mingw.c +2
-1-if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { + osfilehandle = CreateFileW(wfilename, + 0x100 /*FILE_WRITE_ATTRIBUTES*/,
Quoted 6 lines
https://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants
indicates that
FILE_WRITE_ATTRIBUTESis defined in <WinNT.h> which we get from <windows.h> which was included by "win32.h", so it should already be present.
Grr, should have asked the compiler instead of VSCode's autocomplete.
Thx, fixed.
Quoted 14 lines
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 986
Quoted 5 lines
compat/mingw.c +5
-0+0 /*FileShare.None*/ , +NULL , +OPEN_EXISTING , + attrs &FILE_ATTRIBUTE_DIRECTORY ? + FILE_FLAG_BACKUP_SEMANTICS : 0,There is a weird error case here. If the
GetFileAttributesW()call at the top fails, it returnsINVALID_FILE_ATTRIBUTES(aka -1). So the (attrs & ...) here expression is questionable.I'm not sure that there is a best way to handle the earlier failure (other than returning an error at the top), but we do try to limp along (for some reason).
Quoted 7 lines
So maybe make this something like:
(attrs !=
INVALID_FILE_ATTRIBUTES&&
(attrs &FILE_ATTRIBUTE_DIRECTORY)) ?
FILE_FLAG_BACKUP_SEMANTICS: 0
Yikes, I didn't realize how negative numbers looked in binary (in C).
Thanks for this catch!
Re: [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
In reply to Junio C HamanoQuoted 13 lines · Junio C Hamano
t/t7063-status-untracked-cache.sh sync_mtime () { · line 32
Quoted 6 lines · Tao Klerks via GitGitGadget
t/t7063-status-untracked-cache.sh +6
-0+chmtime_worktree_root () { + # chmtime doesnt handle relative paths on windows, so need + # to "hardcode" a reference to the worktree folder name. + test-tool -C .. chmtime $1 worktree +} +Enclose $1 in a pair of double-quotes to help readers. They do not have to wonder if the caller is interested in (or has to worry about) triggering word splitting at $IFS if you did so.
Absolutely, thx.
Quoted 23 lines
t/t7063-status-untracked-cache.sh test_expect_success 'setup' ' · line 101
Quoted 11 lines
avoid_racy(){
sleep 1
}t/t7063-status-untracked-cache.sh +3
-0@@ -90,6 +96,9 @@ test_expect_success 'setup' ' cd worktree && mkdir done dtwo dthree && touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && + chmtime_worktree_root =-300 &&I am wondering if it is better to spelling it out like this:
test-tool -C.. chmtime =-300 worktree &&instead of hiding the fact that "../worktree" is being touched behind a one-line helper. Being able to explicitly write "worktree" in the context that this particular code path uses the "worktree" directory is a big plus, but at the same time, bypassing the helper makes it unclear why we just don't chmtime "../worktree", and will strongly tempt future developers into breaking it, so, I dunno.
Quoted 5 lines
What's the reason why
utime()works only on a path in the current directory and cannot take "../worktree" again? If we cannot solve that, I guess an extra helper with a big comment, like we see in this patch, would be the least bad solution.
Heh. It didn't work, in my initial tests. Now it does. It turns out I was initially getting the directory handle with
"FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES", and
everything worked except modifying the current folder via a relative path expression, which yielded a "Permission denied" error. I worked around it by explicitly changing the current directory...
Then I realized FILE_WRITE_DATA wasn't necessary (but didn't connect the dots). Then you noted the "-C .." arg to test-tool (and I still didn't connect the dots).
The problem was never relative paths, but rather trying to get a writable handle to the current directory. The only reason "-C .." worked was that I already stopped trying to get a writable handle.
I have no idea what it means to get a writable handle to a directory, but apparently you can't do it for your current directory. Now I know.
Thanks for the nudge, this is all clean now.
v3 cover Reduce explicit sleep calls in t7063 untracked cache tests
In reply to Tao Klerks via GitGitGadgetReplace sleep calls with backdating of filesystem changes, but first fix the test-tool chmtime functionality to work for directories in Windows.
In this third version, a bug with inaccessible-path-handling that Jeff found is fixed, and a relative-path-avoiding strategy that had been added in t7073 is simplified out after Junio's comments revealed it was misguided.
Tao Klerks (2):
t/helper/test-chmtime: update mingw to support chmtime on directories
t7063: mtime-mangling instead of delays in untracked cache testing
compat/mingw.c | 23 +++++++++++++++++++---- t/t7063-status-untracked-cache.sh | 21 +++++++++------------ 2 files changed, 28 insertions(+), 16 deletions(-)
base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1166
Range-diff vs v2:
1: 7cdef0ad5fb ! 1: 052b3dd9bba t/helper/test-chmtime: update mingw to support chmtime on directories
@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti
- if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
+ osfilehandle = CreateFileW(wfilename,
-+ 0x100 /*FILE_WRITE_ATTRIBUTES*/,
++ FILE_WRITE_ATTRIBUTES,
+ 0 /*FileShare.None*/,
+ NULL,
+ OPEN_EXISTING,
-+ attrs & FILE_ATTRIBUTE_DIRECTORY ?
++ (attrs != INVALID_FILE_ATTRIBUTES &&
++ (attrs & FILE_ATTRIBUTE_DIRECTORY)) ?
+ FILE_FLAG_BACKUP_SEMANTICS : 0,
+ NULL);
+ if (osfilehandle == INVALID_HANDLE_VALUE) {
@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti
aft = mft;
}
- if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {
++
+ if (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {
errno = EINVAL;
rc = -1;
2: 3e3c9c7faac ! 2: dceb2857609 t7063: mtime-mangling instead of delays in untracked cache testing
@@ Commit message
Signed-off-by: Tao Klerks <[email protected]>
## t/t7063-status-untracked-cache.sh ##
-@@ t/t7063-status-untracked-cache.sh: sync_mtime () {
- find . -type d -exec ls -ld {} + >/dev/null
- }
-
-+chmtime_worktree_root () {
-+ # chmtime doesnt handle relative paths on windows, so need
-+ # to "hardcode" a reference to the worktree folder name.
-+ test-tool -C .. chmtime $1 worktree
-+}
-+
- avoid_racy() {
- sleep 1
- }
@@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' '
cd worktree &&
mkdir done dtwo dthree &&
touch one two three done/one dtwo/two dthree/three &&
+ test-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&
+ test-tool chmtime =-300 done dtwo dthree &&
-+ chmtime_worktree_root =-300 &&
++ test-tool chmtime =-300 . &&
git add one two done/one &&
: >.git/info/exclude &&
git update-index --untracked-cache &&
@@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som
- rm base &&
+ test-tool chmtime =-180 done/two done/three done/four done/five done &&
+ # we need to ensure that the root dir is touched (in the past);
-+ chmtime_worktree_root =-180 &&
++ test-tool chmtime =-180 . &&
sync_mtime
'Signature · 1 line
gitgitgadget
PATCH v3 1/2 t/helper/test-chmtime: update mingw to support chmtime on directories
From: Tao Klerks <[email protected]>
The mingw_utime implementation in mingw.c does not support directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped temporarily by Jeff Hostetler, in "t/helper/test-chmtime: skip directories on Windows" in the "Builtin FSMonitor Part 2" work, but not yet fixed.
It would make sense to backdate file and folder changes in untracked cache tests, to avoid needing to insert explicit delays/pauses in the tests.
Add support for directory date manipulation in mingw_utime by replacing the file-oriented _wopen() call with the directory-supporting CreateFileW() windows API explicitly.
--- compat/mingw.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
compat/mingw.c +19 -4
@@ -961,9 +961,11 @@ static inline void time_t_to_filetime(time_t t, FILETIME *ft)int mingw_utime (const char *file_name,const struct utimbuf *times) {FILETIME mft, aft; -int fh, rc; +int rc;DWORD attrs;wchar_t wfilename[MAX_PATH ]; +HANDLE osfilehandle; +if (xutftowcs_path (wfilename, file_name) <0 )return -1 ;
@@ -975,7 +977,17 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)SetFileAttributesW (wfilename, attrs & ~FILE_ATTRIBUTE_READONLY ); } -if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { + osfilehandle =CreateFileW (wfilename, +FILE_WRITE_ATTRIBUTES , +0 /*FileShare.None*/ , +NULL , +OPEN_EXISTING , + (attrs !=INVALID_FILE_ATTRIBUTES && + (attrs &FILE_ATTRIBUTE_DIRECTORY )) ? +FILE_FLAG_BACKUP_SEMANTICS :0 , +NULL ); +if (osfilehandle ==INVALID_HANDLE_VALUE ) { + errno =err_win_to_posix (GetLastError ()); rc =-1 ; goto revert_attrs; }
@@ -987,12 +999,15 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)GetSystemTimeAsFileTime (&mft); aft = mft; } -if (!SetFileTime ((HANDLE )_get_osfhandle (fh),NULL , &aft, &mft)) { + +if (!SetFileTime (osfilehandle,NULL , &aft, &mft)) { errno =EINVAL ; rc =-1 ; }else rc =0 ; -close (fh); + +if (osfilehandle !=INVALID_HANDLE_VALUE ) +CloseHandle (osfilehandle); revert_attrs: if (attrs != INVALID_FILE_ATTRIBUTES &&
Signature · 1 line
gitgitgadget
PATCH v3 2/2 t7063: mtime-mangling instead of delays in untracked cache testing
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
The untracked cache test uses an avoid_racy function to deal with an mtime-resolution challenge in testing: If an untracked cache entry's mtime falls in the same second as the mtime of the index the untracked cache was stored in, then it cannot be trusted.
Explicitly delaying tests is a simple effective strategy to avoid these issues, but should be avoided where possible.
Switch from a delay-based strategy to instead backdating all file changes using test-tool chmtime, where that is an option, to shave 9 seconds off the test run time.
Don't update test cases that delay for other reasons, for now at least (4 seconds).
--- t/t7063-status-untracked-cache.sh | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
t/t7063-status-untracked-cache.sh +9 -12
@@ -90,6 +90,9 @@ test_expect_success 'setup' ' cd worktree && mkdir done dtwo dthree && touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && + test-tool chmtime =-300 . && git add one two done/one && : >.git/info/exclude && git update-index --untracked-cache &&
@@ -142,7 +145,6 @@ two EOF test_expect_success 'status first time (empty cache)' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -166,7 +168,6 @@ test_expect_success 'untracked cache after first status' ' ' test_expect_success 'status second time (fully populated cache)' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -190,8 +191,8 @@ test_expect_success 'untracked cache after second status' ' ' test_expect_success 'modify in root directory, one dir invalidation' ' - avoid_racy && : >four && + test-tool chmtime =-240 four && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../actual &&
@@ -241,7 +242,6 @@ EOF ' test_expect_success 'new .gitignore invalidates recursively' ' - avoid_racy && echo four >.gitignore && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \
@@ -292,7 +292,6 @@ EOF ' test_expect_success 'new info/exclude invalidates everything' ' - avoid_racy && echo three >>.git/info/exclude && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \
@@ -520,14 +519,14 @@ test_expect_success 'create/modify files, some of which are gitignored' ' echo three >done/three && # three is gitignored echo four >done/four && # four is gitignored at a higher level echo five >done/five && # five is not gitignored - echo test >base && #we need to ensure that the root dir is touched - rm base && + test-tool chmtime =-180 done/two done/three done/four done/five done && + # we need to ensure that the root dir is touched (in the past); + test-tool chmtime =-180 . && sync_mtime ' test_expect_success 'test sparse status with untracked cache' ' : >../trace.output && - avoid_racy && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual && iuc status --porcelain >../status.iuc &&
@@ -570,7 +569,6 @@ EOF ' test_expect_success 'test sparse status again with untracked cache' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
@@ -597,11 +595,11 @@ EOF test_expect_success 'set up for test of subdir and sparse checkouts' ' mkdir done/sub && mkdir done/sub/sub && - echo "sub" > done/sub/sub/file + echo "sub" > done/sub/sub/file && + test-tool chmtime =-120 done/sub/sub/file done/sub/sub done/sub done ' test_expect_success 'test sparse status with untracked cache and subdir' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
@@ -651,7 +649,6 @@ EOF ' test_expect_success 'test sparse status again with untracked cache and subdir' ' - avoid_racy && : >../trace.output && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.output" \ git status --porcelain >../status.actual &&
Signature · 1 line
gitgitgadget
Hi Junio,
I see that in the latest "what's coming" update, this patch series is listed as expecting reroll, but as far as I can see there have been no comments since I sent this V3 out.
What should be my next step, if I think this is "as ready as I know to get it" without any comments on the latest submission?
Thanks,
Tao
(Apologies to recipients of duplicate email in cc, I accidentally sent html email that the list rejected)
Quoted 89 lines · On Wed, Mar 2, 2022, 07:05 Tao Klerks via GitGitGadget…
Replace sleep calls with backdating of filesystem changes, but first fix the test-tool chmtime functionality to work for directories in Windows.
In this third version, a bug with inaccessible-path-handling that Jeff found is fixed, and a relative-path-avoiding strategy that had been added in t7073 is simplified out after Junio's comments revealed it was misguided.
Tao Klerks (2):
t/helper/test-chmtime: update mingw to support chmtime on directories
t7063: mtime-mangling instead of delays in untracked cache testingcompat/mingw.c | 23 +++++++++++++++++++---- t/t7063-status-untracked-cache.sh | 21 +++++++++------------ 2 files changed, 28 insertions(+), 16 deletions(-)base-commit:
4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1166Range-diff vs v2:
1:
7cdef0ad5fb! 1:052b3dd9bbat/helper/test-chmtime: update mingw to support chmtime on directories
@@compat/mingw.c: intmingw_utime(const char*file_name, const struct utimbuf *ti- if ((fh =
_wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
+ osfilehandle = CreateFileW(wfilename,
-+0x100/*FILE_WRITE_ATTRIBUTES*/,
++FILE_WRITE_ATTRIBUTES,
+ 0 /*FileShare.None*/,
+ NULL,
+OPEN_EXISTING,
-+ attrs &FILE_ATTRIBUTE_DIRECTORY?
++ (attrs !=INVALID_FILE_ATTRIBUTES&&
++ (attrs &FILE_ATTRIBUTE_DIRECTORY)) ?
+FILE_FLAG_BACKUP_SEMANTICS: 0,
+ NULL);
+ if (osfilehandle ==INVALID_HANDLE_VALUE) {
@@compat/mingw.c: intmingw_utime(const char*file_name, const struct utimbuf *ti
aft = mft;
}
- if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {
++
+ if (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {
errno = EINVAL;
rc = -1;
2:3e3c9c7faac! 2:dceb2857609t7063: mtime-mangling instead of delays in untracked cache testing@@ Commit message Signed-off-by: Tao Klerks <[email protected]> ## t/t7063-status-untracked-cache.sh ## -@@ t/t7063-status-untracked-cache.sh: sync_mtime () { - find . -type d -exec ls -ld {} + >/dev/null - } - -+chmtime_worktree_root () { -+ # chmtime doesnt handle relative paths on windows, so need -+ # to "hardcode" a reference to the worktree folder name. -+ test-tool -C .. chmtime $1 worktree -+} -+ - avoid_racy() { - sleep 1 - } @@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' ' cd worktree && mkdir done dtwo dthree && touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && -+ chmtime_worktree_root =-300 && ++ test-tool chmtime =-300 . && git add one two done/one && : >.git/info/exclude && git update-index --untracked-cache && @@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som - rm base && + test-tool chmtime =-180 done/two done/three done/four done/five done && + # we need to ensure that the root dir is touched (in the past); -+ chmtime_worktree_root =-180 && ++ test-tool chmtime =-180 . && sync_mtime '--
gitgitgadget
Quoted 3 lines · Tao Klerks
I see that in the latest "what's coming" update, this patch series is listed as expecting reroll, but as far as I can see there have been no comments since I sent this V3 out.
I see <v2> reference on the source: line for that topic in the "What's cooking" report you are referring to, so I suspect I have been too busy and haven't got around to replacing it with <v3>, perhaps?
Eek, my apologies. I will remember to check that!
Quoted 12 lines · Junio C Hamano
Quoted 3 lines · Tao Klerks
I see that in the latest "what's coming" update, this patch series is listed as expecting reroll, but as far as I can see there have been no comments since I sent this V3 out.
I see <v2> reference on the source: line for that topic in the "What's cooking" report you are referring to, so I suspect I have been too busy and haven't got around to replacing it with <v3>, perhaps?
Quoted 8 lines · Tao Klerks
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Tao Klerks
I see that in the latest "what's coming" update, this patch series is listed as expecting reroll, but as far as I can see there have been no comments since I sent this V3 out.
Quoted 6 lines
Quoted 5 lines
I see <v2> reference on the source: line for that topic in the "What's cooking" report you are referring to, so I suspect I have been too busy and haven't got around to replacing it with <v3>, perhaps?
Eek, my apologies. I will remember to check that!
No need to apologize.
It often happens that I forget to update the status of a series after a new iteration replaces the old one when sending out a new issue of the "What's cooking" report. It is very much appreciated to be watchful and report such mistakes when you notice it.
This time, it turns out that what happened was that I updated the "What's cooking" report and sent it out, and then continued working for the rest of the day to queue new topics and replace existing ones. And when you pinged me, I already had <v3> queued.
To me, judging from the exchanges between you and Jeff Hostetler, the topic at v3 seems to be ready for 'next', so let me mark it as such in the draft of the next issue of "What's cooking" I keep locally.
Thanks.
Re: [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
In reply to Tao Klerks via GitGitGadgetHi Tao,
compat/mingw.c static inline void time_t_to_filetime(time_t t, FILETIME *ft) · line 967
Quoted 13 lines · Tao Klerks via GitGitGadget
compat/mingw.c +2
-1@@ -961,9 +961,11 @@ static inline void time_t_to_filetime(time_t t, FILETIME *ft)int mingw_utime (const char *file_name,const struct utimbuf *times) {FILETIME mft, aft; -int fh, rc; +int rc;DWORD attrs;wchar_t wfilename[MAX_PATH ]; +HANDLE osfilehandle;
I would prefer the short-and-sweet name handle here. _Especially_ since we are no longer using _get_osfhandle() at all anymore, meaning that the name is actually misleading.
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 982
Quoted 12 lines
compat/mingw.c +4
-1+if (xutftowcs_path (wfilename, file_name) <0 )return -1 ;@@ -975,7 +977,17 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)SetFileAttributesW (wfilename, attrs & ~FILE_ATTRIBUTE_READONLY ); } -if ((fh =_wopen (wfilename,O_RDWR |O_BINARY )) <0 ) { + osfilehandle = CreateFileW(wfilename, + FILE_WRITE_ATTRIBUTES, + 0 /*FileShare.None*/,
Hmm. I wonder why you don't want to allow shared access? Like, why disallow changing the mtime while a file is being read?
This is a change in behavior, and I think we should avoid that. Wine uses FILE_SHARE_READ | FILE_SHARE_WRITE in its _wopen() implementation (there are a couple of layers of indirection leading all the way down to
https://github.com/wine-mirror/wine/blob/1d178982ae5a73b18f367026c8689b56789c39fd/dlls/msvcrt/file.c#L2261).
The closest already-existing code that creates a file handle to a directory is in mingw_getcwd(), and it even adds FILE_SHARE_DELETE to the fray. That would probably be the best here, too.
The rest of the patch
Quoted 16 lines
compat/mingw.c +8
-1+NULL , +OPEN_EXISTING , + (attrs !=INVALID_FILE_ATTRIBUTES && + (attrs &FILE_ATTRIBUTE_DIRECTORY )) ? +FILE_FLAG_BACKUP_SEMANTICS :0 , + NULL); +if (osfilehandle ==INVALID_HANDLE_VALUE ) { + errno =err_win_to_posix (GetLastError ()); rc =-1 ; goto revert_attrs; }@@ -987,12 +999,15 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)GetSystemTimeAsFileTime (&mft); aft = mft; } -if (!SetFileTime ((HANDLE )_get_osfhandle (fh),NULL , &aft, &mft)) {
compat/mingw.c int mingw_utime (const char *file_name, const struct utimbuf *times) · line 1010
Quoted 10 lines
compat/mingw.c +5
-1+ +if (!SetFileTime (osfilehandle,NULL , &aft, &mft)) { errno =EINVAL ; rc =-1 ; }else rc =0 ; -close (fh); + +if (osfilehandle !=INVALID_HANDLE_VALUE ) +CloseHandle (osfilehandle);
It is quite obvious from the diff that it is quite impossible for osfilehandle to equal INVALID_HANDLE_VALUE, because if that is the case, we specifically goto revert_attrs which is two lines below:
Quoted 5 lines
compat/mingw.c +0
-1revert_attrs: if (attrs != INVALID_FILE_ATTRIBUTES && --gitgitgadget
Therefore, I would prefer the CloseHandle() to be as unconditional as the now-replaced close() was.
Thank you,
Dscho
Re: [PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
In reply to Tao Klerks via GitGitGadgetHi Tao,
t/t7063-status-untracked-cache.sh test_expect_success 'setup' ' · line 95
Quoted 11 lines · Tao Klerks via GitGitGadget
t/t7063-status-untracked-cache.sh +3
-0@@ -90,6 +90,9 @@ test_expect_success 'setup' ' cd worktree && mkdir done dtwo dthree && touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && + test-tool chmtime =-300 . &&
At first I puzzled why we need done, dtwo, dthree and .`, too, but it makes sense: the invariant is that all of the files/directories whose mtime is adjusted in these three new lines have the same mtime.
A similar issue is...
t/t7063-status-untracked-cache.sh test_expect_success 'create/modify files, some of which are gitignored' ' · line 524
Quoted 9 lines
t/t7063-status-untracked-cache.sh +3
-2@@ -520,14 +519,14 @@ test_expect_success 'create/modify files, some of which are gitignored' ' echo three >done/three && # three is gitignored echo four >done/four && # four is gitignored at a higher level echo five >done/five && # five is not gitignored - echo test >base && #we need to ensure that the root dir is touched - rm base && + test-tool chmtime =-180 done/two done/three done/four done/five done && + # we need to ensure that the root dir is touched (in the past); + test-tool chmtime =-180 . &&
Here, where I needed a moment to understand the invariant (and hence the need to adjust more than just the root directory's mtime).
t/t7063-status-untracked-cache.sh EOF · line 599
Quoted 7 lines
t/t7063-status-untracked-cache.sh +2
-1@@ -597,11 +595,11 @@ EOF test_expect_success 'set up for test of subdir and sparse checkouts' ' mkdir done/sub && mkdir done/sub/sub && - echo "sub" > done/sub/sub/file + echo "sub" > done/sub/sub/file && + test-tool chmtime =-120 done/sub/sub/file done/sub/sub done/sub done
Similar situation here, too.
All this is to say that this patch looks good to me.
Thanks,
Dscho