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

[PATCH v3] sequencer: fix edit handling for cherry-pick and revert messages

From
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Date
Mar 31, 2021, 06:52 UTC
Message-ID
<pull.988.v3.git.git.1617173541301.gitgitgadget@gmail.com>
In-Reply-To
<pull.988.v2.git.git.1617070174458.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>

save_opts() should save any non-default values. It was intended to do this, but since most options in struct replay_opts default to 0, it only saved non-zero values. Unfortunately, this does not always work for options.edit. Roughly speaking, options.edit had a default value of 0 for cherry-pick but a default value of 1 for revert. Make save_opts() record a value whenever it differs from the default.

options.edit was also overly simplistic; we had more than two cases. The behavior that previously existed was as follows:

                       Non-conflict commits    Right after Conflict
    revert             Edit iff isatty(0)      Edit (ignore isatty(0))
    cherry-pick        No edit                 See above
    Specify --edit     Edit (ignore isatty(0)) See above
    Specify --no-edit  (*)                     See above
    (*) Before stopping for conflicts, No edit is the behavior.  After
        stopping for conflicts, the --no-edit flag is not saved so see
        the first two rows.
However, the expected behavior is:
                       Non-conflict commits    Right after Conflict
    revert             Edit iff isatty(0)      Edit iff isatty(0)
    cherry-pick        No edit                 Edit iff isatty(0)
    Specify --edit     Edit (ignore isatty(0)) Edit (ignore isatty(0))
    Specify --no-edit  No edit                 No edit

In order to get the expected behavior, we need to change options.edit to a tri-state: unspecified, false, or true. When specified, we follow what it says. When unspecified, we need to check whether the current commit being created is resolving a conflict as well as consulting options.action and isatty(0). While at it, add a should_edit() utility function that compresses options.edit down to a boolean based on the additional information for the non-conflict case.

continue_single_pick() is the function responsible for resuming after conflict cases, regardless of whether there is one commit being picked or many. Make this function stop assuming edit behavior in all cases, so that it can correctly handle !isatty(0) and specific requests to not edit the commit message.

Reported-by: Renato Botelho <garga@freebsd.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
    sequencer: fix edit handling for cherry-pick and revert messages
    
    Changes since v2:
    
     * Changed to use <0 for unspecified (instead of == -1), >0 for true
       (instead of == 1).
     * Removed assert() statement.
     * Removed unnecessary "want_edit" local variable
    
    Reported-by: Renato Botelho garga@freebsd.org Signed-off-by: Elijah
    Newren newren@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-988%2Fnewren%2Ffix-sequencer-no-edit-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-988/newren/fix-sequencer-no-edit-v3
