From: Paolo Bonzini <paolo.bonzini@gmail.com>
Date: Thu, 29 May 2008 13:19:37 GMT
Subject: Re: [PATCH] rollback index if git-commit is interrupted by a signal
Message-ID: <483EAD69.9090001@gnu.org>
In-Reply-To: <alpine.DEB.1.00.0805291341290.13507@racer.site.net>


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