threads / patch / 41424

patch, 20 partsdelete_ref_loose(): derive loose reference path from lock

Subject: [PATCH 17/20] delete_ref_loose(): derive loose reference path from lock

## tl;dr

34 messages between Feb 16, 2016 and Feb 19, 2016. Diffs are folded; open one to read it.

replies: 33people: 3as markdown or json

Michael Haggerty· Feb 16, 2016, 13:22 UTC · lore

[PATCH 00/20] Delete directories left empty after ref deletion

Previously, we were pretty sloppy about leaving empty directories behind (under both $GIT_DIR/refs and $GIT_DIR/logs) when deleting references. Such directories could accumulate essentially forever. It's true that `pack-refs` deletes directories that it empties, but if a directory is *already* empty, then `pack-refs` doesn't remove it. It is also true that if an empty directory gets in the way of the creation of a *new* reference, then it is deleted. But otherwise there is no systematic cleanup of empty directories.

Aside from being messy, wasting disk space, and costing extra time when enumerating references, the proliferation of empty directories was triggering a libgit2 bug [1] on our servers. (We use temporary, uniquely-named references for internal purposes, causing a lot of empty directories to accumulate in some cases.)

This problem was also recently reported by Karl Moskowski [2] to be the cause of problem on non-case-sensitive filesystems.

This patch series makes the reference update machinery more aggressive about deleting empty directories under $GIT_DIR/refs and under $GIT_DIR/logs when deleting references and/or reflogs. This doesn't eliminate all situations where empty directories can be left behind [3], but it covers the worst offenders.

As prelude to the main change, there are a number of patches that make the *creation* of reflog directories more robust against races. Since we want to delete such directories more aggressively, we have to worry more about a race between a process that is creating a new reflog, and another process that might be deleting the containing directory at the same time. (We already had protection against this sort of race for reference creation, but not for reflog creation.)

And since I got tired of writing the same code over and over, I abstracted the code for retrying directory creation into a new function, raceproof_create_file(). This function replaces similar code that appeared in two other places and is now also used for creating reflog files. (Can anybody think of any other code that has to deal with the same kind of race and could maybe benefit?)

This patch series is also available from my GitHub account [4] as branch delete-empty-refs-dirs.

As you might imagine, this patch series conflicts with David Turner's dt/refs-backend-lmdb series [5]. But the conflicts are not too bad [6]. When one or the other of these series is ready to progress, I'd be happy to help resolve the conflicts with the other.

Michael
[1] https://github.com/libgit2/libgit2/issues/3576
[2] http://thread.gmane.org/gmane.comp.version-control.git/283504
[3] Off the top of my head, one case that is not covered is when a
    reflog is expired: a lock is acquired under $GIT_DIR/refs, but the
    directory that held it is not deleted when the lock is released.
[4] http://github.com/mhagger/git
[5] http://thread.gmane.org/gmane.comp.version-control.git/285604
[6] For a merge of this patch series with dt/refs-backend-lmdb v4, see
    branch merge-delete-empty-refs-dirs-refs-backend-lmdb at my GitHub
    repository [4].
Michael Haggerty (20):
  safe_create_leading_directories_const(): preserve errno
  safe_create_leading_directories(): set errno on SCLD_EXISTS
  raceproof_create_file(): new function
  lock_ref_sha1_basic(): use raceproof_create_file()
  rename_tmp_log(): use raceproof_create_file()
  rename_tmp_log(): improve error reporting
  log_ref_setup(): separate code for create vs non-create
  log_ref_setup(): improve robustness against races
  log_ref_setup(): pass the open file descriptor back to the caller
  log_ref_write_1(): don't depend on logfile
  log_ref_setup(): manage the name of the reflog file internally
  log_ref_write_1(): inline function
  try_remove_empty_parents(): rename parameter "name" -> "refname"
  try_remove_empty_parents(): don't trash argument contents
  try_remove_empty_parents(): don't accommodate consecutive slashes
  t5505: use "for-each-ref" to test for the non-existence of references
  delete_ref_loose(): derive loose reference path from lock
  delete_ref_loose(): inline function
  try_remove_empty_parents(): teach to remove parents of reflogs, too
  ref_transaction_commit(): clean up empty directories
 cache.h               |  26 +++-
 refs/files-backend.c  | 370 ++++++++++++++++++++++++++------------------------
 refs/refs-internal.h  |   9 +-
 sha1_file.c           |  77 ++++++++++-
 t/t1400-update-ref.sh |  27 ++++
 t/t5505-remote.sh     |   2 +-
 6 files changed, 325 insertions(+), 186 deletions(-)
-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 01/20] safe_create_leading_directories_const(): preserve errno

Theoretically, free() is allowed to change errno. So preserve the errno from safe_create_leading_directories() across the call to free().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 sha1_file.c | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to sha1_file.c +4 −0
diff --git a/sha1_file.c b/sha1_file.c
index aab1872..568120e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -164,10 +164,14 @@ enum scld_error safe_create_leading_directories(char *path)
 
 enum scld_error safe_create_leading_directories_const(const char *path)
 {
+	int save_errno;
 	/* path points to cache entries, so xstrdup before messing with it */
 	char *buf = xstrdup(path);
 	enum scld_error result = safe_create_leading_directories(buf);
+
+	save_errno = errno;
 	free(buf);
+	errno = save_errno;
 	return result;
 }
 

-- 
2.7.0
Jeff King· Feb 16, 2016, 23:45 UTC · re: Michael Haggerty · lore

Re: [PATCH 01/20] safe_create_leading_directories_const(): preserve errno

On Tue, Feb 16, 2016 at 02:22:14PM +0100, Michael Haggerty wrote:
> Theoretically, free() is allowed to change errno. So preserve the errno
> from safe_create_leading_directories() across the call to free().

I wondered if this was actually a problem in practice. POSIX forbids recreational setting of errno to "0" , and there are no errors defined for free(). But apparently it is indeed a thing:

  https://sourceware.org/bugzilla/show_bug.cgi?id=17924

and our desired behavior will even become the standard in a future POSIX (not that we can rely on that for a long time...).

I suspect in practice that most allocators would use the sbrk() heap for a small allocation like this, so we wouldn't run into any munmap() errors during the free. But obviously that's being way more intimate with the allocator than we should be, and your patch is a simple protection that works either way.

-Peff
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 02/20] safe_create_leading_directories(): set errno on SCLD_EXISTS

The exit path for SCLD_EXISTS wasn't setting errno, as expected by at least one caller. Fix the problem and document that the function sets errno correctly to help avoid similar regressions in the future.