Pull-Request: https://github.com/git/git/pull/988
Range-diff vs v2:
 1:  376799bd3d99 ! 1:  8fce8c772dee sequencer: fix edit handling for cherry-pick and revert messages
     @@ builtin/revert.c: static int run_sequencer(int argc, const char **argv, struct r
       				"--no-commit", opts->no_commit,
       				"-x", opts->record_origin,
      -				"--edit", opts->edit,
     -+				"--edit", opts->edit == 1,
     ++				"--edit", opts->edit > 0,
       				NULL);
       
       	if (cmd) {
     @@ sequencer.c: static void record_in_rewritten(struct object_id *oid,
       }
       
      +static int should_edit(struct replay_opts *opts) {
     -+	assert(opts->edit >= -1 && opts->edit <= 1);
     -+	if (opts->edit == -1)
     ++	if (opts->edit < 0)
      +		/*
      +		 * Note that we only handle the case of non-conflicted
      +		 * commits; continue_single_pick() handles the conflicted
     @@ sequencer.c: static int save_opts(struct replay_opts *opts)
      -	if (opts->edit)
      -		res |= git_config_set_in_file_gently(opts_file,
      -					"options.edit", "true");
     -+	if (opts->edit != -1)
     ++	if (opts->edit >= 0)
      +		res |= git_config_set_in_file_gently(opts_file, "options.edit",
      +						     opts->edit ? "true" : "false");
       	if (opts->allow_empty)
     @@ sequencer.c: static int pick_commits(struct repository *r,
       {
      -	const char *argv[] = { "commit", NULL };
      +	struct strvec argv = STRVEC_INIT;
     -+	int want_edit;
      +	int ret;
       
       	if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
     @@ sequencer.c: static int pick_commits(struct repository *r,
      +	 * we want to edit if the user asked for it, or if they didn't specify
      +	 * and stdin is a tty.
      +	 */
     -+	want_edit = (opts->edit == 1) || ((opts->edit == -1) && isatty(0));
     -+	if (!want_edit)
     ++	if (!opts->edit || (opts->edit < 0 && !isatty(0)))
      +		/*
      +		 * Include --cleanup=strip as well because we don't want the
      +		 * "# Conflicts:" messages.
 builtin/revert.c                |  4 +--
 sequencer.c                     | 52 ++++++++++++++++++++++++++-------
 sequencer.h                     |  6 ++--
 t/t3510-cherry-pick-sequence.sh | 32 +++++++++++++++++++-
 4 files changed, 77 insertions(+), 17 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 314a86c5621b..237f2f18d4c0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 				"--signoff", opts->signoff,
 				"--no-commit", opts->no_commit,
 				"-x", opts->record_origin,
-				"--edit", opts->edit,
+				"--edit", opts->edit > 0,
 				NULL);
 
 	if (cmd) {
@@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int res;
 
-	if (isatty(0))
-		opts.edit = 1;
 	opts.action = REPLAY_REVERT;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, &opts);
diff --git a/sequencer.c b/sequencer.c
index 848204d3dc3f..8ef597ac3a9e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1860,14 +1860,25 @@ static void record_in_rewritten(struct object_id *oid,
 		flush_rewritten_pending();
 }
 
+static int should_edit(struct replay_opts *opts) {
+	if (opts->edit < 0)
+		/*
+		 * Note that we only handle the case of non-conflicted
+		 * commits; continue_single_pick() handles the conflicted
+		 * commits itself instead of calling this function.
+		 */
+		return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;
+	return opts->edit;
+}
+
 static int do_pick_commit(struct repository *r,
 			  enum todo_command command,
 			  struct commit *commit,
 			  struct replay_opts *opts,
 			  int final_fixup, int *check_todo)
 {
-	unsigned int flags = opts->edit ? EDIT_MSG : 0;
-	const char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);
+	unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
+	const char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);
 	struct object_id head;
 	struct commit *base, *next, *parent;
 	const char *base_label, *next_label;
@@ -3101,9 +3112,9 @@ static int save_opts(struct replay_opts *opts)
 	if (opts->no_commit)
 		res |= git_config_set_in_file_gently(opts_file,
 					"options.no-commit", "true");
-	if (opts->edit)
-		res |= git_config_set_in_file_gently(opts_file,
-					"options.edit", "true");
+	if (opts->edit >= 0)
+		res |= git_config_set_in_file_gently(opts_file, "options.edit",
+						     opts->edit ? "true" : "false");
 	if (opts->allow_empty)
 		res |= git_config_set_in_file_gently(opts_file,
 					"options.allow-empty", "true");
@@ -4077,7 +4088,7 @@ static int pick_commits(struct repository *r,
 	prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
 	if (opts->allow_ff)
 		assert(!(opts->signoff || opts->no_commit ||
-			 opts->record_origin || opts->edit ||
+			 opts->record_origin || should_edit(opts) ||
 			 opts->committer_date_is_author_date ||
 			 opts->ignore_date));
 	if (read_and_refresh_cache(r, opts))
@@ -4370,14 +4381,33 @@ static int pick_commits(struct repository *r,
 	return sequencer_remove_state(opts);
 }
 
-static int continue_single_pick(struct repository *r)
+static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 {
-	const char *argv[] = { "commit", NULL };
+	struct strvec argv = STRVEC_INIT;
+	int ret;
 
 	if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 	    !refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD"))
 		return error(_("no cherry-pick or revert in progress"));
-	return run_command_v_opt(argv, RUN_GIT_CMD);
+
+	strvec_push(&argv, "commit");
+
+	/*
+	 * continue_single_pick() handles the case of recovering from a
+	 * conflict.  should_edit() doesn't handle that case; for a conflict,
+	 * we want to edit if the user asked for it, or if they didn't specify
+	 * and stdin is a tty.
+	 */
+	if (!opts->edit || (opts->edit < 0 && !isatty(0)))
+		/*
+		 * Include --cleanup=strip as well because we don't want the
+		 * "# Conflicts:" messages.
+		 */
+		strvec_pushl(&argv, "--no-edit", "--cleanup=strip", NULL);
+
+	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
+	strvec_clear(&argv);
+	return ret;
 }
 
 static int commit_staged_changes(struct repository *r,
@@ -4547,7 +4577,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			goto release_todo_list;
 		}
 	} else if (!file_exists(get_todo_path(opts)))
