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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 4, 2012, 17:48 UTC
Message-ID: <vpqk3znrmrh.fsf@bauges.imag.fr>
In-Reply-To: <1338817674-22877-1-git-send-email-Lucien.Kong@ensimag.imag.fr>

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

Show 21 quoted lines

> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -8,9 +8,9 @@ git-rebase - Forward-port local commits to the updated upstream head
>  SYNOPSIS
>  --------
>  [verse]
> -'git rebase' [-i | --interactive] [options] [--onto <newbase>]
> +'git rebase' [-i | --interactive] [options] [--exec <cmd>] [--onto <newbase>]
>  	[<upstream>] [<branch>]
> -'git rebase' [-i | --interactive] [options] --onto <newbase>
> +'git rebase' [-i | --interactive] [options] [--exec <cmd>] --onto <newbase>
>  	--root [<branch>]
>  'git rebase' --continue | --skip | --abort
>  
> @@ -210,6 +210,17 @@ rebase.autosquash::
>  
>  OPTIONS
>  -------
> +<cmd>::
> +	Shell command executed between each commit applications. The
> +	--exec option has to be specified.

[...]

> +-x::
> +--exec::

That seems weird to have --exec <cmd> specification split into one for --exec, and another for <cmd> ...

> +You may execute several commands between each commit applications.
> +Therefore, you can use one instance of exec:
> +	git rebase -i --exec "cmd1; cmd2; ...".

s/Therefore/For this/ ?

(Therefore = donc in French)

Show 10 quoted lines

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

Isn't this executed from the top of the workdir? What if the user already has a file named tmp there?

Show 6 quoted lines

> +			if test -n "$cmd"
> +			then
> +				cmd="$2,$cmd"
> +			else
> +				cmd="$2"
> +			fi

What happens when <cmd> contains a comma (e.g. --exec "rm file,with,commas,in,name.txt") ?

If you don't allow this case, then you should error out instead of executing a weird behavior silently. Or you can escape the comma and unescape it later, but that may be overkill.

Show 6 quoted lines

> +if test -n "$exec_flag" &&
> +   test -z "$interactive_rebase"
> +then
> +	echo "--exec option must be used with --interactive option\n"
> +	usage
> +fi

I'd even skip the "usage" here, as the error message is clear enough (and is much less visible before a page of "usage" output). Just die-ing would be better I think.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/