{"thread":{"id":"28312","subject":"[PATCH v2 2/5] sha1_file: remove a buggy value setting","startedAt":"2011-09-06T10:24:00Z","lastAt":"2011-09-07T10:24:46Z","messageCount":11,"participants":["Wang Hui","Junio C Hamano","wanghui"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"174912","messageId":"1315304645-12009-1-git-send-email-Hui.Wang@windriver.com","threadId":"28312","inReplyTo":null,"subject":"[PATCH v2 0/5] sha1_file: remove only current repository can have relative path limitation","fromName":"Wang Hui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-06T10:24:00Z","receivedAt":"2011-09-06T10:24:00Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"Hi Junio & Martin,\n\nIn the V1, Junio said we should investigate why the limitation is originally\ndesigned. This limitation is introduced by Martin in the 2006, so add him\nat this time, hope he can share some information and knowledge for this\nquestion although he is not seen in the maillist recently.\n\nBelow is my investigation:\n\nIn the link_alt_odb_entry(), each alt object path will be added to the struct\nalternate_object_database and linked to a list, but we don't permit a same dir\nis added twice, otherwise it will easily introduce lots of trouble like\ndead-loop reference. To compare if two directories are same, original design\nis using memcmp() to directly compare directory path names, this method can't\ngive a accurate result if paths include .. and multiple slash, e.g. ../../a\nand ../../b/../a is the same dir, but this method will report they are\ndifferent. Knowing the reason, i implement a new direcotry comparison function\nto replace the old one, then we can safely remove multi-level relative\nalternates limitation now.\n\n\n0001 is a simple cleanup patch, has no relation with multi-level relative\nlimitation.\n0002 is a bug fix, has no relation with multi-level relative limitation.\nWithout this fix, all tests under t/ can work well so far, this is because we\nare lucky that the ent->base[pfxlen+1] is 0, if it is not 0, we will see lots\nof fails. After apply this fix, it is safe now.\n0003 introduce a new directory comparison function to replace the old one.\n0004 remove the multi-level relative alternates limitation\n0005 change and add testcase to validate multi-level relative alternates.\n\n\nafter apply those 5 patches, all testcase under t can pass.\n\nHui Wang (5):\n sha1_file cleanup: remove redundant variable check\n sha1_file: remove a buggy value setting\n sha1_file: improve directories comparison method\n sha1_file: remove relative entries limitation\n t5710: add testcase for multi-level relative alternates\n\n abspath.c | 26 ++++++++++++++++++++++++++\n cache.h | 1 +\n sha1_file.c | 22 +++++++---------------\n t/t5710-info-alternate.sh | 21 +++++++++++++++++++--\n 4 files changed, 53 insertions(+), 17 deletions(-)\n"},{"id":"174914","messageId":"1315304645-12009-2-git-send-email-Hui.Wang@windriver.com","threadId":"28312","inReplyTo":"1315304645-12009-1-git-send-email-Hui.Wang@windriver.com","subject":"[PATCH v2 1/5] sha1_file cleanup: remove redundant variable check","fromName":"Wang Hui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-06T10:24:01Z","receivedAt":"2011-09-06T10:24:01Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"From: Hui Wang \n\nThis variable check is always true, so it is redundant and need to be\nremoved.\n\nWe can't remove the init value for this variable, since removing\nit will introduce building warning:\n'base_len' may be used uninitialized in this function.\n\nSigned-off-by: Hui Wang \n---\n sha1_file.c | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f7c3408..d12a675 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -255,8 +255,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\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\tbase_len = strlen(relative_base) + 1;\n \t\tentlen += base_len;\n \t\tpfxlen += base_len;\n \t}\n-- \n1.6.3.1\n"},{"id":"174911","messageId":"1315304645-12009-3-git-send-email-Hui.Wang@windriver.com","threadId":"28312","inReplyTo":"1315304645-12009-2-git-send-email-Hui.Wang@windriver.com","subject":"[PATCH v2 2/5] sha1_file: remove a buggy value setting","fromName":"Wang Hui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-06T10:24:02Z","receivedAt":"2011-09-06T10:24:02Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"From: Hui Wang \n\nThe ent->base[] is a character array, it has pfxlen characters from\nposition 0 to (pfxlen-1) to contain an alt object dir name, the\nposition pfxlen should be the string terminating character '\\0' and\nis deliberately set to '\\0' at the previous code line. The position\n(pfxlen+1) is given to ent->name.\n\n>From above analysis, there is no reason to set ent->base[pfxlen] to\n'/' at the end of this function, first it doesn't make sense to append\na '/' at the end of a dir name, second if you are not lucky that the\nent->base[pfxlen+1] is not 0, you will get a wrong alt object dir\nname.\n\nSigned-off-by: Hui Wang \n---\n sha1_file.c | 2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d12a675..5940d84 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -304,8 +304,6 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \t/* recursively add alternates */\n \tread_info_alternates(ent->base, depth + 1);\n \n-\tent->base[pfxlen] = '/';\n-\n \treturn 0;\n }\n \n-- \n1.6.3.1\n"},{"id":"174915","messageId":"1315304645-12009-4-git-send-email-Hui.Wang@windriver.com","threadId":"28312","inReplyTo":"1315304645-12009-3-git-send-email-Hui.Wang@windriver.com","subject":"[PATCH v2 3/5] sha1_file: improve directories comparison method","fromName":"Wang Hui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-06T10:24:03Z","receivedAt":"2011-09-06T10:24:03Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"From: Hui Wang \n\nIn the past, to check if two directory paths are same, we use memcmp()\nto directly compare their path strings, this method can't get an\naccurate result if paths include \"..\" or \".\" or redundant slash, e.g.\ncurrent dir is /, \"/a/b/c\", \"/a/b//c/d/e/../..\" and \"./a/b/f/../c\"\nshould be the same dir, but current method will identify they are\ndifferent.\n\nNow add a global function is_same_directory() to replace the old\nmemcmp() method, this function will change two input paths to real\npath first, then normalized them and compare them.\n\nSigned-off-by: Hui Wang \n---\n abspath.c | 26 ++++++++++++++++++++++++++\n cache.h | 1 +\n sha1_file.c | 4 ++--\n 3 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 3005aed..7717f06 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -138,3 +138,29 @@ const char *absolute_path(const char *path)\n \t}\n \treturn buf;\n }\n+\n+/* Compare two directories, if they are the same dir, return 1, otherwise\n+ * return 0.\n+ *\n+ * The input path can be relative or absolute one, before the comparison, they\n+ * will be changed to real path first, then be normalized to remove .. and\n+ * redundant slash, in the end, we will compare two real and normalized paths.\n+ */\n+int is_same_directory(const char *dir1, const char *dir2)\n+{\n+\tconst char *real_path1, *real_path2;\n+\tchar norm_path1[PATH_MAX], norm_path2[PATH_MAX];\n+\n+\treal_path1 = real_path(dir1);\n+\tif (normalize_path_copy(norm_path1, real_path1))\n+\t\treturn 0;\n+\n+\treal_path2 = real_path(dir2);\n+\tif (normalize_path_copy(norm_path2, real_path2))\n+\t\treturn 0;\n+\n+\tif (strlen(norm_path1) != strlen(norm_path2))\n+\t\treturn 0;\n+\n+\treturn !memcmp(norm_path1, norm_path2, strlen(norm_path1));\n+}\ndiff --git a/cache.h b/cache.h\nindex e11cf6a..70056b5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -758,6 +758,7 @@ int longest_ancestor_length(const char *path, const char *prefix_list);\n char *strip_path_suffix(const char *path, const char *suffix);\n int daemon_avoid_alias(const char *path);\n int offset_1st_component(const char *path);\n+int is_same_directory(const char *dir1, const char *dir2);\n \n /* object replacement */\n #define READ_SHA1_FILE_REPLACE 1\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5940d84..18f7fb3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -286,12 +286,12 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \t * thing twice, or object directory itself.\n \t */\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n-\t\tif (!memcmp(ent->base, alt->base, pfxlen)) {\n+\t\tif (is_same_directory(ent->base, alt->base)) {\n \t\t\tfree(ent);\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (!memcmp(ent->base, objdir, pfxlen)) {\n+\tif (is_same_directory(ent->base, objdir)) {\n \t\tfree(ent);\n \t\treturn -1;\n \t}\n-- \n1.6.3.1\n"},{"id":"174916","messageId":"1315304645-12009-5-git-send-email-Hui.Wang@windriver.com","threadId":"28312","inReplyTo":"1315304645-12009-4-git-send-email-Hui.Wang@windriver.com","subject":"[PATCH v2 4/5] sha1_file: remove relative entries limitation","fromName":"Wang Hui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-06T10:24:04Z","receivedAt":"2011-09-06T10:24:04Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"From: Hui Wang \n\nlink_alt_odb_entries() will be called recursively if alternates has\nvalid object store paths, to avoid nesting too deep, the recursive\ndepth is limited to 5, this limitation is reasonable and safe for\ndead-loop reference situation.\n\nThere is another limitation in this function to only permit the 1st\nlevel alternates has relative paths, the purpose of this limitation\nis to avoid inaccurate result when using memcmp() directly to compare\ntwo directory path names, e.g. \"./a/b/\" and \"./a/c/e/../../b\" should\nbe the same dir, but memcmp() will report they are different dirs,\nthis will introduce the same dir be added twice or dead-loop\nreference.\n\nNow we have new method to compare two directories and can handle both\nabsolute path and relative path comparison, in addition to we already\nhave max depth 5 limitation, we can safely remove this limitation.\n\nMoreover removing this limitation will make below two usage workable.\n\nusage1: base-repos has relative path in the alternates\n %>git clone --reference base-repos src dest\nusage2: src2 has relative path to point src1, src1 has relative path\n\tto point src\n %>git clone src2 dest\n\nSigned-off-by: Hui Wang \n---\n sha1_file.c | 13 ++++---------\n 1 files changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 18f7fb3..98fdb0a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -329,15 +329,10 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n \t\t}\n \t\twhile (cp < ep && *cp != sep)\n \t\t\tcp++;\n-\t\tif (last != cp) {\n-\t\t\tif (!is_absolute_path(last) && depth) {\n-\t\t\t\terror(\"%s: ignoring relative alternate object store %s\",\n-\t\t\t\t\t\trelative_base, last);\n-\t\t\t} else {\n-\t\t\t\tlink_alt_odb_entry(last, cp - last,\n-\t\t\t\t\t\trelative_base, depth);\n-\t\t\t}\n-\t\t}\n+\t\tif (last != cp)\n+\t\t\tlink_alt_odb_entry(last, cp - last,\n+\t\t\t\t\trelative_base, depth);\n+\n \t\twhile (cp < ep && *cp == sep)\n \t\t\tcp++;\n \t\tlast = cp;\n-- \n1.6.3.1\n"},{"id":"174913","messageId":"1315304645-12009-6-git-send-email-Hui.Wang@windriver.com","threadId":"28312","inReplyTo":"1315304645-12009-5-git-send-email-Hui.Wang@windriver.com","subject":"[PATCH v2 5/5] t5710: add testcase for multi-level relative alternates","fromName":"Wang Hui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-06T10:24:05Z","receivedAt":"2011-09-06T10:24:05Z","isPatch":true,"sender":{"key":"hui.wang@windriver.com","avatar":null},"body":"From: Hui Wang \n\nSince we removed \"relative alternates only possible for current dir\"\nlimitation, it is needed to change an existing testcase to make it\npass for the relative path at the second level alternates.\n\nAdd a new testcase to make it pass when it work at a 5-level relative\nalternates repository and make it fail when it work at a 6-level\nrelative alternates repository.\n\nSigned-off-by: Hui Wang \n---\n t/t5710-info-alternate.sh | 21 +++++++++++++++++++--\n 1 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh\nindex ef7127c..a606a94 100755\n--- a/t/t5710-info-alternate.sh\n+++ b/t/t5710-info-alternate.sh\n@@ -102,9 +102,26 @@ test_valid_repo'\n cd \"$base_dir\"\n \n test_expect_success \\\n- 'that relative alternate is only possible for current dir' '\n+ 'that relative alternate is possible for none current dir' '\n cd D &&\n- ! (test_valid_repo)\n+ test_valid_repo\n+'\n+\n+cd \"$base_dir\"\n+\n+test_expect_success 'allow maxium 5 level relative alternate' \\\n+'echo \"\" > A/.git/objects/info/alternates &&\n+echo \"../../../A/.git/objects\" > B/.git/objects/info/alternates &&\n+echo \"../../../B/.git/objects\" > C/.git/objects/info/alternates &&\n+echo \"../../../C/.git/objects\" > D/.git/objects/info/alternates &&\n+echo \"../../../D/.git/objects\" > E/.git/objects/info/alternates &&\n+echo \"../../../E/.git/objects\" > F/.git/objects/info/alternates &&\n+echo \"../../../F/.git/objects\" > G/.git/objects/info/alternates &&\n+cd F &&\n+test_valid_repo &&\n+cd ../G &&\n+git fsck --full > fsck.err 2>&1 &&\n+test `wc -l < fsck.err` != 0\n '\n \n cd \"$base_dir\"\n-- \n1.6.3.1\n"},{"id":"174937","messageId":"7vpqjdab76.fsf@alter.siamese.dyndns.org","threadId":"28312","inReplyTo":"1315304645-12009-3-git-send-email-Hui.Wang@windriver.com","subject":"Re: [PATCH v2 2/5] sha1_file: remove a buggy value setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T16:26:53Z","receivedAt":"2011-09-06T16:26:53Z","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> The ent->base[] is a character array, it has pfxlen characters from\n> position 0 to (pfxlen-1) to contain an alt object dir name, the\n> position pfxlen should be the string terminating character '\\0' and\n> is deliberately set to '\\0' at the previous code line. The position\n> (pfxlen+1) is given to ent->name.\n\nCorrect. Do you understand why?\n\nWe temporarily NUL terminate the ent->base[] so that we can give it to\nis_directory() to see if that is a directory, but the invariants for a\nalternate_object_database instance after it is properly initialized by\nthis function are to have:\n\n - the directory name followed by a slash in the base[] array;\n - the name pointer pointing at one byte beyond the slash;\n - name[2] filled with a slash; and\n - name[41] terminated with NUL.\n\nLater, has_loose_object_nonlocal() calls fill_sha1_path() with the name\npointer to fill name[0..1, 3..40] with the hexadecimal representation of\nthe object name, which would result in base[] array to have the pathname\nfor a loose object found in that alternate. The same thing happens in\nopen_sha1_file() to read from a loose object in an alternate.\n\nAnd you are breaking one of the above invariants by removing that slash\nafter the directory name. These callers of fill_sha1_path() will see the\ndirectory name, your NUL, two hex, slash, and 38 hex in base[].\n\nHow would the code even work with your patch?\n"},{"id":"174938","messageId":"7vliu1aay2.fsf@alter.siamese.dyndns.org","threadId":"28312","inReplyTo":"1315304645-12009-4-git-send-email-Hui.Wang@windriver.com","subject":"Re: [PATCH v2 3/5] sha1_file: improve directories comparison method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T16:32:21Z","receivedAt":"2011-09-06T16:32:21Z","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> In the past, to check if two directory paths are same, we use memcmp()\n> to directly compare their path strings, this method can't get an\n> accurate result if paths include \"..\" or \".\" or redundant slash, e.g.\n> current dir is /, \"/a/b/c\", \"/a/b//c/d/e/../..\" and \"./a/b/f/../c\"\n> should be the same dir, but current method will identify they are\n> different.\n>\n> Now add a global function is_same_directory() to replace the old\n> memcmp() method, this function will change two input paths to real\n> path first, then normalized them and compare them.\n\nI do not like this patch _at all_. While it may result in correct result\nif you _always_ make it absolute before comparing two entities, if you\nwill be storing the normalized result after running the comparison anyway,\nand if you are comparing against the existing and supposedly already\nnormalized entities with a new candidate, why would anybody sane would\nwant to keep paying for the normalization cost at such a low level?\n\nIOW, you are proposing to do:\n\n\tgiven a new candidate;\n\tfor existing entities:\n\t\tnormalize existing\n normalize candiate\n compare the above two\n if they are equal:\n \tignore\n\tif no match found\n \tadd the normalized candidate to the list\n\nWouldn't it make much more sense to do this:\n\n\tgiven a new candidate;\n normalize it\n\tfor existing entities:\n compare existing and normalized candidate\n\t\tthere is no point in normalizing the existing one!\n if they are equal:\n \tignore\n\tif no match found\n \tadd the normalized candidate to the list\n"},{"id":"174940","messageId":"7vaaaha9p2.fsf@alter.siamese.dyndns.org","threadId":"28312","inReplyTo":"1315304645-12009-2-git-send-email-Hui.Wang@windriver.com","subject":"Re: [PATCH v2 1/5] sha1_file cleanup: remove redundant variable check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T16:59:21Z","receivedAt":"2011-09-06T16:59:21Z","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> This variable check is always true, so it is redundant and need to be\n> removed.\n>\n> We can't remove the init value for this variable, since removing\n> it will introduce building warning:\n> 'base_len' may be used uninitialized in this function.\n\nIf we are into cleaning things up, we should instead notice and say \"yuck\"\nto the repeated \"is entry is absolute and relative base is given\" check.\n\nWouldn't something like this makes things easier to follow and also avoids\nthe \"when does the path normalized and made absolute\" issue?\n\nCompletely untested and I may have off-by-one errors and such, but I think\nyou would get the idea...\n\n sha1_file.c | 29 ++++++++++++-----------------\n 1 files changed, 12 insertions(+), 17 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e002056..26aa3be 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -248,27 +248,22 @@ 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, relative_base);\n+\t\tstrbuf_addch(&pathbuf, '/');\n \t}\n-\tent = xmalloc(sizeof(*ent) + entlen);\n+\tstrbuf_add(&pathbuf, entry, len);\n+\tnormalize_path_copy(pathbuf.buf, pathbuf.buf);\n+\tstrbuf_setlen(&pathbuf, strlen(pathbuf.buf));\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+\tpfxlen = pathbuf.len;\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":"175006","messageId":"4E673F74.9060006@windriver.com","threadId":"28312","inReplyTo":"7vpqjdab76.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/5] sha1_file: remove a buggy value setting","fromName":"wanghui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-07T09:55:00Z","receivedAt":"2011-09-07T09:55:00Z","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>> The ent->base[] is a character array, it has pfxlen characters from\n>> position 0 to (pfxlen-1) to contain an alt object dir name, the\n>> position pfxlen should be the string terminating character '\\0' and\n>> is deliberately set to '\\0' at the previous code line. The position\n>> (pfxlen+1) is given to ent->name.\n>> \n>\n> Correct. Do you understand why?\n>\n> We temporarily NUL terminate the ent->base[] so that we can give it to\n> is_directory() to see if that is a directory, but the invariants for a\n> alternate_object_database instance after it is properly initialized by\n> this function are to have:\n>\n> - the directory name followed by a slash in the base[] array;\n> - the name pointer pointing at one byte beyond the slash;\n> - name[2] filled with a slash; and\n> - name[41] terminated with NUL.\n>\n> Later, has_loose_object_nonlocal() calls fill_sha1_path() with the name\n> pointer to fill name[0..1, 3..40] with the hexadecimal representation of\n> the object name, which would result in base[] array to have the pathname\n> for a loose object found in that alternate. The same thing happens in\n> open_sha1_file() to read from a loose object in an alternate.\n>\n> And you are breaking one of the above invariants by removing that slash\n> after the directory name. These callers of fill_sha1_path() will see the\n> directory name, your NUL, two hex, slash, and 38 hex in base[].\n>\n> \nUnderstand now, thanks for your explanation.\n> How would the code even work with your patch?\n>\n>\n> \n"},{"id":"175004","messageId":"4E67466E.9060100@windriver.com","threadId":"28312","inReplyTo":"7vaaaha9p2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/5] sha1_file cleanup: remove redundant variable check","fromName":"wanghui","fromEmail":"hui.wang@windriver.com","sentAt":"2011-09-07T10:24:46Z","receivedAt":"2011-09-07T10:24:46Z","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>> This variable check is always true, so it is redundant and need to be\n>> removed.\n>>\n>> We can't remove the init value for this variable, since removing\n>> it will introduce building warning:\n>> 'base_len' may be used uninitialized in this function.\n>> \n>\n> If we are into cleaning things up, we should instead notice and say \"yuck\"\n> to the repeated \"is entry is absolute and relative base is given\" check.\n>\n> Wouldn't something like this makes things easier to follow and also avoids\n> the \"when does the path normalized and made absolute\" issue?\n>\n> Completely untested and I may have off-by-one errors and such, but I think\n> you would get the idea...\n>\n> \nYes, i got the idea, and some errors are pointed out below. I will \nprepare a V3 patch basing on it.\n> sha1_file.c | 29 ++++++++++++-----------------\n> 1 files changed, 12 insertions(+), 17 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index e002056..26aa3be 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -248,27 +248,22 @@ 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, relative_base);\n> \ns/relative_base/real_path(relative_base)/ is better, since \nnormalize_path_copy is not good at handle relative path. e.g. \". \n./objects/../../test2/objects\" will be normalized to \"objects\"\n> +\t\tstrbuf_addch(&pathbuf, '/');\n> \t}\n> -\tent = xmalloc(sizeof(*ent) + entlen);\n> +\tstrbuf_add(&pathbuf, entry, len);\n> +\tnormalize_path_copy(pathbuf.buf, pathbuf.buf);\n> \nif pathbuf.buf[strlen(pathbuf.buf)] is '/', remove it. this can avoid to \nget a wrong result when comparing \"/a/b\" with \"/a/b/\".\n> +\tstrbuf_setlen(&pathbuf, strlen(pathbuf.buf));\n> \nabove line can be removed, since we will release pathbuf soon.\n\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> +\tpfxlen = pathbuf.len;\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"}]}