[PATCH] rollback index if git-commit is interrupted by a signal

[PATCH] rollback index if git-commit is interrupted by a signal

[Paolo Bonzini]

If git-commit is interrupted by a signal, the index.lock file may be left
in the repository.  This patch teaches git to break them, and adds a test.

This will usually happen if you ^Z the editor, and then either close the
terminal or kill git.  However, the patch is more defensive and sets up
the signal handlers so that the entire creation of the index is protected.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
 builtin-commit.c  |   25 ++++++++++++++-----------
 t/t7502-commit.sh |   14 ++++++++++++++
 2 files changed, 28 insertions(+), 11 deletions(-)

	rollback_index_files handles cleanly the case when the lock
	had not been established; git-status tests check for this.

	The test is a bit tricky.  To find git's PID, I use a separate shell
	so that I can "exec" git: git will then inherit the same PID as the
	shell, which I get with $$.  Using a subshell does not work because bash
	optimizes subshells and does not fork a copy of itself -- this however
	means that it will not be able to really honor the "exec" command,
	and git will get a different PID!
	
diff --git a/builtin-commit.c b/builtin-commit.c
index b294c1f..ef8b1f0 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -111,17 +111,8 @@ static struct option builtin_commit_options[] = {
 
 static void rollback_index_files(void)
 {
-	switch (commit_style) {
-	case COMMIT_AS_IS:
-		break; /* nothing to do */
-	case COMMIT_NORMAL:
-		rollback_lock_file(&index_lock);
-		break;
-	case COMMIT_PARTIAL:
-		rollback_lock_file(&index_lock);
-		rollback_lock_file(&false_lock);
-		break;
-	}
+	rollback_lock_file(&index_lock);
+	rollback_lock_file(&false_lock);
 }
 
 static int commit_index_files(void)
@@ -215,6 +206,13 @@ static void create_base_index(void)
 		exit(128); /* We've already reported the error, finish dying */
 }
 
+static void rollback_on_signal(int signo)
+{
+	rollback_index_files();
+	signal(signo, SIG_DFL);
+	raise(signo);
+}
+
 static char *prepare_index(int argc, const char **argv, const char *prefix)
 {
 	int fd;
@@ -235,6 +233,11 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	if (*argv)
 		pathspec = get_pathspec(prefix, argv);
 
+	signal (SIGINT, rollback_on_signal);
+	signal (SIGHUP, rollback_on_signal);
+	signal (SIGTERM, rollback_on_signal);
+	signal (SIGQUIT, rollback_on_signal);
+
 	/*
 	 * Non partial, non as-is commit.
 	 *
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 3531a99..7d5643d 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -212,4 +212,18 @@ test_expect_success 'do not fire editor in the presence of conflicts' '
 	test "`cat .git/result`" = "editor not started"
 '
 
+pwd=`pwd`
+cat > .git/FAKE_EDITOR << EOF
+#! /bin/sh
+# kill -TERM command added below.
+EOF
+
+test_expect_success 'a signal breaks locks' '
+	echo >>negative &&
+	sh -c '\''
+	  echo kill -TERM $$ >> .git/FAKE_EDITOR
+	  GIT_EDITOR=.git/FAKE_EDITOR exec git commit -a'\'' && exit 1  # should fail
+	! test -f .git/index.lock
+'
+
 test_done
-- 
1.5.5

Re: [PATCH] rollback index if git-commit is interrupted by a signal

[Johannes Schindelin]

Hi,

On Thu, 29 May 2008, Paolo Bonzini wrote:

> If git-commit is interrupted by a signal, the index.lock file may be 
> left in the repository.  This patch teaches git to break them, and adds 
> a test.
> 
> This will usually happen if you ^Z the editor, and then either close the 
> terminal or kill git.  However, the patch is more defensive and sets up 
> the signal handlers so that the entire creation of the index is 
> protected.
> 
> Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
> ---
>  builtin-commit.c  |   25 ++++++++++++++-----------
>  t/t7502-commit.sh |   14 ++++++++++++++
>  2 files changed, 28 insertions(+), 11 deletions(-)
> 
> 	rollback_index_files handles cleanly the case when the lock
> 	had not been established; git-status tests check for this.
> 
> 	The test is a bit tricky.  To find git's PID, I use a separate shell
> 	so that I can "exec" git: git will then inherit the same PID as the
> 	shell, which I get with $$.  Using a subshell does not work because bash
> 	optimizes subshells and does not fork a copy of itself -- this however
> 	means that it will not be able to really honor the "exec" command,
> 	and git will get a different PID!
> 	
> diff --git a/builtin-commit.c b/builtin-commit.c
> index b294c1f..ef8b1f0 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -111,17 +111,8 @@ static struct option builtin_commit_options[] = {
>  
>  static void rollback_index_files(void)
>  {
> -	switch (commit_style) {
> -	case COMMIT_AS_IS:
> -		break; /* nothing to do */
> -	case COMMIT_NORMAL:
> -		rollback_lock_file(&index_lock);
> -		break;
> -	case COMMIT_PARTIAL:
> -		rollback_lock_file(&index_lock);
> -		rollback_lock_file(&false_lock);
> -		break;
> -	}
> +	rollback_lock_file(&index_lock);
> +	rollback_lock_file(&false_lock);
>  }

