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

[Elijah Newren <newren@gmail.com>]

Hi Dscho,

On Thu, Jul 12, 2018 at 8:41 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi Elijah,
>
> On Wed, 27 Jun 2018, Elijah Newren wrote:
>
...
>> 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?
>
>>               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 <Johannes.Schindelin@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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