Re: [PATCH 2/4] Convert resolve_ref+xstrdup to new resolve_refdup function

[Michael Haggerty <mhagger@alum.mit.edu>]

On 12/12/2011 07:07 PM, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> 
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> 
> Thanks.
> 
> The patch looks correct but I have a slight maintainability concern and a
> suggestion for possible improvement.
> 
>> ...
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index b7c6302..a66d3eb 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>>  {
>>  	int ret = 0;
>>  	struct branch_info old;
>> +	char *path;
>>  	unsigned char rev[20];
>>  	int flag;
>>  	memset(&old, 0, sizeof(old));
>> -	old.path = resolve_ref("HEAD", rev, 0, &flag);
>> -	if (old.path)
>> -		old.path = xstrdup(old.path);
>> +	old.path = path = resolve_refdup("HEAD", rev, 0, &flag);
> 
> This uses "one 'const char *' pointer that is used for reading data from
> and an extra 'char *' pointer that is used only for freeing" approach,
> which has two advantages and one disadvantage:
> [...]
> When naming a "for-freeing" pointer variable, the kind of data the area of
> memory happens to contain is of secondary importance. Being deliberately
> vague about what the area of memory may contain is a good thing, because
> it actively discourages the program from looking at the area via the
> pointer if the variable is named "to_free" or something that does not
> specify what it contains.

The to_free variable could even be declared void* to make it even less
(ab)usable.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/