On Thu, Aug 30, 2007 at 11:50:51PM +0100, Andrew Fyfe wrote:
Xavier wrote:
That causes another problem though... in _alpm_trans_release, we have :
184 /* during a commit do not interrupt immediately, just after a target */ 185 if(trans->state == STATE_COMMITING || trans->state == STATE_INTERRUPTED) { 186 if(trans->state == STATE_COMMITING) { 187 trans->state = STATE_INTERRUPTED; 188 } 189 pm_errno = PM_ERR_TRANS_COMMITING; 190 return(-1); 191 }
So since the state now stays on STATE_COMMITING (without going back to STATE_PREPARED) when a transaction fails, this code is now run, and the function returns here, without removing the lock.
Now, I'm totally unable to fix this correctly, the thing is, I really don't understand the purpose of this part, what it's intended to do, in which cases, etc ...
It annoyed me that pacman didn't remove the lock on each error (for example, conflicting targets), so I had to hack this, but that most likely kills totally what the original author had in mind, making the code worse than it is.
I guess what I'm trying to say here is that I need someone to look at this, and fix it correctly. Maybe I could revert that previous change in sync.c, so that the state goes back to STATE_PREPARED on failure, and then do the same in add.c / remove.c , but I still don't like that idea..
I think this is another +1 for a rewrite. It seems for every bug we fix another harder to fix one pops up to take its place.
Andrew
Right, probably. But well, it at least requires a temporary fix in the meantime. I found where this part I don't understand was introduced. Fortunately, it is a small commit. Maybe looking at the commit as the whole, and the one line description can help understand it : http://projects.archlinux.org/git/?p=pacman.git;a=commit;h=54008798efcc9646f...