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

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

[jidanni]

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

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Fri, Dec 19, 2008 at 04:02:48AM +0800, jidanni@jidanni.org 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:

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b7d46e5..1581691 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Fri, Dec 19, 2008 at 04:02:48AM +0800, jidanni@jidanni.org 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:
>
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index b7d46e5..1581691 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -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.

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.

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Thu, Dec 18, 2008 at 04:35:54PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
> 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". ;-)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Fri, Dec 19, 2008 at 04:02:48AM +0800, jidanni@jidanni.org 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:
>
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index b7d46e5..1581691 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -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.

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(-)

diff --git i/lockfile.c w/lockfile.c
index 6d75608..8589155 100644
--- i/lockfile.c
+++ w/lockfile.c
@@ -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();

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

[Junio C Hamano]

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 <gitster@pobox.com>
---
 lockfile.c    |    1 +
 t/t3600-rm.sh |   17 +++++++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git c/lockfile.c w/lockfile.c
index 6d75608..8589155 100644
--- c/lockfile.c
+++ w/lockfile.c
@@ -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();
diff --git c/t/t3600-rm.sh w/t/t3600-rm.sh
index b7d46e5..95542e9 100755
--- c/t/t3600-rm.sh
+++ w/t/t3600-rm.sh
@@ -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