Re: [PATCH 5/7] maintenance: add start/stop subcommands

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Fri, Sep 04, 2020 at 03:42:04PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> Add new subcommands to 'git maintenance' that start or stop background
> maintenance using 'cron', when available. This integration is as simple
> as I could make it, barring some implementation complications.
> 
> The schedule is laid out as follows:
> 
>   0 1-23 * * *   $cmd maintenance run --schedule=hourly
>   0 0    * * 1-6 $cmd maintenance run --schedule=daily
>   0 0    * * 0   $cmd maintenance run --schedule=weekly
> 
> where $cmd is a properly-qualified 'git for-each-repo' execution:
> 
> $cmd=$path/git --exec-path=$path for-each-repo --config=maintenance.repo
> 
> where $path points to the location of the Git executable running 'git
> maintenance start'. This is critical for systems with multiple versions
> of Git. Specifically, macOS has a system version at '/usr/bin/git' while
> the version that users can install resides at '/usr/local/bin/git'
> (symlinked to '/usr/local/libexec/git-core/git'). This will also use
> your locally-built version if you build and run this in your development
> environment without installing first.
> 
> This conditional schedule avoids having cron launch multiple 'git
> for-each-repo' commands in parallel. Such parallel commands would likely
> lead to the 'hourly' and 'daily' tasks competing over the object
> database lock. This could lead to to some tasks never being run! Since
> the --schedule=<frequency> argument will run all tasks with _at least_
> the given frequency, the daily runs will also run the hourly tasks.
> Similarly, the weekly runs will also run the daily and hourly tasks.
> 
> The GIT_TEST_CRONTAB environment variable is not intended for users to
> edit, but instead as a way to mock the 'crontab [-l]' command. This
> variable is set in test-lib.sh to avoid a future test from accidentally
> running anything with the cron integration from modifying the user's
> schedule. We use GIT_TEST_CRONTAB='test-tool crontab <file>' in our
> tests to check how the schedule is modified in 'git maintenance
> (start|stop)' commands.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---


> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 272d1605d2..8803fcf621 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -319,4 +319,32 @@ test_expect_success 'register and unregister' '
>  	test_cmp before actual
>  '
>  
> +test_expect_success 'start from empty cron table' '
> +	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&

This command hangs when run on Travis CI's s390x arch.  Now, Travis
CI's multi-arch support is labelled as an alpha feature and isn't
exactly bug free, so Cc-ing Adrian and Todd, who reported and tested
big-endian issues and fixes in the past, in the hope that they can
confirm.

> +
> +	# start registers the repo
> +	git config --get --global maintenance.repo "$(pwd)" &&
> +
> +	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt &&
> +	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt &&
> +	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
> +'
> +
> +test_expect_success 'stop from existing schedule' '
> +	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop &&
> +
> +	# stop does not unregister the repo
> +	git config --get --global maintenance.repo "$(pwd)" &&
> +
> +	# Operation is idempotent
> +	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop &&
> +	test_must_be_empty cron.txt
> +'
> +
> +test_expect_success 'start preserves existing schedule' '
> +	echo "Important information!" >cron.txt &&
> +	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
> +	grep "Important information!" cron.txt
> +'
> +
>  test_done


> diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c
> new file mode 100644
> index 0000000000..f5db6319c6
> --- /dev/null
> +++ b/t/helper/test-crontab.c
> @@ -0,0 +1,35 @@
> +#include "test-tool.h"
> +#include "cache.h"
> +
> +/*
> + * Usage: test-tool cron <file> [-l]
> + *
> + * If -l is specified, then write the contents of <file> to stdou.

s/stdou/stdout/

> + * Otherwise, write from stdin into <file>.
> + */
> +int cmd__crontab(int argc, const char **argv)
> +{
> +	char a;

So 'a' is a char...

> +	FILE *from, *to;
> +
> +	if (argc == 3 && !strcmp(argv[2], "-l")) {
> +		from = fopen(argv[1], "r");
> +		if (!from)
> +			return 0;
> +		to = stdout;
> +	} else if (argc == 2) {
> +		from = stdin;
> +		to = fopen(argv[1], "w");
> +	} else
> +		return error("unknown arguments");
> +
> +	while ((a = fgetc(from)) != EOF)

fgetc() returns an int, which is assigned to a char, which is then
compared to whatever EOF might be on the platform.  Apparently this
casting and comparison doesn't work as expected on s390x (I haven't
even tried to think it through...), and instead of detecting EOF and
exiting we end up in an endless loop writing 0xff bytes to 'cron.txt',
while 'git maintenance start' in vain waits for 'test-crontab' to
exit.

Changing the type of 'a' to int fixes this issue, and all these tests
pass.

> +		fputc(a, to);
> +
> +	if (argc == 3)
> +		fclose(from);
> +	else
> +		fclose(to);
> +
> +	return 0;
> +}