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

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

[Wang Hui]

Hi Junio & Martin,

In the V3, i prepared a patch basing on Junio's suggestion. If this patch can
pass review and be merged to upstream. What about the last two patches in the
V2 (remove relative alternates only possible at current repos limitation), do
you think it is safe to pick up these two patches?

Hui Wang (1):
      sha1_file: normalize alt_odb path before comparing and storing

 sha1_file.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

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

[Wang Hui]

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Hui Wang <Hui.Wang@windriver.com>
---
 sha1_file.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

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
 	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, real_path(relative_base));
+		strbuf_addch(&pathbuf, '/');
 	}
-	ent = xmalloc(sizeof(*ent) + entlen);
+	strbuf_add(&pathbuf, entry, len);
 
-	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);
+	normalize_path_copy(pathbuf.buf, pathbuf.buf);
+
+	pfxlen = strlen(pathbuf.buf);
+
+	/* Drop the last '/' from path can make memcmp more accurate */
+	if (pathbuf.buf[pfxlen-1] == '/')
+		pfxlen -= 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] = '/';
-- 
1.6.3.1

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

[Junio C Hamano]

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

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

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

[wanghui]

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