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.
For fun, here's a version without any goto's in it, that should behave
the same. But it would be very easy to miss a case. So I don't know if
it is worth the regression risk, and I don't blame you if you delete
this message without looking carefully. ;)
Diff is kind of hard to read, so you may want to apply (on top of your
patches) and just look at the post-image.
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
