Re: [PATCH v2] merge: break out of all_strategy loop when strategy is found

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

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

> From: Seija Kijin <doremylover123@gmail.com>
>
> strncmp does not modify any of the memory.

It may be a correct statement, but so what?  It does not seem to
have relevance to this change.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 0f093f2a4f2..74de2ebd2b3 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -188,7 +188,7 @@ static struct strategy *get_strategy(const char *name)
>  		for (i = 0; i < main_cmds.cnt; i++) {
>  			int j, found = 0;
>  			struct cmdname *ent = main_cmds.names[i];
> -			for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
> +			for (j = 0; !found && j < ARRAY_SIZE(all_strategy); j++)
>  				if (!strncmp(ent->name, all_strategy[j].name, ent->len)
>  						&& !all_strategy[j].name[ent->len])
>  					found = 1;

I am not sure if this micro-optimization is worth it.  

If this loop is so costly that it needs optimization, a better thing
to do would be to rethink the way it filters main_cmds.names[] array
with all_strategy[].  The latter is a fairly small, and more
importantly, a constant set of known strategies, so there should be
a more efficient way than O(n*m) nested loop.

The code churn has already costed us too much, mostly reviewer and
maintainer time, for the value of the change itself.  I'll queue the
patch as-is, because this change is not making anything worse
per-se, but primarily because I do not want the topic to take any
more of our resources.

Thanks.