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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano wrote:
Wang Hui <Hui.Wang@xxxxxxxxxxxxx> writes:

From: Hui Wang <Hui.Wang@xxxxxxxxxxxxx>

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

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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]