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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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