Re: [PATCH 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 1, 2014 at 5:43 AM, René Scharfe <l.s.r@xxxxxx> wrote:
> If the first 18 bytes of the SHA1's of all entries are the same then
> sha1_pos() dies and reports that the lower and upper limits of the
> binary search were the same that this wasn't supposed to happen.  This
> is wrong because the remaining two bytes could still differ.
>
> Furthermore: It wouldn't be a problem if they actually were the same,
> i.e. if all entries have the same SHA1.  The code already handles
> duplicates just fine otherwise.  Simply remove the erroneous check.
>
> Signed-off-by: Rene Scharfe <l.s.r@xxxxxx>
> ---
>  sha1-lookup.c         |  2 --
>  t/t0064-sha1-array.sh | 20 ++++++++++++++++++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/sha1-lookup.c b/sha1-lookup.c
> index 2dd8515..5f06921 100644
> --- a/sha1-lookup.c
> +++ b/sha1-lookup.c
> @@ -84,8 +84,6 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
>                                 die("BUG: assertion failed in binary search");
>                         }
>                 }
> -               if (18 <= ofs)
> -                       die("cannot happen -- lo and hi are identical");
>         }
>
>         do {
> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
> index bd68789..3fcb8d8 100755
> --- a/t/t0064-sha1-array.sh
> +++ b/t/t0064-sha1-array.sh
> @@ -61,4 +61,24 @@ test_expect_success 'lookup with duplicates' '
>         test "$n" -le 3
>  '
>
> +test_expect_success 'lookup with almost duplicate values' '
> +       {
> +               echo "append 5555555555555555555555555555555555555555" &&
> +               echo "append 555555555555555555555555555555555555555f" &&
> +               echo20 "lookup " 55
> +       } | test-sha1-array >actual &&
> +       n=$(cat actual) &&
> +       test "$n" -eq 0
> +'
> +
> +test_expect_success 'lookup with single duplicate value' '
> +       {
> +               echo20 "append " 55 55 &&
> +               echo20 "lookup " 55
> +       } | test-sha1-array >actual &&
> +       n=$(cat actual) &&
> +       test "$n" -ge 0 &&
> +       test "$n" -le 1
> +'

An alternative would be to introduce these two tests in patch 1/2 as
test_expect_failure and flip them to test_expect_success in this patch
which fixes the problem.

> +
>  test_done
> --
> 2.1.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]