{"thread":{"id":"38486","subject":"[PATCHv4] sha1_file: fix iterating loose alternate objects","startedAt":"2015-02-02T18:48:12Z","lastAt":"2015-02-02T20:02:58Z","messageCount":5,"participants":["Jonathon Mah","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"255528","messageId":"4727F1DC-2FC3-49BE-8C6D-0C4D7D8B107C@jonathonmah.com","threadId":"38486","inReplyTo":null,"subject":"[PATCHv4] sha1_file: fix iterating loose alternate objects","fromName":"Jonathon Mah","fromEmail":"me@jonathonmah.com","sentAt":"2015-02-02T18:48:12Z","receivedAt":"2015-02-02T18:48:12Z","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---\nSquashed test and fix.\n\n sha1_file.c | 10 +++++++---\n t/t5304-prune.sh | 14 ++++++++++++++\n 2 files changed, 21 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..c65cf9b 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -253,4 +253,18 @@ 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+\t\t(cd A &&\n+\t\techo \"Hello World\" >file1 &&\n+\t\tgit add file1 &&\n+\t\tgit commit -m \"Initial commit\" file1) &&\n+\tgit clone -s A B &&\n+\t\t(cd B &&\n+\t\techo \"foo bar\" >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -m \"next commit\" file2 &&\n+\t\tgit prune)\n+'\n+\n test_done\n-- \n2.3.0.rc2.2.g184f7a0\n"},{"id":"255529","messageId":"20150202185049.GA27399@peff.net","threadId":"38486","inReplyTo":"4727F1DC-2FC3-49BE-8C6D-0C4D7D8B107C@jonathonmah.com","subject":"Re: [PATCHv4] sha1_file: fix iterating loose alternate objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-02T18:50:49Z","receivedAt":"2015-02-02T18:50:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2015 at 10:48:12AM -0800, Jonathon Mah wrote:\n\n> The string in 'base' contains a path suffix to a specific object; when\n> its value is used, the suffix must either be filled (as in\n> stat_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\n> treated 'base' as a complete path to the \"base\" object directory,\n> instead of a pointer to the \"base\" of the full path string.\n> \n> The trailing path after 'base' is still initialized to NUL, hiding the\n> bug in some common cases. Additionally the descendent\n> for_each_file_in_obj_subdir function swallows ENOENT, so an error only\n> shows if the alternate's path was last filled with a valid object\n> (where statting /path/to/existing/00/0bjectfile/00 fails).\n> \n> Signed-off-by: Jonathon Mah \n> ---\n> Squashed test and fix.\n\nThanks, this version looks good to me.\n\n-Peff\n"},{"id":"255532","messageId":"xmqqoapct8bl.fsf@gitster.dls.corp.google.com","threadId":"38486","inReplyTo":"20150202185049.GA27399@peff.net","subject":"Re: [PATCHv4] sha1_file: fix iterating loose alternate objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T19:35:58Z","receivedAt":"2015-02-02T19:35:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 02, 2015 at 10:48:12AM -0800, Jonathon Mah wrote:\n>\n>> The string in 'base' contains a path suffix to a specific object; when\n>> its value is used, the suffix must either be filled (as in\n>> stat_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\n>> treated 'base' as a complete path to the \"base\" object directory,\n>> instead of a pointer to the \"base\" of the full path string.\n>> \n>> The trailing path after 'base' is still initialized to NUL, hiding the\n>> bug in some common cases. Additionally the descendent\n>> for_each_file_in_obj_subdir function swallows ENOENT, so an error only\n>> shows if the alternate's path was last filled with a valid object\n>> (where statting /path/to/existing/00/0bjectfile/00 fails).\n>> \n>> Signed-off-by: Jonathon Mah \n>> ---\n>> Squashed test and fix.\n>\n> Thanks, this version looks good to me.\n\nThanks, both of you.\n\nThe analysis, the fix and the test all look reasonable.\n"},{"id":"255533","messageId":"xmqqk300t772.fsf@gitster.dls.corp.google.com","threadId":"38486","inReplyTo":"4727F1DC-2FC3-49BE-8C6D-0C4D7D8B107C@jonathonmah.com","subject":"Re: [PATCHv4] sha1_file: fix iterating loose alternate objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T20:00:17Z","receivedAt":"2015-02-02T20:00:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathon Mah writes:\n\n> +test_expect_success 'prune: handle alternate object database' '\n> +\ttest_create_repo A &&\n> +\t\t(cd A &&\n> +\t\techo \"Hello World\" >file1 &&\n> +\t\tgit add file1 &&\n> +\t\tgit commit -m \"Initial commit\" file1) &&\n> +\tgit clone -s A B &&\n> +\t\t(cd B &&\n> +\t\techo \"foo bar\" >file2 &&\n> +\t\tgit add file2 &&\n> +\t\tgit commit -m \"next commit\" file2 &&\n> +\t\tgit prune)\n> +'\n\nThe issue does not have much to do with introducing new path to the\ncloned repository, or the original having any specific content for\nthat matter, so I am tempted to simplify the above to something like\nthis intead:\n\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\nThanks.\n"},{"id":"255535","messageId":"20150202200258.GA28915@peff.net","threadId":"38486","inReplyTo":"xmqqk300t772.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv4] sha1_file: fix iterating loose alternate objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-02T20:02:58Z","receivedAt":"2015-02-02T20:02:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2015 at 12:00:17PM -0800, Junio C Hamano wrote:\n\n> Jonathon Mah writes:\n> \n> > +test_expect_success 'prune: handle alternate object database' '\n> > +\ttest_create_repo A &&\n> > +\t\t(cd A &&\n> > +\t\techo \"Hello World\" >file1 &&\n> > +\t\tgit add file1 &&\n> > +\t\tgit commit -m \"Initial commit\" file1) &&\n> > +\tgit clone -s A B &&\n> > +\t\t(cd B &&\n> > +\t\techo \"foo bar\" >file2 &&\n> > +\t\tgit add file2 &&\n> > +\t\tgit commit -m \"next commit\" file2 &&\n> > +\t\tgit prune)\n> > +'\n> \n> The issue does not have much to do with introducing new path to the\n> cloned repository, or the original having any specific content for\n> that matter, so I am tempted to simplify the above to something like\n> this intead:\n> \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\nYeah, I'd agree that more clearly demonstrates the issue (I didn't check\nthat it actually triggers the failure, but presumably you did).\n\nI think we could also construct a more elaborate example where we fail\nto pick up an unreachable segment of history based on the mtime of a tip\ncommit found only in the alternate (whereas this is only testing that we\ndon't bungle the alternate filename so completely that prune barfs).\n\n-Peff\n"}]}