Re: [PATCH v3 2/2] Fix use of strategy options with interactive rebases

Hi Dscho,

On Thu, Jul 12, 2018 at 8:41 AM, Johannes Schindelin [off-list ref] wrote:

Hi Elijah,

On Wed, 27 Jun 2018, Elijah Newren wrote:

...

quoted

diff --git a/git-rebase.sh b/git-rebase.sh
index 19bdebb480..f3b10c7f62 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -328,7 +328,7 @@ do
              do_merge=t
              ;;
      --strategy-option=*)
-             strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--${1#--strategy-option=}")"
+             strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--${1#--strategy-option=}" | sed -e s/^.//)"

Didn't you mean to use "s/^ *//" instead?

quoted

              do_merge=t
              test -z "$strategy" && strategy=recursive
              ;;

diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..ef9237c814 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -2206,6 +2206,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 {
      int i;
+     char *strategy_opts_string;

      strbuf_reset(buf);
      if (!read_oneliner(buf, rebase_path_strategy(), 0))

@@ -2214,7 +2215,11 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
      if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
              return;

-     opts->xopts_nr = split_cmdline(buf->buf, (const char ***)&opts->xopts);
+     strategy_opts_string = buf->buf;
+     if (*strategy_opts_string == ' ')

I think that this would ideally even be a `while` instead of an `if`.

Thanks for taking a look; both sound like good suggestions.  Since the
patch in question has already reached next, here's a patch on top of
en/rebase-i-microfixes to make these two changes:

-- 8< --
Subject: [PATCH] Whitespace handling improvements with interactive rebase
 strategy options

In commit 0060041df ("Fix use of strategy options with interactive
rebases", 2018-06-27), extra whitespace was removed from a generated
string to fix up parsing.  Instead of assuming one extra space, though, we
can just remove all leading whitespace and make the code slightly more
robust.

Suggested-by: Johanness Schindelin <redacted>
Signed-off-by: Elijah Newren <redacted>
---
 git-rebase.sh | 2 +-
 sequencer.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index f3b10c7f62..e572980bbc 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -328,7 +328,7 @@ do
 		do_merge=t
 		;;
 	--strategy-option=*)
-		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--${1#--strategy-option=}" | sed -e s/^.//)"
+		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--${1#--strategy-option=}" | sed -e "s/ *//")"
 		do_merge=t
 		test -z "$strategy" && strategy=recursive
 		;;

diff --git a/sequencer.c b/sequencer.c
index ef9237c814..3f780f8f50 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -2216,7 +2216,7 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 		return;
 
 	strategy_opts_string = buf->buf;
-	if (*strategy_opts_string == ' ')
+	while (*strategy_opts_string == ' ')
 		strategy_opts_string++;
 	opts->xopts_nr = split_cmdline(strategy_opts_string,
 				       (const char ***)&opts->xopts);

-- 
2.18.0.645.g72fe132ec2