All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 01/20] write_index_as_tree: cleanup tempfile on error
Date: Tue, 5 Sep 2017 08:14:07 -0400	[thread overview]
Message-ID: <20170905121407.4hom32etywlzaa26@sigill.intra.peff.net> (raw)
In-Reply-To: <20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net>

If we failed to write our new index file, we rollback our
lockfile to remove the temporary index. But if we fail
before we even get to the write step (because reading the
old index failed), we leave the lockfile in place, which
makes no sense.

In practice this hasn't been a big deal because failing at
write_index_as_tree() typically results in the whole program
exiting (and thus the tempfile handler kicking in and
cleaning up the files). But this function should
consistently take responsibility for the resources it
allocates.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache-tree.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 2440d1dc89..2690113a6a 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -604,6 +604,7 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 {
 	int entries, was_valid, newfd;
 	struct lock_file *lock_file;
+	int ret = 0;
 
 	/*
 	 * We can't free this memory, it becomes part of a linked list
@@ -614,8 +615,10 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 	newfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);
 
 	entries = read_index_from(index_state, index_path);
-	if (entries < 0)
-		return WRITE_TREE_UNREADABLE_INDEX;
+	if (entries < 0) {
+		ret = WRITE_TREE_UNREADABLE_INDEX;
+		goto out;
+	}
 	if (flags & WRITE_TREE_IGNORE_CACHE_TREE)
 		cache_tree_free(&index_state->cache_tree);
 
@@ -624,8 +627,10 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 
 	was_valid = cache_tree_fully_valid(index_state->cache_tree);
 	if (!was_valid) {
-		if (cache_tree_update(index_state, flags) < 0)
-			return WRITE_TREE_UNMERGED_INDEX;
+		if (cache_tree_update(index_state, flags) < 0) {
+			ret = WRITE_TREE_UNMERGED_INDEX;
+			goto out;
+		}
 		if (0 <= newfd) {
 			if (!write_locked_index(index_state, lock_file, COMMIT_LOCK))
 				newfd = -1;
@@ -641,17 +646,19 @@ int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, co
 	if (prefix) {
 		struct cache_tree *subtree;
 		subtree = cache_tree_find(index_state->cache_tree, prefix);
-		if (!subtree)
-			return WRITE_TREE_PREFIX_ERROR;
+		if (!subtree) {
+			ret = WRITE_TREE_PREFIX_ERROR;
+			goto out;
+		}
 		hashcpy(sha1, subtree->oid.hash);
 	}
 	else
 		hashcpy(sha1, index_state->cache_tree->oid.hash);
 
+out:
 	if (0 <= newfd)
 		rollback_lock_file(lock_file);
-
-	return 0;
+	return ret;
 }
 
 int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
-- 
2.14.1.721.gc5bc1565f1


  reply	other threads:[~2017-09-05 12:14 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-05 12:13 [PATCH 0/20] allowing struct lock_file to be deleted Jeff King
2017-09-05 12:14 ` Jeff King [this message]
2017-09-05 12:14 ` [PATCH 02/20] setup_temporary_shallow: avoid using inactive tempfile Jeff King
2017-09-05 12:14 ` [PATCH 03/20] setup_temporary_shallow: move tempfile struct into function Jeff King
2017-09-05 12:14 ` [PATCH 04/20] verify_signed_buffer: prefer close_tempfile() to close() Jeff King
2017-09-05 12:14 ` [PATCH 05/20] always check return value of close_tempfile Jeff King
2017-09-05 12:14 ` [PATCH 06/20] tempfile: do not delete tempfile on failed close Jeff King
2017-09-05 12:14 ` [PATCH 07/20] lockfile: do not rollback lock " Jeff King
2017-09-05 12:14 ` [PATCH 08/20] tempfile: prefer is_tempfile_active to bare access Jeff King
2017-09-05 12:14 ` [PATCH 09/20] tempfile: handle NULL tempfile pointers gracefully Jeff King
2017-09-05 12:14 ` [PATCH 10/20] tempfile: replace die("BUG") with BUG() Jeff King
2017-09-05 12:14 ` [PATCH 11/20] tempfile: factor out activation Jeff King
2017-09-05 12:14 ` [PATCH 12/20] tempfile: factor out deactivation Jeff King
2017-09-05 12:14 ` [PATCH 13/20] tempfile: robustify cleanup handler Jeff King
2017-09-05 12:14 ` [PATCH 14/20] tempfile: release deactivated strbufs instead of resetting Jeff King
2017-09-05 12:15 ` [PATCH 15/20] tempfile: use list.h for linked list Jeff King
2017-09-05 12:15 ` [PATCH 16/20] tempfile: remove deactivated list entries Jeff King
2017-09-05 12:15 ` [PATCH 17/20] tempfile: auto-allocate tempfiles on heap Jeff King
2017-09-05 12:15 ` [PATCH 18/20] lockfile: update lifetime requirements in documentation Jeff King
2017-09-05 12:15 ` [PATCH 19/20] ref_lock: stop leaking lock_files Jeff King
2017-09-05 12:15 ` [PATCH 20/20] stop leaking lock structs in some simple cases Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170905121407.4hom32etywlzaa26@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.