On 06/19/2014 05:28 AM, Fabian Ruch wrote:
> `pick_one` and `pick_one_preserving_merges` are wrappers around
> `cherry-pick` in `rebase --interactive`. They take the hash of a commit
> and build a `cherry-pick` command line that
>
> - respects the user-supplied merge options
> - disables complaints about empty commits
> - tries to fast-forward the rebase head unless rebase is forced
> - suppresses output unless the user requested higher verbosity
> - rewrites merge commits to point to their rebased parents.
>
> `pick_one` is used to implement not only `pick` but also `squash`, which
> amends the previous commit rather than creating a new one. When
> `pick_one` is called from `squash`, it receives a second argument `-n`.
> This tells `pick_one` to apply the changes to the index without
> committing them. Since the argument is almost directly passed to
> `cherry-pick`, we might want to do the same with other `cherry-pick`
> options. Currently, `pick_one` expects `-n` to be the first and only
> argument except for the commit hash.
>
> Prepare `pick_one` for additional `cherry-pick` options by allowing `-n`
> to appear anywhere before the commit hash in the argument list. Loop
> over the argument list and pop each handled item until the commit hash
> is the only parameter left on the list. If an option is not supported,
> ignore it and issue a warning on the console. Construct a new arguments
> list `extra_args` of recognized options that shall be passed to
> `cherry-pick` on the command line.
>
> Signed-off-by: Fabian Ruch <bafain@xxxxxxxxx>
> ---
> git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 45 insertions(+), 16 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f267d8b..ea5514e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -237,8 +237,26 @@ git_sequence_editor () {
>
> pick_one () {
> ff=--ff
> + extra_args=
> + while test $# -gt 0
> + do
> + case "$1" in
> + -n)
> + ff=
> + extra_args="$extra_args -n"
> + ;;
> + -*)
> + warn "pick_one: ignored option -- $1"
> + ;;
This is an internal interface, right? I.e., user input isn't being
processed here? If so, then the presence of an unrecognized option is a
bug and it is preferable to "die" here rather than "warn".
The same below and in at least one later commit.
> + *)
> + break
> + ;;
> + esac
> + shift
> + done
> + test $# -ne 1 && die "pick_one: wrong number of arguments"
> + sha1=$1
>
> - case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
> case "$force_rebase" in '') ;; ?*) ff= ;; esac
> output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
>
> @@ -248,24 +266,35 @@ pick_one () {
> fi
>
> test -d "$rewritten" &&
> - pick_one_preserving_merges "$@" && return
> + pick_one_preserving_merges $extra_args $sha1 && return
> output eval git cherry-pick \
> ${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
> - "$strategy_args" $empty_args $ff "$@"
> + "$strategy_args" $empty_args $ff $extra_args $sha1
> }
It might be confusing that extra_args is used both in pick_one and in
pick_one_preserving_merges. Since these are not local variables, the
call to the latter changes the value of the variable in the former. I
don't know if that could be a problem now (can
pick_one_preserving_merges return with a nonzero exit code?) but even if
not, it is a trap for future developers. I recommend giving the two
variables different names.
> pick_one_preserving_merges () {
> fast_forward=t
> - case "$1" in
> - -n)
> - fast_forward=f
> - sha1=$2
> - ;;
> - *)
> - sha1=$1
> - ;;
> - esac
> - sha1=$(git rev-parse $sha1)
> + no_commit=
> + extra_args=
> + while test $# -gt 0
> + do
> + case "$1" in
> + -n)
> + fast_forward=f
> + extra_args="$extra_args -n"
> + no_commit=y
> + ;;
> + -*)
> + warn "pick_one_preserving_merges: ignored option -- $1"
> + ;;
> + *)
> + break
> + ;;
> + esac
> + shift
> + done
> + test $# -ne 1 && die "pick_one_preserving_merges: wrong number of arguments"
> + sha1=$(git rev-parse $1)
>
> if test -f "$state_dir"/current-commit
> then
> @@ -335,7 +364,7 @@ pick_one_preserving_merges () {
> f)
> first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
>
> - if [ "$1" != "-n" ]
> + if test -z "$no_commit"
> then
> # detach HEAD to current parent
> output git checkout $first_parent 2> /dev/null ||
> @@ -344,7 +373,7 @@ pick_one_preserving_merges () {
>
> case "$new_parents" in
> ' '*' '*)
> - test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
> + test -n "$no_commit" && die "Refusing to squash a merge: $sha1"
>
> # redo merge
> author_script_content=$(get_author_ident_from_commit $sha1)
> @@ -365,7 +394,7 @@ pick_one_preserving_merges () {
> *)
> output eval git cherry-pick \
> ${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
> - "$strategy_args" "$@" ||
> + "$strategy_args" $extra_args $sha1 ||
> die_with_patch $sha1 "Could not pick $sha1"
> ;;
> esac
>
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html