Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 3, 2025, 22:29 UTC
- Message-ID
- <xmqqed1jo7k6.fsf@gitster.g>
- In-Reply-To
- <20250103195942.GA3212696@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> 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 tries = 5;
>
> + while (tries-- > 0) {
> + int ret = 0;
> + if (object_creation_mode != OBJECT_CREATION_USES_RENAMES) {Platforms that do not want hardlinks set CREATION_USES_RENAMES flag. We skip this block on them.
> + if (!link(tmpfile, filename)) {
> + unlink_or_warn(tmpfile);
> + break;If we successfully hardlink, we remove the temporary and happily leave the retry loop.
Show 17 quoted lines
> + }
> + ret = errno;
> + }
>
> + /*
> + * 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;Either we skipped the hardlink step (then ret==0), or tried to hardlink and saw EEXIST, we come here and try renaming.
> + if (!stat(filename, &st)) > + ret = EEXIST;
We check if the destination already exists, and do the same thing as the case where hardlink failed due to EEXIST, if that is the case.
Otherwise, any failure of stat() we assume we are free to rename the new thing there. It is a bit strange that we do not check ENOENT here.
The reason why this stat() fails is not all that interesting, because it is subject to a TOCTOU race, and the case we are more interested in is when this stat() succeeds, which positively tells us that there is something at that path (hence we do not have to trigger a failure from rename() to notice a potential collision).
Wait, what if stat() succeeds and !S_ISREG(st.st_mode)? But that's the original code for "Coda hack", and that is not something we are trying to "fix" at this point (yet).
> + else if (!rename(tmpfile, filename)) > + break;
If we manage to rename(), we happily leave the retry loop. Unlike the hardlink case, there is no tmpfile to unlink. Good.
> + else > + ret = errno;
Here errno is guaranteed from the failure of rename(). If the destination was created immediately after we got ENOENT from stat(), it is likely rename() gave us EEXIST, which we would check for collission and retry.
Show 8 quoted lines
> + }
>
> + /* 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);Sensible.
Show 15 quoted lines
> }
> +
> + 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;
> }
> +
> + if (ret != CHECK_COLLISION_DEST_VANISHED)
> + return -1; /* check_collision() already complained */
> +
> + /* loop again to retry vanished destination */OK.
Show 5 quoted lines
> }
>
> + if (tries < 0)
> + return error(_("unable to write repeatedly vanishing file %s"), filename);
> +OK.
> if (adjust_shared_perm(filename))
> return error(_("unable to set permission to '%s'"), filename);
> return 0;