From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Mon, 04 Jun 2012 17:48:50 GMT
Subject: Re: [PATCH] rebase [-i --exec | -ix] <CMD>...
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:

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

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

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

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