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

[Derrick Stolee <stolee@gmail.com>]

On 9/8/2020 2:29 AM, SZEDER Gábor wrote:
> On Fri, Sep 04, 2020 at 03:42:04PM +0000, Derrick Stolee via GitGitGadget wrote:
>> +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.

Sounds like you have found the issue below.

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

Thanks.

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

Thanks for the help here. I'll fix this in the next version.

-Stolee