threads / discuss / 38486

[PATCHv4] sha1_file: fix iterating loose alternate objects

Subject: [PATCHv4] sha1_file: fix iterating loose alternate objects

## tl;dr

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

replies: 4people: 3as markdown or json

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

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>
---
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· Feb 2, 2015, 18:50 UTC · re: Jonathon Mah · lore

Re: [PATCHv4] sha1_file: fix iterating loose alternate objects

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

Show 17 quoted lines

> 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>
> ---
> Squashed test and fix.

Thanks, this version looks good to me.

-Peff

Junio C Hamano· Feb 2, 2015, 19:35 UTC · re: Jeff King · lore

Re: [PATCHv4] sha1_file: fix iterating loose alternate objects

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

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

Re: [PATCHv4] sha1_file: fix iterating loose alternate objects

Jonathon Mah <me@jonathonmah.com> writes:

Show 13 quoted lines

> +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· Feb 2, 2015, 20:02 UTC · re: Junio C Hamano · lore

Re: [PATCHv4] sha1_file: fix iterating loose alternate objects

On Mon, Feb 02, 2015 at 12:00:17PM -0800, Junio C Hamano wrote:

Show 26 quoted lines

> Jonathon Mah <me@jonathonmah.com> 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