Re: [PATCH v2] merge: break out of all_strategy loop when strategy is found
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 13, 2023, 18:24 UTC
- Message-ID
- <xmqqbkn247jd.fsf@gitster.g>
- In-Reply-To
- <pull.1429.v2.git.git.1673285669004.gitgitgadget@gmail.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.
Show 13 quoted lines
> 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.