While we're at it, document the difference between safe_create_leading_directories() and safe_create_leading_directories_const().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h     | 10 ++++++++--
 sha1_file.c |  4 +++-
 2 files changed, 11 insertions(+), 3 deletions(-)
Show changes to 2 files +11 −3

cache.h, sha1_file.c

diff --git a/cache.h b/cache.h
index 26640b4..7d3f80c 100644
--- a/cache.h
+++ b/cache.h
@@ -950,8 +950,9 @@ int adjust_shared_perm(const char *path);
 
 /*
  * Create the directory containing the named path, using care to be
- * somewhat safe against races.  Return one of the scld_error values
- * to indicate success/failure.
+ * somewhat safe against races. Return one of the scld_error values to
+ * indicate success/failure. On error, set errno to describe the
+ * problem.
  *
  * SCLD_VANISHED indicates that one of the ancestor directories of the
  * path existed at one point during the function call and then
@@ -959,6 +960,11 @@ int adjust_shared_perm(const char *path);
  * directory while we were working.  To be robust against this kind of
  * race, callers might want to try invoking the function again when it
  * returns SCLD_VANISHED.
+ *
+ * The non _const version of this function temporarily modifies its
+ * path parameter while it is working but restores it before exiting.
+ * The _const version does not modify path (at the cost of having to
+ * make a temporary scratch copy).
  */
 enum scld_error {
 	SCLD_OK = 0,
diff --git a/sha1_file.c b/sha1_file.c
index 568120e..a1ac646 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -135,8 +135,10 @@ enum scld_error safe_create_leading_directories(char *path)
 		*slash = '\0';
 		if (!stat(path, &st)) {
 			/* path exists */
-			if (!S_ISDIR(st.st_mode))
+			if (!S_ISDIR(st.st_mode)) {
+				errno = EEXIST;
 				ret = SCLD_EXISTS;
+			}
 		} else if (mkdir(path, 0777)) {
 			if (errno == EEXIST &&
 			    !stat(path, &st) && S_ISDIR(st.st_mode))

-- 
2.7.0
Junio C Hamano· Feb 17, 2016, 19:23 UTC · re: Michael Haggerty · lore

Re: [PATCH 02/20] safe_create_leading_directories(): set errno on SCLD_EXISTS

Michael Haggerty <mhagger@alum.mit.edu> writes:
> The exit path for SCLD_EXISTS wasn't setting errno, as expected by at
> least one caller. Fix the problem and document that the function sets
> errno correctly to help avoid similar regressions in the future.
Show 12 quoted lines
> diff --git a/sha1_file.c b/sha1_file.c
> index 568120e..a1ac646 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -135,8 +135,10 @@ enum scld_error safe_create_leading_directories(char *path)
>  		*slash = '\0';
>  		if (!stat(path, &st)) {
>  			/* path exists */
> -			if (!S_ISDIR(st.st_mode))
> +			if (!S_ISDIR(st.st_mode)) {
> +				errno = EEXIST;
>  				ret = SCLD_EXISTS;

Hmm, when does this trigger? There is a non-directory A/B, you are preparing leading directories to create A/B/C/D, and you find A/B exists but is not a directory so you cannot create A/B/C underneath it?

That sounds more like ENOTDIR to me.
Does the caller expect EEXIST, or both?
> +			}
>  		} else if (mkdir(path, 0777)) {
>  			if (errno == EEXIST &&
>  			    !stat(path, &st) && S_ISDIR(st.st_mode))
Michael Haggerty· Feb 18, 2016, 15:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 02/20] safe_create_leading_directories(): set errno on SCLD_EXISTS

On 02/17/2016 08:23 PM, Junio C Hamano wrote:
Show 25 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> The exit path for SCLD_EXISTS wasn't setting errno, as expected by at
>> least one caller. Fix the problem and document that the function sets
>> errno correctly to help avoid similar regressions in the future.
> 
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 568120e..a1ac646 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -135,8 +135,10 @@ enum scld_error safe_create_leading_directories(char *path)
>>  		*slash = '\0';
>>  		if (!stat(path, &st)) {
>>  			/* path exists */
>> -			if (!S_ISDIR(st.st_mode))
>> +			if (!S_ISDIR(st.st_mode)) {
>> +				errno = EEXIST;
>>  				ret = SCLD_EXISTS;
> 
> Hmm, when does this trigger?  There is a non-directory A/B, you are
> preparing leading directories to create A/B/C/D, and you find A/B
> exists but is not a directory so you cannot create A/B/C underneath
> it?
> 
> That sounds more like ENOTDIR to me.
Yes, you're right. Thanks.
> Does the caller expect EEXIST, or both?

I don't know of any callers that branch based on errno, but several use strerror() or die_errno() to report the error to the user. The few callers that care about the reason for the failure base their behavior on the return value.

But there are a lot of callers and I haven't audited each of them carefully. Given that this error path currently doesn't necessarily set errno *at all*, I think using a plausible value is strictly an improvement.

I'll change it to ENOTDIR in the next round.
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

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

Add a function that tries to create a file and any containing directories in a way that is robust against races with other processes that might be cleaning up empty directories at the same time.

The actual file creation is done by a callback function, which, if it fails, should set errno to EISDIR or ENOENT according to the convention of open(). raceproof_create_file() detects such failures, and respectively either tries to delete empty directories that might be in the way of the file or tries to create the containing directories. Then it retries the callback function.

This function is not yet used.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
The actual file creation is done by a callback function, so I think
this function is flexible enough to be applicable in other
circumstances where similar races might occur. Perhaps when creating
loose object files in the ODB?

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.

 cache.h     | 16 ++++++++++++++
 sha1_file.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)
Show changes to 2 files +85 −0

cache.h, sha1_file.c

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);
+
+/*
+ * 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);
+
 int mkdir_in_gitdir(const char *path);
 extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
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)
+			strbuf_addstr(&path_copy, path);
+
+		if (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY)) {
+			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;

-- 
2.7.0
Junio C Hamano· Feb 17, 2016, 19:38 UTC · re: Michael Haggerty · lore

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

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;
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 04/20] lock_ref_sha1_basic(): use raceproof_create_file()

Instead of coding the retry loop inline, use raceproof_create_file() to make lock acquisition safe against directory creation/deletion races.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 47 +++++++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 28 deletions(-)
Show changes to refs/files-backend.c +19 −28
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b569762..a549942 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1889,6 +1889,19 @@ static int remove_empty_directories(struct strbuf *path)
 	return remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY);
 }
 
+struct create_reflock_data {
+	struct lock_file *lk;
+	int lflags;
+};
+
+static int create_reflock(const char *path, void *cb)
+{
+	struct create_reflock_data *data = cb;
+
+	return hold_lock_file_for_update(data->lk, path, data->lflags) < 0
+		? -1 : 0;
+}
+
 /*
  * Locks a ref returning the lock on success and NULL on failure.
  * On failure errno is set to something meaningful.
@@ -1906,10 +1919,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	struct ref_lock *lock;
 	int last_errno = 0;
 	int type;
-	int lflags = 0;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 	int resolve_flags = 0;
-	int attempts_remaining = 3;
+	struct create_reflock_data create_reflock_data = {NULL, 0};
 
 	assert(err);
 
@@ -1921,7 +1933,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 	if (flags & REF_NODEREF) {
 		resolve_flags |= RESOLVE_REF_NO_RECURSE;
-		lflags |= LOCK_NO_DEREF;
+		create_reflock_data.lflags |= LOCK_NO_DEREF;
 	}
 
 	refname = resolve_ref_unsafe(refname, resolve_flags,
@@ -1980,35 +1992,14 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	lock->orig_ref_name = xstrdup(orig_refname);
 	strbuf_git_path(&ref_file, "%s", refname);
 
- retry:
-	switch (safe_create_leading_directories_const(ref_file.buf)) {
-	case SCLD_OK:
-		break; /* success */
-	case SCLD_VANISHED:
-		if (--attempts_remaining > 0)
-			goto retry;
-		/* fall through */
-	default:
+	create_reflock_data.lk = lock->lk;
+
+	if (raceproof_create_file(ref_file.buf, create_reflock, &create_reflock_data)) {
 		last_errno = errno;
-		strbuf_addf(err, "unable to create directory for %s",
-			    ref_file.buf);
+		unable_to_lock_message(ref_file.buf, errno, err);
 		goto error_return;
 	}
 
-	if (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) {
-		last_errno = errno;
-		if (errno == ENOENT && --attempts_remaining > 0)
-			/*
-			 * Maybe somebody just deleted one of the
-			 * directories leading to ref_file.  Try
-			 * again:
-			 */
-			goto retry;
-		else {
-			unable_to_lock_message(ref_file.buf, errno, err);
-			goto error_return;
-		}
-	}
 	if (verify_lock(lock, old_sha1, mustexist, err)) {
 		last_errno = errno;
 		goto error_return;

-- 
2.7.0
Junio C Hamano· Feb 17, 2016, 20:44 UTC · re: Michael Haggerty · lore

Re: [PATCH 04/20] lock_ref_sha1_basic(): use raceproof_create_file()

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 5 quoted lines
> Instead of coding the retry loop inline, use raceproof_create_file() to
> make lock acquisition safe against directory creation/deletion races.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
Makes sense.
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

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

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(-)
Show changes to refs/files-backend.c +32 −44
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;
+		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;
 }
 

-- 
2.7.0
Junio C Hamano· Feb 17, 2016, 20:53 UTC · re: Michael Haggerty · lore

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

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;
>  }
Michael Haggerty· Feb 19, 2016, 16:07 UTC · re: Junio C Hamano · lore

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

On 02/17/2016 09:53 PM, Junio C Hamano wrote:
Show 35 quoted lines
> 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.
Thanks; will fix.
> More importantly, is ENOTDIR expected only on a buggy platform?

Here I was just mimicking the old behavior, which I think was correct, but let's check more carefully...

Show 27 quoted lines
> [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...

We just created TMP_RENAMED_LOG ourselves, so I don't think we need to expect errors from that argument. (Though I don't recall that there is any locking to prevent two `git branch -m` processes from clobbering each others' temporary files. Oh well; renaming branches is relatively rare and probably interactive, so I'll declare that potential problem to be out of scope for this patch series.)

So let's consider the cases where we can get ENOTDIR for `path`:
> A component of either path prefix names an existing file that is
> neither a directory nor a symbolic link to a directory.

This can certainly happen for `path`, but it is not a case that can be rescued by raceproof_create_file().

> or the old argument names a directory [...]
This is not the case.
> or the old argument contains [...]
Also not interesting.
> 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

The new argument doesn't end with trailing <slash> characters, so this can't happen.

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

So while it is true that a non-buggy implementation can give ENOTDIR, it is for a case that we can't rescue. So if it weren't for the buggy implementation, we could just leave ENOTDIR un-handled.

Now, we have to consider the opposite case, namely that we are calling a non-buggy implementation of `rename()`, and we artificially change ENOTDIR to EISDIR. Can that cause any bad effects?

I don't think so, because the case where a non-buggy implementation can yield ENOTDIR is a case, the consequent call to `remove_dir_recursively()` would fail with ENOTDIR too, and `raceproof_create_file()` would give up immediately.

So I think everything is OK, though I admit that it is not especially elegant. We could limit ourselves to doing the workaround only on Solaris 5.8, but that seems like a lot of effort for not much benefit. Or we could drop the workaround; after all, Solaris 5.8 was released in 2000 and end-of-lifed in 2012. (Though I don't know whether the behavior was fixed in later versions of Solaris.)

> [...]
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Junio C Hamano· Feb 19, 2016, 17:15 UTC · re: Michael Haggerty · lore

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

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 11 quoted lines
> Now, we have to consider the opposite case, namely that we are calling a
> non-buggy implementation of `rename()`, and we artificially change
> ENOTDIR to EISDIR. Can that cause any bad effects?
>
> I don't think so, because the case where a non-buggy implementation can
> yield ENOTDIR is a case, the consequent call to
> `remove_dir_recursively()` would fail with ENOTDIR too, and
> `raceproof_create_file()` would give up immediately.
>
> So I think everything is OK, though I admit that it is not especially
> elegant.
OK, thanks for a clear analysis.

I knew the original and the update were meant to (and would) behave the same, but the workaround logic in the original looked iffy.

Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 06/20] rename_tmp_log(): improve error reporting

* Don't capitalize error strings
* Report true paths of affected files
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Maybe these error strings should be internationalized? If so, there
are a bunch of similar error strings in related functions that should
be changed at the same time.
 refs/files-backend.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Show changes to refs/files-backend.c +4 −3
diff --git a/refs/files-backend.c b/refs/files-backend.c
index e5f964c..4266da9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2430,10 +2430,11 @@ static int rename_tmp_log(const char *newrefname)
 	ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
 	if (ret) {
 		if (true_errno==EISDIR)
-			error("Directory not empty: %s", path);
+			error("directory not empty: %s", path);
 		else
-			error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
-				newrefname, strerror(true_errno));
+			error("unable to move logfile %s to %s: %s",
+			      git_path(TMP_RENAMED_LOG), path,
+			      strerror(true_errno));
 	}
 
 	free(path);

