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

From: Johannes Sixt <j6t@kdbg.org>
Date: Jun 7, 2012, 08:40 UTC
Message-ID: <4FD06906.1080007@kdbg.org>
In-Reply-To: <1338978856-26838-1-git-send-email-Lucien.Kong@ensimag.imag.fr>

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.

Show 9 quoted lines

> +# 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
> +		;;

Show 8 quoted lines

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

Show 5 quoted lines

> +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