Your commit message gives _no_ good reason for this change.  As a matter 
of fact, I imagine that this could be a regression.

Ciao,
Dscho

Re: [PATCH] rollback index if git-commit is interrupted by a signal

[Paolo Bonzini]

>>  static void rollback_index_files(void)
>>  {
>> -	switch (commit_style) {
>> -	case COMMIT_AS_IS:
>> -		break; /* nothing to do */
>> -	case COMMIT_NORMAL:
>> -		rollback_lock_file(&index_lock);
>> -		break;
>> -	case COMMIT_PARTIAL:
>> -		rollback_lock_file(&index_lock);
>> -		rollback_lock_file(&false_lock);
>> -		break;
>> -	}
>> +	rollback_lock_file(&index_lock);
>> +	rollback_lock_file(&false_lock);
>>  }
> 
> Your commit message gives _no_ good reason for this change.  As a matter 
> of fact, I imagine that this could be a regression.

Without this change, there could be races between the time the lock file 
is created and the time the commit_style variable is set, leading to the 
rollback not being performed.  You're right however about my sloppiness 
in the commit message: I thought the cover letter did explain this,

	rollback_index_files handles cleanly the case when the lock
	had not been established

but what I meant was *rollback_lock_file* "handles cleanly the case when 
the lock had not been established".  In fact, rollback_lock_file is very 
careful:

         if (lk->filename[0]) {
                 if (lk->fd >= 0)
                         close(lk->fd);
                 unlink(lk->filename);
         }
         lk->filename[0] = 0;

and in turn, lock_file never leaves the filename set

         lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
         if (0 <= lk->fd) {
		...
	}
         else
                 lk->filename[0] = 0;

This has always been like this.  It was there in commit 021b6e45, which 
introduces lockfile.c based on index.c; and it was also there in 415e96c 
which introduced index.c.

Paolo

Re: [PATCH] rollback index if git-commit is interrupted by a signal

[Johannes Schindelin]

Hi,

On Thu, 29 May 2008, Paolo Bonzini wrote:

> > >  static void rollback_index_files(void)
> > >  {
> > > -	switch (commit_style) {
> > > -	case COMMIT_AS_IS:
> > > -		break; /* nothing to do */
> > > -	case COMMIT_NORMAL:
> > > -		rollback_lock_file(&index_lock);
> > > -		break;
> > > -	case COMMIT_PARTIAL:
> > > -		rollback_lock_file(&index_lock);
> > > -		rollback_lock_file(&false_lock);
> > > -		break;
> > > -	}
> > > +	rollback_lock_file(&index_lock);
> > > +	rollback_lock_file(&false_lock);
> > >  }
> > 
> > Your commit message gives _no_ good reason for this change.  As a 
> > matter of fact, I imagine that this could be a regression.
> 
> Without this change, there could be races between the time the lock file 
> is created and the time the commit_style variable is set, leading to the 
> rollback not being performed.

IMO it would make much more sense to _guarantee_ that the commity_style 
variable is set before the index is locked.  It is feasible, and there is 
no good reason not to do that.

Ciao,
Dscho

Re: [PATCH] rollback index if git-commit is interrupted by a signal

[Paolo Bonzini]

>> Without this change, there could be races between the time the lock file 
>> is created and the time the commit_style variable is set, leading to the 
>> rollback not being performed.
> 
> IMO it would make much more sense to _guarantee_ that the commity_style 
> variable is set before the index is locked.  It is feasible, and there is 
> no good reason not to do that.

