hello, comments inline
On Sun, Aug 11, 2013 at 4:56 PM, Stephen Haberman
<[email protected]> wrote:
> If a user is working on master, and has merged in their feature branch, but
> now
> has to "git pull" because master moved, with pull.rebase their feature branch
> will be flattened into master.
>
> This is because "git pull" currently does not know about rebase's preserve
> merges flag, which would avoid this behavior, as it would instead replay just
> the merge commit of the feature branch onto the new master, and not replay
> each
> individual commit in the feature branch.
>
> Add a --rebase=preserve option, which will pass along --preserve-merges to
> rebase.
>
> Also add 'preserve' to the allowed values for the pull.rebase config setting.
>
> Signed-off-by: Stephen Haberman <[email protected]>
> ---
> Hi,
>
> This is v3 of my previous pull.rebase=preserve patch, previously discussed
> here:
>
> http://thread.gmane.org/gmane.comp.version-control.git/232061
> http://thread.gmane.org/gmane.comp.version-control.git/231909
>
> In this version, I addressed all of Eric's excellent feedback.
>
> I believe the patch is much better now, but would still appreciate more
> detailed feedback. In particular, I kind of made up how to handle and
> invalid "--rebase=invalid" value, and the resulting error message.
>
> Also, I changed git-pull's usage to include the -r parameter...not
> sure if that's okay or not. Let me know if not.
>
> Thanks!
>
> Documentation/config.txt | 8 +++++
> Documentation/git-pull.txt | 18 +++++++----
> git-pull.sh | 42 ++++++++++++++++++++----
> t/t5520-pull.sh | 81
> ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 137 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ec57a15..4c22be2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -766,6 +766,10 @@ branch.<name>.rebase::
> "git pull" is run. See "pull.rebase" for doing this in a non
> branch-specific manner.
> +
> + When preserve, also pass `--preserve-merges` along to 'git rebase'
> + so that locally committed merge commits will not be flattened
> + by running 'git pull'.
> ++
> *NOTE*: this is a possibly dangerous operation; do *not* use
> it unless you understand the implications (see linkgit:git-rebase[1]
> for details).
> @@ -1826,6 +1830,10 @@ pull.rebase::
> pull" is run. See "branch.<name>.rebase" for setting this on a
> per-branch basis.
> +
> + When preserve, also pass `--preserve-merges` along to 'git rebase'
> + so that locally committed merge commits will not be flattened
> + by running 'git pull'.
> ++
> *NOTE*: this is a possibly dangerous operation; do *not* use
> it unless you understand the implications (see linkgit:git-rebase[1]
> for details).
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 6ef8d59..beea10b 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -102,12 +102,18 @@ include::merge-options.txt[]
> :git-pull: 1
>
> -r::
> ---rebase::
> - Rebase the current branch on top of the upstream branch after
> - fetching. If there is a remote-tracking branch corresponding to
> - the upstream branch and the upstream branch was rebased since last
> - fetched, the rebase uses that information to avoid rebasing
> - non-local changes.
> +--rebase[=false|true|preserve]::
> + When true, rebase the current branch on top of the upstream
> + branch after fetching. If there is a remote-tracking branch
> + corresponding to the upstream branch and the upstream branch
> + was rebased since last fetched, the rebase uses that information
> + to avoid rebasing non-local changes.
> ++
> +When preserve, also rebase the current branch on top of the upstream
> +branch, but pass `--preserve-merges` along to `git rebase` so that
> +locally created merge commits will not be flattened.
> ++
> +When false, merge the current branch into the upstream branch.
> +
> See `pull.rebase`, `branch.<name>.rebase` and `branch.autosetuprebase` in
> linkgit:git-config[1] if you want to make `git pull` always use
> diff --git a/git-pull.sh b/git-pull.sh
> index f0df41c..78ad52d 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -4,7 +4,7 @@
> #
> # Fetch one or more remote refs and merge it/them into the current HEAD.
>
> -USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-s
> strategy]... [<fetch-options>] <repo> <head>...'
> +USAGE='[-n | --no-stat] [--[no-]commit] [--[no-]squash] [--[no-]ff] [-r
> [true|false|preserve]] [-s strategy]... [<fetch-options>] <repo> <head>...'
> LONG_USAGE='Fetch one or more remote refs and integrate it/them with the
> current HEAD.'
> SUBDIRECTORY_OK=Yes
> OPTIONS_SPEC=
> @@ -40,13 +40,13 @@ test -f "$GIT_DIR/MERGE_HEAD" && die_merge
>
> strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
> log_arg= verbosity= progress= recurse_submodules= verify_signatures=
> -merge_args= edit=
> +merge_args= edit= rebase_args=
> curr_branch=$(git symbolic-ref -q HEAD)
> curr_branch_short="${curr_branch#refs/heads/}"
> -rebase=$(git config --bool branch.$curr_branch_short.rebase)
> +rebase=$(git config branch.$curr_branch_short.rebase)
> if test -z "$rebase"
> then
> - rebase=$(git config --bool pull.rebase)
> + rebase=$(git config pull.rebase)
> fi
> dry_run=
> while :
> @@ -110,8 +110,27 @@ do
> esac
> merge_args="$merge_args$xx "
> ;;
> + -r=*|--r=*|--re=*|--reb=*|--reba=*|--rebas=*|--rebase=*|\
> -r|--r|--re|--reb|--reba|--rebas|--rebase)
> - rebase=true
> + case "$#,$1" in
> + *,*=*)
> + rebase="${1#*=}"
> + ;;
> + 1,*)
> + rebase=true
> + ;;
> + *)
> + # if the user typed 'git pull -r . copy', don't treat
> '.'
> + # as an argument to -r
> + if test true = "$2" || test false = "$2" || test
> preserve = "$3"
> + then
> + rebase="$2"
> + shift
> + else
> + rebase=true
> + fi
1. i'm not sure why you are testing $3 == preserve. it looks like a typo
2. clearer than a string of yoda conditions:
case $2 in
true|false|preserve)
rebase=$2
shift
;;
*)
rebase=true
;;
esac
> + ;;
> + esac
> ;;
> --no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
> rebase=false
> @@ -145,6 +164,17 @@ do
> shift
> done
>
> +if test preserve = "$rebase"
> +then
> + rebase=true
> + rebase_args=--preserve-merges
> +elif test ! -z "$rebase" && test false != "$rebase" && test true != "$rebase"
> +then
> + echo "Invalid value for --rebase, should be true, false, or preserve"
> + usage
> + exit 1
> +fi
> +
1. in the error message you say that rebase should be a trystate of
true, false, or preserve. why then do you allow $rebase == '' ?
2. clearer than a string of yoda conditions:
case $rebase in
preserve)
rebase_args=--preserve-merges
rebase=true
;;
true|false)
;;
*)
echo "Invalid value for --rebase, should be true, false, or preserve" >&2
usage
exit 1
esac
> error_on_no_merge_candidates () {
> exec >&2
> for opt
> @@ -292,7 +322,7 @@ fi
> merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
> case "$rebase" in
> true)
> - eval="git-rebase $diffstat $strategy_args $merge_args $verbosity"
> + eval="git-rebase $diffstat $strategy_args $merge_args $rebase_args
> $verbosity"
> eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
> ;;
> *)
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index ed4d9c8..8be0482 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -148,6 +148,87 @@ test_expect_success 'branch.to-rebase.rebase should
> override pull.rebase' '
> test new = $(git show HEAD:file2)
> '
>
> +# add a feature branch, keep-merge, that is merged into master, so the
> +# test can try preserving the merge commit (or not) with various
> +# --rebase flags/pull.rebase settings.
> +test_expect_success 'preserve merge setup' '
> + git reset --hard before-rebase &&
> + git checkout -b keep-merge second^ &&
> + test_commit file3 &&
> + git checkout to-rebase &&
> + git merge keep-merge &&
> + git tag before-preserve-rebase
> +'
> +
> +test_expect_success 'pull.rebase=false create a new merge commit' '
> + git reset --hard before-preserve-rebase &&
> + test_config pull.rebase false &&
> + git pull . copy &&
> + test $(git rev-parse HEAD^1) = $(git rev-parse
> before-preserve-rebase) &&
> + test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
> + test file3 = $(git show HEAD:file3.t)
> +'
> +
> +test_expect_success 'pull.rebase=true flattens keep-merge' '
> + git reset --hard before-preserve-rebase &&
> + test_config pull.rebase true &&
> + git pull . copy &&
> + test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
> + test file3 = $(git show HEAD:file3.t)
> +'
> +
> +test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
> + git reset --hard before-preserve-rebase &&
> + test_config pull.rebase preserve &&
> + git pull . copy &&
> + test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
> + test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
> +'
> +
> +test_expect_success 'pull.rebase=invalid fails' '
> + git reset --hard before-preserve-rebase &&
> + test_config pull.rebase invalid &&
> + ! git pull . copy
> +'
> +
> +test_expect_success '--rebase=false create a new merge commit' '
> + git reset --hard before-preserve-rebase &&
> + test_config pull.rebase true &&
> + git pull --rebase=false . copy &&
> + test $(git rev-parse HEAD^1) = $(git rev-parse
> before-preserve-rebase) &&
> + test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
> + test file3 = $(git show HEAD:file3.t)
> +'
> +
> +test_expect_success '--rebase=true rebases and flattens keep-merge' '
> + git reset --hard before-preserve-rebase &&
> + test_config pull.rebase preserve &&
> + git pull --rebase=true . copy &&
> + test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
> + test file3 = $(git show HEAD:file3.t)
> +'
> +
> +test_expect_success '--rebase=preserve rebases and merges keep-merge' '
> + git reset --hard before-preserve-rebase &&
> + test_config pull.rebase true &&
> + git pull --rebase=preserve . copy &&
> + test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
> + test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
> +'
> +
> +test_expect_success '--rebase=invalid fails' '
> + git reset --hard before-preserve-rebase &&
> + ! git pull --rebase=invalid . copy
> +'
> +
> +test_expect_success '--rebase overrides pull.rebase=preserve and flattens
> keep-merge' '
> + git reset --hard before-preserve-rebase &&
> + test_config pull.rebase preserve &&
> + git pull --rebase . copy &&
> + test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
> + test file3 = $(git show HEAD:file3.t)
> +'
> +
> test_expect_success '--rebase with rebased upstream' '
>
> git remote add -f me . &&
> --
> 1.8.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
