git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Phillip Wood" <phillip.wood123@gmail.com>
Subject: [PATCH v4 4/5] rebase -m: fix serialization of strategy options
Date: Mon, 10 Apr 2023 10:08:30 +0100	[thread overview]
Message-ID: <671ee03503dd1642a8d38b9d86b913ec39b57ba3.1681117706.git.phillip.wood@dunelm.org.uk> (raw)
In-Reply-To: <cover.1681117706.git.phillip.wood@dunelm.org.uk>

From: Phillip Wood <phillip.wood@dunelm.org.uk>

To store the strategy options rebase prepends " --" to each one and
writes them to a file. To load them it reads the file and passes the
contents to split_cmdline(). This roughly mimics the behavior of the
scripted rebase but has a couple of limitations, (1) options containing
whitespace are not properly preserved (this is true of the scripted
rebase as well) and (2) options containing '"' or '\' are incorrectly
parsed and may cause the parser to return an error.

Fix these limitations by quoting each option when they are stored so
that they can be parsed correctly. Now that "--preserve-merges" no
longer exist this change also stops prepending "--" to the options when
they are stored as that was an artifact of the scripted rebase.

These changes are backwards compatible so the files written by an older
version of git can still be read. They are also forwards compatible,
the file can still be parsed by recent versions of git as they treat the
"--" prefix as optional.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 alias.c                        | 18 +++++++++++++++++
 alias.h                        |  3 +++
 sequencer.c                    | 11 ++++++-----
 t/t3418-rebase-continue.sh     | 36 ++++++++++++++++++++++------------
 t/t3436-rebase-more-options.sh | 24 -----------------------
 5 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/alias.c b/alias.c
index e814948ced..54a1a23d2c 100644
--- a/alias.c
+++ b/alias.c
@@ -3,6 +3,7 @@
 #include "alloc.h"
 #include "config.h"
 #include "gettext.h"
+#include "strbuf.h"
 #include "string-list.h"
 
 struct config_alias_data {
@@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)
 	read_early_config(config_alias_cb, &data);
 }
 
+void quote_cmdline(struct strbuf *buf, const char **argv)
+{
+	for (const char **argp = argv; *argp; argp++) {
+		if (argp != argv)
+			strbuf_addch(buf, ' ');
+		strbuf_addch(buf, '"');
+		for (const char *p = *argp; *p; p++) {
+			const char c = *p;
+
+			if (c == '"' || c =='\\')
+				strbuf_addch(buf, '\\');
+			strbuf_addch(buf, c);
+		}
+		strbuf_addch(buf, '"');
+	}
+}
+
 #define SPLIT_CMDLINE_BAD_ENDING 1
 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
 #define SPLIT_CMDLINE_ARGC_OVERFLOW 3
diff --git a/alias.h b/alias.h
index aef4843bb7..43db736484 100644
--- a/alias.h
+++ b/alias.h
@@ -1,9 +1,12 @@
 #ifndef ALIAS_H
 #define ALIAS_H
 
+struct strbuf;
 struct string_list;
 
 char *alias_lookup(const char *alias);
+/* Quote argv so buf can be parsed by split_cmdline() */
+void quote_cmdline(struct strbuf *buf, const char **argv);
 int split_cmdline(char *cmdline, const char ***argv);
 /* Takes a negative value returned by split_cmdline */
 const char *split_cmdline_strerror(int cmdline_errno);
diff --git a/sequencer.c b/sequencer.c
index 587a473d6e..fc6ea75895 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 
 	count = split_cmdline(strategy_opts_string, &argv);
 	if (count < 0)
