threads / discuss / 38487

[PATCHv5] sha1_file: fix iterating loose alternate objects

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

## tl;dr

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

replies: 3people: 3as markdown or json

Jonathon Mah· Feb 2, 2015, 20:05 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>
---

Simplified test per Junio (verified that it fails before and passes now). Punting on Jeff's "more elaborate example".

 sha1_file.c      | 10 +++++++---
 t/t5304-prune.sh |  8 ++++++++
 2 files changed, 15 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..0794d33 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -253,4 +253,12 @@ test_expect_success 'prune .git/shallow' '
 	test_path_is_missing .git/shallow
 '
 
+test_expect_success 'prune: handle alternate object database' '
+	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
+'
+
 test_done

-- 
2.3.0.rc2.2.g184f7a0

Jeff King· Feb 2, 2015, 20:27 UTC · re: Jonathon Mah · lore

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

On Mon, Feb 02, 2015 at 12:05:54PM -0800, Jonathon Mah wrote:

> Simplified test per Junio (verified that it fails before and passes
> now). Punting on Jeff's "more elaborate example".

I think that's fine. I started to try to create such an example, but it's actually rather tricky. If the alternate has the tip object, then one of these must be true:

  1. It has all of the objects the tip depends on.

  2. It is missing an object, and this tip is part of the referenced
     history.

  3. It is missing an object, but this part of history is not
     referenced.

In case (1), we do not care about deleting objects from the base repository; we already have another copy in the alternate.

In case (2), the alternate is corrupt, and all bets are off.

In case (3), we can only have dropped the object from the alternate by pruning it and keeping the tip object that refers to it. Which is the exact thing that this new code was added to avoid (to always keep depended-upon objects).

So I actually do not see how the situation would come up in practice, and possibly we could drop the iteration of the alternates' loose objects entirely from this code. But certainly that is orthogonal to Jonathon's fix (which is a true regression for the less-exotic case that his test demonstrates).

-Peff

Junio C Hamano· Feb 2, 2015, 20:49 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> So I actually do not see how the situation would come up in practice,
> and possibly we could drop the iteration of the alternates' loose
> objects entirely from this code. But certainly that is orthogonal to
> Jonathon's fix (which is a true regression for the less-exotic case that
> his test demonstrates).

Sure.

This needs to go to both 'maint' and 'master', right?

Jeff King· Feb 2, 2015, 21:02 UTC · re: Junio C Hamano · lore

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

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

Show 11 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > So I actually do not see how the situation would come up in practice,
> > and possibly we could drop the iteration of the alternates' loose
> > objects entirely from this code. But certainly that is orthogonal to
> > Jonathon's fix (which is a true regression for the less-exotic case that
> > his test demonstrates).
> 
> Sure.
> 
> This needs to go to both 'maint' and 'master', right?

Yes (on the jk/prune-mtime topic).

-Peff