Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes

From

Jeff King <peff@peff.net>

Date

Jan 3, 2025, 19:59 UTC

Message-ID

<20250103195942.GA3212696@coredump.intra.peff.net>

In-Reply-To

<20250103194058.GE3208749@coredump.intra.peff.net>

On Fri, Jan 03, 2025 at 02:40:58PM -0500, Jeff King wrote:

> I suspect there's a way to write this as a loop that would be more
> structured, but it would be a bigger refactor. Bonus points if it also
> get rid of the try_rename goto, too. ;)
> 
> I'm OK punting on that, though.

diff --git a/object-file.c b/object-file.c
index 88432cc9c0..923d75a889 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2037,58 +2037,66 @@ int finalize_object_file(const char *tmpfile, const char *filename)
 int finalize_object_file_flags(const char *tmpfile, const char *filename,
 			       enum finalize_object_file_flags flags)
 {
-	int ret;
+	int tries = 5;
 
-retry:
-	ret = 0;
+	while (tries-- > 0) {
+		int ret = 0;
+		if (object_creation_mode != OBJECT_CREATION_USES_RENAMES) {
+			if (!link(tmpfile, filename)) {
+				unlink_or_warn(tmpfile);
+				break;
+			}
+			ret = errno;
+		}
 
-	if (object_creation_mode == OBJECT_CREATION_USES_RENAMES)
-		goto try_rename;
-	else if (link(tmpfile, filename))
-		ret = errno;
-	else
-		unlink_or_warn(tmpfile);
+		/*
+		 * Coda hack - coda doesn't like cross-directory links,
+		 * so we fall back to a rename, which will mean that it
+		 * won't be able to check collisions, but that's not a
+		 * big deal.
+		 *
+		 * The same holds for FAT formatted media.
+		 *
+		 * When this succeeds, we just return.  We have nothing
+		 * left to unlink.
+		 */
+		if (!ret || ret == EEXIST) {
+			struct stat st;
 
-	/*
-	 * Coda hack - coda doesn't like cross-directory links,
-	 * so we fall back to a rename, which will mean that it
-	 * won't be able to check collisions, but that's not a
-	 * big deal.
-	 *
-	 * The same holds for FAT formatted media.
-	 *
-	 * When this succeeds, we just return.  We have nothing
-	 * left to unlink.
-	 */
-	if (ret && ret != EEXIST) {
-		struct stat st;
+			if (!stat(filename, &st))
+				ret = EEXIST;
+			else if (!rename(tmpfile, filename))
+				break;
+			else
+				ret = errno;
+		}
 
-	try_rename:
-		if (!stat(filename, &st))
-			ret = EEXIST;
-		else if (!rename(tmpfile, filename))
-			goto out;
-		else
-			ret = errno;
-	}
-	if (ret) {
+		/* Do not retry most filesystem errors */
 		if (ret != EEXIST) {
 			int saved_errno = errno;
 			unlink_or_warn(tmpfile);
 			errno = saved_errno;
 			return error_errno(_("unable to write file %s"), filename);
 		}
-		if (!(flags & FOF_SKIP_COLLISION_CHECK)) {
-			ret = check_collision(tmpfile, filename);
-			if (ret == CHECK_COLLISION_DEST_VANISHED)
-				goto retry;
-			else if (ret)
-				return -1;
+
+		ret = (flags & FOF_SKIP_COLLISION_CHECK) ? 0 :
+			check_collision(tmpfile, filename);
+
+		if (!ret) {
+			/* Same contents (or we are allowed to assume such). */
+			unlink_or_warn(tmpfile);
+			break;
 		}
-		unlink_or_warn(tmpfile);
+
+		if (ret != CHECK_COLLISION_DEST_VANISHED)
+			return -1; /* check_collision() already complained */
+
+		/* loop again to retry vanished destination */
 	}
 
-out:
+	if (tries < 0)
+		return error(_("unable to write repeatedly vanishing file %s"), filename);
+
 	if (adjust_shared_perm(filename))
 		return error(_("unable to set permission to '%s'"), filename);
 	return 0;

-Peff