{"thread":{"id":"38487","subject":"[PATCHv5] sha1_file: fix iterating loose alternate objects","startedAt":"2015-02-02T20:05:54Z","lastAt":"2015-02-02T21:02:08Z","messageCount":4,"participants":["Jonathon Mah","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"255536","messageId":"E05CAD49-755C-4F26-A527-597B1AD412D8@jonathonmah.com","threadId":"38487","inReplyTo":null,"subject":"[PATCHv5] sha1_file: fix iterating loose alternate objects","fromName":"Jonathon Mah","fromEmail":"me@jonathonmah.com","sentAt":"2015-02-02T20:05:54Z","receivedAt":"2015-02-02T20:05:54Z","isPatch":false,"sender":{"key":"me@jonathonmah.com","avatar":"https://avatars.githubusercontent.com/u/2748?v=4"},"body":"The string in 'base' contains a path suffix to a specific object; when\nits value is used, the suffix must either be filled (as in\nstat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared\n(as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb\n(introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and\ntreated 'base' as a complete path to the \"base\" object directory,\ninstead of a pointer to the \"base\" of the full path string.\n\nThe trailing path after 'base' is still initialized to NUL, hiding the\nbug in some common cases. Additionally the descendent\nfor_each_file_in_obj_subdir function swallows ENOENT, so an error only\nshows if the alternate's path was last filled with a valid object\n(where statting /path/to/existing/00/0bjectfile/00 fails).\n\nSigned-off-by: Jonathon Mah \n---\n\nSimplified test per Junio (verified that it fails before and passes now). Punting on Jeff's \"more elaborate example\".\n\n sha1_file.c | 10 +++++++---\n t/t5304-prune.sh | 8 ++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 30995e6..fcb1c4b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3396,9 +3396,13 @@ static int loose_from_alt_odb(struct alternate_object_database *alt,\n \t\t\t void *vdata)\n {\n \tstruct loose_alt_odb_data *data = vdata;\n-\treturn for_each_loose_file_in_objdir(alt->base,\n-\t\t\t\t\t data->cb, NULL, NULL,\n-\t\t\t\t\t data->data);\n+\tint r;\n+\talt->name[-1] = 0;\n+\tr = for_each_loose_file_in_objdir(alt->base,\n+\t\t\t\t\t data->cb, NULL, NULL,\n+\t\t\t\t\t data->data);\n+\talt->name[-1] = '/';\n+\treturn r;\n }\n \n int for_each_loose_object(each_loose_object_fn cb, void *data)\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex e32e46d..0794d33 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -253,4 +253,12 @@ test_expect_success 'prune .git/shallow' '\n \ttest_path_is_missing .git/shallow\n '\n \n+test_expect_success 'prune: handle alternate object database' '\n+\ttest_create_repo A &&\n+\tgit -C A commit --allow-empty -m \"initial commit\" &&\n+\tgit clone --shared A B &&\n+\tgit -C B commit --allow-empty -m \"next commit\" &&\n+\tgit -C B prune\n+'\n+\n test_done\n-- \n2.3.0.rc2.2.g184f7a0\n"},{"id":"255539","messageId":"20150202202733.GB28915@peff.net","threadId":"38487","inReplyTo":"E05CAD49-755C-4F26-A527-597B1AD412D8@jonathonmah.com","subject":"Re: [PATCHv5] sha1_file: fix iterating loose alternate objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-02T20:27:33Z","receivedAt":"2015-02-02T20:27:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2015 at 12:05:54PM -0800, Jonathon Mah wrote:\n\n> Simplified test per Junio (verified that it fails before and passes\n> now). Punting on Jeff's \"more elaborate example\".\n\nI think that's fine. I started to try to create such an example, but\nit's actually rather tricky. If the alternate has the tip object, then\none of these must be true:\n\n 1. It has all of the objects the tip depends on.\n\n 2. It is missing an object, and this tip is part of the referenced\n history.\n\n 3. It is missing an object, but this part of history is not\n referenced.\n\nIn case (1), we do not care about deleting objects from the base\nrepository; we already have another copy in the alternate.\n\nIn case (2), the alternate is corrupt, and all bets are off.\n\nIn case (3), we can only have dropped the object from the alternate by\npruning it and keeping the tip object that refers to it. Which is the\nexact thing that this new code was added to avoid (to always keep\ndepended-upon objects).\n\nSo I actually do not see how the situation would come up in practice,\nand possibly we could drop the iteration of the alternates' loose\nobjects entirely from this code. But certainly that is orthogonal to\nJonathon's fix (which is a true regression for the less-exotic case that\nhis test demonstrates).\n\n-Peff\n"},{"id":"255541","messageId":"xmqq7fw0t4x8.fsf@gitster.dls.corp.google.com","threadId":"38487","inReplyTo":"20150202202733.GB28915@peff.net","subject":"Re: [PATCHv5] sha1_file: fix iterating loose alternate objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T20:49:23Z","receivedAt":"2015-02-02T20:49:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I actually do not see how the situation would come up in practice,\n> and possibly we could drop the iteration of the alternates' loose\n> objects entirely from this code. But certainly that is orthogonal to\n> Jonathon's fix (which is a true regression for the less-exotic case that\n> his test demonstrates).\n\nSure.\n\nThis needs to go to both 'maint' and 'master', right?\n"},{"id":"255543","messageId":"20150202210208.GA31675@peff.net","threadId":"38487","inReplyTo":"xmqq7fw0t4x8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv5] sha1_file: fix iterating loose alternate objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-02T21:02:08Z","receivedAt":"2015-02-02T21:02:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2015 at 12:49:23PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So I actually do not see how the situation would come up in practice,\n> > and possibly we could drop the iteration of the alternates' loose\n> > objects entirely from this code. But certainly that is orthogonal to\n> > Jonathon's fix (which is a true regression for the less-exotic case that\n> > his test demonstrates).\n> \n> Sure.\n> \n> This needs to go to both 'maint' and 'master', right?\n\nYes (on the jk/prune-mtime topic).\n\n-Peff\n"}]}