From: Bradford Smith <bradford.carl.smith@gmail.com>
Date: Thu, 26 Jul 2007 18:48:49 GMT
Subject: Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()
Message-ID: <f158199e0707261148r29419a39h7d83fc7bd0ea7df1@mail.gmail.com>
In-Reply-To: <Pine.LNX.4.64.0707261926590.14781@racer.site>

On 7/26/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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