threads / patch / 34677

patchpull: Allow pull to preserve merges when rebasing.

Subject: [PATCH] pull: Allow pull to preserve merges when rebasing.

## tl;dr

8 messages between Aug 11, 2013 and Aug 12, 2013. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Stephen Haberman· Aug 11, 2013, 21:26 UTC · lore

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 <stephen@exigencecorp.com>
---
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(-)
Show changes to 4 files +137 −11

Documentation/config.txt, Documentation/git-pull.txt, git-pull.sh, t/t5520-pull.sh

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
+			;;
+		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
+
 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
Andres Perera· Aug 11, 2013, 23:03 UTC · re: Stephen Haberman · lore

Re: [PATCH] pull: Allow pull to preserve merges when rebasing.

hello, comments inline

On Sun, Aug 11, 2013 at 4:56 PM, Stephen Haberman <stephen@exigencecorp.com> wrote:

Show 140 quoted lines
> 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 <stephen@exigencecorp.com>
> ---
> 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
> +                       ;;
> +               *)
Show 9 quoted lines
> +                       # 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
Show 9 quoted lines
> +                       ;;
> +               esac
>                 ;;
>         --no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
>                 rebase=false
> @@ -145,6 +164,17 @@ do
>         shift
>  done
>
Show 11 quoted lines
> +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
Show 112 quoted lines
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Stephen Haberman· Aug 11, 2013, 23:09 UTC · re: Andres Perera · lore

Re: [PATCH] pull: Allow pull to preserve merges when rebasing.

> 1. i'm not sure why you are testing $3 == preserve. it looks like a
> typo
Yes, good catch. I've added a test that fails, and will fix that.
> 2. clearer than a string of yoda conditions:
> 
> case $2 in
> true|false|preserve)
Makes sense, will change.
> 1. in the error message you say that rebase should be a trystate of
> true, false, or preserve. why then do you allow $rebase == '' ?

Because it may be unset, like if the user ran "git pull . copy" and the pull.rebase setting was not set.

> 2. clearer than a string of yoda conditions:
Will change again.

I'll wait to see if I get any more feedback and then will send out another version.

Thanks!
- Stephen
Andres Perera· Aug 11, 2013, 23:31 UTC · re: Stephen Haberman · lore

Re: [PATCH] pull: Allow pull to preserve merges when rebasing.

On Sun, Aug 11, 2013 at 6:39 PM, Stephen Haberman <stephen@exigencecorp.com> wrote:

Show 25 quoted lines
>
>> 1. i'm not sure why you are testing $3 == preserve. it looks like a
>> typo
>
> Yes, good catch. I've added a test that fails, and will fix that.
>
>> 2. clearer than a string of yoda conditions:
>>
>> case $2 in
>> true|false|preserve)
>
> Makes sense, will change.
>
>> 1. in the error message you say that rebase should be a trystate of
>> true, false, or preserve. why then do you allow $rebase == '' ?
>
> Because it may be unset, like if the user ran "git pull . copy" and
> the pull.rebase setting was not set.
>
>> 2. clearer than a string of yoda conditions:
>
> Will change again.
>
> I'll wait to see if I get any more feedback and then will send out
> another version.
i just realized that there are ambiguities:
pull -r (true|false|preserve) foo
there are 2 ways to interpret this:
pull --rebase=(true|false|preserve) foo # pull from remote named foo

pull --rebase (true|false|preserve) foo # pull from remote named (true|false|preserve), branch foo

options with optional operands usually require that the operands be concatenated with the option argument, so that

pull --rebase[=(true|false|preserve)] | -r[(true|false|preserve)]
avoids the ambiguity of
pull --rebase [(true|false|preserve)] | -r [(true|false|preserve)]
1. you can make it a disambiguation by appending ? to the optspec
(according to man git-rev-parse)
2. you could also disambiguate by testing if the argument is a
configured remote and warn the user, but this makes option parsing
inconsistent, requires additional logic, and is overall inelegant
Show 7 quoted lines
>
> Thanks!
>
> - Stephen
>
>
>
Stephen Haberman· Aug 11, 2013, 23:38 UTC · re: Andres Perera · lore

Re: [PATCH] pull: Allow pull to preserve merges when rebasing.