No, it's not possible because the COMMIT_PARTIAL case first creates the 
index_lock and then the false_lock.  It would be curious at least to set 
the commit_style to COMMIT_NORMAL after creating the index_lock, and 
upgrade it to COMMIT_PARTIAL later on.  I contemplated that, and my 
patch is the simplest code that's needed and works.

Paolo

Re: [PATCH] rollback index if git-commit is interrupted by a signal

[Johannes Schindelin]

Hi,

On Thu, 29 May 2008, Paolo Bonzini wrote:

> > IMO it would make much more sense to _guarantee_ that the 
> > commity_style variable is set before the index is locked.  It is 
> > feasible, and there is no good reason not to do that.
> 
> No, it's not possible because the COMMIT_PARTIAL case first creates the 
> index_lock and then the false_lock.

So why don't you fix _that_?

> It would be curious at least to set the commit_style to COMMIT_NORMAL 
> after creating the index_lock, and upgrade it to COMMIT_PARTIAL later 
> on.  I contemplated that, and my patch is the simplest code that's 
> needed and works.

As I said, I think it is a regression, because you change code.  Your 
argument as to why leaves me desiring another solution.

Nuff said,
Dscho

[PATCH v2] rollback lock files on more signals than just SIGINT

[Paolo Bonzini]

Other signals are also common, for example SIGTERM and SIGHUP.
This patch modifies the lock file mechanism to catch more signals.
It also modifies http-push.c which was missing SIGTERM.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
 http-push.c       |    1 +
 lockfile.c        |    3 +++
 t/t7502-commit.sh |   14 ++++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

	>> It would be curious at least to set the commit_style to COMMIT_NORMAL
	>> after creating the index_lock, and upgrade it to COMMIT_PARTIAL later
	>> on.  I contemplated that, and my patch is the simplest code that's 
	>> needed and works.
	> 
	> As I said, I think it is a regression, because you change code.  Your 
	> argument as to why leaves me desiring another solution.
	
	Ok, here it is.  But it's not what you were expecting. :-P

diff --git a/http-push.c b/http-push.c
index f173dcd..c93e781 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2277,6 +2277,7 @@ int main(int argc, char **argv)
 	signal(SIGINT, remove_locks_on_signal);
 	signal(SIGHUP, remove_locks_on_signal);
 	signal(SIGQUIT, remove_locks_on_signal);
