Re: [PATCH/RFC] rebase: new convenient option to edit/reword/delete a single commit

[Michael Haggerty <mhagger@alum.mit.edu>]

On 03/09/2014 03:49 AM, Nguyễn Thái Ngọc Duy wrote:
> 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.

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

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

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

>  		;;
>  	--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".

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

Cheers,
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/