-- 
2.7.0
Junio C Hamano· Feb 18, 2016, 22:14 UTC · re: Michael Haggerty · lore

Re: [PATCH 06/20] rename_tmp_log(): improve error reporting

Michael Haggerty <mhagger@alum.mit.edu> writes:
> * Don't capitalize error strings
> * Report true paths of affected files
Obvious improvements.  Good.
Show 28 quoted lines
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> Maybe these error strings should be internationalized? If so, there
> are a bunch of similar error strings in related functions that should
> be changed at the same time.
>
>  refs/files-backend.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index e5f964c..4266da9 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2430,10 +2430,11 @@ static int rename_tmp_log(const char *newrefname)
>  	ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
>  	if (ret) {
>  		if (true_errno==EISDIR)
> -			error("Directory not empty: %s", path);
> +			error("directory not empty: %s", path);
>  		else
> -			error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
> -				newrefname, strerror(true_errno));
> +			error("unable to move logfile %s to %s: %s",
> +			      git_path(TMP_RENAMED_LOG), path,
> +			      strerror(true_errno));
>  	}
>  
>  	free(path);
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 07/20] log_ref_setup(): separate code for create vs non-create

The behavior of this function (especially how it handles errors) is quite different depending on whether we are willing to create the reflog vs. whether we are only trying to open an existing reflog. So separate the code paths.

