* [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
2022-03-01 9:45 ` [PATCH v2 " Tao Klerks via GitGitGadget
@ 2022-03-01 9:45 ` Tao Klerks via GitGitGadget
2022-03-01 16:34 ` Jeff Hostetler
2022-03-01 9:45 ` [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing Tao Klerks via GitGitGadget
` (3 subsequent siblings)
4 siblings, 1 reply; 30+ messages in thread
From: Tao Klerks via GitGitGadget @ 2022-03-01 9:45 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Jeff Hostetler,
Tao Klerks, Tao Klerks, Tao Klerks
From: Tao Klerks <tao@klerks.biz>
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.
Signed-off-by: Tao Klerks <tao@klerks.biz>
---
compat/mingw.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 03af369b2b9..11c43ad2dfb 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
2022-03-01 9:45 ` [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories Tao Klerks via GitGitGadget
@ 2022-03-01 16:34 ` Jeff Hostetler
2022-03-01 21:14 ` Tao Klerks
0 siblings, 1 reply; 30+ messages in thread
From: Jeff Hostetler @ 2022-03-01 16:34 UTC (permalink / raw)
To: Tao Klerks via GitGitGadget, git
Cc: Ævar Arnfjörð Bjarmason, Tao Klerks
On 3/1/22 4:45 AM, Tao Klerks via GitGitGadget wrote:
> From: Tao Klerks <tao@klerks.biz>
>
> 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.
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
> compat/mingw.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 03af369b2b9..11c43ad2dfb 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -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*/,
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.
> + 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
> + 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 &&
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
2022-03-01 16:34 ` Jeff Hostetler
@ 2022-03-01 21:14 ` Tao Klerks
0 siblings, 0 replies; 30+ messages in thread
From: Tao Klerks @ 2022-03-01 21:14 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Tao Klerks via GitGitGadget, git, Ævar Arnfjörð Bjarmason
On Tue, Mar 1, 2022 at 5:34 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
>
> On 3/1/22 4:45 AM, Tao Klerks via GitGitGadget wrote:
> >
> > - 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.
>
Grr, should have asked the compiler instead of VSCode's autocomplete.
Thx, fixed.
> > + 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
>
>
Yikes, I didn't realize how negative numbers looked in binary (in C).
Thanks for this catch!
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
2022-03-01 9:45 ` [PATCH v2 " Tao Klerks via GitGitGadget
2022-03-01 9:45 ` [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories Tao Klerks via GitGitGadget
@ 2022-03-01 9:45 ` Tao Klerks via GitGitGadget
2022-03-01 18:03 ` Junio C Hamano
2022-03-01 9:49 ` [PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests Tao Klerks
` (2 subsequent siblings)
4 siblings, 1 reply; 30+ messages in thread
From: Tao Klerks via GitGitGadget @ 2022-03-01 9:45 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Jeff Hostetler,
Tao Klerks, Tao Klerks, Tao Klerks
From: Tao Klerks <tao@klerks.biz>
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).
Signed-off-by: Tao Klerks <tao@klerks.biz>
---
t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index a0c123b0a77..eae57dc78a6 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
2022-03-01 9:45 ` [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing Tao Klerks via GitGitGadget
@ 2022-03-01 18:03 ` Junio C Hamano
2022-03-01 22:13 ` Tao Klerks
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2022-03-01 18:03 UTC (permalink / raw)
To: Tao Klerks via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler, Tao Klerks
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +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.
> 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 &&
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
2022-03-01 18:03 ` Junio C Hamano
@ 2022-03-01 22:13 ` Tao Klerks
0 siblings, 0 replies; 30+ messages in thread
From: Tao Klerks @ 2022-03-01 22:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Tao Klerks via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Jeff Hostetler
On Tue, Mar 1, 2022 at 7:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +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.
>
> > 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 &&
>
> 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.
>
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests
2022-03-01 9:45 ` [PATCH v2 " Tao Klerks via GitGitGadget
2022-03-01 9:45 ` [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories Tao Klerks via GitGitGadget
2022-03-01 9:45 ` [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing Tao Klerks via GitGitGadget
@ 2022-03-01 9:49 ` Tao Klerks
2022-03-01 17:49 ` Junio C Hamano
2022-03-02 6:05 ` [PATCH v3 " Tao Klerks via GitGitGadget
4 siblings, 0 replies; 30+ messages in thread
From: Tao Klerks @ 2022-03-01 9:49 UTC (permalink / raw)
To: Tao Klerks via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler
> 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.
(sorry, failed to update the cover letter to remove the now-answered
question :( )
On Tue, Mar 1, 2022 at 10:45 AM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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 | 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 <tao@klerks.biz>
>
> ## 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
> '
>
>
> --
> gitgitgadget
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests
2022-03-01 9:45 ` [PATCH v2 " Tao Klerks via GitGitGadget
` (2 preceding siblings ...)
2022-03-01 9:49 ` [PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests Tao Klerks
@ 2022-03-01 17:49 ` Junio C Hamano
2022-03-02 6:05 ` [PATCH v3 " Tao Klerks via GitGitGadget
4 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2022-03-01 17:49 UTC (permalink / raw)
To: Tao Klerks via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler, Tao Klerks
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests
2022-03-01 9:45 ` [PATCH v2 " Tao Klerks via GitGitGadget
` (3 preceding siblings ...)
2022-03-01 17:49 ` Junio C Hamano
@ 2022-03-02 6:05 ` Tao Klerks via GitGitGadget
2022-03-02 6:05 ` [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories Tao Klerks via GitGitGadget
` (2 more replies)
4 siblings, 3 replies; 30+ messages in thread
From: Tao Klerks via GitGitGadget @ 2022-03-02 6:05 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Jeff Hostetler,
Tao Klerks, Tao Klerks
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 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 <tao@klerks.biz>
## 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
2022-03-02 6:05 ` [PATCH v3 " Tao Klerks via GitGitGadget
@ 2022-03-02 6:05 ` Tao Klerks via GitGitGadget
2022-03-09 21:46 ` Johannes Schindelin
2022-03-02 6:05 ` [PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing Tao Klerks via GitGitGadget
2022-03-05 4:24 ` [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests Tao Klerks
2 siblings, 1 reply; 30+ messages in thread
From: Tao Klerks via GitGitGadget @ 2022-03-02 6:05 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Jeff Hostetler,
Tao Klerks, Tao Klerks, Tao Klerks
From: Tao Klerks <tao@klerks.biz>
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.
Signed-off-by: Tao Klerks <tao@klerks.biz>
---
compat/mingw.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 03af369b2b9..58f347d6ae5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
2022-03-02 6:05 ` [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories Tao Klerks via GitGitGadget
@ 2022-03-09 21:46 ` Johannes Schindelin
0 siblings, 0 replies; 30+ messages in thread
From: Johannes Schindelin @ 2022-03-09 21:46 UTC (permalink / raw)
To: Tao Klerks via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler,
Tao Klerks, Tao Klerks, Tao Klerks
Hi Tao,
On Wed, 2 Mar 2022, Tao Klerks via GitGitGadget wrote:
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 03af369b2b9..58f347d6ae5 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -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.
> +
> 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
> + 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);
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:
>
> revert_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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
2022-03-02 6:05 ` [PATCH v3 " Tao Klerks via GitGitGadget
2022-03-02 6:05 ` [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories Tao Klerks via GitGitGadget
@ 2022-03-02 6:05 ` Tao Klerks via GitGitGadget
2022-03-09 21:53 ` Johannes Schindelin
2022-03-05 4:24 ` [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests Tao Klerks
2 siblings, 1 reply; 30+ messages in thread
From: Tao Klerks via GitGitGadget @ 2022-03-02 6:05 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Jeff Hostetler,
Tao Klerks, Tao Klerks, Tao Klerks
From: Tao Klerks <tao@klerks.biz>
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).
Signed-off-by: Tao Klerks <tao@klerks.biz>
---
t/t7063-status-untracked-cache.sh | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index a0c123b0a77..ca90ee805e7 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing
2022-03-02 6:05 ` [PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing Tao Klerks via GitGitGadget
@ 2022-03-09 21:53 ` Johannes Schindelin
0 siblings, 0 replies; 30+ messages in thread
From: Johannes Schindelin @ 2022-03-09 21:53 UTC (permalink / raw)
To: Tao Klerks via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler,
Tao Klerks, Tao Klerks, Tao Klerks
Hi Tao,
On Wed, 2 Mar 2022, Tao Klerks via GitGitGadget wrote:
> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
> index a0c123b0a77..ca90ee805e7 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -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...
> @@ -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).
> @@ -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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests
2022-03-02 6:05 ` [PATCH v3 " Tao Klerks via GitGitGadget
2022-03-02 6:05 ` [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories Tao Klerks via GitGitGadget
2022-03-02 6:05 ` [PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing Tao Klerks via GitGitGadget
@ 2022-03-05 4:24 ` Tao Klerks
2022-03-06 21:57 ` Junio C Hamano
2 siblings, 1 reply; 30+ messages in thread
From: Tao Klerks @ 2022-03-05 4:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler
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)
On Wed, Mar 2, 2022, 07:05 Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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 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 <tao@klerks.biz>
>
> ## 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests
2022-03-05 4:24 ` [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests Tao Klerks
@ 2022-03-06 21:57 ` Junio C Hamano
2022-03-07 5:37 ` Tao Klerks
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2022-03-06 21:57 UTC (permalink / raw)
To: Tao Klerks; +Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler
Tao Klerks <tao@klerks.biz> writes:
> 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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests
2022-03-06 21:57 ` Junio C Hamano
@ 2022-03-07 5:37 ` Tao Klerks
2022-03-07 18:15 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Tao Klerks @ 2022-03-07 5:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler
Eek, my apologies. I will remember to check that!
On Sun, Mar 6, 2022 at 10:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > 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?
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests
2022-03-07 5:37 ` Tao Klerks
@ 2022-03-07 18:15 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2022-03-07 18:15 UTC (permalink / raw)
To: Tao Klerks; +Cc: git, Ævar Arnfjörð Bjarmason, Jeff Hostetler
Tao Klerks <tao@klerks.biz> writes:
> On Sun, Mar 6, 2022 at 10:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Tao Klerks <tao@klerks.biz> writes:
>>
>> > 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!
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.
^ permalink raw reply [flat|nested] 30+ messages in thread