Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()

On 7/26/07, Johannes Schindelin [off-list ref] wrote:

On Thu, 26 Jul 2007, Bradford C. Smith wrote:

quoted

+     /* 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