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

Wang Hui [off-list ref] writes:

From: Hui Wang <redacted>

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.

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

Junio C Hamano wrote:

Wang Hui [off-list ref] writes:

  

quoted

From: Hui Wang <redacted>

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.

  

quoted

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;

quoted

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