+	signal(SIGTERM, remove_locks_on_signal);
 
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
diff --git a/lockfile.c b/lockfile.c
index cfc7335..4023797 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -135,6 +135,9 @@ static int lock_file(struct lock_file *lk, const char *path)
 	if (0 <= lk->fd) {
 		if (!lock_file_list) {
 			signal(SIGINT, remove_lock_file_on_signal);
+			signal(SIGHUP, remove_lock_file_on_signal);
+			signal(SIGTERM, remove_lock_file_on_signal);
+			signal(SIGQUIT, remove_lock_file_on_signal);
 			atexit(remove_lock_file);
 		}
 		lk->owner = getpid();
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 3531a99..46ec1ce 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -212,4 +212,18 @@ test_expect_success 'do not fire editor in the presence of conflicts' '
 	test "`cat .git/result`" = "editor not started"
 '
 
+pwd=`pwd`
+cat > .git/FAKE_EDITOR << EOF
+#! /bin/sh
+# kill -TERM command added below.
+EOF
+
+test_expect_success 'a SIGTERM should break locks' '
+	echo >>negative &&
+	sh -c '\''
+	  echo kill -TERM $$ >> .git/FAKE_EDITOR
+	  GIT_EDITOR=.git/FAKE_EDITOR exec git commit -a'\'' && exit 1  # should fail
+	! test -f .git/index.lock
+'
+
 test_done
-- 
1.5.5

Re: [PATCH v2] rollback lock files on more signals than just SIGINT

[Mike Ralphson]

2008/5/29 Paolo Bonzini <bonzini@gnu.org>:
>
> Other signals are also common, for example SIGTERM and SIGHUP.
> This patch modifies the lock file mechanism to catch more signals.
> It also modifies http-push.c which was missing SIGTERM.
>
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 3531a99..46ec1ce 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -212,4 +212,18 @@ test_expect_success 'do not fire editor in the presence of conflicts' '
>        test "`cat .git/result`" = "editor not started"
>  '
>
> +pwd=`pwd`
> +cat > .git/FAKE_EDITOR << EOF
> +#! /bin/sh
> +# kill -TERM command added below.
> +EOF
> +
> +test_expect_success 'a SIGTERM should break locks' '
> +       echo >>negative &&
> +       sh -c '\''
> +         echo kill -TERM $$ >> .git/FAKE_EDITOR
> +         GIT_EDITOR=.git/FAKE_EDITOR exec git commit -a'\'' && exit 1  # should fail
> +       ! test -f .git/index.lock
> +'
> +
>  test_done

This addition to the testsuite breaks it on AIX with the default sh
(ksh). Replacing the explicit sh -c with $SHELL_PATH -c fixes it for
me (as I have SHELL_PATH pointing to bash). If that's acceptable I can
post a patch if necessary.

Happy to test any other suggested fixes.

Mike

Re: [PATCH v2] rollback lock files on more signals than just SIGINT

[Junio C Hamano]

"Mike Ralphson" <mike.ralphson@gmail.com> writes:

> 2008/5/29 Paolo Bonzini <bonzini@gnu.org>:
> ...
> This addition to the testsuite breaks it on AIX with the default sh
> (ksh). Replacing the explicit sh -c with $SHELL_PATH -c fixes it for
> me (as I have SHELL_PATH pointing to bash). If that's acceptable I can
> post a patch if necessary.

Like the attached one?

I noticed quite a many "sh" dependencies in other test scripts:

 - t0021: rot13.sh does not begin with "#!$SHELL_PATH" nor any "#!"
 - t6026: custom-merge begins with "#!/bin/sh", not "#!$SHELL_PATH".
 - t6030: test_script.sh begins with "#!/bin/sh", not "#!$SHELL_PATH".
 - t7005: e-<editor>.sh do not begin with "#!$SHELL_PATH" nor any "#!"
 - t7402: fake-editor.sh begins with "#!/bin/sh", not "#!$SHELL_PATH".
 - t7502: you noticed
 - t9100: exec.sh begins with "#!/bin/sh", not "#!$SHELL_PATH".

I presume most of the scripts the above incorrectly feeds to "sh" not to
the shell you specify are plain vanilla scripts that not-quite-POSIX
shells can grok them, so in practice this may not be a problem, but we
should eventually fix them.

 t/t7502-commit.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index a5801df..f43c1b1 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -217,14 +217,14 @@ test_expect_success 'do not fire editor in the presence of conflicts' '
 '
 
 pwd=`pwd`
-cat > .git/FAKE_EDITOR << EOF
-#! /bin/sh
+cat >.git/FAKE_EDITOR <<"EOF"
+#! $SHELL_PATH
 # kill -TERM command added below.
 EOF
 
 test_expect_success 'a SIGTERM should break locks' '
 	echo >>negative &&
-	sh -c '\''
+	"$SHELL_PATH" -c '\''
 	  echo kill -TERM $$ >> .git/FAKE_EDITOR
 	  GIT_EDITOR=.git/FAKE_EDITOR exec git commit -a'\'' && exit 1  # should fail
 	! test -f .git/index.lock

Re: [PATCH v2] rollback lock files on more signals than just SIGINT

[Mike Ralphson]

2008/6/4 Junio C Hamano <gitster@pobox.com>:
> "Mike Ralphson" <mike.ralphson@gmail.com> writes:
>
>> 2008/5/29 Paolo Bonzini <bonzini@gnu.org>:
>> ...
>> This addition to the testsuite breaks it on AIX with the default sh
>> (ksh). Replacing the explicit sh -c with $SHELL_PATH -c fixes it for
>> me (as I have SHELL_PATH pointing to bash). If that's acceptable I can
>> post a patch if necessary.
>
> Like the attached one?

Yes, something like that 8-) Thanks for applying. Actually getting
patches out from my work domain without the egregious disclaimer, and
to get them to actually show up in the list is still a bit of a faff
here.

> I noticed quite a many "sh" dependencies in other test scripts:

I'm happy to pick up a bit of janitor work on the tests after this
release. I notice we have a few non-executable test scripts, and
several 'duplicate' test numbers which might also want to be looked
at?

Cheers, Mike