git-rm -n leaves .git/index.lock if not allowed to finish

DISCUSSION [email protected] 3 participants 6 messages Jun 15, 2016

Share & tools

[email protected]

author

2016-06-15 22:45 UTC #0 lore raw

Bug: if git-rm -n is not allowed to write all it wants to write, it will leave a .git/index.lock file:

# git-rm -n -r . 2>&1|sed q
error: '.etckeeper' has changes staged in the index
# git-rm -n -r . 2>&1|sed q
fatal: unable to create '.git/index.lock': File exists

Miklos Vajna

2016-06-15 22:45 UTC #1 lore raw

Quoted 6 lines · On Fri, Dec 19, 2008 at 04:02:48AM +0800, [email protected] wrote:

Bug: if git-rm -n is not allowed to write all it wants to write, it will leave a .git/index.lock file:
# git-rm -n -r . 2>&1|sed q
error: '.etckeeper' has changes staged in the index
# git-rm -n -r . 2>&1|sed q
fatal: unable to create '.git/index.lock': File exists

Can't reproduce:

t/t3600-rm.sh +8 -0

index b7d46e5..1581691 100755
@@ -251,4 +251,12 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 
 '
 
+test_expect_success 'test from jidanni' '
+
+	git reset --hard &&
+	git rm -n -r . 2>&1|sed q &&
+	git rm -n -r . 2>&1|sed q
+
+'
+
 test_done

passes here just fine. Yes, there are multiple files in the repo.

Junio C Hamano

2016-06-15 22:45 UTC #2 lore raw

Quoted 28 lines · Miklos Vajna

Quoted 6 lines · On Fri, Dec 19, 2008 at 04:02:48AM +0800, [email protected] wrote:
Bug: if git-rm -n is not allowed to write all it wants to write, it will leave a .git/index.lock file:
# git-rm -n -r . 2>&1|sed q
error: '.etckeeper' has changes staged in the index
# git-rm -n -r . 2>&1|sed q
fatal: unable to create '.git/index.lock': File exists
Can't reproduce:
t/t3600-rm.sh +8 -0
index b7d46e5..1581691 100755
@@ -251,4 +251,12 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 
 '
 
+test_expect_success 'test from jidanni' '
+
+	git reset --hard &&
+	git rm -n -r . 2>&1|sed q &&
+	git rm -n -r . 2>&1|sed q
+
+'
+
 test_done

Quoted 1 line

passes here just fine. Yes, there are multiple files in the repo.

I think you need to have tons of files to cause the pipe buffer to fill up with the "rm 'frotz'" output, triggering a SIGPIPE to kill the upstream process of the pipe.

Would this patch help?

 lockfile.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

lockfile.c +1 -0

index 6d75608..8589155 100644
@@ -140,6 +140,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 			signal(SIGHUP, remove_lock_file_on_signal);
 			signal(SIGTERM, remove_lock_file_on_signal);
 			signal(SIGQUIT, remove_lock_file_on_signal);
+			signal(SIGPIPE, remove_lock_file_on_signal);
 			atexit(remove_lock_file);
 		}
 		lk->owner = getpid();

Junio C Hamano

2016-06-15 22:45 UTC #3 lore raw

Subject: [PATCH] Make sure lockfiles are unlocked when dying on SIGPIPE

We cleaned up lockfiles upon receiving the usual suspects HUP, TERM, QUIT but a wicked user could kill us of asphyxiation by piping our output to a pipe that does not read. Protect ourselves by catching SIGPIPE and clean up the lockfiles as well in such a case.

Signed-off-by Junio C Hamano <[email protected]>

---
 lockfile.c    |    1 +
 t/t3600-rm.sh |   17 +++++++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

lockfile.c +1 -0

index 6d75608..8589155 100644
@@ -140,6 +140,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 			signal(SIGHUP, remove_lock_file_on_signal);
 			signal(SIGTERM, remove_lock_file_on_signal);
 			signal(SIGQUIT, remove_lock_file_on_signal);
+			signal(SIGPIPE, remove_lock_file_on_signal);
 			atexit(remove_lock_file);
 		}
 		lk->owner = getpid();

t/t3600-rm.sh +17 -0

index b7d46e5..95542e9 100755
@@ -251,4 +251,21 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 
 '
 
+test_expect_success 'choking "git rm" should not let it die with cruft' '
+	git reset -q --hard &&
+	H=0000000000000000000000000000000000000000 &&
+	i=0 &&
+	while test $i -lt 12000
+	do
+	    echo "100644 $H 0	some-file-$i"
+	    i=$(( $i + 1 ))
+	done | git update-index --index-info &&
+	git rm -n "some-file-*" | :;
+	test -f .git/index.lock
+	status=$?
+	rm -f .git/index.lock
+	git reset -q --hard
+	test "$status" != 0
+'
+
 test_done

Junio C Hamano

2016-06-15 22:45 UTC #4 lore raw

In reply to Miklos Vajna

Quoted 28 lines · Miklos Vajna

Quoted 6 lines · On Fri, Dec 19, 2008 at 04:02:48AM +0800, [email protected] wrote:
Bug: if git-rm -n is not allowed to write all it wants to write, it will leave a .git/index.lock file:
# git-rm -n -r . 2>&1|sed q
error: '.etckeeper' has changes staged in the index
# git-rm -n -r . 2>&1|sed q
fatal: unable to create '.git/index.lock': File exists
Can't reproduce:
t/t3600-rm.sh +8 -0
index b7d46e5..1581691 100755
@@ -251,4 +251,12 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 
 '
 
+test_expect_success 'test from jidanni' '
+
+	git reset --hard &&
+	git rm -n -r . 2>&1|sed q &&
+	git rm -n -r . 2>&1|sed q
+
+'
+
 test_done

Quoted 1 line

passes here just fine. Yes, there are multiple files in the repo.

I think you need to have tons of files to cause the pipe buffer to fill up with the "rm 'frotz'" output, triggering a SIGPIPE to kill the upstream process of the pipe.

Miklos Vajna

2016-06-15 22:45 UTC #5 lore raw

Quoted 3 lines · Junio C Hamano

I think you need to have tons of files to cause the pipe buffer to fill up with the "rm 'frotz'" output, triggering a SIGPIPE to kill the upstream process of the pipe.

Ah, you are right. I did exactly what is in the lockfile.c part of your patch, but I did not think about "if I don't get a SIGPIPE for 2 lines, I may get one for 1000 lines". ;-)