Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode

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

Taylor Blau <me@ttaylorr.com> writes:

> -int cmd_hash_impl(int ac, const char **av, int algo)
> +int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
>  {
>  	git_hash_ctx ctx;
>  	unsigned char hash[GIT_MAX_HEXSZ];
> @@ -27,7 +27,10 @@ int cmd_hash_impl(int ac, const char **av, int algo)
>  			die("OOPS");
>  	}
>  
> -	algop->init_fn(&ctx);
> +	if (unsafe)
> +		algop->unsafe_init_fn(&ctx);
> +	else
> +		algop->init_fn(&ctx);

It may be just me, and it would not matter all that much within the
context of the project because this is merely a test helper, but
giving a pair of init/unsafe_init methods to algop looks unnerving.
It gives an impression that every design of hash algorithm must come
with normal and unsafe variant, which is not what you want to say.

I would have expected that there are different algorighm instances,
and one of them would be "unsafe SHA-1", among "normal SHA-1" and
"SHA-256" (as the last one would not even have unsafe_init_fn
method), and the callers that want to measure the performance of
each algorithm would simply pick one of these instances and go
through the usual "init", "update", "final" flow, regardless of the
"unsafe" ness of the algorithm.

IOW, ...

>  	while (1) {
>  		ssize_t sz, this_sz;
> @@ -46,9 +49,15 @@ int cmd_hash_impl(int ac, const char **av, int algo)
>  		}
>  		if (this_sz == 0)
>  			break;
> -		algop->update_fn(&ctx, buffer, this_sz);
> +		if (unsafe)
> +			algop->unsafe_update_fn(&ctx, buffer, this_sz);
> +		else
> +			algop->update_fn(&ctx, buffer, this_sz);
>  	}
> -	algop->final_fn(hash, &ctx);
> +	if (unsafe)
> +		algop->unsafe_final_fn(hash, &ctx);
> +	else
> +		algop->final_fn(hash, &ctx);

... I didn't expect any of these "if (unsafe) .. else .." switches.