This also simplifies the next steps.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 56 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 19 deletions(-)
Show changes to refs/files-backend.c +37 −19
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4266da9..f54d95b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2590,7 +2590,7 @@ static int commit_ref(struct ref_lock *lock)
  */
 static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
 {
-	int logfd, oflags = O_APPEND | O_WRONLY;
+	int logfd;
 
 	strbuf_git_path(logfile, "logs/%s", refname);
 	if (force_create || should_autocreate_reflog(refname)) {
@@ -2599,36 +2599,54 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 				    "%s", logfile->buf, strerror(errno));
 			return -1;
 		}
-		oflags |= O_CREAT;
-	}
-
-	logfd = open(logfile->buf, oflags, 0666);
-	if (logfd < 0) {
-		if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))
-			return 0;
+		logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
+		if (logfd < 0) {
+			if (errno == EISDIR) {
+				/*
+				 * The directory that is in the way might be
+				 * empty. Try to remove it.
+				 */
+				if (remove_empty_directories(logfile)) {
+					strbuf_addf(err, "There are still logs under "
+						    "'%s'", logfile->buf);
+					return -1;
+				}
+				logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
+			}
 
-		if (errno == EISDIR) {
-			if (remove_empty_directories(logfile)) {
-				strbuf_addf(err, "There are still logs under "
-					    "'%s'", logfile->buf);
+			if (logfd < 0) {
+				strbuf_addf(err, "unable to append to %s: %s",
+					    logfile->buf, strerror(errno));
 				return -1;
 			}
-			logfd = open(logfile->buf, oflags, 0666);
 		}
 
+		adjust_shared_perm(logfile->buf);
+		close(logfd);
+	} else {
+		logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
 		if (logfd < 0) {
-			strbuf_addf(err, "unable to append to %s: %s",
-				    logfile->buf, strerror(errno));
-			return -1;
+			if (errno == ENOENT || errno == EISDIR) {
+				/*
+				 * The logfile doesn't already exist,
+				 * but that is not an error; it only
+				 * means that we won't write log
+				 * entries to it.
+				 */
+			} else {
+				strbuf_addf(err, "unable to append to %s: %s",
+					    logfile->buf, strerror(errno));
+				return -1;
+			}
+		} else {
+			adjust_shared_perm(logfile->buf);
+			close(logfd);
 		}
 	}
 
-	adjust_shared_perm(logfile->buf);
-	close(logfd);
 	return 0;
 }
 
-
 int safe_create_reflog(const char *refname, int force_create, struct strbuf *err)
 {
 	int ret;

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 08/20] log_ref_setup(): improve robustness against races

Change log_ref_setup() to use raceproof_create_file() to create the new logfile. This makes it more robust against a race against another process that might be trying to clean up empty directories while we are trying to create a new logfile.

This also means that it will only call create_leading_directories() if open() fails, which should be a net win. Even in the cases where we are willing to create a new logfile, it will usually be the case that the logfile already exists, or if not then that the directory containing the logfile already exists. In such cases, we will save some work that was previously done unconditionally.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 46 +++++++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 25 deletions(-)
Show changes to refs/files-backend.c +21 −25
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f54d95b..0cfe1ce 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2582,6 +2582,14 @@ static int commit_ref(struct ref_lock *lock)
 	return 0;
 }
 
+static int open_or_create_logfile(const char *path, void *cb)
+{
+	int *fd = cb;
+
+	*fd = open(path, O_APPEND | O_WRONLY | O_CREAT, 0666);
+	return (*fd < 0) ? -1 : 0;
+}
+
 /*
  * Create a reflog for a ref.  If force_create = 0, the reflog will
  * only be created for certain refs (those for which
@@ -2593,36 +2601,24 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 	int logfd;
 
 	strbuf_git_path(logfile, "logs/%s", refname);
+
 	if (force_create || should_autocreate_reflog(refname)) {
-		if (safe_create_leading_directories(logfile->buf) < 0) {
-			strbuf_addf(err, "unable to create directory for %s: "
-				    "%s", logfile->buf, strerror(errno));
-			return -1;
-		}
-		logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
-		if (logfd < 0) {
-			if (errno == EISDIR) {
-				/*
-				 * The directory that is in the way might be
-				 * empty. Try to remove it.
-				 */
-				if (remove_empty_directories(logfile)) {
-					strbuf_addf(err, "There are still logs under "
-						    "'%s'", logfile->buf);
-					return -1;
-				}
-				logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
-			}
-
-			if (logfd < 0) {
+		if (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd) < 0) {
+			if (errno == ENOENT) {
+				strbuf_addf(err, "unable to create directory for %s: "
+					    "%s", logfile->buf, strerror(errno));
+			} else if (errno == EISDIR) {
+				strbuf_addf(err, "there are still logs under %s",
+					    logfile->buf);
+			} else {
 				strbuf_addf(err, "unable to append to %s: %s",
 					    logfile->buf, strerror(errno));
-				return -1;
 			}
+			return -1;
+		} else {
+			adjust_shared_perm(logfile->buf);
+			close(logfd);
 		}
