{"thread":{"id":"28328","subject":"[PATCH v3 1/1] sha1_file: normalize alt_odb path before comparing and storing","startedAt":"2011-09-07T10:37:46Z","lastAt":"2011-09-08T02:01:06Z","messageCount":4,"participants":["Wang Hui","Junio C Hamano","wanghui"],"isPatch":true,"patchVersion":3,"patchTotal":1},"messages":[{"id":"175003","messageId":"1315391867-31277-1-git-send-email-Hui.Wang@windriver.com","threadId":"28328","inReplyTo":null,"subject":"[PATCH v3 0/1] sha1_file: normalize alt_odb path before comparing and storing","fromName":"Wang Hui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-07T10:37:46Z","receivedAt":"2011-09-07T10:37:46Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"Hi Junio & Martin,\n\nIn the V3, i prepared a patch basing on Junio's suggestion. If this patch can\npass review and be merged to upstream. What about the last two patches in the\nV2 (remove relative alternates only possible at current repos limitation), do\nyou think it is safe to pick up these two patches?\n\nHui Wang (1):\n sha1_file: normalize alt_odb path before comparing and storing\n\n sha1_file.c | 34 +++++++++++++++++-----------------\n 1 files changed, 17 insertions(+), 17 deletions(-)\n"},{"id":"175002","messageId":"1315391867-31277-2-git-send-email-Hui.Wang@windriver.com","threadId":"28328","inReplyTo":"1315391867-31277-1-git-send-email-Hui.Wang@windriver.com","subject":"[PATCH v3 1/1] sha1_file: normalize alt_odb path before comparing and storing","fromName":"Wang Hui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-07T10:37:47Z","receivedAt":"2011-09-07T10:37:47Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"From: Hui Wang \n\nWhen it needs to compare and add an alt object path to the\nalt_odb_list, we normalize this path first since comparing normalized\npath is easy to get correct result.\n\nUse strbuf to replace some string operations, since it is cleaner and\nsafer.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Hui Wang \n---\n sha1_file.c | 34 +++++++++++++++++-----------------\n 1 files changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f7c3408..fa2484b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -248,27 +248,27 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \tconst char *objdir = get_object_directory();\n \tstruct alternate_object_database *ent;\n \tstruct alternate_object_database *alt;\n-\t/* 43 = 40-byte + 2 '/' + terminating NUL */\n-\tint pfxlen = len;\n-\tint entlen = pfxlen + 43;\n-\tint base_len = -1;\n+\tint pfxlen, entlen;\n+\tstruct strbuf pathbuf = STRBUF_INIT;\n \n \tif (!is_absolute_path(entry) && relative_base) {\n-\t\t/* Relative alt-odb */\n-\t\tif (base_len < 0)\n-\t\t\tbase_len = strlen(relative_base) + 1;\n-\t\tentlen += base_len;\n-\t\tpfxlen += base_len;\n+\t\tstrbuf_addstr(&pathbuf, real_path(relative_base));\n+\t\tstrbuf_addch(&pathbuf, '/');\n \t}\n-\tent = xmalloc(sizeof(*ent) + entlen);\n+\tstrbuf_add(&pathbuf, entry, len);\n \n-\tif (!is_absolute_path(entry) && relative_base) {\n-\t\tmemcpy(ent->base, relative_base, base_len - 1);\n-\t\tent->base[base_len - 1] = '/';\n-\t\tmemcpy(ent->base + base_len, entry, len);\n-\t}\n-\telse\n-\t\tmemcpy(ent->base, entry, pfxlen);\n+\tnormalize_path_copy(pathbuf.buf, pathbuf.buf);\n+\n+\tpfxlen = strlen(pathbuf.buf);\n+\n+\t/* Drop the last '/' from path can make memcmp more accurate */\n+\tif (pathbuf.buf[pfxlen-1] == '/')\n+\t\tpfxlen -= 1;\n+\n+\tentlen = pfxlen + 43; /* '/' + 2 hex + '/' + 38 hex + NUL */\n+\tent = xmalloc(sizeof(*ent) + entlen);\n+\tmemcpy(ent->base, pathbuf.buf, pfxlen);\n+\tstrbuf_release(&pathbuf);\n \n \tent->name = ent->base + pfxlen + 1;\n \tent->base[pfxlen + 3] = '/';\n-- \n1.6.3.1\n"},{"id":"175022","messageId":"7vk49k2nsz.fsf@alter.siamese.dyndns.org","threadId":"28328","inReplyTo":"1315391867-31277-2-git-send-email-Hui.Wang@windriver.com","subject":"Re: [PATCH v3 1/1] sha1_file: normalize alt_odb path before comparing and storing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-07T18:46:20Z","receivedAt":"2011-09-07T18:46:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wang Hui writes:\n\n> From: Hui Wang \n>\n> When it needs to compare and add an alt object path to the\n> alt_odb_list, we normalize this path first since comparing normalized\n> path is easy to get correct result.\n>\n> Use strbuf to replace some string operations, since it is cleaner and\n> safer.\n\nThanks, will queue.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index f7c3408..fa2484b 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -248,27 +248,27 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n> ...\n> +\t/* Drop the last '/' from path can make memcmp more accurate */\n> +\tif (pathbuf.buf[pfxlen-1] == '/')\n> +\t\tpfxlen -= 1;\n\nBy the way, I do not necessarily agree with the above comment. As long as\nyou consistently strip the trailing slashes from all directory paths, or\nyou consistently leave a single trailing slash after all directory paths,\nyou can get accurate comparison either way.\n\n\tSide note: I tend to prefer keeping a single trailing slash when I\n\tknow what we are talking about is a directory in general, because\n\tyou do not have to worry about the corner case near the root.\n\tCompare ('/' and '/bin/') vs ('/' and '/bin').\n\nIn this particular case, the real reason you want to remove the trailing\nslash is that the invariants of ent->base[] demands it (after all, it\nplaces another slash immediately after it), and making pathbuf.buf[] an\nempty string (i.e. pfxlen == 0) would still be OK to represent an\nalternate object store at the root level (this function assigns '/' at\nent->base[pfxlen] immediately before returning, and that '/' names the\nroot directory).\n\n> +\tentlen = pfxlen + 43; /* '/' + 2 hex + '/' + 38 hex + NUL */\n> +\tent = xmalloc(sizeof(*ent) + entlen);\n> +\tmemcpy(ent->base, pathbuf.buf, pfxlen);\n> +\tstrbuf_release(&pathbuf);\n> \n> \tent->name = ent->base + pfxlen + 1;\n> \tent->base[pfxlen + 3] = '/';\n"},{"id":"175058","messageId":"4E6821E2.3060900@windriver.com","threadId":"28328","inReplyTo":"7vk49k2nsz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/1] sha1_file: normalize alt_odb path before comparing and storing","fromName":"wanghui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-08T02:01:06Z","receivedAt":"2011-09-08T02:01:06Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"Junio C Hamano wrote:\n> Wang Hui writes:\n>\n> \n>> From: Hui Wang \n>>\n>> When it needs to compare and add an alt object path to the\n>> alt_odb_list, we normalize this path first since comparing normalized\n>> path is easy to get correct result.\n>>\n>> Use strbuf to replace some string operations, since it is cleaner and\n>> safer.\n>> \n>\n> Thanks, will queue.\n>\n> \n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index f7c3408..fa2484b 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -248,27 +248,27 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n>> ...\n>> +\t/* Drop the last '/' from path can make memcmp more accurate */\n>> +\tif (pathbuf.buf[pfxlen-1] == '/')\n>> +\t\tpfxlen -= 1;\n>> \n>\n> By the way, I do not necessarily agree with the above comment. As long as\n> you consistently strip the trailing slashes from all directory paths, or\n> you consistently leave a single trailing slash after all directory paths,\n> you can get accurate comparison either way.\n>\n> \tSide note: I tend to prefer keeping a single trailing slash when I\n> \tknow what we are talking about is a directory in general, because\n> \tyou do not have to worry about the corner case near the root.\n> \tCompare ('/' and '/bin/') vs ('/' and '/bin').\n>\n> In this particular case, the real reason you want to remove the trailing\n> slash is that the invariants of ent->base[] demands it (after all, it\n> places another slash immediately after it), and making pathbuf.buf[] an\n> empty string (i.e. pfxlen == 0) would still be OK to represent an\n> alternate object store at the root level (this function assigns '/' at\n> ent->base[pfxlen] immediately before returning, and that '/' names the\n> root directory).\n>\n> \nYes, your concern is right, I didn't even think about root directory \nsituation. If the pathbuf.buf is really '/', stripping last slash will \nmake pathbuf.buf a empty string, this will make is_directory() return \nfalse, this is a bug, how about replace those three line codes to:\n\n/* Except root dir, all paths are stripped the last slash if they have */\nif (pathbuf.buf[pfxlen-1] == '/' && pxflen != 1)\npxflen -= 1;\n\n>> +\tentlen = pfxlen + 43; /* '/' + 2 hex + '/' + 38 hex + NUL */\n>> +\tent = xmalloc(sizeof(*ent) + entlen);\n>> +\tmemcpy(ent->base, pathbuf.buf, pfxlen);\n>> +\tstrbuf_release(&pathbuf);\n>> \n>> \tent->name = ent->base + pfxlen + 1;\n>> \tent->base[pfxlen + 3] = '/';\n>> \n>\n> \n"}]}