Re: [PATCH v7 01/10] help.c: refactor drop_prefix() to use a "switch" statement"
From: Junio C Hamano <hidden>
Date: 2022-08-02 23:02:09
Ævar Arnfjörð Bjarmason [off-list ref] writes:
Refactor the drop_prefix() function in in help.c to make it easier to strip prefixes from categories that aren't "CAT_guide". There are no functional changes here, by doing this we make a subsequent functional change's diff smaller. As before we first try to strip "git-" unconditionally, if that works we'll return the stripped string. Then we'll strip "git" if the command is in "CAT_guide".
OK. From the code structure's point of view, it somehow not exactly satisfactory that we still need two "skip and then if skipped yield the remainder" in this function. Especially because we only strip once.
This means that we'd in principle strip "git-gitfoo" down to "foo" if it's in CAT_guide. That doesn't make much sense, and we don't have such an entry in command-list.txt, but let's preserve that behavior for now.
I am not sure if that is what the code means. "git-gitfoo" will become "gitfoo" regardless of what category we are calling drop_prefix() for, because we will return the resulting name without falling through to the new switch statement, if the first "strip 'git-'" succeeds, no?
quoted hunk
While we're at it remove a stray newline that had been added after the "return name;" statement. Signed-off-by: Ævar Arnfjörð Bjarmason <redacted> --- help.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)diff --git a/help.c b/help.c index 41c41c2aa11..7e594d291b0 100644 --- a/help.c +++ b/help.c@@ -44,13 +44,19 @@ static struct category_description main_categories[] = { static const char *drop_prefix(const char *name, uint32_t category) { const char *new_name; + const char *prefix = NULL; if (skip_prefix(name, "git-", &new_name)) return new_name; - if (category == CAT_guide && skip_prefix(name, "git", &new_name)) + switch (category) { + case CAT_guide: + prefix = "git"; + break; + } + if (prefix && skip_prefix(name, prefix, &new_name)) return new_name; - return name; + return name; }
The diff algorighm made an interesting choice as to which line to
consider common here. I would have expected to see
return new_name;
+
return name;
-
}
especially after reading the last paragraph of the proposed log
message.