Re: [PATCH/RFC] rebase: new convenient option to edit/reword/delete a single commit
On 03/09/2014 03:49 AM, Nguyễn Thái Ngọc Duy wrote:
Show 10 quoted lines
> Prepare the todo list for you to edit/reword/delete the given commit.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Allowing multiple actions is a bit too much for my shell skills. I
> don't really need it so I won't push it, but if somebody gives me a
> sketch, I'll try to polish it.
>
> --squash and --fixup would require two commits, making it a bit
> awkward in option handling. Or is the fixup/squash always HEAD?
These commands always squash/fixup the indicated commit with the previous one. I think the same approach that you use below should work for these commands, too.
Show 30 quoted lines
> Documentation/git-rebase.txt | 11 +++++++++++
> git-rebase--interactive.sh | 17 ++++++++++++++---
> git-rebase.sh | 22 +++++++++++++++++++++-
> 3 files changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 2a93c64..becb749 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -13,6 +13,7 @@ SYNOPSIS
> 'git rebase' [-i | --interactive] [options] [--exec <cmd>] [--onto <newbase>]
> --root [<branch>]
> 'git rebase' --continue | --skip | --abort | --edit-todo
> +'git rebase' [--edit | -E | --reword | -R | --delete | -D ] <commit-ish>
>
> DESCRIPTION
> -----------
> @@ -356,6 +357,16 @@ unless the `--fork-point` option is specified.
> user edit that list before rebasing. This mode can also be used to
> split commits (see SPLITTING COMMITS below).
>
> +-E=<commit>::
> +--edit=<commit>::
> +-R=<commit>::
> +--reword=<commit>::
> +-D=<commit>::
> +--delete=<commit>::
> + Prepare the todo list to edit or reword or delete the
> + specified commit. Configuration variable `rebase.autostash` is
> + ignored.
If I understand correctly, when one of these options is used, the editor is not presented to the user at all. If so, then it is probably confusing to emphasize "the todo list", because the user will never see it. How about
Edit, reword, or delete the specified commit, replaying subsequent
commits on top of it (like running `git rebase --interactive
commit^` and then changing the command on the line containing
commit). If conflicts arise when replaying the later commits,
resolve them and run "git rebase --continue", as usual. The
configuration variable `rebase.autosquash` is ignored when these
options are used.Show 19 quoted lines
> +
> -p::
> --preserve-merges::
> Instead of ignoring merges, try to recreate them.
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index a1adae8..3ded4c7 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1040,9 +1040,20 @@ fi
> has_action "$todo" ||
> die_abort "Nothing to do"
>
> -cp "$todo" "$todo".backup
> -git_sequence_editor "$todo" ||
> - die_abort "Could not execute editor"
> +if test -n "$one_action"
> +then
> + commit="`git rev-parse --short $one_commit`"
> + sed "1s/pick $commit /$one_action $commit /" "$todo" > "$todo.new" ||
It wouldn't hurt to anchor this pattern at the beginning of the line. I understand that it wouldn't help, either (assuming everything else is working right), but it makes the intention clearer.
> + die_abort "failed to update todo list"
> + grep "^$one_action $commit " "$todo.new" >/dev/null ||
> + die_abort "expected to find '$one_action $commit' line but did not"
The die_aborts above is really an internal consistency check, right? If so, maybe it should start with "internal error:" so that the user doesn't think that he has done something wrong.
Show 37 quoted lines
> + mv "$todo.new" "$todo" ||
> + die_abort "failed to update todo list"
> +else
> + cp "$todo" "$todo".backup
> + git_sequence_editor "$todo" ||
> + die_abort "Could not execute editor"
> +fi
>
> has_action "$todo" ||
> die_abort "Nothing to do"
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 5f6732b..2acffb4 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -32,6 +32,9 @@ verify allow pre-rebase hook to run
> rerere-autoupdate allow rerere to update index with resolved conflicts
> root! rebase all reachable commits up to the root(s)
> autosquash move commits that begin with squash!/fixup! under -i
> +E,edit=! generate todo list to edit this commit
> +R,reword=! generate todo list to reword this commit's message
> +D,delete=! generate todo list to delete this commit
> committer-date-is-author-date! passed to 'git am'
> ignore-date! passed to 'git am'
> whitespace=! passed to 'git apply'
> @@ -228,6 +231,7 @@ then
> fi
> test -n "$type" && in_progress=t
>
> +one_action=
> total_argc=$#
> while test $# != 0
> do
> @@ -290,6 +294,7 @@ do
> ;;
> --autostash)
> autostash=true
> + explicit_autosquash=t
Should that be "explicit_autostash"?
Show 14 quoted lines
> ;;
> --verbose)
> verbose=t
> @@ -335,6 +340,13 @@ do
> --gpg-sign=*)
> gpg_sign_opt="-S${1#--gpg-sign=}"
> ;;
> + --edit=*|--reword=*|--delete=*)
> + test -n "$one_action" && die "$(gettext "--edit, --reword or --delete cannot be used multiple times")"
> + interactive_rebase=explicit
> + one_action="${1%=*}"
> + one_action="${one_action#--}"
> + one_commit="${1#--*=}"
> + ;;Is "delete" a valid todo-list command? I would have thought that you would change the command to "#pick" in the case of "--delete".
Show 24 quoted lines
> --)
> shift
> break
> @@ -342,6 +354,7 @@ do
> esac
> shift
> done
> +test -n "$one_action" && test $# -gt 0 && usage
> test $# -gt 2 && usage
>
> if test -n "$cmd" &&
> @@ -438,7 +451,14 @@ else
> state_dir="$apply_dir"
> fi
>
> -if test -z "$rebase_root"
> +if test -n "$one_action"
> +then
> + upstream_name="$one_commit^"
> + upstream=$(peel_committish "${upstream_name}") ||
> + die "$(eval_gettext "invalid upstream \$upstream_name")"
> + upstream_arg="$upstream_name"
> + test -n "$explicit_autosquash" || autosquash=
> +elif test -z "$rebase_root"It would be nice if these options (though not --squash and --fixup) would support editing the root commit. The logic would be similar to the code in the "else" branch of this "if" chain.
> then
> case "$#" in
> 0)
>
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Nguyễn Thái Ngọc Duy
