| Message ID | ced1df0fc76c97c9896b6cbb5b4154532461716d.1552275703.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix scissors bug during conflict | expand |
On Sun, Mar 10, 2019 at 11:42 PM Denton Liu <liu.denton@gmail.com> 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@gmail.com> > --- > 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().
Denton Liu <liu.denton@gmail.com> writes: > +struct cleanup_config_mapping { > + const char *config_value; > + enum commit_msg_cleanup_mode editor_cleanup; > + enum commit_msg_cleanup_mode no_editor_cleanup; > +}; Is this code using 4-space indent? Please don't. Also, I found that Eric's comment on naming gave a good suggestion. Is the cleanup_config_mapping[] array we see below supposed to be constant, or does it allow further runtime configuration? I am assuming the former (i.e. when the user says "default", then editor_cleanup will always become CLEANUP_ALL and no_editor_cleanup will always become CLEANUP_SPACE), in which case, I wonder if we can be more explicit about constness of the table. > +/* note that we assume that cleanup_config_mapping[0] contains the default settings */ That sounds as if it is bad to make that assumption. Be more positive and direct to clearly tell future programmers what rule they need to honor, e.g. /* the 0th element of this array must be the "default" */ > +struct cleanup_config_mapping cleanup_config_mappings[] = { Do not give a plural name to an array. Access to an element in a array "type thing[]" can be written thing[4] and can be more naturally read as "the fourth thing" (you do not say "the fourth things") that way. An exception is when you very often pass around the array as a whole as one unit of datum across callchains. > + struct cleanup_config_mapping *default_mapping = &cleanup_config_mappings[0]; > + struct cleanup_config_mapping *current_mapping; > + > + if (!cleanup_arg) { > + return use_editor ? default_mapping->editor_cleanup : > + default_mapping->no_editor_cleanup; > + } No need for extra {}. > + > + 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; > + } > + } Ditto. In addition, perhaps split the for (...) like so: for (current_mapping = cleanup_config_mappings; current_mapping->config_value; current_mapping++) if (...) return ...; > +const char *get_config_from_cleanup(enum commit_msg_cleanup_mode cleanup_mode) static??? Is this really getting "config" from "cleanup"? It rather smells backwards i.e. grabbing the clean-up settings from the config system to me. > +{ > + 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")); Is that a bug (i.e. programming error) or a bad configuration file? I think you meant the former, but then I do not think we want _() around the message. Instead, however, we may want to show the cleanup_mode that was provided, possibly with the available values in the .editor_cleanup field of cleanup_config_mapping[] entries.
diff --git a/sequencer.c b/sequencer.c index 612621f221..5d94e2c865 100644 --- a/sequencer.c +++ b/sequencer.c @@ -160,6 +160,22 @@ static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts") static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate") static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec") +struct cleanup_config_mapping { + const char *config_value; + enum commit_msg_cleanup_mode editor_cleanup; + enum commit_msg_cleanup_mode no_editor_cleanup; +}; + +/* 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 } +}; + static int git_sequencer_config(const char *k, const char *v, void *cb) { struct replay_opts *opts = cb; @@ -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) { - if (!cleanup_arg || !strcmp(cleanup_arg, "default")) - return use_editor ? COMMIT_MSG_CLEANUP_ALL : - COMMIT_MSG_CLEANUP_SPACE; - else if (!strcmp(cleanup_arg, "verbatim")) - return COMMIT_MSG_CLEANUP_NONE; - else if (!strcmp(cleanup_arg, "whitespace")) - return COMMIT_MSG_CLEANUP_SPACE; - else if (!strcmp(cleanup_arg, "strip")) - return COMMIT_MSG_CLEANUP_ALL; - else if (!strcmp(cleanup_arg, "scissors")) - return use_editor ? COMMIT_MSG_CLEANUP_SCISSORS : - COMMIT_MSG_CLEANUP_SPACE; - else if (!die_on_error) { + struct cleanup_config_mapping *default_mapping = &cleanup_config_mappings[0]; + struct cleanup_config_mapping *current_mapping; + + 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; + } + } + + 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); } +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")); +} + void append_conflicts_hint(struct index_state *istate, struct strbuf *msgbuf) { @@ -2350,6 +2382,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data) opts->allow_rerere_auto = git_config_bool_or_int(key, value, &error_flag) ? RERERE_AUTOUPDATE : RERERE_NOAUTOUPDATE; + else if (!strcmp(key, "options.default-msg-cleanup")) + opts->default_msg_cleanup = get_cleanup_mode(value, 1, 1); else return error(_("invalid key: %s"), key); @@ -2754,6 +2788,9 @@ static int save_opts(struct replay_opts *opts) res |= git_config_set_in_file_gently(opts_file, "options.allow-rerere-auto", opts->allow_rerere_auto == RERERE_AUTOUPDATE ? "true" : "false"); + + res |= git_config_set_in_file_gently(opts_file, "options.default-msg-cleanup", + get_config_from_cleanup(opts->default_msg_cleanup)); return res; } diff --git a/sequencer.h b/sequencer.h index e7908f558e..e3c1f44807 100644 --- a/sequencer.h +++ b/sequencer.h @@ -118,6 +118,7 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag); void append_conflicts_hint(struct index_state *istate, struct strbuf *msgbuf); 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); void cleanup_message(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode, int verbose);
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. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- sequencer.c | 67 +++++++++++++++++++++++++++++++++++++++++------------ sequencer.h | 1 + 2 files changed, 53 insertions(+), 15 deletions(-)