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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2012, 17:42 UTC
Message-ID: <7vhaurt1m6.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1338817674-22877-1-git-send-email-Lucien.Kong@ensimag.imag.fr>

Kong Lucien <Lucien.Kong@ensimag.imag.fr> writes:

Show 19 quoted lines

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 147fa1a..96dbf26 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -210,6 +210,17 @@ rebase.autosquash::
>  
>  OPTIONS
>  -------
> +<cmd>::
> +	Shell command executed between each commit applications. The
> +	--exec option has to be specified.
> ++
> +You may execute several commands between each commit applications.
> +Therefore, you can use one instance of exec:
> +	git rebase -i --exec "cmd1; cmd2; ...".
> +You can also insert several instances of exec, if you wish to
> +only have one command per line for example:
> +	git rebase -i --exec "cmd1" --exec "cmd2" ...
> +

The description of this and exisitng <newbase> look very odd. The usual way to describe an option that take a parameter in the OPTIONS section is to instead do:

	--onto <newbase>::
		Description of what --onto option does and how <newbase>
		is used in what it does.

	--exec <cmd>::
		Description of what --exec option does, and how	<cmd> 
                is used in what it does.

Show 11 quoted lines

> @@ -336,6 +347,13 @@ link:howto/revert-a-faulty-me
>  	user edit that list before rebasing.  This mode can also be used to
>  	split commits (see SPLITTING COMMITS below).
>  
> +-x::
> +--exec::
> +	Automatically add "exec" followed by <cmd> between each commit
> +	applications (see INTERACTIVE MODE below).
> ++
> +This has to be used along with the `--interactive` option explicitly.
> +

OK.

> @@ -521,6 +539,27 @@ in `$SHELL`, or the default shell if
> ...
> +If the option '-i' is missing, The command will return the usage page
> +of "git rebase". Same if there is no <cmd> specified behind --exec.

OK.

Show 20 quoted lines

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0c19b7c..7444160 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -876,6 +876,19 @@ cat >> "$todo" << EOF
>  #
>  EOF
>  
> +if test -n "$cmd"
> +then
> +	OIFS=$IFS
> +	IFS=','
> +	for i in $cmd
> +	do
> +		sed "/^pick .*/aexec $i" "$todo" >tmp
> +		cat tmp >"$todo"
> +	done
> +	rm tmp
> +	IFS=$OIFS
> +fi

Where does this "we split at comma", implying "the user is forbidden to have a comma in the executed command and its arguments" come from?

A more conventional way that would be less troublesome in the real life is to use LF instead in a case like this (and git-rebase.sh already defines it for exactly a case like this).

I am not sure if that "aexec" is portable outside GNUism, though. Traditionally the a(ppend) command is spelled like this:

	[1addr]a\
        text

to write one line (cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html#tag_20_116_13_03).

Why is "pick" special? Don't you want to run the commands after "edit", "reword", "squash", "fixup"?

This should come immediately after autosquash rearranged the insns in the $todo file, before the help text is appended to it.

Show 12 quoted lines

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 24a2840..a8b1793 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -76,6 +78,7 @@ If you would prefer to skip this patch, instead run \"git rebase --skip\".
>  To check out the original branch and stop rebasing run \"git rebase --abort\".
>  "
>  unset onto
> +unset cmd
>  strategy=
>  strategy_opts=
>  do_merge=

You copied badness from the existing "unset onto"; I do not think the rest of the code (and especially the new code you add) treats a variable that is not set and set to an empty string any different, and setting it to empty is the right thing to do in such a case.

Show 6 quoted lines

> @@ -219,6 +222,24 @@ do
>  		onto="$2"
>  		shift
>  		;;
> +	-x)
> +		exec_flag=true

exec_flag is not cleared at the beginning of the script, so if the command is run without "-x" on the command line, it can have whatever garbage value that was in the environment. You know that $cmd is empty if "-x" was not given, and you can make sure $cmd becomes non-empty when "-x" is given, so remove use of this variable and replace any 'test -n "$exec_flag"' with 'test -n "$cmd"'.

> +		test 3 -le "$#" || usage

Why 3? You only care about the presense of $2 you are about to read, no?

Show 15 quoted lines

> +		if orig_head=$(git rev-parse -q --verify "$2") ||
> +		   test `expr substr "$2" 1 1` = -
> +		then
> +			echo "You must specify a command after --exec option\n"
> +			usage
> +		else
> +			if test -n "$cmd"
> +			then
> +				cmd="$2,$cmd"
> +			else
> +				cmd="$2"
> +			fi
> +		fi
> +		shift
> +		;;

Are you telling me that I cannot have a branch named "make"? Drop this nonsense, and just write it in a more straightforward way, e.g.

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

Show 10 quoted lines

>  	-i)
>  		interactive_rebase=explicit
>  		;;
> @@ -304,6 +325,13 @@ do
>  done
>  test $# -gt 2 && usage
>  
> +if test -n "$exec_flag" &&
> +   test -z "$interactive_rebase"
> +then

I think this should be

	if test -n "$cmd" && test "$interactive_rebase" != explicit
        then

Show 8 quoted lines

> @@ -348,7 +376,6 @@ abort)
>  	exit
>  	;;
>  esac
> -
>  # Make sure no rebase is in progress
>  if test -n "$in_progress"
>  then

Why?