Re: [PATCH v3 1/4] test-lib: introduce API for verifying file mtime

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2022, 23:55 UTC
Message-ID: <xmqqmtk8a083.fsf@gitster.g>
In-Reply-To: <e6301e9d770bc7b6a2a3eeddcaf4e0123a0b23ab.1641508499.git.gitgitgadget@gmail.com>

"Marc Strapetz via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

> +# Set a fixed "magic" mtime to the given file,
> +# with an optional increment specified as second argument.
> +# Use in combination with test_is_magic_mtime.
> +test_set_magic_mtime () {
> +	# We are using 1234567890 because it's a common timestamp used in
> +	# various tests. It represents date 2009-02-13 which should be safe
> +	# to use as long as the filetime clock is reasonably accurate.

In the original context of "setting an ancient time, and detect filesystem modification by noticing that the timestamp has or has not changed", such an ancient timestamp "should be safe to use", but if you expose it to more general audience, the context of their use must be in line with your intended use to be safe.

	# Set mtime to mid February 2009, before we run an operation
	# that may or may not touch the file.  If the file was
	# touched, its timestamp will not accidentally have such an
	# old timestamp, as long as your filesystem clock is
	# reasonably correct.

perhaps?

Show 5 quoted lines

> +	local inc=${2:-0} &&
> +	local mtime=$((1234567890 + $inc)) &&
> +	test-tool chmtime =$mtime $1 &&
> +	test_is_magic_mtime $1 $inc
> +}

Also as a helper function in the library that is (hopefully) useful to many other callers, make sure you got your quoting correct.

There is no rule that you must use filenames without SP in it in your tests, for example, so make sure "$1" above are quoted. The same applies to the next function.

Show 12 quoted lines

> +# Test whether the given file has the "magic" mtime set,
> +# with an optional increment specified as second argument.
> +# Use in combination with test_set_magic_mtime.
> +test_is_magic_mtime () {
> +	local inc=${2:-0} &&
> +	local mtime=$((1234567890 + $inc)) &&
> +	echo $mtime >.git/test-mtime-expect &&
> +	test-tool chmtime --get $1 >.git/test-mtime-actual &&
> +	test_cmp .git/test-mtime-expect .git/test-mtime-actual
> +	local ret=$?
> +	rm .git/test-mtime-expect
> +	rm .git/test-mtime-actual

Use "rm -f" here? Otherwise, if the main test failed somewhere before it runs test_cmp, we'd see an error from an attempt to remove a file that does not exist.

> +	return $ret
> +}

Other than that, quite nicely done (both these two functions and its users).

Thanks.