Merge branch 'tb/commit-graph-use-tempfile'

"git update-server-info" and "git commit-graph --write" have been
updated to use the tempfile API to avoid leaving cruft after
failing.

* tb/commit-graph-use-tempfile:
  server-info.c: remove temporary info files on exit
  commit-graph.c: remove temporary graph layers on exit

Author: gitster

commit-graph.c

@@ -2002,8 +2002,8 @@ static int write_graph_chunk_base(struct hashfile *f,
 static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 {
 	uint32_t i;
-	int fd;
 	struct hashfile *f;
+	struct tempfile *graph_layer; /* when ctx->split is non-zero */
 	struct lock_file lk = LOCK_INIT;
 	const unsigned hashsz = the_hash_algo->rawsz;
 	struct strbuf progress_title = STRBUF_INIT;
@@ -2035,24 +2035,23 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 					       LOCK_DIE_ON_ERROR, 0444);
 		free(lock_name);
 
-		fd = git_mkstemp_mode(ctx->graph_name, 0444);
-		if (fd < 0) {
+		graph_layer = mks_tempfile_m(ctx->graph_name, 0444);
+		if (!graph_layer) {
 			error(_("unable to create temporary graph layer"));
 			return -1;
 		}
 
-		if (adjust_shared_perm(ctx->graph_name)) {
+		if (adjust_shared_perm(get_tempfile_path(graph_layer))) {
 			error(_("unable to adjust shared permissions for '%s'"),
-			      ctx->graph_name);
+			      get_tempfile_path(graph_layer));
 			return -1;
 		}
 
-		f = hashfd(fd, ctx->graph_name);
+		f = hashfd(get_tempfile_fd(graph_layer), get_tempfile_path(graph_layer));
 	} else {
 		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
 					       LOCK_DIE_ON_ERROR, 0444);
-		fd = get_lock_file_fd(&lk);
-		f = hashfd(fd, get_lock_file_path(&lk));
+		f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 	}
 
 	cf = init_chunkfile(f);
@@ -2133,8 +2132,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 		char *final_graph_name;
 		int result;
 
-		close(fd);
-
 		if (!chainf) {
 			error(_("unable to open commit-graph chain file"));
 			return -1;
@@ -2169,7 +2166,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 		free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
 		ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
 
-		result = rename(ctx->graph_name, final_graph_name);
+		result = rename_tempfile(&graph_layer, final_graph_name);
 
 		for (i = 0; i < ctx->num_commit_graphs_after; i++)
 			fprintf(get_lock_file_fp(&lk), "%s\n", ctx->commit_graph_hash_after[i]);

server-info.c

@@ -13,6 +13,7 @@
 #include "object-store-ll.h"
 #include "server-info.h"
 #include "strbuf.h"
+#include "tempfile.h"
 
 struct update_info_ctx {
 	FILE *cur_fp;
@@ -75,9 +76,8 @@ static int update_info_file(char *path,
 			int force)
 {
 	char *tmp = mkpathdup("%s_XXXXXX", path);
+	struct tempfile *f = NULL;
 	int ret = -1;
-	int fd = -1;
-	FILE *to_close;
 	struct update_info_ctx uic = {
 		.cur_fp = NULL,
 		.old_fp = NULL,
@@ -86,13 +86,12 @@ static int update_info_file(char *path,
 	};
 
 	safe_create_leading_directories(path);
-	fd = git_mkstemp_mode(tmp, 0666);
-	if (fd < 0)
+	f = mks_tempfile_m(tmp, 0666);
+	if (!f)
 		goto out;
-	to_close = uic.cur_fp = fdopen(fd, "w");
+	uic.cur_fp = fdopen_tempfile(f, "w");
 	if (!uic.cur_fp)
 		goto out;
-	fd = -1;
 
 	/* no problem on ENOENT and old_fp == NULL, it's stale, now */
 	if (!force)
@@ -121,27 +120,22 @@ static int update_info_file(char *path,
 	}
 
 	uic.cur_fp = NULL;
-	if (fclose(to_close))
-		goto out;
 
 	if (uic_is_stale(&uic)) {
-		if (adjust_shared_perm(tmp) < 0)
+		if (adjust_shared_perm(get_tempfile_path(f)) < 0)
 			goto out;
-		if (rename(tmp, path) < 0)
+		if (rename_tempfile(&f, path) < 0)
 			goto out;
 	} else {
-		unlink(tmp);
+		delete_tempfile(&f);
 	}
 	ret = 0;
 
 out:
 	if (ret) {
 		error_errno("unable to update %s", path);
-		if (uic.cur_fp)
-			fclose(uic.cur_fp);
-		else if (fd >= 0)
-			close(fd);
-		unlink(tmp);
+		if (f)
+			delete_tempfile(&f);
 	}
 	free(tmp);
 	if (uic.old_fp)

t/t5324-split-commit-graph.sh

@@ -13,7 +13,8 @@ test_expect_success 'setup repo' '
 	git init &&
 	git config core.commitGraph true &&
 	git config gc.writeCommitGraph false &&
-	infodir=".git/objects/info" &&
+	objdir=".git/objects" &&
+	infodir="$objdir/info" &&
 	graphdir="$infodir/commit-graphs" &&
 	test_oid_cache <<-EOM
 	shallow sha1:2132
@@ -718,4 +719,27 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl
 	)
 '
 
+test_expect_success 'temporary graph layer is discarded upon failure' '
+	git init layer-discard &&
+	(
+		cd layer-discard &&
+
+		test_commit A &&
+		test_commit B &&
+
+		# Intentionally remove commit "A" from the object store
+		# so that the commit-graph machinery fails to parse the
+		# parents of "B".
+		#
+		# This takes place after the commit-graph machinery has
+		# initialized a new temporary file to store the contents
+		# of the new graph layer, so will allow us to ensure
+		# that the temporary file is discarded upon failure.
+		rm $objdir/$(test_oid_to_path $(git rev-parse HEAD^)) &&
+
+		test_must_fail git commit-graph write --reachable --split &&
+		test_dir_is_empty $graphdir
+	)
+'
+
 test_done