[PATCH 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[PATCH 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[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.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
-- 
gitgitgadget

[PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Tao Klerks via GitGitGadget]

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
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.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
 compat/mingw.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 03af369b2b9..2284ea90511 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -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 &&
-- 
gitgitgadget

Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Jeff Hostetler]

On 2/28/22 4:40 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
> 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.
> 
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
>   compat/mingw.c | 33 ++++++++++++++++++++++++++++-----
>   1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 03af369b2b9..2284ea90511 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -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.

> +
>   	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.

> +		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.

> +					   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);

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.

>   
>   revert_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

Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

>> +	HANDLE osfilehandle;
>
> I'd be more comfortable initializing this variable to
> INVALID_HANDLE_VALUE.

Both directories/files branches assign, so it is not needed.

>> +	if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
>> +		fh = 0;
>
> and here initializing fh = -1.

This does make it safer.

> And then this becomes:
>
> 	if (fh != -1)
> 		close(fh)
> 	else if (osfilehandle != INVALID_HANDLE_VALUE)
> 		Closehandle(osfilehandle);

Exactly.

Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Tao Klerks]

On Mon, Feb 28, 2022 at 4:27 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
>
>
>
> On 2/28/22 4:40 AM, Tao Klerks via GitGitGadget wrote:

> > +     HANDLE osfilehandle;
>
> I'd be more comfortable initializing this variable to
> INVALID_HANDLE_VALUE.
>

Makes sense, thanks. (but less relevant after switch to CreateFileW)

> > +             fh = 0;
>
> and here initializing fh = -1.

Makes sense, thanks. (but overshadowed by switch to CreateFileW)

> > +             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?

> > +     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.

Perfect, thx.

>
> >
> >   revert_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.
>

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

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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
> 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?

> 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.

OK.

> @@ -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.

>  	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) {

Excellent.  We can reuse existing attrs without any extra calls to
introduce the new codepath to deal with directories.

> +		fh = 0;

This should be

		fh = -1;

instead.  More on this later.

> +		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).

> +	} 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.

> @@ -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.

>  		errno = 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 ...

>  revert_attrs:
>  	if (attrs != INVALID_FILE_ATTRIBUTES &&

Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Tao Klerks]

On Mon, Feb 28, 2022 at 11:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> 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"?

> > +             fh = 0;
>
> This should be
>
>                 fh = -1;
>
> instead.  More on this later.
>

Makes sense, but obviated by full switch to CreateFileW().

> > +     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 ...
>

Makes sense, but obviated by full switch to CreateFileW().

[PATCH 2/2] t7063: mtime-mangling instead of delays in untracked cache testing

[Tao Klerks via GitGitGadget]

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 | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index a0c123b0a77..039b4de8d25 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -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 &&
-- 
gitgitgadget

Re: [PATCH 2/2] t7063: mtime-mangling instead of delays in untracked cache testing

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

OK.

> Don't update test cases that delay for other reasons, for now at
> least (4 seconds).

Sad but one step at a time.

> +chmmtime_worktree_root () {

Not "chmtime_worktree_root"?  Is doubled-m intended?

> +	# 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?

> +}
> +
>  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 2/2] t7063: mtime-mangling instead of delays in untracked cache testing

[Tao Klerks]

On Mon, Feb 28, 2022 at 11:19 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +chmmtime_worktree_root () {
>
> Not "chmtime_worktree_root"?  Is doubled-m intended?
>

Oops, typo, fixing.

> > +     # 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?

Ah, lovely, thank you!

Re: [PATCH 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 28 2022, Tao Klerks via GitGitGadget 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.

Thanks, it will be nice to have that test run a bit faster in wallclock
terms.

> 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.

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 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[Jeff Hostetler]

On 2/28/22 4:40 AM, Tao Klerks via GitGitGadget 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                    | 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
> 

Thanks for working on this.  It would be nice to be able
to drop that commit (read: hack) from my fsmonitor series.

Jeff

[PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[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.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

[PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Tao Klerks via GitGitGadget]

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

Re: [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Jeff Hostetler]

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 &&
> 

Re: [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Tao Klerks]

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!

[PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing

[Tao Klerks via GitGitGadget]

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

Re: [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing

[Junio C Hamano]

"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.

Re: [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing

[Tao Klerks]

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.

Re: [PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[Tao Klerks]

> 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

Re: [PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[Junio C Hamano]

"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.

[PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[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 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

[PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Tao Klerks via GitGitGadget]

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

Re: [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

[Johannes Schindelin]

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

[PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing

[Tao Klerks via GitGitGadget]

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

Re: [PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing

[Johannes Schindelin]

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

Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[Tao Klerks]

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

Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[Junio C Hamano]

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?

Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[Tao Klerks]

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?
>

Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests

[Junio C Hamano]

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.