-
-		adjust_shared_perm(logfile->buf);
-		close(logfd);
 	} else {
 		logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
 		if (logfd < 0) {

-- 
2.7.0
Junio C Hamano· Feb 18, 2016, 22:17 UTC · re: Michael Haggerty · lore

Re: [PATCH 08/20] log_ref_setup(): improve robustness against races

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 11 quoted lines
> Change log_ref_setup() to use raceproof_create_file() to create the new
> logfile. This makes it more robust against a race against another
> process that might be trying to clean up empty directories while we are
> trying to create a new logfile.
>
> This also means that it will only call create_leading_directories() if
> open() fails, which should be a net win. Even in the cases where we are
> willing to create a new logfile, it will usually be the case that the
> logfile already exists, or if not then that the directory containing the
> logfile already exists. In such cases, we will save some work that was
> previously done unconditionally.
Makes sense.
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 09/20] log_ref_setup(): pass the open file descriptor back to the caller

This function will most often be called by log_ref_write_1(), which wants to append to the reflog file. In that case, it is silly to close the file only for the caller to reopen it immediately. So, in the case that the file was opened, pass the open file descriptor back to the caller.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)
Show changes to refs/files-backend.c +21 −16
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0cfe1ce..78a6213 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2591,19 +2591,23 @@ static int open_or_create_logfile(const char *path, void *cb)
 }
 
 /*
- * Create a reflog for a ref.  If force_create = 0, the reflog will
- * only be created for certain refs (those for which
- * should_autocreate_reflog returns non-zero.  Otherwise, create it
- * regardless of the ref name.  Fill in *err and return -1 on failure.
+ * Create a reflog for a ref. Store its path to *logfile. If
+ * force_create = 0, only create the reflog for certain refs (those
+ * for which should_autocreate_reflog returns non-zero). Otherwise,
+ * create it regardless of the reference name. If the logfile already
+ * existed or was created, return 0 and set *logfd to the file
+ * descriptor opened for appending to the file. If no logfile exists
+ * and we decided not to create one, return 0 and set *logfd to -1. On
+ * failure, fill in *err and return -1.
  */
-static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname,
+			 struct strbuf *logfile, int *logfd,
+			 struct strbuf *err, int force_create)
 {
-	int logfd;
-
 	strbuf_git_path(logfile, "logs/%s", refname);
 
 	if (force_create || should_autocreate_reflog(refname)) {
-		if (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd) < 0) {
+		if (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd) < 0) {
 			if (errno == ENOENT) {
 				strbuf_addf(err, "unable to create directory for %s: "
 					    "%s", logfile->buf, strerror(errno));
@@ -2617,11 +2621,10 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 			return -1;
 		} else {
 			adjust_shared_perm(logfile->buf);
-			close(logfd);
 		}
 	} else {
-		logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
-		if (logfd < 0) {
+		*logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
+		if (*logfd < 0) {
 			if (errno == ENOENT || errno == EISDIR) {
 				/*
 				 * The logfile doesn't already exist,
@@ -2636,7 +2639,6 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 			}
 		} else {
 			adjust_shared_perm(logfile->buf);
-			close(logfd);
 		}
 	}
 
@@ -2647,8 +2649,11 @@ int safe_create_reflog(const char *refname, int force_create, struct strbuf *err
 {
 	int ret;
 	struct strbuf sb = STRBUF_INIT;
+	int fd;
 
-	ret = log_ref_setup(refname, &sb, err, force_create);
+	ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+	if (!ret && fd >= 0)
+		close(fd);
 	strbuf_release(&sb);
 	return ret;
 }
@@ -2684,17 +2689,17 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 			   struct strbuf *logfile, int flags,
 			   struct strbuf *err)
 {
-	int logfd, result, oflags = O_APPEND | O_WRONLY;
+	int logfd, result;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);
+	result = log_ref_setup(refname, logfile, &logfd, err,
+			       flags & REF_FORCE_CREATE_REFLOG);
 
 	if (result)
 		return result;
 
-	logfd = open(logfile->buf, oflags);
 	if (logfd < 0)
 		return 0;
 	result = log_ref_write_fd(logfd, old_sha1, new_sha1,

-- 
2.7.0
Junio C Hamano· Feb 18, 2016, 22:21 UTC · re: Michael Haggerty · lore

Re: [PATCH 09/20] log_ref_setup(): pass the open file descriptor back to the caller

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 5 quoted lines
> This function will most often be called by log_ref_write_1(), which
> wants to append to the reflog file. In that case, it is silly to close
> the file only for the caller to reopen it immediately. So, in the case
> that the file was opened, pass the open file descriptor back to the
> caller.
Makes sense.
>  		} else {
>  			adjust_shared_perm(logfile->buf);
> -			close(logfd);
>  		}

I briefly wondered if permission twiddling on an open fd may have negative effect on certain platforms, but the original was doing so already (even before step 08/20), so that shouldn't be an issue with this change, either.

Nicely done.
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 10/20] log_ref_write_1(): don't depend on logfile

It's unnecessary to pass a strbuf holding the reflog path up and down the call stack now that it is hardly needed by the callers. Remove the places where log_ref_write_1() uses it, in preparation for making it internal to log_ref_setup().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to refs/files-backend.c +4 −4
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 78a6213..dcd19ee 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2705,14 +2705,14 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
 				  git_committer_info(0), msg);
 	if (result) {
-		strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
-			    strerror(errno));
+		strbuf_addf(err, "unable to append to %s: %s",
+			    git_path("logs/%s", refname), strerror(errno));
 		close(logfd);
 		return -1;
 	}
 	if (close(logfd)) {
-		strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
-			    strerror(errno));
+		strbuf_addf(err, "unable to append to %s: %s",
+			    git_path("logs/%s", refname), strerror(errno));
 		return -1;
 	}
 	return 0;

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 11/20] log_ref_setup(): manage the name of the reflog file internally

Instead of writing the name of the reflog file into a strbuf that is supplied by the caller but not needed there, write it into a local temporary buffer and remove the strbuf parameter entirely.

And while we're adjusting the function signature, reorder the arguments to move the input parameters before the output parameters.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 66 +++++++++++++++++++++++-----------------------------
 1 file changed, 29 insertions(+), 37 deletions(-)
Show changes to refs/files-backend.c +29 −37
diff --git a/refs/files-backend.c b/refs/files-backend.c
index dcd19ee..120189c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2591,39 +2591,38 @@ static int open_or_create_logfile(const char *path, void *cb)
 }
 
 /*
- * Create a reflog for a ref. Store its path to *logfile. If
- * force_create = 0, only create the reflog for certain refs (those
- * for which should_autocreate_reflog returns non-zero). Otherwise,
- * create it regardless of the reference name. If the logfile already
- * existed or was created, return 0 and set *logfd to the file
- * descriptor opened for appending to the file. If no logfile exists
- * and we decided not to create one, return 0 and set *logfd to -1. On
- * failure, fill in *err and return -1.
+ * Create a reflog for a ref. If force_create = 0, only create the
+ * reflog for certain refs (those for which should_autocreate_reflog
+ * returns non-zero). Otherwise, create it regardless of the reference
+ * name. If the logfile already existed or was created, return 0 and
+ * set *logfd to the file descriptor opened for appending to the file.
+ * If no logfile exists and we decided not to create one, return 0 and
+ * set *logfd to -1. On failure, fill in *err and return -1.
  */
-static int log_ref_setup(const char *refname,
-			 struct strbuf *logfile, int *logfd,
-			 struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname, int force_create,
+			 int *logfd, struct strbuf *err)
 {
-	strbuf_git_path(logfile, "logs/%s", refname);
+	char *logfile = git_pathdup("logs/%s", refname);
+	int ret = 0;
 
 	if (force_create || should_autocreate_reflog(refname)) {
-		if (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd) < 0) {
+		if (raceproof_create_file(logfile, open_or_create_logfile, logfd) < 0) {
 			if (errno == ENOENT) {
 				strbuf_addf(err, "unable to create directory for %s: "
-					    "%s", logfile->buf, strerror(errno));
+					    "%s", logfile, strerror(errno));
 			} else if (errno == EISDIR) {
 				strbuf_addf(err, "there are still logs under %s",
-					    logfile->buf);
+					    logfile);
 			} else {
 				strbuf_addf(err, "unable to append to %s: %s",
-					    logfile->buf, strerror(errno));
+					    logfile, strerror(errno));
 			}
-			return -1;
+			ret = -1;
 		} else {
-			adjust_shared_perm(logfile->buf);
+			adjust_shared_perm(logfile);
 		}
 	} else {
-		*logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
+		*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
 		if (*logfd < 0) {
 			if (errno == ENOENT || errno == EISDIR) {
 				/*
@@ -2634,27 +2633,25 @@ static int log_ref_setup(const char *refname,
 				 */
 			} else {
 				strbuf_addf(err, "unable to append to %s: %s",
-					    logfile->buf, strerror(errno));
-				return -1;
+					    logfile, strerror(errno));
+				ret = -1;
 			}
 		} else {
-			adjust_shared_perm(logfile->buf);
+			adjust_shared_perm(logfile);
 		}
 	}
 
-	return 0;
+	free(logfile);
+	return ret;
 }
 
 int safe_create_reflog(const char *refname, int force_create, struct strbuf *err)
 {
-	int ret;
-	struct strbuf sb = STRBUF_INIT;
-	int fd;
+	int ret, fd;
 
-	ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+	ret = log_ref_setup(refname, force_create, &fd, err);
 	if (!ret && fd >= 0)
 		close(fd);
-	strbuf_release(&sb);
 	return ret;
 }
 
@@ -2686,16 +2683,15 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 
 static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 			   const unsigned char *new_sha1, const char *msg,
-			   struct strbuf *logfile, int flags,
-			   struct strbuf *err)
+			   int flags, struct strbuf *err)
 {
 	int logfd, result;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, logfile, &logfd, err,
-			       flags & REF_FORCE_CREATE_REFLOG);
+	result = log_ref_setup(refname, flags & REF_FORCE_CREATE_REFLOG,
+			       &logfd, err);
 
 	if (result)
 		return result;
