From: wanghui <hui.wang@windriver.com>
Date: Thu, 08 Sep 2011 02:01:06 GMT
Subject: Re: [PATCH v3 1/1] sha1_file: normalize alt_odb path before comparing and storing
Message-ID: <4E6821E2.3060900@windriver.com>
In-Reply-To: <7vk49k2nsz.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Wang Hui <Hui.Wang@windriver.com> writes:
>
>   
>> From: Hui Wang <Hui.Wang@windriver.com>
>>
>> When it needs to compare and add an alt object path to the
>> alt_odb_list, we normalize this path first since comparing normalized
>> path is easy to get correct result.
>>
>> Use strbuf to replace some string operations, since it is cleaner and
>> safer.
>>     
>
> Thanks, will queue.
>
>   
>> diff --git a/sha1_file.c b/sha1_file.c
>> index f7c3408..fa2484b 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -248,27 +248,27 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
>> ...
>> +	/* Drop the last '/' from path can make memcmp more accurate */
>> +	if (pathbuf.buf[pfxlen-1] == '/')
>> +		pfxlen -= 1;
>>     
>
> By the way, I do not necessarily agree with the above comment. As long as
> you consistently strip the trailing slashes from all directory paths, or
> you consistently leave a single trailing slash after all directory paths,
> you can get accurate comparison either way.
>
> 	Side note: I tend to prefer keeping a single trailing slash when I
> 	know what we are talking about is a directory in general, because
> 	you do not have to worry about the corner case near the root.
> 	Compare ('/' and '/bin/') vs ('/' and '/bin').
>
> In this particular case, the real reason you want to remove the trailing
> slash is that the invariants of ent->base[] demands it (after all, it
> places another slash immediately after it), and making pathbuf.buf[] an
> empty string (i.e. pfxlen == 0) would still be OK to represent an
> alternate object store at the root level (this function assigns '/' at
> ent->base[pfxlen] immediately before returning, and that '/' names the
> root directory).
>
>   
Yes, your concern is right, I didn't even think about root directory 
situation. If the pathbuf.buf is really '/', stripping last slash will 
make pathbuf.buf a empty string, this will make is_directory() return 
false, this is a bug, how about replace those three line codes to:

/* Except root dir, all paths are stripped the last slash if they have */
if (pathbuf.buf[pfxlen-1] == '/' && pxflen != 1)
pxflen -= 1;

>> +	entlen = pfxlen + 43; /* '/' + 2 hex + '/' + 38 hex + NUL */
>> +	ent = xmalloc(sizeof(*ent) + entlen);
>> +	memcpy(ent->base, pathbuf.buf, pfxlen);
>> +	strbuf_release(&pathbuf);
>>  
>>  	ent->name = ent->base + pfxlen + 1;
>>  	ent->base[pfxlen + 3] = '/';
>>     
>
>