Re: [PATCH 3/4] repack: use tempfiles for signal cleanup

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

Jeff King <peff@peff.net> writes:

> This patch also fixes another small problem. We only hook signals, and
> don't set up an atexit handler. So if we see an error that causes us to
> die(), we'll leave the .tmp-* files in place. But since the tempfile API
> handles this for us, this is now fixed for free. The new test covers
> this by stimulating a failure of pack-objects when generating a cruft
> pack. Before this patch, the .tmp-* file for the main pack would have
> been left, but now we correctly clean it up.
>
> Reported-by: Jan Pokorný <poki@fnusa.cz>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/repack.c  | 17 ++++-------------
>  t/t7700-repack.sh |  8 ++++++++
>  2 files changed, 12 insertions(+), 13 deletions(-)

Nice.  

>  struct generated_pack_data {
> -	char exts[ARRAY_SIZE(exts)];
> +	struct tempfile *tempfiles[ARRAY_SIZE(exts)];
> ...
> -		data->exts[i] = 1;
> +		data->tempfiles[i] = register_tempfile(path.buf);

OK.

>  	}
>  
>  	strbuf_release(&path);
> @@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		split_pack_geometry(geometry, geometric_factor);
>  	}
>  
> -	sigchain_push_common(remove_pack_on_signal);
> -
>  	prepare_pack_objects(&cmd, &po_args);
>  
>  	show_progress = !po_args.quiet && isatty(2);
> @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  			fname_old = mkpathdup("%s-%s%s",
>  					packtmp, item->string, exts[ext].name);
>  
> -			if (data->exts[ext]) {
> +			if (data->tempfiles[ext]) {
>  				struct stat statbuffer;
>  				if (!stat(fname_old, &statbuffer)) {
>  					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
>  					chmod(fname_old, statbuffer.st_mode);
>  				}
>  
> -				if (rename(fname_old, fname))
> +				if (rename_tempfile(&data->tempfiles[ext], fname))
>  					die_errno(_("renaming '%s' failed"), fname_old);

It now got a bit confusing that we have 'fname', 'fname_old', and
the tempfile.  The path.buf used as the argument to register_tempfile()
matches what is used to compute fname_old above.  I wonder if tempfile
API does not give us that name so that we can stop using fname_old here?