# [PATCHv4] sha1_file: fix iterating loose alternate objects 5 messages from 2015-02-02 to 2015-02-02. Participants: Jonathon Mah, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/38486 ## Jonathon Mah, 2015-02-02 18:48 Subject: [PATCHv4] sha1_file: fix iterating loose alternate objects Message-ID: <4727F1DC-2FC3-49BE-8C6D-0C4D7D8B107C@jonathonmah.com> URL: https://gitlist.dev/e/4727F1DC-2FC3-49BE-8C6D-0C4D7D8B107C%40jonathonmah.com ``` The string in 'base' contains a path suffix to a specific object; when its value is used, the suffix must either be filled (as in stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared (as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb (introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and treated 'base' as a complete path to the "base" object directory, instead of a pointer to the "base" of the full path string. The trailing path after 'base' is still initialized to NUL, hiding the bug in some common cases. Additionally the descendent for_each_file_in_obj_subdir function swallows ENOENT, so an error only shows if the alternate's path was last filled with a valid object (where statting /path/to/existing/00/0bjectfile/00 fails). Signed-off-by: Jonathon Mah --- Squashed test and fix. sha1_file.c | 10 +++++++--- t/t5304-prune.sh | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 30995e6..fcb1c4b 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -3396,9 +3396,13 @@ static int loose_from_alt_odb(struct alternate_object_database *alt, void *vdata) { struct loose_alt_odb_data *data = vdata; - return for_each_loose_file_in_objdir(alt->base, - data->cb, NULL, NULL, - data->data); + int r; + alt->name[-1] = 0; + r = for_each_loose_file_in_objdir(alt->base, + data->cb, NULL, NULL, + data->data); + alt->name[-1] = '/'; + return r; } int for_each_loose_object(each_loose_object_fn cb, void *data) diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh index e32e46d..c65cf9b 100755 --- a/t/t5304-prune.sh +++ b/t/t5304-prune.sh @@ -253,4 +253,18 @@ test_expect_success 'prune .git/shallow' ' test_path_is_missing .git/shallow ' +test_expect_success 'prune: handle alternate object database' ' + test_create_repo A && + (cd A && + echo "Hello World" >file1 && + git add file1 && + git commit -m "Initial commit" file1) && + git clone -s A B && + (cd B && + echo "foo bar" >file2 && + git add file2 && + git commit -m "next commit" file2 && + git prune) +' + test_done -- 2.3.0.rc2.2.g184f7a0 ``` ## Jeff King, 2015-02-02 18:50 Subject: Re: [PATCHv4] sha1_file: fix iterating loose alternate objects Message-ID: <20150202185049.GA27399@peff.net> URL: https://gitlist.dev/e/20150202185049.GA27399%40peff.net In-Reply-To: <4727F1DC-2FC3-49BE-8C6D-0C4D7D8B107C@jonathonmah.com> ``` On Mon, Feb 02, 2015 at 10:48:12AM -0800, Jonathon Mah wrote: > The string in 'base' contains a path suffix to a specific object; when > its value is used, the suffix must either be filled (as in > stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared > (as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb > (introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and > treated 'base' as a complete path to the "base" object directory, > instead of a pointer to the "base" of the full path string. > > The trailing path after 'base' is still initialized to NUL, hiding the > bug in some common cases. Additionally the descendent > for_each_file_in_obj_subdir function swallows ENOENT, so an error only > shows if the alternate's path was last filled with a valid object > (where statting /path/to/existing/00/0bjectfile/00 fails). > > Signed-off-by: Jonathon Mah > --- > Squashed test and fix. Thanks, this version looks good to me. -Peff ``` ## Junio C Hamano, 2015-02-02 19:35 Subject: Re: [PATCHv4] sha1_file: fix iterating loose alternate objects Message-ID: URL: https://gitlist.dev/e/xmqqoapct8bl.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150202185049.GA27399@peff.net> ``` Jeff King writes: > On Mon, Feb 02, 2015 at 10:48:12AM -0800, Jonathon Mah wrote: > >> The string in 'base' contains a path suffix to a specific object; when >> its value is used, the suffix must either be filled (as in >> stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared >> (as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb >> (introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and >> treated 'base' as a complete path to the "base" object directory, >> instead of a pointer to the "base" of the full path string. >> >> The trailing path after 'base' is still initialized to NUL, hiding the >> bug in some common cases. Additionally the descendent >> for_each_file_in_obj_subdir function swallows ENOENT, so an error only >> shows if the alternate's path was last filled with a valid object >> (where statting /path/to/existing/00/0bjectfile/00 fails). >> >> Signed-off-by: Jonathon Mah >> --- >> Squashed test and fix. > > Thanks, this version looks good to me. Thanks, both of you. The analysis, the fix and the test all look reasonable. ``` ## Junio C Hamano, 2015-02-02 20:00 Subject: Re: [PATCHv4] sha1_file: fix iterating loose alternate objects Message-ID: URL: https://gitlist.dev/e/xmqqk300t772.fsf%40gitster.dls.corp.google.com In-Reply-To: <4727F1DC-2FC3-49BE-8C6D-0C4D7D8B107C@jonathonmah.com> ``` Jonathon Mah writes: > +test_expect_success 'prune: handle alternate object database' ' > + test_create_repo A && > + (cd A && > + echo "Hello World" >file1 && > + git add file1 && > + git commit -m "Initial commit" file1) && > + git clone -s A B && > + (cd B && > + echo "foo bar" >file2 && > + git add file2 && > + git commit -m "next commit" file2 && > + git prune) > +' The issue does not have much to do with introducing new path to the cloned repository, or the original having any specific content for that matter, so I am tempted to simplify the above to something like this intead: test_create_repo A && git -C A commit --allow-empty -m "initial commit" && git clone --shared A B && git -C B commit --allow-empty -m "next commit" && git -C B prune Thanks. ``` ## Jeff King, 2015-02-02 20:02 Subject: Re: [PATCHv4] sha1_file: fix iterating loose alternate objects Message-ID: <20150202200258.GA28915@peff.net> URL: https://gitlist.dev/e/20150202200258.GA28915%40peff.net In-Reply-To: ``` On Mon, Feb 02, 2015 at 12:00:17PM -0800, Junio C Hamano wrote: > Jonathon Mah writes: > > > +test_expect_success 'prune: handle alternate object database' ' > > + test_create_repo A && > > + (cd A && > > + echo "Hello World" >file1 && > > + git add file1 && > > + git commit -m "Initial commit" file1) && > > + git clone -s A B && > > + (cd B && > > + echo "foo bar" >file2 && > > + git add file2 && > > + git commit -m "next commit" file2 && > > + git prune) > > +' > > The issue does not have much to do with introducing new path to the > cloned repository, or the original having any specific content for > that matter, so I am tempted to simplify the above to something like > this intead: > > test_create_repo A && > git -C A commit --allow-empty -m "initial commit" && > git clone --shared A B && > git -C B commit --allow-empty -m "next commit" && > git -C B prune Yeah, I'd agree that more clearly demonstrates the issue (I didn't check that it actually triggers the failure, but presumably you did). I think we could also construct a more elaborate example where we fail to pick up an unreachable segment of history based on the mtime of a tip commit found only in the alternate (whereas this is only testing that we don't bungle the alternate filename so completely that prune barfs). -Peff ```