From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 17 Nov 2011 10:39:24 GMT
Subject: Re: [PATCH 0/8] nd/resolve-ref v2
Message-ID: <20111117103924.GA5277@elie.hsd1.il.comcast.net>
In-Reply-To: <1321522335-24193-1-git-send-email-pclouds@gmail.com>

Hi,

Nguyễn Thái Ngọc Duy wrote:

> Nguyễn Thái Ngọc Duy (8):

Here are some comments from a quick glance over the series, to avoid
too much noise that would distract from reports about the upcoming
release.

>   Convert many resolve_ref() calls to read_ref*() and ref_exists()
>   Rename resolve_ref() to resolve_ref_unsafe()

I like the general direction of the series (and especially patch 1/8).
As Junio nicely explains at [1], it is too tempting to keep and pass
around the canonical representation of a refname returned by
resolve_ref() without remembering to copy it.

>   Re-add resolve_ref() that always returns an allocated buffer

Makes me nervous, since it would introduce memory leaks if some other
patch in flight calls resolve_ref().  Why not call it ref_resolve() or
something?

>   cmd_merge: convert to single exit point
>   Use resolve_ref() instead of resolve_ref_unsafe()

The print_summary() change introduces a leak.

[...]
>   Guard memory overwriting in resolve_ref_unsafe's static buffer
>   Enable GIT_DEBUG_MEMCHECK on git_pathname()

__VA_ARGS__ was introduced in C99.  I suspect some compilers that
currently can compile git don't support it.  But if you need to use
it, that wouldn't rule out doing so in some corner guarded with an
#ifdef.

Looks pleasant overall.  I look forward to looking more closely at
this later.

Ciao,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/185446/focus=185519