Re: [PATCH v3 1/1] sha1_file: normalize alt_odb path before comparing and storing

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 7, 2011, 18:46 UTC
Message-ID: <7vk49k2nsz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1315391867-31277-2-git-send-email-Hui.Wang@windriver.com>

Wang Hui <Hui.Wang@windriver.com> writes:

Show 8 quoted lines

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

Show 9 quoted lines

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

Show 7 quoted lines

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