Re: [PATCH 05/20] rename_tmp_log(): use raceproof_create_file()

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

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Besides shortening the code, this saves an unnecessary call to
> safe_create_leading_directories_const() in almost all cases.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs/files-backend.c | 76 ++++++++++++++++++++++------------------------------
>  1 file changed, 32 insertions(+), 44 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a549942..e5f964c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2400,55 +2400,43 @@ out:
>   */
>  #define TMP_RENAMED_LOG  "logs/refs/.tmp-renamed-log"
>  
> +static int rename_tmp_log_callback(const char *path, void *cb)
> +{
> +	int *true_errno = cb;
> +
> +	if (rename(git_path(TMP_RENAMED_LOG), path)) {
> +		/*
> +		 * rename(a, b) when b is an existing directory ought
> +		 * to result in ISDIR, but Solaris 5.8 gives ENOTDIR.
> +		 * Sheesh. Record the true errno for error reporting,
> +		 * but report EISDIR to raceproof_create_file() so
> +		 * that it knows to retry.
> +		 */
> +		*true_errno = errno;
> +		if (errno==ENOTDIR)
> +			errno = EISDIR;

Style: SP on both sides of a binary operator.

More importantly, is ENOTDIR expected only on a buggy platform?  

[ENOTDIR]
    A component of either path prefix names an existing file that is
    neither a directory nor a symbolic link to a directory; or the old
    argument names a directory and the new argument names a
    non-directory file; or the old argument contains at least one non-
    <slash> character and ends with one or more trailing <slash>
    characters and the last pathname component names an existing file
    that is neither a directory nor a symbolic link to a directory; or
    the old argument names an existing non-directory file and the new
    argument names a nonexistent file, contains at least one non-
    <slash> character, and ends with one or more trailing <slash>
    characters; or the new argument names an existing non-directory
    file, contains at least one non- <slash> character, and ends with
    one or more trailing <slash> characters.

i.e. when a leading component of "path" or TMP_RENAMED_LOG is an
existing non-directory, we could get ENOTDIR on a valid system.

If another instance of Git created a file A/B when this process is
trying to rename the temporary thing to its final location A/B/C,
isn't that the errno we would see here?

[EISDIR]
    The new argument points to a directory and the old argument
    points to a file that is not a directory.

Puzzled...

> +		return -1;
> +	} else {
> +		return 0;
> +	}
> +}
> +
>  static int rename_tmp_log(const char *newrefname)
>  {
> -	int attempts_remaining = 4;
> -	struct strbuf path = STRBUF_INIT;
> -	int ret = -1;
> +	char *path = git_pathdup("logs/%s", newrefname);
> +	int true_errno;
> +	int ret;
>  
> - retry:
> -	strbuf_reset(&path);
> -	strbuf_git_path(&path, "logs/%s", newrefname);
> -	switch (safe_create_leading_directories_const(path.buf)) {
> -	case SCLD_OK:
> -		break; /* success */
> -	case SCLD_VANISHED:
> -		if (--attempts_remaining > 0)
> -			goto retry;
> -		/* fall through */
> -	default:
> -		error("unable to create directory for %s", newrefname);
> -		goto out;
> -	}
> -
> -	if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
> -		if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {
> -			/*
> -			 * rename(a, b) when b is an existing
> -			 * directory ought to result in ISDIR, but
> -			 * Solaris 5.8 gives ENOTDIR.  Sheesh.
> -			 */
> -			if (remove_empty_directories(&path)) {
> -				error("Directory not empty: logs/%s", newrefname);
> -				goto out;
> -			}
> -			goto retry;
> -		} else if (errno == ENOENT && --attempts_remaining > 0) {
> -			/*
> -			 * Maybe another process just deleted one of
> -			 * the directories in the path to newrefname.
> -			 * Try again from the beginning.
> -			 */
> -			goto retry;
> -		} else {
> +	ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
> +	if (ret) {
> +		if (true_errno==EISDIR)
> +			error("Directory not empty: %s", path);
> +		else
>  			error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
> -				newrefname, strerror(errno));
> -			goto out;
> -		}
> +				newrefname, strerror(true_errno));
>  	}
> -	ret = 0;
> -out:
> -	strbuf_release(&path);
> +
> +	free(path);
>  	return ret;
>  }