git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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?

Previous: Jeff KingNext: Jeff King
Message 16 of 34 in “repack tempfile-cleanup signal deadlock”
  1. 0/4 repack tempfile-cleanup signal deadlockJeff King, Oct 21, 2022
  2. 1/4 repack: convert "names" util bitfield to arrayJeff King, Oct 21, 2022
  3. Junio C HamanoOct 21, 2022
  4. Taylor BlauOct 21, 2022
  5. Jeff KingOct 21, 2022
  6. Junio C HamanoOct 21, 2022
  7. Jeff KingOct 21, 2022
  8. Junio C HamanoOct 21, 2022
  9. Jeff KingOct 22, 2022
  10. 2/4 repack: populate extension bits incrementallyJeff King, Oct 21, 2022
  11. Taylor BlauOct 21, 2022
  12. Jeff KingOct 21, 2022
  13. Taylor BlauOct 21, 2022
  14. Jeff KingOct 21, 2022
  15. 3/4 repack: use tempfiles for signal cleanupJeff King, Oct 21, 2022
  16. Junio C HamanoOct 21, 2022
  17. Jeff KingOct 21, 2022
  18. Taylor BlauOct 21, 2022
  19. Jeff KingOct 22, 2022
  20. Jeff KingOct 22, 2022
  21. 4/4 repack: drop remove_temporary_files()Jeff King, Oct 21, 2022
  22. Taylor BlauOct 21, 2022
  23. 0/5 repack tempfile-cleanup signal deadlockJeff King, Oct 22, 2022
  24. 1/5 repack: convert "names" util bitfield to arrayJeff King, Oct 22, 2022
  25. 2/5 repack: populate extension bits incrementallyJeff King, Oct 22, 2022
  26. 3/5 repack: expand error message for missing pack filesJeff King, Oct 22, 2022
  27. 4/5 repack: use tempfiles for signal cleanupJeff King, Oct 22, 2022
  28. Jeff KingOct 22, 2022
  29. Junio C HamanoOct 23, 2022
  30. Jeff KingOct 23, 2022
  31. Junio C HamanoOct 23, 2022
  32. Jeff KingOct 23, 2022
  33. Junio C HamanoOct 23, 2022
  34. 5/5 repack: drop remove_temporary_files()Jeff King, Oct 22, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.