On Sun, Mar 10, 2019 at 11:42 PM Denton Liu <liu.denton@xxxxxxxxx> wrote:
> Define a function which allows us to get the string configuration value
> of a enum commit_msg_cleanup_mode. This is done by refactoring
> get_cleanup_mode such that it uses a lookup table to find the mappings
> between string and enum and then using the same LUT in reverse to define
> get_config_from_cleanup.
Aside from a missing 'static', the comments below are mostly style
suggestions to make the new code less noisy. The basic idea is to
reduce the "wordiness" of the code so that the eye can glide over it
more easily, thus allowing the reader to grasp its meaning "at a
glance", without necessarily having to read it attentively. You may or
may not consider the suggestions actionable.
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -160,6 +160,22 @@ static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
> +struct cleanup_config_mapping {
> + const char *config_value;
> + enum commit_msg_cleanup_mode editor_cleanup;
> + enum commit_msg_cleanup_mode no_editor_cleanup;
> +};
These members are already inside a struct named
"cleanup_config_mapping", so we can drop some of the wordiness from
the member names. For instance:
config --or-- value --or-- val
editor
no_editor
> +/* note that we assume that cleanup_config_mapping[0] contains the default settings */
> +struct cleanup_config_mapping cleanup_config_mappings[] = {
> + { "default", COMMIT_MSG_CLEANUP_ALL, COMMIT_MSG_CLEANUP_SPACE },
> + { "verbatim", COMMIT_MSG_CLEANUP_NONE, COMMIT_MSG_CLEANUP_NONE },
> + { "whitespace", COMMIT_MSG_CLEANUP_SPACE, COMMIT_MSG_CLEANUP_SPACE },
> + { "strip", COMMIT_MSG_CLEANUP_ALL, COMMIT_MSG_CLEANUP_ALL },
> + { "scissors", COMMIT_MSG_CLEANUP_SCISSORS, COMMIT_MSG_CLEANUP_SPACE },
> + { NULL, 0, 0 }
> +};
This table should be 'static'.
> @@ -504,26 +520,42 @@ static int fast_forward_to(struct repository *r,
> enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> int use_editor, int die_on_error)
> {
> + struct cleanup_config_mapping *default_mapping = &cleanup_config_mappings[0];
> + struct cleanup_config_mapping *current_mapping;
We can shorten these two variable names to "def" and "p",
respectively, without losing clarity; there are only two variables in
the function, so it's not difficult to remember what they are. More
importantly, the rest of the code becomes considerably shorter,
allowing the eye to glide over it while easily taking in its meaning
rather than having to spend time actively reading it.
> + if (!cleanup_arg) {
> + return use_editor ? default_mapping->editor_cleanup :
> + default_mapping->no_editor_cleanup;
> + }
> +
> + for (current_mapping = cleanup_config_mappings; current_mapping->config_value; current_mapping++) {
> + if (!strcmp(cleanup_arg, current_mapping->config_value)) {
> + return use_editor ? current_mapping->editor_cleanup :
> + current_mapping->no_editor_cleanup;
> + }
> + }
For instance, with the shorter names, the above loop (while also
dropping unnecessary braces) becomes:
for (p = cleanup_config_mappings; p->val; p++)
if (!strcmp(cleanup_arg, p->val))
return use_editor ? p->editor : p->no_editor;
> + if (!die_on_error) {
> warning(_("Invalid cleanup mode %s, falling back to default"), cleanup_arg);
> - return use_editor ? COMMIT_MSG_CLEANUP_ALL :
> - COMMIT_MSG_CLEANUP_SPACE;
> + return use_editor ? default_mapping->editor_cleanup :
> + default_mapping->no_editor_cleanup;
> } else
> die(_("Invalid cleanup mode %s"), cleanup_arg);
> }
Same comments apply to other new code introduced by this patch.
> +const char *get_config_from_cleanup(enum commit_msg_cleanup_mode cleanup_mode)
> +{
> + struct cleanup_config_mapping *current_mapping;
> +
> + for (current_mapping = &cleanup_config_mappings[1]; current_mapping->config_value; current_mapping++) {
> + if (cleanup_mode == current_mapping->editor_cleanup) {
> + return current_mapping->config_value;
> + }
> + }
> +
> + BUG(_("invalid cleanup_mode provided"));
Don't localize BUG() messages; they are intended only for developer
eyes, not end-users. So:
BUG("invalid cleanup_mode provided");
> +}
> diff --git a/sequencer.h b/sequencer.h
> @@ -118,6 +118,7 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);
> enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> int use_editor, int die_on_error);
> +const char *get_config_from_cleanup(enum commit_msg_cleanup_mode cleanup_mode);
A more intuitive name might be describe_cleanup_mode().
