{"thread":{"id":"35802","subject":"[PATCH] repack.c: rename and unlink pack file if it exists","startedAt":"2014-02-04T23:40:15Z","lastAt":"2014-02-05T21:09:54Z","messageCount":15,"participants":["Junio C Hamano","Jeff King","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"234261","messageId":"xmqqwqha5twg.fsf@gitster.dls.corp.google.com","threadId":"35802","inReplyTo":null,"subject":"[PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-04T23:40:15Z","receivedAt":"2014-02-04T23:40:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This comment in builtin/repack.c:\n\n    First see if there are packs of the same name and if so\n    if we can move them out of the way (this can happen if we\n    repacked immediately after packing fully).\n\nWhen a repo was fully repacked, and is repacked again, we may run\ninto the situation that \"new\" packfiles have the same name as\nalready existing ones (traditionally packfiles have been named after\nthe list of names of objects in them, so repacking all the objects\nin a single pack would have produced a packfile with the same name).\n\nThe logic is to rename the existing ones into filename like\n\"old-XXX\", create the new ones and then remove the \"old-\" ones.\nWhen something went wrong in the middle, this sequence is rolled\nback by renaming the \"old-\" files back.\n\nThe renaming into \"old-\" did not work as designed, because\nfile_exists() was done on the wrong file name.  This could give\nproblems for a repo on a network share under Windows, as reported by\nJochen Haag <zwanzig12@googlemail.com>.\n\nFormulate the filenames objects/pack/pack-XXXX.{pack,idx} correctly\n(the code originally lacked \"pack-\" prefix when checking if the file\nexists).\n\nAlso rename the variables to match what they are used for:\nfname for the final name of the new packfile, fname_old for the name\nof the existing one, and fname_tmp for the temporary name pack-objects\ncreated the new packfile in.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Somehow this came to my private mailbox without Cc to list, so I\n   am forwarding it.\n\n   I think with 1190a1ac (pack-objects: name pack files after\n   trailer hash, 2013-12-05), repacking the same set of objects may\n   have less chance of producing colliding names, especially if you\n   are on a box with more than one core, but it still would be a\n   good idea to get this part right in the upcoming release.\n\n   The bug in the first hunk will cause us not to find any existing\n   file, even when there is a name clash.  And then we try to\n   immediately the final rename without any provision for rolling\n   back.  The other two hunks are pure noise renaming variables.\n\n   I think the patch looks correct, but I'd appreciate a second set\n   of eyeballs.\n\n   Thanks.\n\n builtin/repack.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex bca7710..3b01353 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < 2; ext++) {\n \t\t\tchar *fname, *fname_old;\n-\t\t\tfname = mkpathdup(\"%s/%s%s\", packdir,\n+\t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n \t\t\t\t\t\titem->string, exts[ext]);\n \t\t\tif (!file_exists(fname)) {\n \t\t\t\tfree(fname);\n@@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t/* Now the ones with the same name are out of the way... */\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < 2; ext++) {\n-\t\t\tchar *fname, *fname_old;\n+\t\t\tchar *fname, *fname_tmp;\n \t\t\tstruct stat statbuffer;\n \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n \t\t\t\t\tpackdir, item->string, exts[ext]);\n-\t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n+\t\t\tfname_tmp = mkpathdup(\"%s-%s%s\",\n \t\t\t\t\tpacktmp, item->string, exts[ext]);\n-\t\t\tif (!stat(fname_old, &statbuffer)) {\n+\t\t\tif (!stat(fname_tmp, &statbuffer)) {\n \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n-\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n+\t\t\t\tchmod(fname_tmp, statbuffer.st_mode);\n \t\t\t}\n-\t\t\tif (rename(fname_old, fname))\n-\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n+\t\t\tif (rename(fname_tmp, fname))\n+\t\t\t\tdie_errno(_(\"renaming '%s' into '%s' failed\"), fname_tmp, fname);\n \t\t\tfree(fname);\n-\t\t\tfree(fname_old);\n+\t\t\tfree(fname_tmp);\n \t\t}\n \t}\n \n \t/* Remove the \"old-\" files */\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < 2; ext++) {\n-\t\t\tchar *fname;\n-\t\t\tfname = mkpath(\"%s/old-pack-%s%s\",\n+\t\t\tchar *fname_old;\n+\t\t\tfname_old = mkpath(\"%s/old-%s%s\",\n \t\t\t\t\tpackdir,\n \t\t\t\t\titem->string,\n \t\t\t\t\texts[ext]);\n-\t\t\tif (remove_path(fname))\n-\t\t\t\twarning(_(\"removing '%s' failed\"), fname);\n+\t\t\tif (remove_path(fname_old))\n+\t\t\t\twarning(_(\"removing '%s' failed\"), fname_old);\n \t\t}\n \t}\n \n-- \n1.9-rc2-217-g24a8b2e\n"},{"id":"234262","messageId":"xmqqsiry5trb.fsf@gitster.dls.corp.google.com","threadId":"35802","inReplyTo":"xmqqwqha5twg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-04T23:43:20Z","receivedAt":"2014-02-04T23:43:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> This comment in builtin/repack.c:\n> ...\n\nOops; there was supposed to be these lines before anythning else:\n\n\tFrom: Torsten Bögershausen <tboegi@web.de>\n\tDate: Sun Feb 2 16:09:56 2014 +0100\n\n>     First see if there are packs of the same name and if so\n>     if we can move them out of the way (this can happen if we\n>     repacked immediately after packing fully).\n>\n> When a repo was fully repacked, and is repacked again, we may run\n> into the situation that \"new\" packfiles have the same name as\n> already existing ones (traditionally packfiles have been named after\n> the list of names of objects in them, so repacking all the objects\n> in a single pack would have produced a packfile with the same name).\n>\n> The logic is to rename the existing ones into filename like\n> \"old-XXX\", create the new ones and then remove the \"old-\" ones.\n> When something went wrong in the middle, this sequence is rolled\n> back by renaming the \"old-\" files back.\n>\n> The renaming into \"old-\" did not work as designed, because\n> file_exists() was done on the wrong file name.  This could give\n> problems for a repo on a network share under Windows, as reported by\n> Jochen Haag <zwanzig12@googlemail.com>.\n>\n> Formulate the filenames objects/pack/pack-XXXX.{pack,idx} correctly\n> (the code originally lacked \"pack-\" prefix when checking if the file\n> exists).\n>\n> Also rename the variables to match what they are used for:\n> fname for the final name of the new packfile, fname_old for the name\n> of the existing one, and fname_tmp for the temporary name pack-objects\n> created the new packfile in.\n>\n> Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * Somehow this came to my private mailbox without Cc to list, so I\n>    am forwarding it.\n>\n>    I think with 1190a1ac (pack-objects: name pack files after\n>    trailer hash, 2013-12-05), repacking the same set of objects may\n>    have less chance of producing colliding names, especially if you\n>    are on a box with more than one core, but it still would be a\n>    good idea to get this part right in the upcoming release.\n>\n>    The bug in the first hunk will cause us not to find any existing\n>    file, even when there is a name clash.  And then we try to\n>    immediately the final rename without any provision for rolling\n>    back.  The other two hunks are pure noise renaming variables.\n>\n>    I think the patch looks correct, but I'd appreciate a second set\n>    of eyeballs.\n>\n>    Thanks.\n>\n>  builtin/repack.c | 24 ++++++++++++------------\n>  1 file changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index bca7710..3b01353 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tfor_each_string_list_item(item, &names) {\n>  \t\tfor (ext = 0; ext < 2; ext++) {\n>  \t\t\tchar *fname, *fname_old;\n> -\t\t\tfname = mkpathdup(\"%s/%s%s\", packdir,\n> +\t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n>  \t\t\t\t\t\titem->string, exts[ext]);\n>  \t\t\tif (!file_exists(fname)) {\n>  \t\t\t\tfree(fname);\n> @@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t/* Now the ones with the same name are out of the way... */\n>  \tfor_each_string_list_item(item, &names) {\n>  \t\tfor (ext = 0; ext < 2; ext++) {\n> -\t\t\tchar *fname, *fname_old;\n> +\t\t\tchar *fname, *fname_tmp;\n>  \t\t\tstruct stat statbuffer;\n>  \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n>  \t\t\t\t\tpackdir, item->string, exts[ext]);\n> -\t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n> +\t\t\tfname_tmp = mkpathdup(\"%s-%s%s\",\n>  \t\t\t\t\tpacktmp, item->string, exts[ext]);\n> -\t\t\tif (!stat(fname_old, &statbuffer)) {\n> +\t\t\tif (!stat(fname_tmp, &statbuffer)) {\n>  \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> -\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> +\t\t\t\tchmod(fname_tmp, statbuffer.st_mode);\n>  \t\t\t}\n> -\t\t\tif (rename(fname_old, fname))\n> -\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> +\t\t\tif (rename(fname_tmp, fname))\n> +\t\t\t\tdie_errno(_(\"renaming '%s' into '%s' failed\"), fname_tmp, fname);\n>  \t\t\tfree(fname);\n> -\t\t\tfree(fname_old);\n> +\t\t\tfree(fname_tmp);\n>  \t\t}\n>  \t}\n>  \n>  \t/* Remove the \"old-\" files */\n>  \tfor_each_string_list_item(item, &names) {\n>  \t\tfor (ext = 0; ext < 2; ext++) {\n> -\t\t\tchar *fname;\n> -\t\t\tfname = mkpath(\"%s/old-pack-%s%s\",\n> +\t\t\tchar *fname_old;\n> +\t\t\tfname_old = mkpath(\"%s/old-%s%s\",\n>  \t\t\t\t\tpackdir,\n>  \t\t\t\t\titem->string,\n>  \t\t\t\t\texts[ext]);\n> -\t\t\tif (remove_path(fname))\n> -\t\t\t\twarning(_(\"removing '%s' failed\"), fname);\n> +\t\t\tif (remove_path(fname_old))\n> +\t\t\t\twarning(_(\"removing '%s' failed\"), fname_old);\n>  \t\t}\n>  \t}\n"},{"id":"234264","messageId":"20140205011632.GA3923@sigill.intra.peff.net","threadId":"35802","inReplyTo":"xmqqwqha5twg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T01:16:32Z","receivedAt":"2014-02-05T01:16:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote:\n\n>  * Somehow this came to my private mailbox without Cc to list, so I\n>    am forwarding it.\n> \n>    I think with 1190a1ac (pack-objects: name pack files after\n>    trailer hash, 2013-12-05), repacking the same set of objects may\n>    have less chance of producing colliding names, especially if you\n>    are on a box with more than one core, but it still would be a\n>    good idea to get this part right in the upcoming release.\n\nActually, since 1190a1ac, if you have repacked and gotten the same pack\nname, then you do not have to do any rename dance at all; you can throw\naway what you just generated because you know that it is byte-for-byte\nidentical.\n\nYou could collide with a pack created by an older version of git that\nused the original scheme, but that is quite unlikely (on the order of\n2^-160).\n\n-Peff\n"},{"id":"234301","messageId":"52F27AF0.7030007@web.de","threadId":"35802","inReplyTo":"20140205011632.GA3923@sigill.intra.peff.net","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-02-05T17:54:56Z","receivedAt":"2014-02-05T17:54:56Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-02-05 02.16, Jeff King wrote:\n> On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote:\n> \n>>  * Somehow this came to my private mailbox without Cc to list, so I\n>>    am forwarding it.\n>>\n>>    I think with 1190a1ac (pack-objects: name pack files after\n>>    trailer hash, 2013-12-05), repacking the same set of objects may\n>>    have less chance of producing colliding names, especially if you\n>>    are on a box with more than one core, but it still would be a\n>>    good idea to get this part right in the upcoming release.\n> \n> Actually, since 1190a1ac, if you have repacked and gotten the same pack\n> name, then you do not have to do any rename dance at all; you can throw\n> away what you just generated because you know that it is byte-for-byte\n> identical.\n> \n> You could collide with a pack created by an older version of git that\n> used the original scheme, but that is quite unlikely (on the order of\n> 2^-160).\n> \n> -Peff\nOK, I messed up the email so it went only to Junios mailbox. Sorry for confusion.\n\nJochen, it could be good if you could test this version of the patch\n(I couldn't reproduce the problem here)\n\n/Torsten\n"},{"id":"234329","messageId":"xmqq1tzh494e.fsf@gitster.dls.corp.google.com","threadId":"35802","inReplyTo":"20140205011632.GA3923@sigill.intra.peff.net","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-05T20:06:41Z","receivedAt":"2014-02-05T20:06:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote:\n>\n>>  * Somehow this came to my private mailbox without Cc to list, so I\n>>    am forwarding it.\n>> \n>>    I think with 1190a1ac (pack-objects: name pack files after\n>>    trailer hash, 2013-12-05), repacking the same set of objects may\n>>    have less chance of producing colliding names, especially if you\n>>    are on a box with more than one core, but it still would be a\n>>    good idea to get this part right in the upcoming release.\n>\n> Actually, since 1190a1ac, if you have repacked and gotten the same pack\n> name, then you do not have to do any rename dance at all; you can throw\n> away what you just generated because you know that it is byte-for-byte\n> identical.\n>\n> You could collide with a pack created by an older version of git that\n> used the original scheme, but that is quite unlikely (on the order of\n> 2^-160).\n\nYes, so in that sense this is not so urgent, but I'm tempted to\nsplit the original patch into two and merge only the first one to\n'master' before -rc3 (see below).  The renaming of the variables\nadded enough noise to cause me fail to spot a change mixed within.\n\n-- >8 --\nFrom: Torsten Bögershausen <tboegi@web.de>\nDate: Sun, 2 Feb 2014 16:09:56 +0100\n\nWhen a repo was fully repacked, and is repacked again, we may run\ninto the situation that \"new\" packfiles have the same name as\nalready existing ones (traditionally packfiles have been named after\nthe list of names of objects in them, so repacking all the objects\nin a single pack would have produced a packfile with the same name).\n\nThe logic is to rename the existing ones into filename like\n\"old-XXX\", create the new ones and then remove the \"old-\" ones.\nWhen something went wrong in the middle, this sequence is rolled\nback by renaming the \"old-\" files back.\n\nThe renaming into \"old-\" did not work as intended, because\nfile_exists() was done on \"XXX\", not \"pack-XXX\".  Also when rolling\nback the change, the code tried to rename \"old-pack-XXX\" but the\nsaved ones are named \"old-XXX\", so this couldn't have worked.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/repack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex bca7710..fe31577 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < 2; ext++) {\n \t\t\tchar *fname, *fname_old;\n-\t\t\tfname = mkpathdup(\"%s/%s%s\", packdir,\n+\t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n \t\t\t\t\t\titem->string, exts[ext]);\n \t\t\tif (!file_exists(fname)) {\n \t\t\t\tfree(fname);\n@@ -337,7 +337,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < 2; ext++) {\n \t\t\tchar *fname;\n-\t\t\tfname = mkpath(\"%s/old-pack-%s%s\",\n+\t\t\tfname = mkpath(\"%s/old-%s%s\",\n \t\t\t\t\tpackdir,\n \t\t\t\t\titem->string,\n \t\t\t\t\texts[ext]);\n-- \n1.9-rc2-217-g24a8b2e\n"},{"id":"234324","messageId":"xmqqwqh92uds.fsf@gitster.dls.corp.google.com","threadId":"35802","inReplyTo":"20140205011632.GA3923@sigill.intra.peff.net","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-05T20:10:23Z","receivedAt":"2014-02-05T20:10:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"... and this is the other half that is supposed to be only about\nrenaming variables.\n\n-- >8 --\nFrom: Torsten Bögershausen <tboegi@web.de>\nDate: Sun, 2 Feb 2014 16:09:56 +0100\nSubject: [PATCH] repack.c: rename a few variables\n\nRename the variables to match what they are used for: fname for the\nfinal name of the new packfile, fname_old for the name of the\nexisting one, and fname_tmp for the temporary name pack-objects\ncreated the new packfile in.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/repack.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex fe31577..3b01353 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t/* Now the ones with the same name are out of the way... */\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < 2; ext++) {\n-\t\t\tchar *fname, *fname_old;\n+\t\t\tchar *fname, *fname_tmp;\n \t\t\tstruct stat statbuffer;\n \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n \t\t\t\t\tpackdir, item->string, exts[ext]);\n-\t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n+\t\t\tfname_tmp = mkpathdup(\"%s-%s%s\",\n \t\t\t\t\tpacktmp, item->string, exts[ext]);\n-\t\t\tif (!stat(fname_old, &statbuffer)) {\n+\t\t\tif (!stat(fname_tmp, &statbuffer)) {\n \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n-\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n+\t\t\t\tchmod(fname_tmp, statbuffer.st_mode);\n \t\t\t}\n-\t\t\tif (rename(fname_old, fname))\n-\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n+\t\t\tif (rename(fname_tmp, fname))\n+\t\t\t\tdie_errno(_(\"renaming '%s' into '%s' failed\"), fname_tmp, fname);\n \t\t\tfree(fname);\n-\t\t\tfree(fname_old);\n+\t\t\tfree(fname_tmp);\n \t\t}\n \t}\n \n \t/* Remove the \"old-\" files */\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < 2; ext++) {\n-\t\t\tchar *fname;\n-\t\t\tfname = mkpath(\"%s/old-%s%s\",\n+\t\t\tchar *fname_old;\n+\t\t\tfname_old = mkpath(\"%s/old-%s%s\",\n \t\t\t\t\tpackdir,\n \t\t\t\t\titem->string,\n \t\t\t\t\texts[ext]);\n-\t\t\tif (remove_path(fname))\n-\t\t\t\twarning(_(\"removing '%s' failed\"), fname);\n+\t\t\tif (remove_path(fname_old))\n+\t\t\t\twarning(_(\"removing '%s' failed\"), fname_old);\n \t\t}\n \t}\n \n-- \n1.9-rc2-217-g24a8b2e\n"},{"id":"234325","messageId":"20140205201243.GA16899@sigill.intra.peff.net","threadId":"35802","inReplyTo":"xmqq1tzh494e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T20:12:43Z","receivedAt":"2014-02-05T20:12:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 05, 2014 at 12:06:41PM -0800, Junio C Hamano wrote:\n\n> > Actually, since 1190a1ac, if you have repacked and gotten the same pack\n> > name, then you do not have to do any rename dance at all; you can throw\n> > away what you just generated because you know that it is byte-for-byte\n> > identical.\n> >\n> > You could collide with a pack created by an older version of git that\n> > used the original scheme, but that is quite unlikely (on the order of\n> > 2^-160).\n> \n> Yes, so in that sense this is not so urgent, but I'm tempted to\n> split the original patch into two and merge only the first one to\n> 'master' before -rc3 (see below).  The renaming of the variables\n> added enough noise to cause me fail to spot a change mixed within.\n\nThat sounds very sensible. The only reason I did not follow-up 1190a1ac\nimmediately with a patch to drop the rename code was that it is a\nsensitive area, and I wanted to be very sure there would be no other\nfallouts. And then of course I didn't get around to it yet. But\nfollowing the same logic, trying to do it during -rc would be a terrible\nidea. :)\n\nThe minimal fix you posted below does make sense to me as a stopgap, and\nwe can look into dropping the code entirely during the next cycle. It\nwould be nice to have a test to cover this case, though.\n\n-Peff\n"},{"id":"234327","messageId":"xmqqsirx2teh.fsf@gitster.dls.corp.google.com","threadId":"35802","inReplyTo":"20140205201243.GA16899@sigill.intra.peff.net","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-05T20:31:34Z","receivedAt":"2014-02-05T20:31:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The minimal fix you posted below does make sense to me as a stopgap, and\n> we can look into dropping the code entirely during the next cycle. It\n> would be nice to have a test to cover this case, though.\n\nSounds sensible.  Run \"repack -a -d\" once, and then another while\nforcing it to be single threaded, or something?\n"},{"id":"234330","messageId":"20140205203740.GA17077@sigill.intra.peff.net","threadId":"35802","inReplyTo":"xmqqsirx2teh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T20:37:40Z","receivedAt":"2014-02-05T20:37:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 05, 2014 at 12:31:34PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The minimal fix you posted below does make sense to me as a stopgap, and\n> > we can look into dropping the code entirely during the next cycle. It\n> > would be nice to have a test to cover this case, though.\n> \n> Sounds sensible.  Run \"repack -a -d\" once, and then another while\n> forcing it to be single threaded, or something?\n\nCertainly that's the way to trigger the code, but doing this:\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex b45bd1e..6647ba1 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -162,7 +162,12 @@ test_expect_success 'objects made unreachable by grafts only are kept' '\n \tgit reflog expire --expire=$test_tick --expire-unreachable=$test_tick --all &&\n \tgit repack -a -d &&\n \tgit cat-file -t $H1\n-\t'\n+'\n+\n+test_expect_success 'repack can handle generating the same pack again' '\n+\tgit -c pack.threads=1 repack -ad &&\n+\tgit -c pack.threads=1 repack -ad\n+'\n \n test_done\n \n\n...does not seem to fail, and it does not seem to leave any cruft in\nplace. So maybe I am misunderstanding the thing the patch is meant to\nfix. Is it that we simply do not replace the pack in this instance?\n\nI guess we would have to generate a pack with the identical set of\nobjects, then, but somehow different in its pack parameters (perhaps\nturning off deltas?).\n\n-Peff\n"},{"id":"234333","messageId":"20140205205405.GA24100@sigill.intra.peff.net","threadId":"35802","inReplyTo":"20140205203740.GA17077@sigill.intra.peff.net","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T20:54:05Z","receivedAt":"2014-02-05T20:54:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 05, 2014 at 03:37:40PM -0500, Jeff King wrote:\n\n> > Sounds sensible.  Run \"repack -a -d\" once, and then another while\n> > forcing it to be single threaded, or something?\n> \n> Certainly that's the way to trigger the code, but doing this:\n> [...]\n> ...does not seem to fail, and it does not seem to leave any cruft in\n> place. So maybe I am misunderstanding the thing the patch is meant to\n> fix. Is it that we simply do not replace the pack in this instance?\n> \n> I guess we would have to generate a pack with the identical set of\n> objects, then, but somehow different in its pack parameters (perhaps\n> turning off deltas?).\n\nHere's a more robust test that actually checks the pack contents:\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex b45bd1e..c18a318 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -164,5 +164,17 @@ test_expect_success 'objects made unreachable by grafts only are kept' '\n \tgit cat-file -t $H1\n \t'\n \n+test_expect_success 'repack can handle generating the same pack again' '\n+\tshow_deltas() {\n+\t\tgit rev-list --objects --all --reflog |\n+\t\tgit cat-file --batch-check=\"%(objectname) %(deltabase) %(rest)\"\n+\t}\n+\tgit -c pack.threads=1 repack -adf --window=0 &&\n+\tshow_deltas >no-deltas &&\n+\tgit -c pack.threads=1 repack -adf --window=10 &&\n+\tshow_deltas >deltas &&\n+\t! test_cmp no-deltas deltas\n+'\n+\n test_done\n \n\nwhich _also_ does not fail. And then I realized it is because of\n1190a1ac, which gives these two separate names.\n\nSo I am not sure if it is even possible to trigger the bug in a\nmeaningful way at this point.\n\n-Peff\n"},{"id":"234334","messageId":"xmqqfvnx2s7p.fsf@gitster.dls.corp.google.com","threadId":"35802","inReplyTo":"20140205203740.GA17077@sigill.intra.peff.net","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-05T20:57:14Z","receivedAt":"2014-02-05T20:57:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Feb 05, 2014 at 12:31:34PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > The minimal fix you posted below does make sense to me as a stopgap, and\n>> > we can look into dropping the code entirely during the next cycle. It\n>> > would be nice to have a test to cover this case, though.\n>> \n>> Sounds sensible.  Run \"repack -a -d\" once, and then another while\n>> forcing it to be single threaded, or something?\n>\n> Certainly that's the way to trigger the code, but doing this:\n>\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index b45bd1e..6647ba1 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -162,7 +162,12 @@ test_expect_success 'objects made unreachable by grafts only are kept' '\n>  \tgit reflog expire --expire=$test_tick --expire-unreachable=$test_tick --all &&\n>  \tgit repack -a -d &&\n>  \tgit cat-file -t $H1\n> -\t'\n> +'\n> +\n> +test_expect_success 'repack can handle generating the same pack again' '\n> +\tgit -c pack.threads=1 repack -ad &&\n> +\tgit -c pack.threads=1 repack -ad\n> +'\n>  \n>  test_done\n>  \n>\n> ...does not seem to fail, and it does not seem to leave any cruft in\n> place. So maybe I am misunderstanding the thing the patch is meant to\n> fix. Is it that we simply do not replace the pack in this instance?\n\nYes.  Not just the command finishing OK, but the packfile left by\nthe first repack needs to be left intact.  One bug was that after\nlearning that a new packfile XXXX needs to be installed, the command\ndid not check existing .git/objects/pack/pack-XXXX.{pack,idx}, but\nchecked .git/objects/pack/XXXX.{pack,idx}, deciding that there is\nnothing that needs to be saved by renaming with s|pack-|old-|.  This\nwould have caused it to rename the new packfile left by pack-object\nat .git/objects/pack/.tmp-$pid-pack-XXXX.{pack,idx} directly to the\nfinal .git/objects/pack/pack-XXXX.{pack,idx}, which would work only\non filesystems that allow renaming over an existing file.\n\nAnother bug fixed by Torsten was in the codepath to roll the rename\nback from .git/objects/pack/old-XXXX.{pack,idx} to the original (the\ncommand tried to rename .git/objects/pack/old-pack-XXXX.{pack,idx}\nwhich would not have been the ones it renamed), but because of the\nearlier bug, it would never have triggered in the first place.\n\n> I guess we would have to generate a pack with the identical set of\n> objects, then, but somehow different in its pack parameters (perhaps\n> turning off deltas?).\n\nUnfortunately, that would trigger different codepath on v1.9-rc0 and\nlater with 1190a1ac (pack-objects: name pack files after trailer\nhash, 2013-12-05), as it is likely not to name the packfiles the\nsame.\n\nWe could use test-chmtime to reset the timestamp of the packfile\ngenerated by the first repack to somewhere reasonably old and then\nrerun the repack to see that it is a different file, which may be\nmore portable than inspecting the inum of the packfile.\n"},{"id":"234335","messageId":"52F2A69C.50000@web.de","threadId":"35802","inReplyTo":"xmqqsirx2teh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-02-05T21:01:16Z","receivedAt":"2014-02-05T21:01:16Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-02-05 21.31, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> The minimal fix you posted below does make sense to me as a stopgap, and\n>> we can look into dropping the code entirely during the next cycle. It\n>> would be nice to have a test to cover this case, though.\n> \n> Sounds sensible.  Run \"repack -a -d\" once, and then another while\n> forcing it to be single threaded, or something?\nI can put a test case on my todo list,\nand thanks for the minimal patch.\n"},{"id":"234336","messageId":"20140205210129.GA24314@sigill.intra.peff.net","threadId":"35802","inReplyTo":"xmqqfvnx2s7p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T21:01:29Z","receivedAt":"2014-02-05T21:01:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 05, 2014 at 12:57:14PM -0800, Junio C Hamano wrote:\n\n> > ...does not seem to fail, and it does not seem to leave any cruft in\n> > place. So maybe I am misunderstanding the thing the patch is meant to\n> > fix. Is it that we simply do not replace the pack in this instance?\n> \n> Yes.  Not just the command finishing OK, but the packfile left by\n> the first repack needs to be left intact. [...]\n\nThanks for the explanation. Having looked at this now, I'm thinking a\ntest may not be worth the trouble. Due to 1190a1ac, we effectively don't\ncare whether we get the old pack or the new one. So the fact that this\nbug exists doesn't really produce any user-visible behavior, and\nhopefully post-release we would drop the code entirely, and the test\nwould have no reason to exist.\n\n> We could use test-chmtime to reset the timestamp of the packfile\n> generated by the first repack to somewhere reasonably old and then\n> rerun the repack to see that it is a different file, which may be\n> more portable than inspecting the inum of the packfile.\n\nYeah, I think that would work. But it sounds like we also need a\nfilesystem in which rename() does not overwrite. So the test would not\nbe portable.\n\n-Peff\n"},{"id":"234339","messageId":"xmqq7g992ror.fsf@gitster.dls.corp.google.com","threadId":"35802","inReplyTo":"20140205210129.GA24314@sigill.intra.peff.net","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-05T21:08:36Z","receivedAt":"2014-02-05T21:08:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... So the fact that this\n> bug exists doesn't really produce any user-visible behavior, and\n> hopefully post-release we would drop the code entirely, and the test\n> would have no reason to exist.\n\nHeh, thanks, and I agree with the reasoning for the longer-term\ndirection.  Perhaps I can/should hold it off that minimal fix-up\npatch from -rc3, then?  I am on the fence but I already started my\ntoday's integration cycle _with_ the fix merged to 'master', so...\n"},{"id":"234340","messageId":"20140205210954.GA24142@sigill.intra.peff.net","threadId":"35802","inReplyTo":"xmqq7g992ror.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack.c: rename and unlink pack file if it exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-05T21:09:54Z","receivedAt":"2014-02-05T21:09:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 05, 2014 at 01:08:36PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > ... So the fact that this\n> > bug exists doesn't really produce any user-visible behavior, and\n> > hopefully post-release we would drop the code entirely, and the test\n> > would have no reason to exist.\n> \n> Heh, thanks, and I agree with the reasoning for the longer-term\n> direction.  Perhaps I can/should hold it off that minimal fix-up\n> patch from -rc3, then?  I am on the fence but I already started my\n> today's integration cycle _with_ the fix merged to 'master', so...\n\nI would say leave the fix for -rc3, but not worry about the test.\n\n-Peff\n"}]}