threads / patch / 28328

v3sha1_file: normalize alt_odb path before comparing and storing

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

## tl;dr

4 messages between Sep 7, 2011 and Sep 8, 2011. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Wang Hui· Sep 7, 2011, 10:37 UTC · lore

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

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(-)
Wang Hui· Sep 7, 2011, 10:37 UTC · re: Wang Hui · lore
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(-)
Show changes to sha1_file.c +17 −17
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
Junio C Hamano· Sep 7, 2011, 18:46 UTC · re: Wang Hui · lore

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

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] = '/';
wanghui· Sep 8, 2011, 02:01 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 46 quoted lines
> 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;

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

← back to recent threads