@@ -2730,11 +2726,7 @@ int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
 			const unsigned char *new_sha1, const char *msg,
 			int flags, struct strbuf *err)
 {
-	struct strbuf sb = STRBUF_INIT;
-	int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, flags,
-				  err);
-	strbuf_release(&sb);
-	return ret;
+	return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);
 }
 
 /*

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 12/20] log_ref_write_1(): inline function

Now log_ref_write_1() doesn't do anything, so inline it.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)
Show changes to refs/files-backend.c +11 −18
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 120189c..9c13653 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2681,9 +2681,17 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 	return 0;
 }
 
-static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
-			   const unsigned char *new_sha1, const char *msg,
-			   int flags, struct strbuf *err)
+static int log_ref_write(const char *refname, const unsigned char *old_sha1,
+			 const unsigned char *new_sha1, const char *msg,
+			 int flags, struct strbuf *err)
+{
+	return files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,
+				   err);
+}
+
+int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+			const unsigned char *new_sha1, const char *msg,
+			int flags, struct strbuf *err)
 {
 	int logfd, result;
 
@@ -2714,21 +2722,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 	return 0;
 }
 
-static int log_ref_write(const char *refname, const unsigned char *old_sha1,
-			 const unsigned char *new_sha1, const char *msg,
-			 int flags, struct strbuf *err)
-{
-	return files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,
-				   err);
-}
-
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
-			const unsigned char *new_sha1, const char *msg,
-			int flags, struct strbuf *err)
-{
-	return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);
-}
-
 /*
  * Write sha1 into the open lockfile, then close the lockfile. On
  * errors, rollback the lockfile, fill in *err and

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 13/20] try_remove_empty_parents(): rename parameter "name" -> "refname"

This is the standard nomenclature.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to refs/files-backend.c +4 −4
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9c13653..7e870fc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2200,13 +2200,13 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
 
 /*
  * Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *name.
+ * Note: munges *refname.
  */
-static void try_remove_empty_parents(char *name)
+static void try_remove_empty_parents(char *refname)
 {
 	char *p, *q;
 	int i;
-	p = name;
+	p = refname;
 	for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
 		while (*p && *p != '/')
 			p++;
@@ -2224,7 +2224,7 @@ static void try_remove_empty_parents(char *name)
 		if (q == p)
 			break;
 		*q = '\0';
-		if (rmdir(git_path("%s", name)))
+		if (rmdir(git_path("%s", refname)))
 			break;
 	}
 }

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 14/20] try_remove_empty_parents(): don't trash argument contents

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
Show changes to refs/files-backend.c +9 −7
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7e870fc..897ff4c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2200,13 +2200,15 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
 
 /*
  * Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *refname.
  */
-static void try_remove_empty_parents(char *refname)
+static void try_remove_empty_parents(const char *refname)
 {
+	struct strbuf buf = STRBUF_INIT;
 	char *p, *q;
 	int i;
-	p = refname;
+
+	strbuf_addstr(&buf, refname);
+	p = buf.buf;
 	for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
 		while (*p && *p != '/')
 			p++;
@@ -2214,8 +2216,7 @@ static void try_remove_empty_parents(char *refname)
 		while (*p == '/')
 			p++;
 	}
