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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 12, 2018, 15:41 UTC
Message-ID: <nycvar.QRO.7.76.6.1807121740270.75@tvgsbejvaqbjf.bet>
In-Reply-To: <20180627154804.3743-3-newren@gmail.com>

Hi Elijah,

On Wed, 27 Jun 2018, Elijah Newren wrote:

Show 37 quoted lines

> git-rebase.sh wrote strategy options to .git/rebase/merge/strategy_opts
> in the following format:
>   '--ours'  '--renormalize'
> Note the double spaces.
> 
> git-rebase--interactive uses sequencer.c to parse that file, and
> sequencer.c used split_cmdline() to get the individual strategy options.
> After splitting, sequencer.c prefixed each "option" with a double dash,
> so, concatenating all its options would result in:
>   -- --ours -- --renormalize
> 
> So, when it ended up calling try_merge_strategy(), that in turn would run
>   git merge-$strategy -- --ours -- --renormalize $merge_base -- $head $remote
> 
> instead of the expected/desired
>   git merge-$strategy --ours --renormalize $merge_base -- $head $remote
> 
> Remove the extra spaces so that when it goes through split_cmdline() we end
> up with the desired command line.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  git-rebase.sh              | 2 +-
>  sequencer.c                | 7 ++++++-
>  t/t3418-rebase-continue.sh | 2 +-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> 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?

Show 22 quoted lines

>  		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`.

Show 19 quoted lines

> +		strategy_opts_string++;
> +	opts->xopts_nr = split_cmdline(strategy_opts_string,
> +				       (const char ***)&opts->xopts);
>  	for (i = 0; i < opts->xopts_nr; i++) {
>  		const char *arg = opts->xopts[i];
>  
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 11546d6e14..c145dbac38 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -74,7 +74,7 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
>  	test -f funny.was.run
>  '
>  
> -test_expect_failure 'rebase -i --continue handles merge strategy and options' '
> +test_expect_success 'rebase -i --continue handles 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-for-dash-i" F3 32 &&

Thanks! Dscho