Re: [PATCH 5/5] rename_ref(): fix a mkdir()/rmdir() race

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 26, 2013, 23:20 UTC
Message-ID: <20131226232023.GA20443@google.com>
In-Reply-To: <1387696451-32224-6-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty wrote:

>  refs.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

A test or example reproduction recipe would be nice. (But I can understand not having one --- races are hard to test.)

[...]

> --- a/refs.c
> +++ b/refs.c

[...]

Show 11 quoted lines

> @@ -2574,6 +2575,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  			}
>  			goto retry;
>  		} else {
> +			if (errno == ENOENT && --attempts)
> +				/*
> +				 * Perhaps somebody just pruned the empty
> +				 * directory into which we wanted to move the
> +				 * file.
> +				 */
> +				goto retry;

Style nit: it's easier to read a test of errno when the 'else's cascade (i.e., using 'else if' here).

This patch doesn't depend on any of the others from the series. For what it's worth, with or without the following squashed in,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

diff --git i/refs.c w/refs.c
index 3ab1491..ea62395 100644
--- i/refs.c
+++ w/refs.c
@@ -2574,14 +2574,14 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 				goto rollback;
 			}
 			goto retry;
+		} else if (errno == ENOENT && --attempts)
+			/*
+			 * Perhaps somebody just pruned the empty
+			 * directory into which we wanted to move the
+			 * file.
+			 */
+			goto retry;
 		} else {
-			if (errno == ENOENT && --attempts)
-				/*
-				 * Perhaps somebody just pruned the empty
-				 * directory into which we wanted to move the
-				 * file.
-				 */
-				goto retry;
 			error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
 				newrefname, strerror(errno));
 			goto rollback;