From: wanghui <hui.wang@windriver.com>
Date: Wed, 07 Sep 2011 10:24:46 GMT
Subject: Re: [PATCH v2 1/5] sha1_file cleanup: remove redundant variable check
Message-ID: <4E67466E.9060100@windriver.com>
In-Reply-To: <7vaaaha9p2.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Wang Hui <Hui.Wang@windriver.com> writes:
>
>   
>> From: Hui Wang <Hui.Wang@windriver.com>
>>
>> This variable check is always true, so it is redundant and need to be
>> removed.
>>
>> We can't remove the init value for this variable, since removing
>> it will introduce building warning:
>> 'base_len' may be used uninitialized in this function.
>>     
>
> If we are into cleaning things up, we should instead notice and say "yuck"
> to the repeated "is entry is absolute and relative base is given" check.
>
> Wouldn't something like this makes things easier to follow and also avoids
> the "when does the path normalized and made absolute" issue?
>
> Completely untested and I may have off-by-one errors and such, but I think
> you would get the idea...
>
>   
Yes, i got the idea, and some errors are pointed out below. I will 
prepare a V3 patch basing on it.
>  sha1_file.c |   29 ++++++++++++-----------------
>  1 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index e002056..26aa3be 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -248,27 +248,22 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
>  	const char *objdir = get_object_directory();
>  	struct alternate_object_database *ent;
>  	struct alternate_object_database *alt;
> -	/* 43 = 40-byte + 2 '/' + terminating NUL */
> -	int pfxlen = len;
> -	int entlen = pfxlen + 43;
> -	int base_len = -1;
> +	int pfxlen, entlen;
> +	struct strbuf pathbuf = STRBUF_INIT;
>  
>  	if (!is_absolute_path(entry) && relative_base) {
> -		/* Relative alt-odb */
> -		if (base_len < 0)
> -			base_len = strlen(relative_base) + 1;
> -		entlen += base_len;
> -		pfxlen += base_len;
> +		strbuf_addstr(&pathbuf, relative_base);
>   
s/relative_base/real_path(relative_base)/ is better, since 
normalize_path_copy is not good at handle relative path. e.g. ". 
./objects/../../test2/objects" will be normalized to "objects"
> +		strbuf_addch(&pathbuf, '/');
>  	}
> -	ent = xmalloc(sizeof(*ent) + entlen);
> +	strbuf_add(&pathbuf, entry, len);
> +	normalize_path_copy(pathbuf.buf, pathbuf.buf);
>   
if pathbuf.buf[strlen(pathbuf.buf)] is '/', remove it. this can avoid to 
get a wrong result when comparing "/a/b" with "/a/b/".
> +	strbuf_setlen(&pathbuf, strlen(pathbuf.buf));
>   
above line can be removed, since we will release pathbuf soon.

>  
> -	if (!is_absolute_path(entry) && relative_base) {
> -		memcpy(ent->base, relative_base, base_len - 1);
> -		ent->base[base_len - 1] = '/';
> -		memcpy(ent->base + base_len, entry, len);
> -	}
> -	else
> -		memcpy(ent->base, entry, pfxlen);
> +	pfxlen = pathbuf.len;
> +	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] = '/';
>
>