On 7/26/07, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote:
On Thu, 26 Jul 2007, Bradford C. Smith wrote: > + /* fd is closed, so don't try to close it below. */ > + fd = -1; > + /* > + * lock is committed, so don't try to roll it back below. > + * NOTE: Since lockfile.c keeps a linked list of all created > + * lock_file structures, it isn't safe to free(lock). It's > + * better to just leave it hanging around. > + */ > + lock = NULL; > ret = 0; > > out_free: > if (0 <= fd) > close(fd); > + if (lock) > + rollback_lock_file(lock); Wouldn't it be better to put the rollback_lock_file() into the if clause when commit failed?
Actually no. There are multiple goto statements that lead to out_free. It isn't even needed at the point that the commit failed, because commit_lock_file() sets the lock file name to "" even when it fails.
Besides, I think you can safely call rollback_lock_file(lock) on a committed lock_file, since the name will be set to "" by the latter, which is checked by the former.
Quite right. I really just put in the comment and 'lock= NULL' line to increase readability. I wanted to make it very clear to the reader that the commit wouldn't be undone by the rollback.
But I am fine with the patch as is (have not tested it, though).
Thanks! FWIW, I have successfully run 'make test' and also verified that it behaves as I expect with my ~/.gitconfig symlink (in conjunction with the my other patch for resolving symlinks). Best Regards, Bradford - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
