Re: [PATCH] diffcore-delta: avoid ignoring final 'line' of file

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

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/diffcore-delta.c b/diffcore-delta.c
> index c30b56e983b..7136c3dd203 100644
> --- a/diffcore-delta.c
> +++ b/diffcore-delta.c
> @@ -159,6 +159,10 @@ static struct spanhash_top *hash_chars(struct repository *r,
>  		n = 0;
>  		accum1 = accum2 = 0;
>  	}
> +	if (n > 0) {
> +		hashval = (accum1 + accum2 * 0x61) % HASHBASE;
> +		hash = add_spanhash(hash, hashval, n);
> +	}

OK, so we were ignoring the final short bit that is not terminated
with LF due to the "continue".  Nicely found.

> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> index 85be1367de6..29299acbce7 100755
> --- a/t/t4001-diff-rename.sh
> +++ b/t/t4001-diff-rename.sh
> @@ -286,4 +286,23 @@ test_expect_success 'basename similarity vs best similarity' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'last line matters too' '
> +	test_write_lines a 0 1 2 3 4 5 6 7 8 9 >nonewline &&
> +	printf "git ignores final up to 63 characters if not newline terminated" >>nonewline &&
> +	git add nonewline &&
> +	git commit -m "original version of file with no final newline" &&

I found it misleading that the file whose name is nonewline has
bunch of LF including on its last line ;-).

> +	# Change ONLY the first character of the whole file
> +	test_write_lines b 0 1 2 3 4 5 6 7 8 9 >nonewline &&

Same here, but it is too much to bother rewriting it ...

	{
		test_write_lines ...
		printf ...
	} >incomplete

... like so ("incomplete" stands for "file ending with an incomplete line"),
so I'll let it pass.

> +	printf "git ignores final up to 63 characters if not newline terminated" >>nonewline &&


> +	git add nonewline &&
> +	git mv nonewline still-no-newline &&
> +	git commit -a -m "rename nonewline -> still-no-newline" &&
> +	git diff-tree -r -M01 --name-status HEAD^ HEAD >actual &&
> +	cat >expected <<-\EOF &&
> +	R097	nonewline	still-no-newline

I am not very happy with the hardcoded 97.  You are already using
the non-standard 10% threshold.  If the delta detection that
forgets about the last line is so broken as your proposed log
message noted, shouldn't you be able to construct a sample pair of
preimage and postimage for which the broken version gives so low
similarity to be judged not worth treating as a rename, while the
fixed version gives reasonable similarity to be made into a rename,
by the default threshold?  That way, the test only needs to see if
we got a rename (with any similarity) or a delete and an add.