Re: [PATCH] gc: use temporary file for editing crontab

[Derrick Stolee <derrickstolee@github.com>]

On 8/23/2022 5:12 AM, Johannes Schindelin wrote:
> Hi brian,
> 
> On Tue, 23 Aug 2022, brian m. carlson wrote:

>> +	tmpedit = mks_tempfile_t(".git_cron_edit_tmpXXXXXX");
>> +	if (!tmpedit)
>> +		return error(_("failed to create crontab temporary file"));
> 
> It might make sense to use the same `goto out;` pattern here, to make it
> easier to reason about the early exit even six years from now.
> 
> We do not even have to guard the `close_tempfile_gently()` behind an `if
> (tempfile)` conditional because that function handles `NULL` parameters
> gently.

I don't think this is hard to reason about. It might mean that we
need to change this if block in the future to use 'goto out', if we
added another resource initialization before this one. That "future
need" is the only thing making me lean towards using the goto, but
we are just as likely to be in YAGNI territory here.
 
>> +	cron_in = fdopen_tempfile(tmpedit, "w");
>> +	if (!cron_in) {
>> +		result = error(_("failed to open temporary file"));
>> +		goto out;
>> +	}
>> +
>>  	/*
>>  	 * Read from the .lock file, filtering out the old
>>  	 * schedule while appending the new schedule.
>> @@ -2086,19 +2096,6 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>>  	cron_list = fdopen(fd, "r");
>>  	rewind(cron_list);
>>
>> -	strvec_split(&crontab_edit.args, cmd);
>> -	crontab_edit.in = -1;
>> -	crontab_edit.git_cmd = 0;
>> -
>> -	if (start_command(&crontab_edit))
>> -		return error(_("failed to run 'crontab'; your system might not support 'cron'"));
>> -
>> -	cron_in = fdopen(crontab_edit.in, "w");
>> -	if (!cron_in) {
>> -		result = error(_("failed to open stdin of 'crontab'"));
>> -		goto done_editing;
>> -	}
>> -
>>  	while (!strbuf_getline_lf(&line, cron_list)) {
>>  		if (!in_old_region && !strcmp(line.buf, BEGIN_LINE))
>>  			in_old_region = 1;
>> @@ -2132,14 +2129,22 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>>  	}
>>
>>  	fflush(cron_in);
>> -	fclose(cron_in);
>> -	close(crontab_edit.in);
> 
> This worries me a bit. I could imagine that keeping the file open and then
> expecting a spawned process to read its stdin from that file won't work on
> Windows.

This is focused only on the cron integration, which is not used on Windows,
so I'm not worried about that.

I was initially worried that we lost the fclose(cron_in), but of course it
is handled by the close_tempfile_gently() at the end.

> In any case, I would consider it the correct thing to do to close
> the temp file here. In other words, I would like to move the
> `close_tempfile_gently()` call to this location.
> 
>>
>> -done_editing:
>> +	strvec_split(&crontab_edit.args, cmd);
>> +	strvec_push(&crontab_edit.args, get_tempfile_path(tmpedit));
>> +	crontab_edit.git_cmd = 0;
>> +
>> +	if (start_command(&crontab_edit)) {
>> +		result = error(_("failed to run 'crontab'; your system might not support 'cron'"));
>> +		goto out;
>> +	}
>> +

Here's the crux of the matter: we are no longer using stdin but
instead passing an argument to point to a file with our desired
schedule. I tested that this worked on my machine, and I'm glad
this use is the POSIX standard.

There is something wrong with this patch: it needs to update
t/helper/test-crontab.c in order to pass t7900-maintenance.sh.

Something like this works for me:

--- >8 ---

diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c
index e7c0137a477..29425430466 100644
--- a/t/helper/test-crontab.c
+++ b/t/helper/test-crontab.c
@@ -17,8 +17,8 @@ int cmd__crontab(int argc, const char **argv)
 		if (!from)
 			return 0;
 		to = stdout;
-	} else if (argc == 2) {
-		from = stdin;
+	} else if (argc == 3) {
+		from = fopen(argv[2], "r");
 		to = fopen(argv[1], "w");
 	} else
 		return error("unknown arguments");

--- >8 ---

>>  	if (finish_command(&crontab_edit))
>>  		result = error(_("'crontab' died"));
>>  	else
>>  		fclose(cron_list);
>> +out:
>> +	close_tempfile_gently(tmpedit);
> 
> Here, I would like to call `delete_tempfile(&tmpedit);` instead. That way,
> the memory is released correctly, the temporary file is deleted, and
> everything is neatly cleaned up.
> 
> The way I read the code, `delete_tempfile(&tmpedit)` would return early if
> `tmpedit == NULL`, and otherwise clean everything up and release the
> memory, so there is no need to guard this call behind an `if (tmpedit)`
> conditional.

While the memory release is nice, I also think it would be good to use
delete_tempfile() so the temporary file is deleted within this method,
not waiting until the end of the process to do that cleanup.

Thanks,
-Stolee