From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sat, 10 Dec 2011 20:55:19 GMT
Subject: Re: [PATCH 3/3] Rename resolve_ref() to resolve_ref_unsafe()
Message-ID: <20111210205519.GA24817@elie.hsd1.il.comcast.net>
In-Reply-To: <1323521631-24320-3-git-send-email-pclouds@gmail.com>

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

> resolve_ref() may return a pointer to a shared buffer and can be
> overwritten by the next resolve_ref() calls. Callers need to
> pay attention, not to keep the pointer when the next call happens.
[...]
> --- a/branch.c
> +++ b/branch.c
> @@ -182,7 +182,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
>  		const char *head;
>  		unsigned char sha1[20];
>  
> -		head = resolve_ref("HEAD", sha1, 0, NULL);
> +		head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);

I wonder if it would make sense to have a separate function that
maintains a shared buffer describing what HEAD resolves to, lazily
loaded.  Would invalidating the cache when appropriate be too fussy
and subtle?

[...]
> +++ b/transport.c
> @@ -163,7 +163,7 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
>  		/* Follow symbolic refs (mainly for HEAD). */
>  		localname = ref->peer_ref->name;
>  		remotename = ref->name;
> -		tmp = resolve_ref(localname, sha, 1, &flag);
> +		tmp = resolve_ref_unsafe(localname, sha, 1, &flag);

Anyway, this patch looks sane.  The reminder seems useful and doesn't
feel over-the-top.