[v8,10/11] sequencer.c: define describe_cleanup_mode

Commit Message

Denton Liu March 17, 2019, 10:16 a.m. UTC

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
describe_cleanup_mode.

Reviewed-by: Eric Sunshine <[e-mail]>
Reviewed-by: Junio C Hamano <[e-mail]>
Signed-off-by: Ramsay Jones <[e-mail]>
Signed-off-by: Denton Liu <[e-mail]>
---
 sequencer.c | 62 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 15 deletions(-)

Comments

Eric Sunshine March 18, 2019, 8:04 p.m. UTC | #1

On Sun, Mar 17, 2019 at 6:16 AM Denton Liu <[e-mail]> 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
> describe_cleanup_mode.
>
> Reviewed-by: Eric Sunshine <[e-mail]>
> Reviewed-by: Junio C Hamano <[e-mail]>

These two Reviewed-by: lines should be dropped for a couple reasons.

First, neither Junio nor I reviewed _this_ version of the patch.

Second, a Reviewed-by: is given explicitly (not taken). When a
reviewer has thoroughly read and understood a patch and considers it
problem-free, he or she may say explicitly "Reviewed-by: <me>",
stating satisfaction that the patch seems worthy of inclusion in the
project. If he sees fit, Junio may then pick up that Reviewed-by: at
the time he queues the patch in his tree.

> Signed-off-by: Ramsay Jones <[e-mail]>
> Signed-off-by: Denton Liu <[e-mail]>

Denton Liu March 18, 2019, 8:30 p.m. UTC | #2

On Mon, Mar 18, 2019 at 04:04:22PM -0400, Eric Sunshine wrote:
> On Sun, Mar 17, 2019 at 6:16 AM Denton Liu <[e-mail]> 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
> > describe_cleanup_mode.
> >
> > Reviewed-by: Eric Sunshine <[e-mail]>
> > Reviewed-by: Junio C Hamano <[e-mail]>
> 
> These two Reviewed-by: lines should be dropped for a couple reasons.
> 
> First, neither Junio nor I reviewed _this_ version of the patch.
> 
> Second, a Reviewed-by: is given explicitly (not taken). When a
> reviewer has thoroughly read and understood a patch and considers it
> problem-free, he or she may say explicitly "Reviewed-by: <me>",
> stating satisfaction that the patch seems worthy of inclusion in the
> project. If he sees fit, Junio may then pick up that Reviewed-by: at
> the time he queues the patch in his tree.

My mistake, I misunderstood the purpose of the Reviewed-by tag.

I was using it to give credit for changes that I incorporated in
response to a review. In addition, it was helpful for bookkeeping who
suggested what where.

Would a Helped-by tag be more appropriate in this situation?

Thanks,

Denton

> 
> > Signed-off-by: Ramsay Jones <[e-mail]>
> > Signed-off-by: Denton Liu <[e-mail]>

Eric Sunshine March 18, 2019, 8:32 p.m. UTC | #3

On Mon, Mar 18, 2019 at 4:30 PM Denton Liu <[e-mail]> wrote:
> On Mon, Mar 18, 2019 at 04:04:22PM -0400, Eric Sunshine wrote:
> > These two Reviewed-by: lines should be dropped for a couple reasons.
> >
> I was using it to give credit for changes that I incorporated in
> response to a review. In addition, it was helpful for bookkeeping who
> suggested what where.
>
> Would a Helped-by tag be more appropriate in this situation?

Using Helped-by: would be quite appropriate.

Ramsay Jones March 19, 2019, 12:55 a.m. UTC | #4

On 18/03/2019 20:04, Eric Sunshine wrote:
> On Sun, Mar 17, 2019 at 6:16 AM Denton Liu <[e-mail]> 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
>> describe_cleanup_mode.
>>
>> Reviewed-by: Eric Sunshine <[e-mail]>
>> Reviewed-by: Junio C Hamano <[e-mail]>
> 
> These two Reviewed-by: lines should be dropped for a couple reasons.
> 
> First, neither Junio nor I reviewed _this_ version of the patch.
> 
> Second, a Reviewed-by: is given explicitly (not taken). When a
> reviewer has thoroughly read and understood a patch and considers it
> problem-free, he or she may say explicitly "Reviewed-by: <me>",
> stating satisfaction that the patch seems worthy of inclusion in the
> project. If he sees fit, Junio may then pick up that Reviewed-by: at
> the time he queues the patch in his tree.
> 
>> Signed-off-by: Ramsay Jones <[e-mail]>

I was similarly surprised to see this SOB line as well. There was
nothing copyright-able in the 'squash patch' I sent, so I don't
think this is warranted. (A 'Helped-by:' at _most_, I would think).
;-)

ATB,
Ramsay Jones

Patch

diff --git a/sequencer.c b/sequencer.c
index 2cbfb848dd..f782e8bab2 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 *value;
+	enum commit_msg_cleanup_mode editor;
+	enum commit_msg_cleanup_mode no_editor;
+};
+
+/* the 0th element of this array must be the "default" */
+static struct cleanup_config_mapping cleanup_config_mapping[] = {
+	{ "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,37 @@  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 *def = &cleanup_config_mapping[0];
+	struct cleanup_config_mapping *p;
+
+	if (!cleanup_arg)
+		return use_editor ? def->editor :
+				    def->no_editor;
+
+	for (p = cleanup_config_mapping; p->value; p++)
+		if (!strcmp(cleanup_arg, p->value))
+			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 ? def->editor :
+				    def->no_editor;
 	} else
 		die(_("invalid cleanup mode %s"), cleanup_arg);
 }
 
+static const char *describe_cleanup_mode(enum commit_msg_cleanup_mode cleanup_mode)
+{
+	struct cleanup_config_mapping *curr;
+
+	for (curr = &cleanup_config_mapping[1]; curr->value; curr++)
+		if (cleanup_mode == curr->editor)
+			return curr->value;
+
+	BUG("invalid cleanup_mode provided (%d)", cleanup_mode);
+}
+
 void append_conflicts_hint(struct index_state *istate,
 			   struct strbuf *msgbuf)
 {
@@ -2350,6 +2377,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 +2783,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",
+					     describe_cleanup_mode(opts->default_msg_cleanup));
 	return res;
 }