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

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