# [PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates 3 messages from 2015-02-02 to 2015-02-02. Participants: Jonathon Mah, Jeff King. Thread: https://gitlist.dev/t/38484 ## Jonathon Mah, 2015-02-02 18:33 Subject: [PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates Message-ID: <0BD44E44-686B-44B2-A4C0-9E14A99BA96B@jonathonmah.com> URL: https://gitlist.dev/e/0BD44E44-686B-44B2-A4C0-9E14A99BA96B%40jonathonmah.com ``` Signed-off-by: Jonathon Mah --- Adjust prune test directly, much nicer. t/t5304-prune.sh | 13 +++++++++++++ t/t5710-info-alternate.sh | 4 ++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh index e32e46d..e825be7 100755 --- a/t/t5304-prune.sh +++ b/t/t5304-prune.sh @@ -253,4 +253,17 @@ 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 && + cd .. && + git clone -l -s A B && cd B && + echo "foo bar" > file2 && + git add file2 && + git commit -m "next commit" file2 && + git prune +' + test_done diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh index 5a6e49d..d82844a 100755 --- a/t/t5710-info-alternate.sh +++ b/t/t5710-info-alternate.sh @@ -18,6 +18,7 @@ reachable_via() { test_valid_repo() { git fsck --full > fsck.log && + git prune && test_line_count = 0 fsck.log } @@ -47,8 +48,7 @@ test_expect_success 'preparing third repository' \ 'git clone -l -s B C && cd C && echo "Goodbye, cruel world" > file3 && git add file3 && -git commit -m "one more" file3 && -git repack -a -d -l && +git commit -m "one more without packing" file3 && git prune' cd "$base_dir" -- 2.3.0.rc2.2.g184f7a0 ``` ## Jonathon Mah, 2015-02-02 18:34 Subject: [PATCHv2 2/2] sha1_file: fix iterating loose alternate objects Message-ID: URL: https://gitlist.dev/e/D37068A8-F7D3-4BF6-986C-09528EF443C6%40jonathonmah.com In-Reply-To: <0BD44E44-686B-44B2-A4C0-9E14A99BA96B@jonathonmah.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 --- sha1_file.c | 10 +++++++--- 1 file changed, 7 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) -- 2.3.0.rc2.2.g184f7a0 ``` ## Jeff King, 2015-02-02 18:41 Subject: Re: [PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates Message-ID: <20150202184115.GA25421@peff.net> URL: https://gitlist.dev/e/20150202184115.GA25421%40peff.net In-Reply-To: <0BD44E44-686B-44B2-A4C0-9E14A99BA96B@jonathonmah.com> ``` On Mon, Feb 02, 2015 at 10:33:02AM -0800, Jonathon Mah wrote: > Signed-off-by: Jonathon Mah > --- > Adjust prune test directly, much nicer. Agreed, this is much nicer. A few comments: > +test_expect_success 'prune: handle alternate object database' ' This test fails, so we either need expect_failure here, or it just needs to be squashed in with the fix (I generally prefer the latter). > + test_create_repo A && cd A && We generally prefer to chdir in a subshell, so that a failure in the test does not leave further tests in a confusing spot. Like: test_create_repo A && ( cd A && ... do stuff in repo ... # no need to cd .. ) && .. do stuff outside repo ... > + echo "Hello World" > file1 && Style nit: we prefer ">file1" with no space. > + git add file1 && > + git commit -m "Initial commit" file1 && > + cd .. && > + git clone -l -s A B && cd B && "-l" is a noop these days. I don't think it is hurting, but I'd prefer not to propagate bad habits in our tests. > diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh > index 5a6e49d..d82844a 100755 > --- a/t/t5710-info-alternate.sh > +++ b/t/t5710-info-alternate.sh We can drop this change, then, right? -Peff ```