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

[Jeff King <peff@peff.net>]

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