-	for (q = p; *q; q++)
-		;
+	q = buf.buf + buf.len;
 	while (1) {
 		while (q > p && *q != '/')
 			q--;
@@ -2223,10 +2224,11 @@ static void try_remove_empty_parents(char *refname)
 			q--;
 		if (q == p)
 			break;
-		*q = '\0';
-		if (rmdir(git_path("%s", refname)))
+		strbuf_setlen(&buf, q - buf.buf);
+		if (rmdir(git_path("%s", buf.buf)))
 			break;
 	}
+	strbuf_release(&buf);
 }
 
 /* make sure nobody touched the ref, and unlink */

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 15/20] try_remove_empty_parents(): don't accommodate consecutive slashes

"refname" has already been checked by check_refname_format(), so it cannot have consecutive slashes.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to refs/files-backend.c +2 −3
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 897ff4c..0a9f330 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2212,15 +2212,14 @@ static void try_remove_empty_parents(const char *refname)
 	for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
 		while (*p && *p != '/')
 			p++;
-		/* tolerate duplicate slashes; see check_refname_format() */
-		while (*p == '/')
+		if (*p == '/')
 			p++;
 	}
 	q = buf.buf + buf.len;
 	while (1) {
 		while (q > p && *q != '/')
 			q--;
-		while (q > p && *(q-1) == '/')
+		if (q > p && *(q-1) == '/')
 			q--;
 		if (q == p)
 			break;

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 16/20] t5505: use "for-each-ref" to test for the non-existence of references

Instead of looking on the filesystem inside ".git/refs/remotes/origin", use "git for-each-ref" to check for leftover references under the remote's old name.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t5505-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t5505-remote.sh +1 −1
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1a8e3b8..d2ac346 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -687,7 +687,7 @@ test_expect_success 'rename a remote' '
 	(
 		cd four &&
 		git remote rename origin upstream &&
-		rmdir .git/refs/remotes/origin &&
+		test -z "$(git for-each-ref refs/remotes/origin)" &&
 		test "$(git symbolic-ref refs/remotes/upstream/HEAD)" = "refs/remotes/upstream/master" &&
 		test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" &&
 		test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" &&

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

It is simpler to derive the path to the file that must be deleted from "lock->ref_name" than from the lock_file object.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Show changes to refs/files-backend.c +1 −4
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0a9f330..754d254 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2345,10 +2345,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
 		 * loose.  The loose file name is the same as the
 		 * lockfile name, minus ".lock":
 		 */
-		char *loose_filename = get_locked_file_path(lock->lk);
-		int res = unlink_or_msg(loose_filename, err);
-		free(loose_filename);
-		if (res)
+		if (unlink_or_msg(git_path("%s", lock->ref_name), err))
 			return 1;
 	}
 	return 0;

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 18/20] delete_ref_loose(): inline function

It was hardly doing anything anymore.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)
Show changes to refs/files-backend.c +7 −18
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 754d254..b2b0f39 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2336,21 +2336,6 @@ static int repack_without_refs(struct string_list *refnames, struct strbuf *err)
 	return ret;
 }
 
-static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
-{
-	assert(err);
-
-	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
-		/*
-		 * loose.  The loose file name is the same as the
-		 * lockfile name, minus ".lock":
-		 */
-		if (unlink_or_msg(git_path("%s", lock->ref_name), err))
-			return 1;
-	}
-	return 0;
-}
-
 int delete_refs(struct string_list *refnames)
 {
 	struct strbuf err = STRBUF_INIT;
@@ -3257,9 +3242,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (update->flags & REF_DELETING) {
-			if (delete_ref_loose(update->lock, update->type, err)) {
-				ret = TRANSACTION_GENERIC_ERROR;
-				goto cleanup;
+			if (!(update->type & REF_ISPACKED) ||
+			    update->type & REF_ISSYMREF) {
+				/* It is a loose reference. */
+				if (unlink_or_msg(git_path("%s", update->lock->ref_name), err)) {
+					ret = TRANSACTION_GENERIC_ERROR;
+					goto cleanup;
+				}
 			}
 
 			if (!(update->flags & REF_ISPRUNING))

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 19/20] try_remove_empty_parents(): teach to remove parents of reflogs, too

Add a new "flags" parameter that tells the function whether to remove empty parent directories of the loose reference file, of the reflog file, or both. The new functionality is not yet used.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
Show changes to refs/files-backend.c +18 −6
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b2b0f39..9d9151c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2198,10 +2198,18 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
+enum {
+	REMOVE_EMPTY_PARENTS_REF = 0x01,
+	REMOVE_EMPTY_PARENTS_REFLOG = 0x02
+};
+
 /*
- * Remove empty parents, but spare refs/ and immediate subdirs.
+ * Remove empty parent directories associated with the specified
+ * reference and/or its reflog, but spare [logs/]refs/ and immediate
+ * subdirs. flags is a combination of REMOVE_EMPTY_PARENTS_REF and/or
+ * REMOVE_EMPTY_PARENTS_REFLOG.
  */
-static void try_remove_empty_parents(const char *refname)
+static void try_remove_empty_parents(const char *refname, unsigned int flags)
 {
 	struct strbuf buf = STRBUF_INIT;
 	char *p, *q;
@@ -2216,7 +2224,7 @@ static void try_remove_empty_parents(const char *refname)
 			p++;
 	}
 	q = buf.buf + buf.len;
-	while (1) {
+	while (flags & (REMOVE_EMPTY_PARENTS_REF | REMOVE_EMPTY_PARENTS_REFLOG)) {
 		while (q > p && *q != '/')
 			q--;
 		if (q > p && *(q-1) == '/')
@@ -2224,8 +2232,12 @@ static void try_remove_empty_parents(const char *refname)
 		if (q == p)
 			break;
 		strbuf_setlen(&buf, q - buf.buf);
-		if (rmdir(git_path("%s", buf.buf)))
-			break;
+		if ((flags & REMOVE_EMPTY_PARENTS_REF) &&
+		    rmdir(git_path("%s", buf.buf)))
+			flags &= ~REMOVE_EMPTY_PARENTS_REF;
+		if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) &&
+		    rmdir(git_path("logs/%s", buf.buf)))
+			flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
 	}
 	strbuf_release(&buf);
 }
@@ -2251,7 +2263,7 @@ static void prune_ref(struct ref_to_prune *r)
 	}
 	ref_transaction_free(transaction);
 	strbuf_release(&err);
-	try_remove_empty_parents(r->name);
+	try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);
 }
 
 static void prune_refs(struct ref_to_prune *r)

-- 
2.7.0
Michael Haggerty· Feb 16, 2016, 13:22 UTC · re: Michael Haggerty · lore

[PATCH 20/20] ref_transaction_commit(): clean up empty directories

When deleting/pruning references, remove any directories that are made empty by the deletion of loose references or of reflogs. Otherwise such empty directories can survive forever and accumulate over time. (Even 'pack-refs', which is smart enough to remove the parent directories of loose references that it prunes, leaves directories that were already empty.)

