Re: [PATCH] Fix infinite loop when deleting multiple packed refs.

[Junio C Hamano <junkio@cox.net>]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Nicholas Miell reported that `git branch -D A B` failed if both refs
> A and B were packed into .git/packed-refs.  This happens because the
> same pack_lock instance was being enqueued into the lock list twice,

I do not like this, actually.

It was stupid to link the same element twice to lock_file_list
and end up in a loop, so we certainly need a fix.

But it is not like we are taking a lock on multiple files in
this case.  It is just that we leave the linked list element on
the list even after commit_lock_file() successfully removes the
cruft.

We obviously cannot remove the list element commit_lock_file();
if we are interrupted in the middle of list manipulation, the
call to remove_lock_file_on_signal will happen with a broken
lock_file_list, which would cause the cruft to remain, so not
unlinking is the right thing to do.  Instead we should be
reusing the element already on the list.

I notice that there is already a code for that in lock_file()
function in lockfile.c.  Notice that lk->next is checked and the
element is linked only when it is not already on the list?  I
think the check is wrong for the last element on the list which
has NULL in next but is on the list, but if you read the check
as "is this element already on the list?" it actually makes
sense.  We do not want to link it on the list again, nor we
would want to set up signal/atexit over and over.

In other words, I am suspecting this might be a cleaner fix.

-- >8 --

diff --git a/cache.h b/cache.h
index a5fc232..4dbf658 100644
--- a/cache.h
+++ b/cache.h
@@ -179,6 +179,7 @@ extern int refresh_cache(unsigned int flags);
 
 struct lock_file {
 	struct lock_file *next;
+	char on_list;
 	char filename[PATH_MAX];
 };
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
diff --git a/lockfile.c b/lockfile.c
index 261baff..731bbf3 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -27,9 +27,12 @@ static int lock_file(struct lock_file *lk, const char *path)
 	sprintf(lk->filename, "%s.lock", path);
 	fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= fd) {
-		if (!lk->next) {
+		if (!lk->on_list) {
 			lk->next = lock_file_list;
 			lock_file_list = lk;
+			lk->on_list = 1;
+		}
+		if (lock_file_list) {
 			signal(SIGINT, remove_lock_file_on_signal);
 			atexit(remove_lock_file);
 		}
@@ -37,6 +40,8 @@ static int lock_file(struct lock_file *lk, const char *path)
 			return error("cannot fix permission bits on %s",
 				     lk->filename);
 	}
+	else
+		lk->filename[0] = 0;
 	return fd;
 }
 
diff --git a/refs.c b/refs.c
index 121774c..5205745 100644
--- a/refs.c
+++ b/refs.c
@@ -726,7 +726,6 @@ static int repack_without_ref(const char *refname)
 	}
 	if (!found)
 		return 0;
-	memset(&packlock, 0, sizeof(packlock));
 	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
 	if (fd < 0)
 		return error("cannot delete '%s' from packed refs", refname);