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

2014-07-10 Thread Junio C Hamano
Fabian Ruch  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 
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-07-02 Thread Fabian Ruch
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`.

Signed-off-by: Fabian Ruch 
---
 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
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html