Re: [PATCH] fast-import: Don't count delta attempts against an empty buffer

[Junio C Hamano <gitster@pobox.com>]

Mike Hommey <mh@glandium.org> writes:

> When the reference buffer is empty, diff_delta returns NULL without
> really attempting anything, yet fast-import counts that as a delta
> attempt.

But that is an attempt nevertheless, no?  Attempted and failed to
find anything useful, that is.  What problem are you trying to solve
and what issue are you trying to address, exactly?

... goes and reads the patch ...

> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  fast-import.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index 4d55910ab9..12195d54d7 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1076,7 +1076,7 @@ static int store_object(
>  		return 1;
>  	}
>  
> -	if (last && last->data.buf && last->depth < max_depth
> +	if (last && last->data.len && last->data.buf && last->depth < max_depth
>  		&& dat->len > the_hash_algo->rawsz) {
>  
>  		delta_count_attempts_by_type[type]++;

This is a misleading patch as the title and the proposed log message
both talk about "attempts are counted but we didn't actually do
anything", implying that the only problem is that the counter is not
aligned with reality.  The fact that the post-context we see here
only shows the "counting" part does not help us, either.

But the next line in the post-context is actually code that does
something important, which is ...

		delta = diff_delta(last->data.buf, last->data.len,
			dat->buf, dat->len,
			&deltalen, dat->len - the_hash_algo->rawsz);
	} else
		delta = NULL;


... and makes the reader realize that the change itself is much
better than the patch with 3-line context, the title, and the
proposed log message advertises it as ;-)

How about selling it this way instead?

	fast-import: do not call diff_delta() with empty buffer

	We know diff_delta() returns NULL, saying "no good delta
	exists for it", when fed an empty data.  Check the length of
	the data in the caller to avoid such a call.  

	This incidentally reduces the number of attempted deltification
	we see in the final statistics.

or something like that?