Re: [PATCH RFC v2 15/19] rebase -i: Explicitly distinguish replay commands and exec tasks

[Junio C Hamano <gitster@pobox.com>]

Fabian Ruch <bafain@gmail.com> writes:

> There are two kinds of to-do list commands available. One kind
> replays a commit (`pick`, `reword`, `edit`, `squash` and `fixup` that
> is) and the other executes a shell command (`exec`). We will call the
> first kind replay commands.
>
> The two kinds of tasks are scheduled using different line formats.
> Replay commands expect a commit hash argument following the command
> name and exec concatenates all arguments to assemble a command line.
>
> Adhere to the distinction of formats by not trying to parse the
> `sha1` field unless we are dealing with a replay command. Move the
> replay command handling code to a new function `do_replay` which
> assumes the first argument to be a commit hash and make no more such
> assumptions in `do_next`.

In do_next(), we used read the first line of the insn sheet into
three variables, assuming the common case of handling one of the
replay commands, and (somewhat wastefully) re-read the same line
into two variables when we realize that the command was "exec".

After you split do_replay() out of do_next() with this patch, we
seem to do exactly the same thing.

What exactly is the problem this change wants to fix?

Puzzled...


>
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
>  git-rebase--interactive.sh | 42 ++++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 008f3a0..9de7441 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -585,13 +585,12 @@ do_pick () {
>  	fi
>  }
>  
> -do_next () {
> -	rm -f "$msg" "$author_script" "$amend" || exit
> -	read -r command sha1 rest < "$todo"
> +do_replay () {
> +	command=$1
> +	sha1=$2
> +	rest=$3
> +
>  	case "$command" in
> -	"$comment_char"*|''|noop)
> -		mark_action_done
> -		;;
>  	pick|p)
>  		comment_for_reflog pick
>  
> @@ -656,6 +655,28 @@ do_next () {
>  		esac
>  		record_in_rewritten $sha1
>  		;;
> +	*)
> +		read -r command <"$todo"
> +		warn "Unknown command: $command"
> +		fixtodo="Please fix this using 'git rebase --edit-todo'."
> +		if git rev-parse --verify -q "$sha1" >/dev/null
> +		then
> +			die_with_patch $sha1 "$fixtodo"
> +		else
> +			die "$fixtodo"
> +		fi
> +		;;
> +	esac
> +}
> +
> +do_next () {
> +	rm -f "$msg" "$author_script" "$amend" || exit
> +	read -r command sha1 rest <"$todo"
> +
> +	case "$command" in
> +	"$comment_char"*|''|noop)
> +		mark_action_done
> +		;;
>  	x|"exec")
>  		read -r command rest < "$todo"
>  		mark_action_done
> @@ -695,14 +716,7 @@ do_next () {
>  		fi
>  		;;
>  	*)
> -		warn "Unknown command: $command $sha1 $rest"
> -		fixtodo="Please fix this using 'git rebase --edit-todo'."
> -		if git rev-parse --verify -q "$sha1" >/dev/null
> -		then
> -			die_with_patch $sha1 "$fixtodo"
> -		else
> -			die "$fixtodo"
> -		fi
> +		do_replay $command $sha1 "$rest"
>  		;;
>  	esac
>  	test -s "$todo" && return