[PATCH 1/2] t5710-info-alternate: demonstrate bug in unpacked pruning

[PATCH 1/2] t5710-info-alternate: demonstrate bug in unpacked pruning

[Jonathon Mah]

Signed-off-by: Jonathon Mah <me@JonathonMah.com>
---
 t/t5710-info-alternate.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

[PATCH 2/2] 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>
---
 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

Re: [PATCH 2/2] sha1_file: fix iterating loose alternate objects

[Jeff King]

On Sun, Feb 01, 2015 at 01:55:33PM -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).

Thanks for catching this, and for a nice explanation.

> 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;
>  }

I think this is probably the best fix, and is the pattern we use
elsewhere when touching alt->base.

We _could_ further change this to have for_each_loose_file_in_objdir
actually use alt->base as its scratch buffer, writing the object
filenames into the end of it (i.e., what it was designed for). But:

  1. We still need a strbuf scratch-buffer for the non-alternate object
     directory. So we'd have to push more code there to over-allocate
     the buffer, and then for_each_loose_file_in_objdir would assume
     we always feed it a buffer with the extra slop. That would work,
     but I find the strbuf approach a little safer; there's not an
     implicit over-allocation far away in the code preventing us from
     overflowing a buffer.

  2. The reason for the existing alt->base behavior is that the
     sha1_file code gets fed objects one at a time, and don't want to
     pay strbuf overhead for each. With the iterator, we know we are
     going to hit a bunch of objects, so we only have to pay the strbuf
     overhead once for the iteration. So there's not the same
     performance penalty, and we can stick with the strbuf if we prefer
     it.

-Peff

Re: [PATCH 2/2] sha1_file: fix iterating loose alternate objects

[Jonathon Mah]

> On 2015-02-02, at 09:53, Jeff King <peff@peff.net> wrote:
> 
> I think this is probably the best fix, and is the pattern we use
> elsewhere when touching alt->base.
> 
> We _could_ further change this to have for_each_loose_file_in_objdir
> actually use alt->base as its scratch buffer, writing the object
> filenames into the end of it (i.e., what it was designed for). But:
> 
>  1. We still need a strbuf scratch-buffer for the non-alternate object
>     directory. So we'd have to push more code there to over-allocate
>     the buffer, and then for_each_loose_file_in_objdir would assume
>     we always feed it a buffer with the extra slop. That would work,
>     but I find the strbuf approach a little safer; there's not an
>     implicit over-allocation far away in the code preventing us from
>     overflowing a buffer.
> 
>  2. The reason for the existing alt->base behavior is that the
>     sha1_file code gets fed objects one at a time, and don't want to
>     pay strbuf overhead for each. With the iterator, we know we are
>     going to hit a bunch of objects, so we only have to pay the strbuf
>     overhead once for the iteration. So there's not the same
>     performance penalty, and we can stick with the strbuf if we prefer
>     it.

Thanks for your feedback. I considered the same, and came to a similar conclusion. The strbuf cost is only once per alternate, so I feel on balance it's more robust to use alt->base consistently inside each function, rather than have this a more fragile special case to save allocation of only one path.

Updated the test patch.


Jonathon Mah
me@JonathonMah.com

Re: [PATCH 1/2] t5710-info-alternate: demonstrate bug in unpacked pruning

[Jeff King]

On Sun, Feb 01, 2015 at 01:55:00PM -0800, Jonathon Mah wrote:

> Signed-off-by: Jonathon Mah <me@JonathonMah.com>
> ---
>  t/t5710-info-alternate.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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'

Modifying a test like this makes me a little nervous because now the old
test is not checking the same thing (pruning when we are packed), and
it's not obvious whether the packing was important to the original test.

And it's not clear that this change is testing a totally unrelated
thing.  I haven't looked closely, but would it be hard to introduce a
new test that more explicitly checks for the breakage?

-Peff

Re: [PATCH] sha1_file.c: make sure open_sha1_file does not open a directory

[Jeff King]

On Sun, Feb 08, 2015 at 07:54:44PM -0500, Jeff King wrote:

> However, the first thing for_each_loose_file_in_objdir is going to do is
> stick the path into a strbuf. So perhaps the most sensible thing is to
> just teach it to take a strbuf from the caller. I'll work up a patch.
> 
> It looks like a1b47246 isn't even in "next" yet, so I'll build it
> directly on what is already in master, dropping Jonathan's patch.

Here it is. The first patch is a refactoring to allow this,
and the second is the moral equivalent of Jonathon's patch.
These replace a1b47246 on the tip of jk/prune-mtime.

  [1/2]: for_each_loose_file_in_objdir: take an optional strbuf path
  [2/2]: sha1_file: fix iterating loose alternate objects

-Peff

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

[Jeff King]

From: Jonathon Mah <me@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.

660c889e (sha1_file: add for_each iterators for loose and packed
objects, 2014-10-15) introduced loose_from_alt_odb(), but this 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>
Helped-by: Kyle J. McKay <mackyle@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
I think the S-O-B should still stand, as the code is now a mix of our
work, and the tests are still Jonathon's. But let me know if you do not
want your name attached to this. ;)

I am also happy to build it as a patch on top of the original if that is
simpler.

 sha1_file.c      | 13 ++++++++++---
 t/t5304-prune.sh |  8 ++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 725de7f..a41cc4f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3406,9 +3406,16 @@ 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);
+	struct strbuf buf = STRBUF_INIT;
+	int r;
+
+	/* copy base not including trailing '/' */
+	strbuf_add(&buf, alt->base, alt->name - alt->base - 1);
+	r = for_each_loose_file_in_objdir_buf(&buf,
+					      data->cb, NULL, NULL,
+					      data->data);
+	strbuf_release(&buf);
+	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.rc1.287.g761fd19

Re: [PATCH 2/2] sha1_file: fix iterating loose alternate objects

[Kyle J. McKay]

On Feb 8, 2015, at 17:15, Jeff King wrote:
[...]
> Signed-off-by: Jonathon Mah <me@JonathonMah.com>
> Helped-by: Kyle J. McKay <mackyle@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I think the S-O-B should still stand, as the code is now a mix of our
> work, and the tests are still Jonathon's. But let me know if you do  
> not
> want your name attached to this. ;)

That's fine.

This fix looks much better. :)

Unfortunately I can no longer reproduce the original bug as the  
repository that caused it is no longer in a state that triggers the  
problem (and my backups of it are either slightly too old or slightly  
too new).

-Kyle