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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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.

> +			}
> +			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.

> +		}
>  
> +		/* 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.

>  		}
> +
> +		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.

>  	}
>  
> +	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;