Re: [PATCH 03/20] raceproof_create_file(): new function

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 17, 2016, 19:38 UTC
Message-ID: <xmqqvb5ni0ul.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <338201e98a16f6c53ed1ee447de10c206f2acc33.1455626201.git.mhagger@alum.mit.edu>

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

> I was thinking about moving this function, along with
> safe_create_leading_directories() and
> safe_create_leading_directories_const(), to a more general module like
> path.c. But it didn't seem worth the code churn.

I think it would be a better longer-term endgame state, but let's not do that within this 20-patch series until it stabilizes.

Show 9 quoted lines

> diff --git a/cache.h b/cache.h
> index 7d3f80c..6e53cc8 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -976,6 +976,22 @@ enum scld_error {
>  enum scld_error safe_create_leading_directories(char *path);
>  enum scld_error safe_create_leading_directories_const(const char *path);
>  
> +typedef int create_file_fn(const char *path, void *cb);

What kind of guarantee is a callback function of this type expected to give to the caller? Is being idempotent necessary? etc.

What is the callback function expected to signal the caller and how? How is its return value used? I am guessing that returning zero signals success and any non-zero means a failure?

Show 13 quoted lines

> +/*
> + * Create a file at path using fn, creating leading directories if
> + * necessary. If fn fails with errno==ENOENT, then try to create the
> + * containing directory and call fn again. If fn fails with
> + * errno==EISDIR, then delete the directory that is in the way if it
> + * is empty and call fn again. Retry a few times in case we are racing
> + * with another process that is trying to clean up the directory
> + * that contains path.
> + *
> + * In any case, the return value of this function and the errno that
> + * it sets are those resulting from the last call of fn.
> + */
> +int raceproof_create_file(const char *path, create_file_fn fn, void *cb);

Neat-o. As long as the error comes from filesystem atomic system calls, the approach sounds like a good way to go.

Show 45 quoted lines

> diff --git a/sha1_file.c b/sha1_file.c
> index a1ac646..31dcfe8 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -177,6 +177,75 @@ enum scld_error safe_create_leading_directories_const(const char *path)
>  	return result;
>  }
>  
> +int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
> +{
> +	/*
> +	 * The number of times we will try to remove empty directories
> +	 * in the way of path. This is only 1 because if another
> +	 * process is racily creating directories that conflict with
> +	 * us, we don't want to fight against them.
> +	 */
> +	int remove_directories_remaining = 1;
> +
> +	/*
> +	 * The number of times that we will try to create the
> +	 * directories containing path. We are willing to attempt this
> +	 * more than once, because another process could be trying to
> +	 * clean up empty directories at the same time as we are
> +	 * trying to create them.
> +	 */
> +	int create_directories_remaining = 3;
> +
> +	/* A scratch copy of path, filled lazily if we need it: */
> +	struct strbuf path_copy = STRBUF_INIT;
> +
> +	int save_errno;
> +	int ret;
> +
> +retry_fn:
> +	ret = fn(path, cb);
> +	save_errno = errno;
> +	if (!ret)
> +		goto out;
> +
> +	if (errno == EISDIR && remove_directories_remaining > 0) {
> +		/*
> +		 * A directory is in the way. Maybe it is empty; try
> +		 * to remove it:
> +		 */
> +		if (!path_copy.len)

Perhaps assert(path[0]) is needed at the beginning of this function?

> +			strbuf_addstr(&path_copy, path);
> +
> +		if (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY)) {

We do want to pass empty-only, but are there cases where the caller may want to pass more flags, e.g. keep-toplevel?

Show 32 quoted lines

> +			remove_directories_remaining--;
> +			goto retry_fn;
> +		}
> +	} else if (errno == ENOENT && create_directories_remaining > 0) {
> +		/*
> +		 * Maybe the containing directory didn't exist, or
> +		 * maybe it was just deleted by a process that is
> +		 * racing with us to clean up empty directories. Try
> +		 * to create it:
> +		 */
> +		enum scld_error scld_result;
> +
> +		if (!path_copy.len)
> +			strbuf_addstr(&path_copy, path);
> +
> +		do {
> +			create_directories_remaining--;
> +			scld_result = safe_create_leading_directories(path_copy.buf);
> +			if (scld_result == SCLD_OK)
> +				goto retry_fn;
> +		} while (scld_result == SCLD_VANISHED && create_directories_remaining > 0);
> +	}
> +
> +out:
> +	strbuf_release(&path_copy);
> +	errno = save_errno;
> +	return ret;
> +}
> +
>  static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
>  {
>  	int i;