-		return continue_single_pick(r);
+		return continue_single_pick(r, opts);
 	else if ((res = read_populate_todo(r, &todo_list, opts)))
 		goto release_todo_list;
 
@@ -4556,7 +4586,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 		if (refs_ref_exists(get_main_ref_store(r),
 				    "CHERRY_PICK_HEAD") ||
 		    refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD")) {
-			res = continue_single_pick(r);
+			res = continue_single_pick(r, opts);
 			if (res)
 				goto release_todo_list;
 		}
diff --git a/sequencer.h b/sequencer.h
index f8b2e4ab8527..d57d8ea23d7a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -31,8 +31,10 @@ enum commit_msg_cleanup_mode {
 struct replay_opts {
 	enum replay_action action;
 
-	/* Boolean options */
+	/* Tri-state options: unspecified, false, or true */
 	int edit;
+
+	/* Boolean options */
 	int record_origin;
 	int no_commit;
 	int signoff;
@@ -71,7 +73,7 @@ struct replay_opts {
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
 };
-#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
+#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }
 
 /*
  * Note that ordering matters in this enum. Not only must it match the mapping
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index b76cb6de91d0..49010aa9469d 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -65,7 +65,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	# gets interrupted, use a high-enough number that is larger
 	# than the number of parents of any commit we have created
 	mainline=4 &&
-	test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&
+	test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours --edit initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&
@@ -84,6 +84,36 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	ours
 	EOF
 	git config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&
+	test_cmp expect actual &&
+	echo "true" >expect &&
+	git config --file=.git/sequencer/opts --get-all options.edit >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'revert persists opts correctly' '
+	pristine_detach initial &&
+	# to make sure that the session to revert a sequence
+	# gets interrupted, revert commits that are not in the history
+	# of HEAD.
+	test_expect_code 1 git revert -s --strategy=recursive -X patience -X ours --no-edit picked yetanotherpick &&
+	test_path_is_dir .git/sequencer &&
+	test_path_is_file .git/sequencer/head &&
+	test_path_is_file .git/sequencer/todo &&
+	test_path_is_file .git/sequencer/opts &&
+	echo "true" >expect &&
+	git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
+	test_cmp expect actual &&
+	echo "recursive" >expect &&
+	git config --file=.git/sequencer/opts --get-all options.strategy >actual &&
+	test_cmp expect actual &&
+	cat >expect <<-\EOF &&
+	patience
+	ours
+	EOF
+	git config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&
+	test_cmp expect actual &&
+	echo "false" >expect &&
+	git config --file=.git/sequencer/opts --get-all options.edit >actual &&
 	test_cmp expect actual
 '
 

base-commit: 98164e9585e02e31dcf1377a553efe076c15f8c6

-- 
gitgitgadget
Previous: Johannes SchindelinNext: Johannes Schindelin
Message 27 of 28 in “sequencer: fix edit handling for cherry-pick and revert messages”
  1. sequencer: fix edit handling for cherry-pick and revert messagesElijah Newren via GitGitGadget, Mar 26, 2021
  2. Philip OakleyMar 26, 2021
  3. Elijah NewrenMar 26, 2021
  4. Junio C HamanoMar 28, 2021
  5. Phillip WoodMar 29, 2021
  6. Junio C HamanoMar 29, 2021
  7. Elijah NewrenMar 29, 2021
  8. sequencer: fix edit handling for cherry-pick and revert messagesElijah Newren via GitGitGadget, Mar 30, 2021
  9. Johannes SchindelinMar 30, 2021
  10. Junio C HamanoMar 30, 2021
  11. Elijah NewrenMar 30, 2021
  12. Ævar Arnfjörð BjarmasonMar 31, 2021
  13. Elijah NewrenMar 31, 2021
  14. Junio C HamanoMar 31, 2021
  15. Elijah NewrenApr 1, 2021
  16. Elijah NewrenMar 30, 2021
  17. unifying sequencer's options persisting, was Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messagesJohannes Schindelin, Mar 31, 2021
  18. Phillip WoodApr 2, 2021
  19. Phillip WoodApr 2, 2021
  20. Junio C HamanoApr 2, 2021
  21. Elijah NewrenApr 2, 2021
  22. Junio C HamanoApr 2, 2021
  23. Johannes SchindelinApr 8, 2021
  24. Junio C HamanoApr 8, 2021
  25. Christian CouderApr 8, 2021
  26. Johannes SchindelinApr 9, 2021
  27. sequencer: fix edit handling for cherry-pick and revert messagesElijah Newren via GitGitGadget, Mar 31, 2021
  28. Johannes SchindelinMar 31, 2021

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.