[PATCHv5] sha1_file: fix iterating loose alternate objects

[PATCHv5] sha1_file: fix iterating loose alternate objects

[Jonathon Mah]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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?

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

[Jeff King]

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

> 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