Re: [PATCH v2] merge: break out of all_strategy loop when strategy is found
From: Junio C Hamano <hidden>
Date: 2023-01-13 18:28:18
"Rose via GitGitGadget" [off-list ref] writes:
From: Seija Kijin <redacted> 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.
quoted hunk
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.