-		die(_("could not split '%s': %s"), strategy_opts_string,
+		BUG("could not split '%s': %s", strategy_opts_string,
 			    split_cmdline_strerror(count));
 	for (i = 0; i < count; i++) {
 		const char *arg = argv[i];
@@ -3049,12 +3049,13 @@ static int read_populate_opts(struct replay_opts *opts)
 
 static void write_strategy_opts(struct replay_opts *opts)
 {
-	int i;
 	struct strbuf buf = STRBUF_INIT;
 
-	for (i = 0; i < opts->xopts.nr; ++i)
-		strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
-
+	/*
+	 * Quote strategy options so that they can be read correctly
+	 * by split_cmdline().
+	 */
+	quote_cmdline(&buf, opts->xopts.v);
 	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
 	strbuf_release(&buf);
 }
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 130e2f9b55..42c3954125 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F2-on-topic-branch &&
 	test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run" &&
+	test_when_finished "rm -fr test-bin" &&
 	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
+
+	write_script test-bin/git-merge-funny <<-\EOF &&
+	printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual
+	shift 3 &&
+	exec git merge-recursive "$@"
+	EOF
+
+	cat >expect <<-\EOF &&
+	[7]
+	[--option=arg with space]
+	[--op"tion\]
+	[--new
+	line ]
+	[--]
+	EOF
+
+	rm -f actual &&
 	(
 		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -s funny -Xopt main topic
+		test_must_fail git rebase -s funny -X"option=arg with space" \
+				-Xop\"tion\\ -X"new${LF}line " main topic
 	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
+	test_cmp expect actual &&
+	rm actual &&
 	echo "Resolved" >F2 &&
 	git add F2 &&
 	(
 		PATH=./test-bin:$PATH &&
 		git rebase --continue
 	) &&
-	test -f funny.was.run
+	test_cmp expect actual
 '
 
 test_expect_success 'rebase -i --continue handles merge strategy and options' '
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 3adf42f47d..94671d3c46 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -40,30 +40,6 @@ test_expect_success 'setup' '
 	EOF
 '
 
-test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '
-	test_when_finished "test_might_fail git rebase --abort" &&
-	cat >expect <<-\EOF &&
-	fatal: could not split '\''--bad'\'': unclosed quote
-	EOF
-	GIT_SEQUENCE_EDITOR="echo break >" \
-		git rebase -i -X"bad argument\"" side main &&
-	test_expect_code 128 git rebase --continue >out 2>actual &&
-	test_must_be_empty out &&
-	test_cmp expect actual
-'
-
-test_expect_success 'bad -X <strategy-option> arguments: bad escape' '
-	test_when_finished "test_might_fail git rebase --abort" &&
-	cat >expect <<-\EOF &&
-	fatal: could not split '\''--bad'\'': cmdline ends with \
-	EOF
-	GIT_SEQUENCE_EDITOR="echo break >" \
-		git rebase -i -X"bad escape \\" side main &&
-	test_expect_code 128 git rebase --continue >out 2>actual &&
-	test_must_be_empty out &&
-	test_cmp expect actual
-'
-
 test_expect_success '--ignore-whitespace works with apply backend' '
 	test_must_fail git rebase --apply main side &&
 	git rebase --abort &&
-- 
2.40.0.672.gbd2d2ac924


  parent reply	other threads:[~2023-04-10  9:09 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood
2023-03-15 15:14 ` [PATCH 1/4] rebase: stop reading and writing unnecessary strategy state Phillip Wood
2023-03-15 15:14 ` [PATCH 2/4] rebase -m: cleanup --strategy-option handling Phillip Wood
2023-03-15 15:14 ` [PATCH 3/4] rebase -m: fix serialization of strategy options Phillip Wood
2023-04-01 19:27   ` Elijah Newren
2023-04-04 13:02     ` Phillip Wood
2023-03-15 15:14 ` [PATCH 4/4] rebase: remove a couple of redundant strategy tests Phillip Wood
2023-04-01 19:31   ` Elijah Newren
2023-04-04 13:04     ` Phillip Wood
2023-03-15 16:03 ` [PATCH 0/4] rebase: cleanup merge strategy option handling Junio C Hamano
2023-03-15 16:50 ` Felipe Contreras
2023-04-01 19:32 ` Elijah Newren
2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood
2023-04-05 15:21   ` [PATCH v2 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood
2023-04-05 15:21   ` [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood
2023-04-07  5:44     ` Elijah Newren
2023-04-05 15:21   ` [PATCH v2 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood
2023-04-05 15:21   ` [PATCH v2 4/5] rebase -m: fix serialization of strategy options Phillip Wood
2023-04-07  5:47     ` Elijah Newren
2023-04-05 15:21   ` [PATCH v2 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood
2023-04-07  5:49   ` [PATCH v2 0/5] rebase: cleanup merge strategy option handling Elijah Newren
2023-04-07 13:57     ` Phillip Wood
2023-04-07 17:53       ` Junio C Hamano
2023-04-07 13:55 ` [PATCH v3 " Phillip Wood
2023-04-07 13:55   ` [PATCH v3 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood
2023-04-07 13:55   ` [PATCH v3 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood
2023-04-07 13:55   ` [PATCH v3 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood
2023-04-07 13:55   ` [PATCH v3 4/5] rebase -m: fix serialization of strategy options Phillip Wood
2023-04-07 13:55   ` [PATCH v3 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood
2023-04-10  9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood
2023-04-10  9:08   ` [PATCH v4 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood
2023-04-10  9:08   ` [PATCH v4 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood
2023-04-10  9:08   ` [PATCH v4 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood
2023-04-10  9:08   ` Phillip Wood [this message]
2023-04-10  9:08   ` [PATCH v4 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood
2023-04-10 16:46   ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=671ee03503dd1642a8d38b9d86b913ec39b57ba3.1681117706.git.phillip.wood@dunelm.org.uk \
    --to=phillip.wood123@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).