Re: [PATCH] index-pack: never prune base_cache.

["Björn Steinbrink" <B.Steinbrink@gmx.de>]

On 2008.07.23 14:11:18 +0200, Pierre Habouzit wrote:
> It may belong to something (stdin) that is consumed.

Probably thanks to me, babbling about stdin without having a clue what
I'm talking about, that rationale is wrong.

We may not prune base_cache since that object might come from a
different pack than the one that we are processing. In such a case, we
would try to restore the data for that object from the pack we're
processing and fail miserably.


The patch itself should be fine.

> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> 
>     On Wed, Jul 23, 2008 at 12:00:45PM +0000, Björn Steinbrink wrote:
>     > On 2008.07.23 12:37:00 +0100, Johannes Schindelin wrote:
>     > > Hi,
>     > > 
>     > > Well, I cannot.  However, I get some pread issue on i686.  To be nice to 
>     > > kernel.org, I downloaded the pack in question:
>     > > 
>     > > 	http://pacific.mpi-cbg.de/git/thin-pack.pack
>     > > 
>     > > You should be able to reproduce the behavior by piping this into
>     > > 
>     > > git-index-pack --stdin -v --fix-thin --keep=fetch-pack --pack_header=2,263
>     > 
>     > OK, that gave me a seemingly sane backtrace. What seems to happen (AFA
>     > my limited knowledge tells me):
>     > 
>     > In fix_unresolved_deltas, we read base_obj from an existing pack, other
>     > than the one we're reading. We then link that object to the base cache. 
>     > 
>     > Then in resolve_delta, we create the "result" base_data object and link
>     > that one, too. Now this triggers the pruning, and because the cache is
>     > so small, we prune the object that we read from the existing pack! Fast
>     > forward a few function calls, we end up in get_base_data trying to
>     > re-read the data for that object, but this time from the pack that we
>     > got on stdin. And boom it goes.
>     > 
>     > Does that make any sense to you?
> 
>       Yes, that's obvious, the pack that we read from stdin is consumed, we
>     should *NEVER* prune base_cache. And indeed that little patch works for
>     me.
> 
>  index-pack.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/index-pack.c b/index-pack.c
> index ac20a46..eb81ed4 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -227,7 +227,7 @@ static void prune_base_data(struct base_data *retain)
>  	for (b = base_cache;
>  	     base_cache_used > delta_base_cache_limit && b;
>  	     b = b->child) {
> -		if (b->data && b != retain) {
> +		if (b != base_cache && b->data && b != retain) {
>  			free(b->data);
>  			b->data = NULL;
>  			base_cache_used -= b->size;