threads / bug / 38484

[PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates

Subject: [PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates

## tl;dr

3 messages between Feb 2, 2015 and Feb 2, 2015.

replies: 2people: 2as markdown or json

Jonathon Mah· Feb 2, 2015, 18:33 UTC · lore

Signed-off-by: Jonathon Mah <me@JonathonMah.com>
---
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· Feb 2, 2015, 18:34 UTC · re: Jonathon Mah · lore

[PATCHv2 2/2] sha1_file: fix iterating loose alternate objects

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 <me@JonathonMah.com>
---
 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· Feb 2, 2015, 18:41 UTC · re: Jonathon Mah · lore

Re: [PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates

On Mon, Feb 02, 2015 at 10:33:02AM -0800, Jonathon Mah wrote:

> Signed-off-by: Jonathon Mah <me@JonathonMah.com>
> ---
> 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