Re: [PATCH 3/4] repack: use tempfiles for signal cleanup
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 21, 2022, 22:30 UTC
- Message-ID
- <xmqqmt9oizcq.fsf@gitster.g>
- In-Reply-To
- <Y1MTJz3wy5xDEPEH@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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.
Show 6 quoted lines
> 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.
Show 27 quoted lines
> }
>
> 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?