git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 2/3] sequencer: make commit options more extensible

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 27, 2017, 21:03 UTC
Message-ID
<xmqqwpbazb34.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<xmqqwpbb35e3.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> As this thing is about fixing a regression visible to end users, I
> may get around to fixing things up myself, but I have other topics
> to attend to, so...
So I ended up with this version before merging it to 'next'.

I moved 'allow' back on the line it is declared, but left it uninitialized because it is unconditionally assigned to before its value is looked at.

Also the updated log message stresses more about the reason why piling new parameters on top is a bad practice, and it is a good idea to do this clean-up now. Compared to that, the reason why this clean-up was not done before and left as maintenance burden is much less important to the readers of the log.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Thu, 23 Mar 2017 17:07:11 +0100
Subject: [PATCH] sequencer: make commit options more extensible

So far every time we need to tweak the behaviour of run_git_commit() we have been adding a "int" parameter to it. As the function gains parameters and different callsites having different needs, this is becoming a maintenance burden. When a new knob needs to be added to address a specific need for a single callsite, all the other callsites need to add a "no, I do not want anything special with respect to the new knob" argument.

Consolidate the existing four parameters into a flag word to make it more maintainable, as we will be adding a new one to the mix soon.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 8183a83c1f..677e6ef764 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -602,6 +602,11 @@ N_("you have staged changes in your working tree\n"
 "\n"
 "  git rebase --continue\n");
 