Hi Andres,
> i just realized that there are ambiguities:
> pull --rebase (true|false|preserve) foo # pull from remote named
> (true|false|preserve), branch foo
Yeah.

Right now, I did the latter. Around line 125, when parsing "--rebase <somearg>", we accept <somearg> only if it's true, false, or preserve, and shift it off. Otherwise we leave it alone and assume it's a remote name.

Without this logic, t5520 fails because it uses "git pull --rebase . copy", which, as you noted, is ambiguous, so "." was showing up as the rebase argument.

So, this is technically handled right now, but I'm fine removing the ambiguous "--rebase true|false|preserve" option if that is what is preferred.

- Stephen
Junio C Hamano· Aug 12, 2013, 05:40 UTC · re: Andres Perera · lore

Re: [PATCH] pull: Allow pull to preserve merges when rebasing.

Andres Perera <andres.p@zoho.com> writes:
Show 13 quoted lines
> i just realized that there are ambiguities:
>
> pull -r (true|false|preserve) foo
>
> there are 2 ways to interpret this:
>
> pull --rebase=(true|false|preserve) foo # pull from remote named foo
>
> pull --rebase (true|false|preserve) foo # pull from remote named
> (true|false|preserve), branch foo
>
> options with optional operands usually require that the operands be
> concatenated with the option argument.
Yes.  This command line option should be like this:
 - "--rebase" and "--no-rebase" are accepted as "true" and "false";
 - "--rebase=preserve" should be the _only_ way to spell the new
   mode of operation (if we were to add "--rebase=interactive"
   later, that should follow suit); and
 - "--rebase=true" and "--rebase=false" is nice to have for
   consistency.
Thanks.
Junio C Hamano· Aug 12, 2013, 07:00 UTC · re: Junio C Hamano · lore

Re: [PATCH] pull: Allow pull to preserve merges when rebasing.

Junio C Hamano <gitster@pobox.com> writes:
Show 28 quoted lines
> Andres Perera <andres.p@zoho.com> writes:
>
>> i just realized that there are ambiguities:
>>
>> pull -r (true|false|preserve) foo
>>
>> there are 2 ways to interpret this:
>>
>> pull --rebase=(true|false|preserve) foo # pull from remote named foo
>>
>> pull --rebase (true|false|preserve) foo # pull from remote named
>> (true|false|preserve), branch foo
>>
>> options with optional operands usually require that the operands be
>> concatenated with the option argument.
>
> Yes.  This command line option should be like this:
>
>  - "--rebase" and "--no-rebase" are accepted as "true" and "false";
>
>  - "--rebase=preserve" should be the _only_ way to spell the new
>    mode of operation (if we were to add "--rebase=interactive"
>    later, that should follow suit); and
>
>  - "--rebase=true" and "--rebase=false" is nice to have for
>    consistency.
>
> Thanks.
Oh, another thing.

How should this interact with 949e0d8e (pull: require choice between rebase/merge on non-fast-forward pull, 2013-06-27) which has been in 'next' and will likely to be one of the earlier topics to graduate to 'master' after 1.8.4 is released?

Stephen Haberman· Aug 12, 2013, 17:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] pull: Allow pull to preserve merges when rebasing.

> How should this interact with 949e0d8e (pull: require choice between
> rebase/merge on non-fast-forward pull, 2013-06-27)

I believe there should not be any conflicts in functionality, other than just tweaking the docs to mention --rebase=preserve as an option.

Personally, I would assert that, for people using a rebase workflow with "git pull", --prebase=preserve should be the default behavior, otherwise they'll be surprised when their feature branches get flattened.

Unfortunately, we can't change the behavior of the naked "--rebase" flag to really mean "--rebase=preserve", but I think that would be ideal. I think it's what people mean they do "git pull". If you want a more raw rebase, they would likely (I think/assume) be running "git rebase" directly.

Nonetheless, thanks for pointing out 949e0d8e, I did not know about it.

Perhaps after that commit graduates to master, I can base this commit on it, and tweak the new docs to suggest --rebase=preserve as the least-surprising behavior.

(Since I'm offering opinions, I think --rebase=preserve would be a great default for "git pull" in 2.0, but please ignore this statement if you've already hashed out the future/2.0 behavior of git pull.)

- Stephen

← back to recent threads