From: Jeff King <peff@peff.net>
Date: Wed, 21 Jan 2015 23:44:28 GMT
Subject: Re: [PATCH 5/6] refs.c: write to a lock file only once
Message-ID: <20150121234428.GD11115@peff.net>
In-Reply-To: <1421882625-916-6-git-send-email-sbeller@google.com>

On Wed, Jan 21, 2015 at 03:23:44PM -0800, Stefan Beller wrote:

> +	const char *sha1_lf;
>  
>  	if (!lock) {
>  		errno = EINVAL;
> @@ -3104,8 +3104,9 @@ static int write_ref_sha1(struct ref_lock *lock,
>  		errno = EINVAL;
>  		return -1;
>  	}
> -	if (write_in_full_to_lockfile(lock->lk, sha1_to_hex(sha1), 40) != 40 ||
> -	    write_in_full_to_lockfile(lock->lk, &term, 1) != 1 ||
> +
> +	sha1_lf = xstrfmt("%s\n",  sha1_to_hex(sha1));
> +	if (write_in_full_to_lockfile(lock->lk, sha1_lf, 41) != 41 ||
>  	    close_ref(lock) < 0) {
>  		int save_errno = errno;
>  		error("Couldn't write %s", lock->lk->filename.buf);
> @@ -3113,6 +3114,7 @@ static int write_ref_sha1(struct ref_lock *lock,
>  		errno = save_errno;
>  		return -1;
>  	}
> +	free((void*)sha1_lf);

It looks like you leak sha1_lf in the error code path here.

It's a shame that we must allocate at all, when we are really just
passing through to a buffer. Lockfiles have a "FILE *" pointer these
days. Could we just allow:

  lockfile_printf(lock->lk, "%s\n", sha1_to_hex(sha1));

or similar?

-Peff