Browse code

ignore stale directories when checking reflog existence

When we update a ref, we have two rules for whether or not
we actually update the reflog:

1. If the reflog already exists, we will always append to
it.

2. If log_all_ref_updates is set, we will create a new
reflog file if necessary.

We do the existence check by trying to open the reflog file,
either with or without O_CREAT (depending on log_all_ref_updates).
If it fails, then we check errno to see what happened.

If we were not using O_CREAT and we got ENOENT, the file
doesn't exist, and we return success (there isn't a reflog
already, and we were not told to make a new one).

If we get EISDIR, then there is likely a stale directory
that needs to be removed (e.g., there used to be "foo/bar",
it was deleted, and the directory "foo" was left. Now we
want to create the ref "foo"). If O_CREAT is set, then we
catch this case, try to remove the directory, and retry our
open. So far so good.

But if we get EISDIR and O_CREAT is not set, then we treat
this as any other error, which is not right. Like ENOENT,
EISDIR is an indication that we do not have a reflog, and we
should silently return success (we were not told to create
it). Instead, the current code reports this as an error, and
we fail to update the ref at all.

Note that this is relatively unlikely to happen, as you
would have to have had reflogs turned on, and then later
turned them off (it could also happen due to a bug in fetch,
but that was fixed in the previous commit). However, it's
quite easy to fix: we just need to treat EISDIR like ENOENT
for the non-O_CREAT case, and silently return (note that
this early return means we can also simplify the O_CREAT
case).

Our new tests cover both cases (O_CREAT and non-O_CREAT).
The first one already worked, of course.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King authored on 04/11/2014 13:24:53
Showing 2 changed files
... ...
@@ -2751,10 +2751,10 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
2751 2751
 
2752 2752
 	logfd = open(logfile, oflags, 0666);
2753 2753
 	if (logfd < 0) {
2754
-		if (!(oflags & O_CREAT) && errno == ENOENT)
2754
+		if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))
2755 2755
 			return 0;
2756 2756
 
2757
-		if ((oflags & O_CREAT) && errno == EISDIR) {
2757
+		if (errno == EISDIR) {
2758 2758
 			if (remove_empty_directories(logfile)) {
2759 2759
 				return error("There are still logs under '%s'",
2760 2760
 					     logfile);
... ...
@@ -245,4 +245,38 @@ test_expect_success 'gc.reflogexpire=false' '
245 245
 
246 246
 '
247 247
 
248
+test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '
249
+	test_when_finished "git branch -d a || git branch -d a/b" &&
250
+
251
+	git branch a/b master &&
252
+	echo "a/b@{0} branch: Created from master" >expect &&
253
+	git log -g --format="%gd %gs" a/b >actual &&
254
+	test_cmp expect actual &&
255
+	git branch -d a/b &&
256
+
257
+	# now logs/refs/heads/a is a stale directory, but
258
+	# we should move it out of the way to create "a" reflog
259
+	git branch a master &&
260
+	echo "a@{0} branch: Created from master" >expect &&
261
+	git log -g --format="%gd %gs" a >actual &&
262
+	test_cmp expect actual
263
+'
264
+
265
+test_expect_success 'stale dirs do not cause d/f conflicts (reflogs off)' '
266
+	test_when_finished "git branch -d a || git branch -d a/b" &&
267
+
268
+	git branch a/b master &&
269
+	echo "a/b@{0} branch: Created from master" >expect &&
270
+	git log -g --format="%gd %gs" a/b >actual &&
271
+	test_cmp expect actual &&
272
+	git branch -d a/b &&
273
+
274
+	# same as before, but we only create a reflog for "a" if
275
+	# it already exists, which it does not
276
+	git -c core.logallrefupdates=false branch a master &&
277
+	: >expect &&
278
+	git log -g --format="%gd %gs" a >actual &&
279
+	test_cmp expect actual
280
+'
281
+
248 282
 test_done