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

From: Paolo Bonzini <paolo.bonzini@gmail.com>
Date: May 29, 2008, 13:19 UTC
Message-ID: <483EAD69.9090001@gnu.org>
In-Reply-To: <alpine.DEB.1.00.0805291341290.13507@racer.site.net>

Show 19 quoted lines

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