+#define ALLOW_EMPTY (1<<0)
+#define EDIT_MSG    (1<<1)
+#define AMEND_MSG   (1<<2)
+#define CLEANUP_MSG (1<<3)
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -615,8 +620,7 @@ N_("you have staged changes in your working tree\n"
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
-			  int allow_empty, int edit, int amend,
-			  int cleanup_commit_message)
+			  unsigned int flags)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	const char *value;
@@ -624,7 +628,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	cmd.git_cmd = 1;
 
 	if (is_rebase_i(opts)) {
-		if (!edit) {
+		if (!(flags & EDIT_MSG)) {
 			cmd.stdout_to_stderr = 1;
 			cmd.err = -1;
 		}
@@ -640,7 +644,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	argv_array_push(&cmd.args, "commit");
 	argv_array_push(&cmd.args, "-n");
 
-	if (amend)
+	if ((flags & AMEND_MSG))
 		argv_array_push(&cmd.args, "--amend");
 	if (opts->gpg_sign)
 		argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
@@ -648,16 +652,16 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 		argv_array_push(&cmd.args, "-s");
 	if (defmsg)
 		argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
-	if (cleanup_commit_message)
+	if ((flags & CLEANUP_MSG))
 		argv_array_push(&cmd.args, "--cleanup=strip");
-	if (edit)
+	if ((flags & EDIT_MSG))
 		argv_array_push(&cmd.args, "-e");
-	else if (!cleanup_commit_message &&
+	else if (!(flags & CLEANUP_MSG) &&
 		 !opts->signoff && !opts->record_origin &&
 		 git_config_get_value("commit.cleanup", &value))
 		argv_array_push(&cmd.args, "--cleanup=verbatim");
 
-	if (allow_empty)
+	if ((flags & ALLOW_EMPTY))
 		argv_array_push(&cmd.args, "--allow-empty");
 
 	if (opts->allow_empty_message)
@@ -926,14 +930,14 @@ static void record_in_rewritten(struct object_id *oid,
 static int do_pick_commit(enum todo_command command, struct commit *commit,
 		struct replay_opts *opts, int final_fixup)
 {
-	int edit = opts->edit, cleanup_commit_message = 0;
-	const char *msg_file = edit ? NULL : git_path_merge_msg();
+	unsigned int flags = opts->edit ? EDIT_MSG : 0;
+	const char *msg_file = opts->edit ? NULL : git_path_merge_msg();
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
 	const char *base_label, *next_label;
 	struct commit_message msg = { NULL, NULL, NULL, NULL };
 	struct strbuf msgbuf = STRBUF_INIT;
-	int res, unborn = 0, amend = 0, allow = 0;
+	int res, unborn = 0, allow;
 
 	if (opts->no_commit) {
 		/*
@@ -991,7 +995,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			opts);
 		if (res || command != TODO_REWORD)
 			goto leave;
-		edit = amend = 1;
+		flags |= EDIT_MSG | AMEND_MSG;
 		msg_file = NULL;
 		goto fast_forward_edit;
 	}
@@ -1046,15 +1050,15 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	}
 
 	if (command == TODO_REWORD)
-		edit = 1;
+		flags |= EDIT_MSG;
 	else if (is_fixup(command)) {
 		if (update_squash_messages(command, commit, opts))
 			return -1;
-		amend = 1;
+		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
 		else if (file_exists(rebase_path_fixup_msg())) {
-			cleanup_commit_message = 1;
+			flags |= CLEANUP_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
 			const char *dest = git_path("SQUASH_MSG");
@@ -1064,7 +1068,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 					     rebase_path_squash_msg(), dest);
 			unlink(git_path("MERGE_MSG"));
 			msg_file = dest;
-			edit = 1;
+			flags |= EDIT_MSG;
 		}
 	}
 
@@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	if (allow < 0) {
 		res = allow;
 		goto leave;
-	}
+	} else if (allow)
+		flags |= ALLOW_EMPTY;
 	if (!opts->no_commit)
 fast_forward_edit:
-		res = run_git_commit(msg_file, opts, allow, edit, amend,
-				     cleanup_commit_message);
+		res = run_git_commit(msg_file, opts, flags);
 
 	if (!res && final_fixup) {
 		unlink(rebase_path_fixup_msg());
@@ -2154,7 +2158,7 @@ static int continue_single_pick(void)
 
 static int commit_staged_changes(struct replay_opts *opts)
 {
-	int amend = 0;
+	unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
 
 	if (has_unstaged_changes(1))
 		return error(_("cannot rebase: You have unstaged changes."));
@@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)
 				       "--continue' again."));
 
 		strbuf_release(&rev);
-		amend = 1;
+		flags |= AMEND_MSG;
 	}
 
-	if (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))
+	if (run_git_commit(rebase_path_message(), opts, flags))
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
 	return 0;

-- 
2.12.2-430-ge0ef7fe78c
Previous: Junio C HamanoNext: Johannes Schindelin
Message 24 of 27 in “rebase -i (reword): call the commit-msg hook again”
  1. 0/3 rebase -i (reword): call the commit-msg hook againJohannes Schindelin, Mar 22, 2017
  2. 2/3 sequencer: make commit options more extensibleJohannes Schindelin, Mar 22, 2017
  3. Junio C HamanoMar 22, 2017
  4. Johannes SchindelinMar 23, 2017
  5. Junio C HamanoMar 23, 2017
  6. 1/3 t7504: document regression: reword no longer calls commit-msgJohannes Schindelin, Mar 22, 2017
  7. Sebastian SchuberthMar 22, 2017
  8. Johannes SchindelinMar 22, 2017
  9. Junio C HamanoMar 22, 2017
  10. Johannes SchindelinMar 23, 2017
  11. Sebastian SchuberthMar 23, 2017
  12. Johannes SchindelinMar 23, 2017
  13. Junio C HamanoMar 22, 2017
  14. Johannes SchindelinMar 23, 2017
  15. 3/3 sequencer: allow the commit-msg hooks to run during a `reword`Johannes Schindelin, Mar 22, 2017
  16. Junio C HamanoMar 22, 2017
  17. Johannes SchindelinMar 23, 2017
  18. 0/3 rebase -i (reword): call the commit-msg hook againJohannes Schindelin, Mar 23, 2017
  19. 1/3 t7504: document regression: reword no longer calls commit-msgJohannes Schindelin, Mar 23, 2017
  20. 2/3 sequencer: make commit options more extensibleJohannes Schindelin, Mar 23, 2017
  21. Junio C HamanoMar 24, 2017
  22. Johannes SchindelinMar 25, 2017
  23. Junio C HamanoMar 27, 2017
  24. Junio C HamanoMar 27, 2017
  25. Johannes SchindelinMar 29, 2017
  26. 3/3 sequencer: allow the commit-msg hooks to run during a `reword`Johannes Schindelin, Mar 23, 2017
  27. Junio C HamanoMar 23, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.