Re: [PATCH v3 2/8] sha1-array: Implement oid_array_readonly_contains

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 28, 2019, 02:05 UTC
Message-ID: <xmqqlg35czaf.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190127194415.171035-2-sxenos@google.com>

sxenos@google.com writes:

> Subject: Re: [PATCH v3 2/8] sha1-array: Implement oid_array_readonly_contains

Style: s/: Implement/: implement/

> From: Stefan Xenos <sxenos@gmail.com>

This line wants to say "Stefan Xenos <sxenos@google.com>" to match S-o-b below (I am assuming that you are following your employer's open source recommendation to contribute under your corp address).

Perhaps you would want user.email set to the corp address? I am taking the above as an indication that the commits we are seeing here have been made under your @gmail.com address and that is why git-send-email is adding the in-body header.

Show 12 quoted lines

> diff --git a/sha1-array.c b/sha1-array.c
> index b94e0ec0f5..071fce7e90 100644
> --- a/sha1-array.c
> +++ b/sha1-array.c
> @@ -26,6 +26,21 @@ static const unsigned char *sha1_access(size_t index, void *table)
>  	return array[index].hash;
>  }
>  
> +int oid_array_readonly_contains(const struct oid_array* array,
> +	const struct object_id* oid)
> +{
> +	int i;

Style: blank between decl and first stmt, perhaps?

> +	if (array->sorted) {
> +		return sha1_pos(oid->hash, array->oid, array->nr, sha1_access) >= 0;

No need for {} around a single statement.

> +	}
> +	for (i = 0; i < array->nr; i++) {
> +		if (hashcmp(array->oid[i].hash, oid->hash) == 0) {
> +			return 1;

Likewise.

Show 15 quoted lines

> +		}
> +	}
> +	return 0;
> +}
> ...
> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
> index 5dda570b9a..c1bac6fcdd 100755
> --- a/t/t0064-sha1-array.sh
> +++ b/t/t0064-sha1-array.sh
> @@ -32,6 +32,28 @@ test_expect_success 'ordered enumeration with duplicate suppression' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'readonly_contains finds existing' '
> +	echo 1 > expect &&

Style: no SP between redirection operator and its target, i.e.

	echo 1 >expect &&

> +	echoid "" 88 44 aa 55 >> expect &&

Likewise.

	echoid "" 88 44 aa 55 >>expect &&