{"thread":{"id":"37109","subject":"[Bug] data loss with cyclic alternates","startedAt":"2014-07-11T09:37:25Z","lastAt":"2014-07-16T06:47:43Z","messageCount":16,"participants":["Ephrim Khong","Junio C Hamano","Keller, Jacob E","Jeff King","Duy Nguyen","Johannes Sixt","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"245769","messageId":"53BFB055.206@gmail.com","threadId":"37109","inReplyTo":null,"subject":"[Bug] data loss with cyclic alternates","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2014-07-11T09:37:25Z","receivedAt":"2014-07-11T09:37:25Z","isPatch":false,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"Hi,\n\ngit seems to have issues with alternates when cycles are present (repo A \nhas B/objects as alternates, B has A/objects as alternates). In such \ncases, gc and repack might delete objects that are present in only one \nof the alternates, leading to data loss.\n\nI understand that this is no big use case, and requires manual editing \nof objects/info/alternates. But for the sake of preventing unneccesary \ndata loss, and since I found no warning regarding alternate cycles in \nthe documentation, it might make sense to handle such cases properly \n(maybe it's a simple \"after finding all alternates directories, remove \nduplicates\"?).\n\nHere is a small test case that adds the object storage of a repository \nas alternate to itsself. After git repack -adl, all objects are deleted.\n\n---\nrm -rf test_repo &&\nmkdir test_repo &&\ncd test_repo &&\ngit init &&\ntouch a &&\ngit add a &&\ngit commit -m \"c1\" &&\ngit repack -adl &&\necho $PWD/.git/objects > .git/objects/info/alternates &&\necho \">> re-packing...\" &&\ngit repack -adl &&\necho \">> git fsck...\" &&\ngit fsck\n---\n\nOutput:\n\n---\nInitialized empty Git repository in /somewhere/test_repo/.git/\n[master (root-commit) ab9e123] c1\n  1 file changed, 0 insertions(+), 0 deletions(-)\n  create mode 100644 a\nCounting objects: 3, done.\nWriting objects: 100% (3/3), done.\nTotal 3 (delta 0), reused 0 (delta 0)\n >> re-packing...\nNothing new to pack.\nerror: refs/heads/master does not point to a valid object!\n >> git fsck...\nChecking object directories: 100% (256/256), done.\nChecking object directories: 100% (256/256), done.\nerror: HEAD: invalid sha1 pointer 1494ec24356cbbbd66e19f22cef762dd83de7f54\nerror: refs/heads/master does not point to a valid object!\nnotice: No default references\nmissing blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n---\n\nThanks\n- Eph\n"},{"id":"245788","messageId":"xmqqy4vz51gb.fsf@gitster.dls.corp.google.com","threadId":"37109","inReplyTo":"53BFB055.206@gmail.com","subject":"Re: [Bug] data loss with cyclic alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T16:01:40Z","receivedAt":"2014-07-11T16:01:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ephrim Khong <dr.khong@gmail.com> writes:\n\n> git seems to have issues with alternates when cycles are present (repo\n> A has B/objects as alternates, B has A/objects as alternates).\n\nYeah, don't do that.  A thinks \"eh, the other guy must have it\" and\nB thinks the same.  In general, do not prune or gc a repository\nother repositories borrow from, even if there is no cycle, because\nthe borrowee does not know anythning about objects that it itself no\nlonger needs but are still needed by its borrowers.\n"},{"id":"245808","messageId":"1405101705.22963.27.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37109","inReplyTo":"xmqqy4vz51gb.fsf@gitster.dls.corp.google.com","subject":"Re: [Bug] data loss with cyclic alternates","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-11T18:01:46Z","receivedAt":"2014-07-11T18:01:46Z","isPatch":false,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, 2014-07-11 at 09:01 -0700, Junio C Hamano wrote:\n> Ephrim Khong <dr.khong@gmail.com> writes:\n> \n> > git seems to have issues with alternates when cycles are present (repo\n> > A has B/objects as alternates, B has A/objects as alternates).\n> \n> Yeah, don't do that.  A thinks \"eh, the other guy must have it\" and\n> B thinks the same.  In general, do not prune or gc a repository\n> other repositories borrow from, even if there is no cycle, because\n> the borrowee does not know anythning about objects that it itself no\n> longer needs but are still needed by its borrowers.\n> \n\nDoesn't gc get run automatically at some points? Is the automatic gc run\nsetup to avoid this problem? \n\nThanks,\nJake\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n\n"},{"id":"245886","messageId":"20140712055703.GA9655@sigill.intra.peff.net","threadId":"37109","inReplyTo":"1405101705.22963.27.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [Bug] data loss with cyclic alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-12T05:57:03Z","receivedAt":"2014-07-12T05:57:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 06:01:46PM +0000, Keller, Jacob E wrote:\n\n> > Yeah, don't do that.  A thinks \"eh, the other guy must have it\" and\n> > B thinks the same.  In general, do not prune or gc a repository\n> > other repositories borrow from, even if there is no cycle, because\n> > the borrowee does not know anythning about objects that it itself no\n> > longer needs but are still needed by its borrowers.\n> > \n> \n> Doesn't gc get run automatically at some points? Is the automatic gc run\n> setup to avoid this problem?\n\nNo, the automatic gc doesn't avoid this. It can't in the general case,\nas the parent repository does not know how many or which children are\npointed to it as an alternate. And the borrowing repository does not\neven need to have write permission to the parent, so it cannot write a\nbackpointer.\n\nIf people are using alternates, they should probably turn off gc.auto in\nthe borrowee (it doesn't seem unreasonable to me to do so automatically\nvia \"clone -s\" in cases where we can write to the alternates repo, and\nto issue a warning otherwise).\n\n-Peff\n"},{"id":"245947","messageId":"53C26309.5040401@gmail.com","threadId":"37109","inReplyTo":"xmqqy4vz51gb.fsf@gitster.dls.corp.google.com","subject":"Re: [Bug] data loss with cyclic alternates","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2014-07-13T10:44:25Z","receivedAt":"2014-07-13T10:44:25Z","isPatch":false,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"Am 11.07.14 18:01, schrieb Junio C Hamano:\n> Ephrim Khong <dr.khong@gmail.com> writes:\n>\n>> git seems to have issues with alternates when cycles are present (repo\n>> A has B/objects as alternates, B has A/objects as alternates).\n>\n> Yeah, don't do that.  A thinks \"eh, the other guy must have it\" and\n> B thinks the same.  In general, do not prune or gc a repository\n> other repositories borrow from, even if there is no cycle, because\n> the borrowee does not know anythning about objects that it itself no\n> longer needs but are still needed by its borrowers.\n\nIt seems that there is a safeguard against this in sha1_file.c, \nlink_alt_odb_entry(), that doesn't work as intended:\n\n\tif (!strcmp(ent->base, objdir)) {\n\t\tfree(ent);\n\t\treturn -1;\n\t}\n\nHowever, printf-debugging tells me that ent->base is absolute and objdir \nis relative (\".git/objects\") at this point, so the strings are different \neven though the files are the same.\n\nI never submitted a patch to git. Do you think someone can fix this \nhickup, otherwise I'll give it a shot next week.\n"},{"id":"245975","messageId":"53C39CB0.6040909@gmail.com","threadId":"37109","inReplyTo":"53C26309.5040401@gmail.com","subject":"[PATCH] sha1_file: do not add own object directory as alternate","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2014-07-14T09:02:40Z","receivedAt":"2014-07-14T09:02:40Z","isPatch":true,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"When adding alternate object directories, we try not to add the\ndirectory of the current repository to avoid cycles.  Unfortunately,\nthat test was broken, since it compared an absolute with a relative\npath.\n\nSigned-off-by: Ephrim Khong <dr.khong@gmail.com>\n---\nMy first patch, so be harsh.  I'm not sure about the filename of the \ntest, the behavior is tested with repack, but it affects gc and others \nas well.\n\n  sha1_file.c                        |  2 +-\n  t/t7702-repack-cyclic-alternate.sh | 24 ++++++++++++++++++++++++\n  2 files changed, 25 insertions(+), 1 deletion(-)\n  create mode 100755 t/t7702-repack-cyclic-alternate.sh\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 34d527f..7e98e9e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, \nconst char *relative_base, int\n  \t\t\treturn -1;\n  \t\t}\n  \t}\n-\tif (!strcmp(ent->base, objdir)) {\n+\tif (!strcmp(ent->base, absolute_path(objdir))) {\n  \t\tfree(ent);\n  \t\treturn -1;\n  \t}\ndiff --git a/t/t7702-repack-cyclic-alternate.sh \nb/t/t7702-repack-cyclic-alternate.sh\nnew file mode 100755\nindex 0000000..9a22d98\n--- /dev/null\n+++ b/t/t7702-repack-cyclic-alternate.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2014 Ephrim Khong\n+#\n+\n+test_description='repack involving cyclic alternate'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tgit init repo &&\n+\tcd repo &&\n+\ttouch a &&\n+\tgit add a &&\n+\tgit commit -m 1 &&\n+\tgit repack -adl &&\n+\techo $PWD/.git/objects > .git/objects/info/alternates\n+'\n+\n+test_expect_success 're-packing repository with itsself as alternate' '\n+\tgit repack -adl &&\n+\tgit fsck\n+'\n+\n+test_done\n-- \n1.8.4.3\n"},{"id":"245981","messageId":"CACsJy8B-QFu4SU6HHZukAmmjLrgb+L8KsHPn3GgQhcX6-LGXLg@mail.gmail.com","threadId":"37109","inReplyTo":"53C39CB0.6040909@gmail.com","subject":"Re: [PATCH] sha1_file: do not add own object directory as alternate","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-14T10:47:22Z","receivedAt":"2014-07-14T10:47:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 14, 2014 at 4:02 PM, Ephrim Khong <dr.khong@gmail.com> wrote:\n> When adding alternate object directories, we try not to add the\n> directory of the current repository to avoid cycles.  Unfortunately,\n> that test was broken, since it compared an absolute with a relative\n> path.\n\nNot blaming anyone. I'm simply interested in code archeology. The\nfirst introduction of this strcmp is from 1494e03 (sha1_file.c: make\nsure packs in an alternate odb is named properly. - 2005-12-04). The\nintention is good as described in the commit message. But I think it's\nbroken even back then because \"objdir\" (or get_object_directory())\nwill always be relative (unless someone sets it explicitly) and\nent->base back then is already absolute (but not normalized).\n\n>\n> Signed-off-by: Ephrim Khong <dr.khong@gmail.com>\n> ---\n> My first patch, so be harsh.  I'm not sure about the filename of the test,\n> the behavior is tested with repack, but it affects gc and others as well.\n>\n>  sha1_file.c                        |  2 +-\n>  t/t7702-repack-cyclic-alternate.sh | 24 ++++++++++++++++++++++++\n>  2 files changed, 25 insertions(+), 1 deletion(-)\n>  create mode 100755 t/t7702-repack-cyclic-alternate.sh\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 34d527f..7e98e9e 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, const\n> char *relative_base, int\n>                         return -1;\n>                 }\n>         }\n> -       if (!strcmp(ent->base, objdir)) {\n> +       if (!strcmp(ent->base, absolute_path(objdir))) {\n\nI think you want to normalize objdir in addition to making it\nabsolute, in case someone sets GIT_OBJECT_DIR to foo///bar. ent->base\nis normalized already. Oh and maybe use strcmp_icase to be nice on\ncase-insensitive filesystems..\n\nKinda nit picking, but perhaps this path\nabsolute-ization/normalization should be done by the caller\nlink_alt_odb_entries so you don't have to redo it for every entry in\nalternates file. I know there's a loop of memcmp() right above this\nthat may be more expensive than this micro-optimization when we have\nlots of alternate entries.\n-- \nDuy\n"},{"id":"245986","messageId":"53C3D2AD.9050204@gmail.com","threadId":"37109","inReplyTo":"CACsJy8B-QFu4SU6HHZukAmmjLrgb+L8KsHPn3GgQhcX6-LGXLg@mail.gmail.com","subject":"Re: [PATCH v2] sha1_file: do not add own object directory as alternate","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2014-07-14T12:53:01Z","receivedAt":"2014-07-14T12:53:01Z","isPatch":true,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"When adding alternate object directories, we try not to add the\ndirectory of the current repository to avoid cycles.  Unfortunately,\nthat test was broken, since it compared an absolute with a relative\npath.\n\nSigned-off-by: Ephrim Khong <dr.khong@gmail.com>\n---\nAs proposed by Duy, v2 of the patch normalizes the object directory,\nuses strcmp_icase for case-insensitive file systems, and moves the\npath normalization to link_alt_odb_entries to save some cycles.\n\nAdditionally, the test is now more compact by using the already\nprepared git repository instead of a new one, and tests if the\npath normalization for both objectdir and alternate works.\n\n sha1_file.c                        | 13 +++++++++----\n t/t7702-repack-cyclic-alternate.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 4 deletions(-)\n create mode 100755 t/t7702-repack-cyclic-alternate.sh\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 34d527f..88fd168 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -268,9 +268,9 @@ static struct alternate_object_database **alt_odb_tail;\n  * SHA1, an extra slash for the first level indirection, and the\n  * terminating NUL.\n  */\n-static int link_alt_odb_entry(const char *entry, const char *relative_base, int depth)\n+static int link_alt_odb_entry(const char *entry, const char *relative_base,\n+\tint depth, const char *normalized_objdir)\n {\n-\tconst char *objdir = get_object_directory();\n \tstruct alternate_object_database *ent;\n \tstruct alternate_object_database *alt;\n \tint pfxlen, entlen;\n@@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (!strcmp(ent->base, objdir)) {\n+\tif (!strcmp_icase(ent->base, normalized_objdir)) {\n \t\tfree(ent);\n \t\treturn -1;\n \t}\n@@ -344,6 +344,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \tstruct string_list entries = STRING_LIST_INIT_NODUP;\n \tchar *alt_copy;\n \tint i;\n+\tstruct strbuf objdirbuf = STRBUF_INIT;\n\n \tif (depth > 5) {\n \t\terror(\"%s: ignoring alternate object stores, nesting too deep.\",\n@@ -351,6 +352,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t\treturn;\n \t}\n\n+\tstrbuf_addstr(&objdirbuf, absolute_path(get_object_directory()));\n+\tnormalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n+\n \talt_copy = xmemdupz(alt, len);\n \tstring_list_split_in_place(&entries, alt_copy, sep, -1);\n \tfor (i = 0; i < entries.nr; i++) {\n@@ -361,11 +365,12 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t\t\terror(\"%s: ignoring relative alternate object store %s\",\n \t\t\t\t\trelative_base, entry);\n \t\t} else {\n-\t\t\tlink_alt_odb_entry(entry, relative_base, depth);\n+\t\t\tlink_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);\n \t\t}\n \t}\n \tstring_list_clear(&entries, 0);\n \tfree(alt_copy);\n+\tstrbuf_release(&objdirbuf);\n }\n\n void read_info_alternates(const char * relative_base, int depth)\ndiff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh\nnew file mode 100755\nindex 0000000..3771613\n--- /dev/null\n+++ b/t/t7702-repack-cyclic-alternate.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2014 Ephrim Khong\n+#\n+\n+test_description='repack involving cyclic alternate'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tGIT_OBJECT_DIRECTORY=.git//../.git/objects &&\n+\texport GIT_OBJECT_DIRECTORY &&\n+\ttouch a &&\n+\tgit add a &&\n+\tgit commit -m 1 &&\n+\tgit repack -adl &&\n+\techo $PWD/.git/objects/../objects > .git/objects/info/alternates\n+'\n+\n+test_expect_success 're-packing repository with itsself as alternate' '\n+\tgit repack -adl &&\n+\tgit fsck\n+'\n+\n+test_done\n-- \n1.8.4.3\n"},{"id":"246034","messageId":"02874ECE860811409154E81DA85FBB585719878B@ORSMSX115.amr.corp.intel.com","threadId":"37109","inReplyTo":"20140712055703.GA9655@sigill.intra.peff.net","subject":"RE: [Bug] data loss with cyclic alternates","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-14T22:05:25Z","receivedAt":"2014-07-14T22:05:25Z","isPatch":false,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\n> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Friday, July 11, 2014 10:57 PM\n> To: Keller, Jacob E\n> Cc: gitster@pobox.com; dr.khong@gmail.com; git@vger.kernel.org\n> Subject: Re: [Bug] data loss with cyclic alternates\n> \n> On Fri, Jul 11, 2014 at 06:01:46PM +0000, Keller, Jacob E wrote:\n> \n> > > Yeah, don't do that.  A thinks \"eh, the other guy must have it\" and\n> > > B thinks the same.  In general, do not prune or gc a repository\n> > > other repositories borrow from, even if there is no cycle, because\n> > > the borrowee does not know anythning about objects that it itself no\n> > > longer needs but are still needed by its borrowers.\n> > >\n> >\n> > Doesn't gc get run automatically at some points? Is the automatic gc run\n> > setup to avoid this problem?\n> \n> No, the automatic gc doesn't avoid this. It can't in the general case,\n> as the parent repository does not know how many or which children are\n> pointed to it as an alternate. And the borrowing repository does not\n> even need to have write permission to the parent, so it cannot write a\n> backpointer.\n> \n> If people are using alternates, they should probably turn off gc.auto in\n> the borrowee (it doesn't seem unreasonable to me to do so automatically\n> via \"clone -s\" in cases where we can write to the alternates repo, and\n> to issue a warning otherwise).\n> \n> -Peff\n\nYes, if this is a known problem and we can avoid it by disabling garbage collection, that makes sense to me to do so..\n\nThanks,\nJake\n"},{"id":"246042","messageId":"53C4BFC7.2000507@kdbg.org","threadId":"37109","inReplyTo":"53C3D2AD.9050204@gmail.com","subject":"Re: [PATCH v2] sha1_file: do not add own object directory as alternate","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-07-15T05:44:39Z","receivedAt":"2014-07-15T05:44:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 14.07.2014 14:53, schrieb Ephrim Khong:\n> diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh\n> new file mode 100755\n> index 0000000..3771613\n> --- /dev/null\n> +++ b/t/t7702-repack-cyclic-alternate.sh\n> @@ -0,0 +1,24 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2014 Ephrim Khong\n> +#\n> +\n> +test_description='repack involving cyclic alternate'\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\tGIT_OBJECT_DIRECTORY=.git//../.git/objects &&\n> +\texport GIT_OBJECT_DIRECTORY &&\n> +\ttouch a &&\n> +\tgit add a &&\n> +\tgit commit -m 1 &&\n> +\tgit repack -adl &&\n> +\techo $PWD/.git/objects/../objects > .git/objects/info/alternates\n\nWe need a Windows-style path in the file, double-quotes to protect\nspecial characters, and a small style fix. Therefore:\n\n\techo \"$(pwd)\"/.git/objects/../objects >.git/objects/info/alternates\n\n> +'\n> +\n> +test_expect_success 're-packing repository with itsself as alternate' '\n> +\tgit repack -adl &&\n> +\tgit fsck\n> +'\n> +\n> +test_done\n> \n"},{"id":"246048","messageId":"53C510A9.4010208@gmail.com","threadId":"37109","inReplyTo":"53C4BFC7.2000507@kdbg.org","subject":"[PATCH v3] sha1_file: do not add own object directory as alternate","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2014-07-15T11:29:45Z","receivedAt":"2014-07-15T11:29:45Z","isPatch":true,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"When adding alternate object directories, we try not to add the\ndirectory of the current repository to avoid cycles.  Unfortunately,\nthat test was broken, since it compared an absolute with a relative\npath.\n\nSigned-off-by: Ephrim Khong <dr.khong@gmail.com>\n---\nSince v2: Added Johannes' comments.\n\n sha1_file.c                        | 13 +++++++++----\n t/t7702-repack-cyclic-alternate.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 4 deletions(-)\n create mode 100755 t/t7702-repack-cyclic-alternate.sh\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 34d527f..88fd168 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -268,9 +268,9 @@ static struct alternate_object_database **alt_odb_tail;\n  * SHA1, an extra slash for the first level indirection, and the\n  * terminating NUL.\n  */\n-static int link_alt_odb_entry(const char *entry, const char *relative_base, int depth)\n+static int link_alt_odb_entry(const char *entry, const char *relative_base,\n+\tint depth, const char *normalized_objdir)\n {\n-\tconst char *objdir = get_object_directory();\n \tstruct alternate_object_database *ent;\n \tstruct alternate_object_database *alt;\n \tint pfxlen, entlen;\n@@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (!strcmp(ent->base, objdir)) {\n+\tif (!strcmp_icase(ent->base, normalized_objdir)) {\n \t\tfree(ent);\n \t\treturn -1;\n \t}\n@@ -344,6 +344,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \tstruct string_list entries = STRING_LIST_INIT_NODUP;\n \tchar *alt_copy;\n \tint i;\n+\tstruct strbuf objdirbuf = STRBUF_INIT;\n\n \tif (depth > 5) {\n \t\terror(\"%s: ignoring alternate object stores, nesting too deep.\",\n@@ -351,6 +352,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t\treturn;\n \t}\n\n+\tstrbuf_addstr(&objdirbuf, absolute_path(get_object_directory()));\n+\tnormalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n+\n \talt_copy = xmemdupz(alt, len);\n \tstring_list_split_in_place(&entries, alt_copy, sep, -1);\n \tfor (i = 0; i < entries.nr; i++) {\n@@ -361,11 +365,12 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t\t\terror(\"%s: ignoring relative alternate object store %s\",\n \t\t\t\t\trelative_base, entry);\n \t\t} else {\n-\t\t\tlink_alt_odb_entry(entry, relative_base, depth);\n+\t\t\tlink_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);\n \t\t}\n \t}\n \tstring_list_clear(&entries, 0);\n \tfree(alt_copy);\n+\tstrbuf_release(&objdirbuf);\n }\n\n void read_info_alternates(const char * relative_base, int depth)\ndiff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh\nnew file mode 100755\nindex 0000000..8341d46\n--- /dev/null\n+++ b/t/t7702-repack-cyclic-alternate.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2014 Ephrim Khong\n+#\n+\n+test_description='repack involving cyclic alternate'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tGIT_OBJECT_DIRECTORY=.git//../.git/objects &&\n+\texport GIT_OBJECT_DIRECTORY &&\n+\ttouch a &&\n+\tgit add a &&\n+\tgit commit -m 1 &&\n+\tgit repack -adl &&\n+\techo \"$(pwd)\"/.git/objects/../objects >.git/objects/info/alternates\n+'\n+\n+test_expect_success 're-packing repository with itsself as alternate' '\n+\tgit repack -adl &&\n+\tgit fsck\n+'\n+\n+test_done\n-- \n1.8.4.3\n"},{"id":"246083","messageId":"xmqqd2d6v2yf.fsf@gitster.dls.corp.google.com","threadId":"37109","inReplyTo":"53C510A9.4010208@gmail.com","subject":"Re: [PATCH v3] sha1_file: do not add own object directory as alternate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T19:26:00Z","receivedAt":"2014-07-15T19:26:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ephrim Khong <dr.khong@gmail.com> writes:\n\n> @@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int\n>  \t\t\treturn -1;\n>  \t\t}\n>  \t}\n> -\tif (!strcmp(ent->base, objdir)) {\n> +\tif (!strcmp_icase(ent->base, normalized_objdir)) {\n\nNot a problem with your patch, but we should rethink the name of\nthis function when the code base is more quiet.  It always makes me\nwonder if it is something similar to strcasecmp(), but in fact it is\nnot.  It is meant to be used *only* for pathnames; pathname_cmp() or\nsomething that has \"path\" in its name would be appropriate, but it\nis wrong to call it \"str\"-anything.\n\n> @@ -344,6 +344,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n>  \tstruct string_list entries = STRING_LIST_INIT_NODUP;\n>  \tchar *alt_copy;\n>  \tint i;\n> +\tstruct strbuf objdirbuf = STRBUF_INIT;\n>\n>  \tif (depth > 5) {\n>  \t\terror(\"%s: ignoring alternate object stores, nesting too deep.\",\n> @@ -351,6 +352,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n>  \t\treturn;\n>  \t}\n>\n> +\tstrbuf_addstr(&objdirbuf, absolute_path(get_object_directory()));\n> +\tnormalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n\nThis is somewhat a strange usage of a strbuf.\n\n - it relies on that normalize_path_copy() only shrinks, never\n   lengthens, which is not too bad;\n\n - if the operation ever shrinks, objdirbuf.len becomes\n   meaningless.  The allocated length is objdirbuf.alloc, length\n   of the string is strlen(objdirbuf.buf).\n\n - abspath.c::absolute_path() is still restricted to PATH_MAX, so\n   you are not gaining much by using strbuf here.\n\nBut at least this patch is not making things any worse, so....\n\n> @@ -361,11 +365,12 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n>  \t\t\terror(\"%s: ignoring relative alternate object store %s\",\n>  \t\t\t\t\trelative_base, entry);\n>  \t\t} else {\n> -\t\t\tlink_alt_odb_entry(entry, relative_base, depth);\n> +\t\t\tlink_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);\n>  \t\t}\n>  \t}\n>  \tstring_list_clear(&entries, 0);\n>  \tfree(alt_copy);\n> +\tstrbuf_release(&objdirbuf);\n>  }\n>\n>  void read_info_alternates(const char * relative_base, int depth)\n> diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh\n> new file mode 100755\n> index 0000000..8341d46\n> --- /dev/null\n> +++ b/t/t7702-repack-cyclic-alternate.sh\n\nDo we really have to waste a new test file only for this test?\n\nDon't we have any test that already uses alternate that these two\nnew test pieces can be added to?\n\n    $ git grep info/alternates t/\n\nseems to show a few existing ones, including 1450 (fsck) and 7700\n(repack) that look very relevant (I didn't check what the tests in\nthem are about, though).\n"},{"id":"246085","messageId":"CAPig+cQDbU6Qii4we-8oPdSeXiqhBWmjJ+VwTwBivnAOxBzS_A@mail.gmail.com","threadId":"37109","inReplyTo":"53C510A9.4010208@gmail.com","subject":"Re: [PATCH v3] sha1_file: do not add own object directory as alternate","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-07-15T19:33:46Z","receivedAt":"2014-07-15T19:33:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 15, 2014 at 7:29 AM, Ephrim Khong <dr.khong@gmail.com> wrote:\n> When adding alternate object directories, we try not to add the\n> directory of the current repository to avoid cycles.  Unfortunately,\n> that test was broken, since it compared an absolute with a relative\n> path.\n>\n> Signed-off-by: Ephrim Khong <dr.khong@gmail.com>\n> ---\n> diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh\n> new file mode 100755\n> index 0000000..8341d46\n> --- /dev/null\n> +++ b/t/t7702-repack-cyclic-alternate.sh\n> @@ -0,0 +1,24 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2014 Ephrim Khong\n> +#\n> +\n> +test_description='repack involving cyclic alternate'\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +       GIT_OBJECT_DIRECTORY=.git//../.git/objects &&\n> +       export GIT_OBJECT_DIRECTORY &&\n> +       touch a &&\n\nSince the existence of 'a' is significant here, not its timestamp, it\nwould be clearer to create the file with:\n\n    >a &&\n\n> +       git add a &&\n> +       git commit -m 1 &&\n> +       git repack -adl &&\n> +       echo \"$(pwd)\"/.git/objects/../objects >.git/objects/info/alternates\n> +'\n> +\n> +test_expect_success 're-packing repository with itsself as alternate' '\n> +       git repack -adl &&\n> +       git fsck\n> +'\n> +\n> +test_done\n> --\n> 1.8.4.3\n"},{"id":"246090","messageId":"xmqq8unuv1wm.fsf@gitster.dls.corp.google.com","threadId":"37109","inReplyTo":"53C510A9.4010208@gmail.com","subject":"Re: [PATCH v3] sha1_file: do not add own object directory as alternate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T19:48:41Z","receivedAt":"2014-07-15T19:48:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ephrim Khong <dr.khong@gmail.com> writes:\n\n> diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh\n> new file mode 100755\n> index 0000000..8341d46\n> --- /dev/null\n> +++ b/t/t7702-repack-cyclic-alternate.sh\n> @@ -0,0 +1,24 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2014 Ephrim Khong\n> +#\n> +\n> +test_description='repack involving cyclic alternate'\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\tGIT_OBJECT_DIRECTORY=.git//../.git/objects &&\n> +\texport GIT_OBJECT_DIRECTORY &&\n\nDo you need this artificially strange environment settings for the\nproblem to manifest itself, or is it sufficient to have a\nnon-canonical pathname in the info/alternates file?\n\nExporting an environment early in the test and having later tests in\nthe file depend on it makes it harder to debug when things go wrong,\nthan leaving an info/alternates file in the repository, primarily\nbecause the latter can be inspected more easily in the trash\ndirectory after \"t7702-*.sh -i\" dies, hence the above question.\n\n> +\ttouch a &&\n\nDon't use 'touch' if you are not interested in timestams.  Write this as\n\n\t>a &&\n\nbecause what you care about here in this test is that an empty file\n'a' exists, so that you can run \"git add\" on it.\n\n> +\tgit add a &&\n> +\tgit commit -m 1 &&\n> +\tgit repack -adl &&\n> +\techo \"$(pwd)\"/.git/objects/../objects >.git/objects/info/alternates\n> +'\n> +\n> +test_expect_success 're-packing repository with itsself as alternate' '\n> +\tgit repack -adl &&\n> +\tgit fsck\n> +'\n> +\n> +test_done\n"},{"id":"246149","messageId":"53C61EDB.6050905@gmail.com","threadId":"37109","inReplyTo":"xmqqd2d6v2yf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: do not add own object directory as alternate","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2014-07-16T06:42:35Z","receivedAt":"2014-07-16T06:42:35Z","isPatch":true,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"On 15.07.2014 21:26, Junio C Hamano wrote:\n>> +\tstrbuf_addstr(&objdirbuf, absolute_path(get_object_directory()));\n>> +\tnormalize_path_copy(objdirbuf.buf, objdirbuf.buf);\n>\n> This is somewhat a strange usage of a strbuf.\n\nThere might be a more elegant way, but I tried to mimic the local coding \nstyle and simply copied how the alternate paths were normalized. Let me \nknow if you want this re-rolled without strbuf, otherwise I'll leave it \nas-is.\n\n>> diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh\n>> new file mode 100755\n>> index 0000000..8341d46\n>> --- /dev/null\n>> +++ b/t/t7702-repack-cyclic-alternate.sh\n>\n> Do we really have to waste a new test file only for this test?\n\nAbsolutely not. I'll add it to 7700, which seems appropriate.\n"},{"id":"246150","messageId":"53C6200F.30103@gmail.com","threadId":"37109","inReplyTo":"xmqq8unuv1wm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] sha1_file: do not add own object directory as alternate","fromName":"Ephrim Khong","fromEmail":"dr.khong@gmail.com","sentAt":"2014-07-16T06:47:43Z","receivedAt":"2014-07-16T06:47:43Z","isPatch":true,"sender":{"key":"dr.khong@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2847265?v=4"},"body":"On 15.07.2014 21:48, Junio C Hamano wrote:\n> Ephrim Khong <dr.khong@gmail.com> writes:\n>> +test_expect_success setup '\n>> +\tGIT_OBJECT_DIRECTORY=.git//../.git/objects &&\n>> +\texport GIT_OBJECT_DIRECTORY &&\n>\n> Do you need this artificially strange environment settings for the\n> problem to manifest itself, or is it sufficient to have a\n> non-canonical pathname in the info/alternates file?\n\nThe test should check the normalization of both the paths in \ninfo/alternates and of GIT_OBJECT_DIRECTORY, so I'd prefer to leave it \nin. It is not necessary to reproduce the original problem, though.\n\n> Exporting an environment early in the test and having later tests in\n> the file depend on it makes it harder to debug when things go wrong,\n> than leaving an info/alternates file in the repository, primarily\n> because the latter can be inspected more easily in the trash\n> directory after \"t7702-*.sh -i\" dies, hence the above question.\n\nThat sounds reasonable. The variable is not required at the \ninitialization anyway, so I'll set it right before the actual test and \nwrap it into a subshell to make debugging easier.\n"}]}