And now that ref_transaction_commit() takes care of deleting the parent directories of loose references that it prunes, we don't have to do that in prune_ref() anymore.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  | 37 +++++++++++++++++++++++++++++++------
 refs/refs-internal.h  |  9 ++++++++-
 t/t1400-update-ref.sh | 27 +++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 7 deletions(-)
Show changes to 3 files +66 −7

refs/files-backend.c, refs/refs-internal.h, t/t1400-update-ref.sh

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9d9151c..18f32bb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2263,7 +2263,6 @@ static void prune_ref(struct ref_to_prune *r)
 	}
 	ref_transaction_free(transaction);
 	strbuf_release(&err);
-	try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);
 }
 
 static void prune_refs(struct ref_to_prune *r)
@@ -3261,6 +3260,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					ret = TRANSACTION_GENERIC_ERROR;
 					goto cleanup;
 				}
+				update->flags |= REF_DELETED_LOOSE;
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
@@ -3273,16 +3273,41 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for_each_string_list_item(ref_to_delete, &refs_to_delete)
-		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
+
+	/* Delete the reflogs of any references that were deleted: */
+	for_each_string_list_item(ref_to_delete, &refs_to_delete) {
+		if (!unlink_or_warn(git_path("logs/%s", ref_to_delete->string)))
+			try_remove_empty_parents(ref_to_delete->string,
+						 REMOVE_EMPTY_PARENTS_REFLOG);
+	}
+
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
 	transaction->state = REF_TRANSACTION_CLOSED;
 
-	for (i = 0; i < n; i++)
-		if (updates[i]->lock)
-			unlock_ref(updates[i]->lock);
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (!update->lock)
+			continue;
+
+		if (update->flags & REF_DELETED_LOOSE) {
+			/*
+			 * The loose reference was deleted. We want to
+			 * delete any empty parent directories, but
+			 * that can only work after we have removed
+			 * the lockfile:
+			 */
+			char *path = xstrdup(update->lock->ref_name);
+			unlock_ref(update->lock);
+			try_remove_empty_parents(path, REMOVE_EMPTY_PARENTS_REF);
+			free(path);
+		} else {
+			unlock_ref(update->lock);
+		}
+	}
+
 	string_list_clear(&refs_to_delete, 0);
 	string_list_clear(&affected_refnames, 0);
 	return ret;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index c7dded3..dd09be1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -43,6 +43,12 @@
  */
 
 /*
+ * Used as a flag in ref_update::flags when the loose reference has
+ * been deleted.
+ */
+#define REF_DELETED_LOOSE 0x100
+
+/*
  * Return true iff refname is minimally safe. "Safe" here means that
  * deleting a loose reference by this name will not do any damage, for
  * example by causing a file that is not a reference to be deleted.
@@ -141,7 +147,8 @@ struct ref_update {
 	unsigned char old_sha1[20];
 	/*
 	 * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
-	 * REF_DELETING, and REF_ISPRUNING:
+	 * REF_DELETING, REF_ISPRUNING, REF_NEEDS_COMMIT, and
+	 * REF_DELETED_LOOSE:
 	 */
 	unsigned int flags;
 	struct ref_lock *lock;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index af1b20d..00284eb 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -191,6 +191,33 @@ test_expect_success \
 	 git update-ref HEAD'" $A &&
 	 test $A"' = $(cat .git/'"$m"')'
 
+test_expect_success "empty directory removal" '
+	git branch d1/d2/r1 HEAD &&
+	git branch d1/r2 HEAD &&
+	test -f .git/refs/heads/d1/d2/r1 &&
+	test -f .git/logs/refs/heads/d1/d2/r1 &&
+	git branch -d d1/d2/r1 &&
+	! test -e .git/refs/heads/d1/d2 &&
+	! test -e .git/logs/refs/heads/d1/d2 &&
+	test -f .git/refs/heads/d1/r2 &&
+	test -f .git/logs/refs/heads/d1/r2
+'
+
+test_expect_success "symref empty directory removal" '
+	git branch e1/e2/r1 HEAD &&
+	git branch e1/r2 HEAD &&
+	git checkout e1/e2/r1 &&
+	test_when_finished "git checkout master" &&
+	test -f .git/refs/heads/e1/e2/r1 &&
+	test -f .git/logs/refs/heads/e1/e2/r1 &&
+	git update-ref -d HEAD &&
+	! test -e .git/refs/heads/e1/e2 &&
+	! test -e .git/logs/refs/heads/e1/e2 &&
+	test -f .git/refs/heads/e1/r2 &&
+	test -f .git/logs/refs/heads/e1/r2 &&
+	test -f .git/logs/HEAD
+'
+
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch

-- 
2.7.0
Junio C Hamano· Feb 17, 2016, 00:08 UTC · re: Michael Haggerty · lore

Re: [PATCH 00/20] Delete directories left empty after ref deletion

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 8 quoted lines
> Previously, we were pretty sloppy about leaving empty directories
> behind (under both $GIT_DIR/refs and $GIT_DIR/logs) when deleting
> references. Such directories could accumulate essentially forever.
> It's true that `pack-refs` deletes directories that it empties, but if
> a directory is *already* empty, then `pack-refs` doesn't remove it. It
> is also true that if an empty directory gets in the way of the
> creation of a *new* reference, then it is deleted. But otherwise there
> is no systematic cleanup of empty directories.

I had an impression that all of the above are deliberate design decision, perhaps except "pack-refs do not go into an already empty one", as the races between creation into an directory that is about to become empty by ongoing deletion are unpleasant to deal with.

Show 5 quoted lines
> This patch series makes the reference update machinery more aggressive
> about deleting empty directories under $GIT_DIR/refs and under
> $GIT_DIR/logs when deleting references and/or reflogs. This doesn't
> eliminate all situations where empty directories can be left behind
> [3], but it covers the worst offenders.
Good!!  As long as it is done in a correctly non-racy way, that is.
Show 7 quoted lines
> As prelude to the main change, there are a number of patches that make
> the *creation* of reflog directories more robust against races. Since
> we want to delete such directories more aggressively, we have to worry
> more about a race between a process that is creating a new reflog, and
> another process that might be deleting the containing directory at the
> same time. (We already had protection against this sort of race for
> reference creation, but not for reflog creation.)
Yup.
> And since I got tired of writing the same code over and over, I
> abstracted the code for retrying directory creation into a new
> function, raceproof_create_file().
Nice.

I do not think I can get to the meat of the series tonight, but am looking forward to reading it over.

Thanks.

← back to recent threads