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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2022, 18:03 UTC
Message-ID: <xmqqk0ddr1n1.fsf@gitster.g>
In-Reply-To: <3e3c9c7faace505958aa01ff82bef5fad3204c67.1646127910.git.gitgitgadget@gmail.com>

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

Show 6 quoted lines

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

Show 10 quoted lines

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