Re: [PATCH 05/20] rename_tmp_log(): use raceproof_create_file()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 17, 2016, 20:53 UTC
- Message-ID
- <xmqqmvqzhxdf.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <a19192cb3219de9304af7d9b97be45a5e6f0895f.1455626201.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 31 quoted lines
> 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...
Show 68 quoted lines
> + 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;
> }