Re: [PATCH v2 1/5] sha1_file cleanup: remove redundant variable check

From: wanghui <hui.wang@windriver.com>
Date: Sep 7, 2011, 10:24 UTC
Message-ID: <4E67466E.9060100@windriver.com>
In-Reply-To: <7vaaaha9p2.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 23 quoted lines

> 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.

Show 26 quoted lines

>  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"

Show 6 quoted lines

> +		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.

Show 18 quoted lines

>  
> -	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] = '/';
>
>