Re: [PATCHv2] rebase [-i --exec | -ix] <CMD>...

[Johannes Sixt <j6t@kdbg.org>]

Am 06.06.2012 12:34, schrieb Lucien Kong:
> This patch provides a way to automatically add these "exec" lines
> between each commit applications. For instance, running 'git rebase -i
> --exec "make test"' lets you check that intermediate commits are
> compilable.

While I won't be a heavy user of this feature, I think it has some merit
as a porcelain feature, particularly because it is rather cumbersome to
achieve the same effect as in the given example without plumbing commands.

> +-x <cmd>::
> +--exec <cmd>::
...
> ++
> +This has to be used along with the `--interactive` option explicitly.
...
> +
> +If the option '-i' is missing, The command will return a message
> +error. If there is no <cmd> specified behind --exec, the command will
> +return a message error and the usage page of 'git rebase'.

The important part (that -x needs -i) of this paragraph are already
spelled out above, and the exact error behavior does not need a
description in the manual. Drop this paragraph.

BTW, I don't think it is a good idea to dump the usage if -x was used
without -i.

> +# Add commands after a pick or after a squash/fixup serie
> +# in the todo list.
> +add_exec_commands () {
> +	OIFS=$IFS
> +	IFS=$LF
> +	for i in $cmd
> +	do
> +		tmp=$(sed "/^pick .*/i\
> +				exec $i" "$1")

Does this white-space before 'exec' not end up in the  todo list?

I think it is wise to use introduce sed expressions by using -e. This
applies to all 'sed' invocations that this patch introduces (also in the
test-suite).

> +		echo "$tmp" >"$1"

Some 'echo' implementations expand escape sequences in the supplied
texts. To avoid it (this is user-supplied text!), do this:

		printf "%s\n" "$tmp" >"$1"

> +		tmp=$(sed '1d' "$1")
> +		echo "$tmp" >"$1"
> +		echo "exec $i" >>"$1"

Ditto.

> +	done
> +	IFS=$OIFS
> +}

> +	-x)
> +		test 2 -le "$#" || usage
> +		cmd="${cmd:+"$cmd$LF"} $2"

The quoting here is *very* odd. The outer dquotes do extend their effect
into the replacement word after the :+ operator. I am surprised that so
many shells grok it. ash does not, by the way. Also, you don't need the
space anymore. Therefore:

		cmd="${cmd:+$cmd$LF}$2"

> +		shift
> +		;;

> +test_expect_success 'running "git rebase -i --exec git show HEAD"' '
> +	git rebase -i --exec "git show HEAD" HEAD~2 >actual &&
> +	(
> +		FAKE_LINES="1 exec_git_show_HEAD 2 exec_git_show_HEAD" &&
> +		export FAKE_LINES &&
> +		git rebase -i HEAD~2 >expected
> +	) &&
> +	sed '1,9d' expected >expect &&

Here and everywhere else: Single quotes do not nest :-) use dquotes (and
-e).

> +	mv expect expected &&

Why not

	( ... git rebase ... >expect ) &&
	sed -e ... expect >expected &&

without the mv?

You could even line up the commands in a pipeline, but since the first
one contains a git command, it is better not to do that because breakage
of the git command would not be detected if it is not the last command
in the pipeline.

> +test_expect_success 'rebase --exec without -i shows error message' '
> +	git reset --hard execute &&
> +	test_must_fail git rebase --exec "git show HEAD" HEAD~2 2>actual &&
> +	echo "--exec option must be used with --interactive option\n" >expected &&
> +	test_cmp expected actual

Sooner or later this text will be translated. Therefore:

	test_i18ncmp ...

-- Hannes