Re: [PATCH 3/3] Make rebase--interactive use OPTIONS_SPEC

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 20, 2008, 13:15 UTC
Message-ID: <alpine.DEB.1.00.0806201407230.6439@racer>
In-Reply-To: <1213929935-15093-4-git-send-email-s-beyer@gmx.net>

Hi,

I like the patch.  Just a couple comments:

On Fri, 20 Jun 2008, Stephan Beyer wrote:

Show 14 quoted lines

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index e8ac2ae..aeb9628 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -10,10 +10,25 @@
>  # The original idea comes from Eric W. Biederman, in
>  # http://article.gmane.org/gmane.comp.version-control.git/22407
>  
> -USAGE='(--continue | --abort | --skip | [--preserve-merges] [--first-parent]
> -	[--preserve-tags] [--verbose] [--onto <branch>] <upstream> [<branch>])'
> +OPTIONS_KEEPDASHDASH=
> +OPTIONS_SPEC='
> +git-rebase -i [options] <upstream> [<branch>]
> +git-rebase (--continue | --abort | --skip)

Here, "-i" is not an error, but not required either, so it should be

+git-rebase [-i] (--continue | --abort | --skip)

> +--
> +continue           Continue rebasing process
> +abort              Abort rebasing process and restore original branch
> +skip               Skip current patch and continue rebasing process

I wonder if these options (which really should be subcommands) should not go below the options, for chronological reasons: you are likely to need the other options before --continue and friends.

> +p,preserve-merges  Try to recreate merges instead of ignoring

s/$/ them/

> +t,preserve-tags    Update tags to the new commit object
> +m,merge            Used anyways (no-op)
> +i,interactive      Used anyways (no-op)

s/anyways/always/

Hmm?

Show 20 quoted lines

> +onto=              Rebase onto given branch instead of upstream
> +v,verbose          Display a diffstat of what changed upstream
> + When preserving merges:
> +f,first-parent     Show only commits following the first parent of each commit
> +s,strategy=        Use the given merge strategy
> +'
>  
> -OPTIONS_SPEC=
>  . git-sh-setup
>  require_work_tree
>  
> @@ -595,32 +611,36 @@ do
>
> [...]
>
> +	--onto)
> +		shift
> +		ONTO=$(git rev-parse --verify "$1") ||
> +			die "Does not point to a valid commit: $1"
> +		;;

You probably need to introduce checks against "git rebase --continue --onto blabla", then.

Show 7 quoted lines

>  	''|-h)
>  		usage
>  		;;
> -	*)
> +	--)
> +		shift
> +		test $# -eq 1 -o $# -eq 2 || usage

I consider this a bugfix.  Thanks.

Show 8 quoted lines

> @@ -720,6 +731,7 @@ EOF
>  			die_abort "Nothing to do"
>  
>  		output git checkout $ONTO && do_rest
> +		exit 0
>  		;;
>  	esac
>  	shift

This exit is not really necessary, is it? Besides, a simple "exit" would be more correct: it reuses the exit status of the most recently executed command, which is what we want here.

Thanks, Dscho