threads / rfc / 12431

RFC patchgit-merge.sh: better handling of combined --squash,--no-ff,--no-commit options

Subject: [PATCH/RFC] git-merge.sh: better handling of combined --squash,--no-ff,--no-commit options

## tl;dr

3 messages between Mar 2, 2008 and Mar 3, 2008. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Gerrit Pape· Mar 2, 2008, 17:58 UTC · lore

git-merge used to use either the --squash,--no-squash, --no-ff,--ff, --no-commit,--commit option, whichever came last in the command line. This lead to some un-intuitive behavior, having

 git merge --no-commit --no-ff <branch>

actually commit the merge. Now git-merge respects --no-commit together with --no-ff, as well as other combinations of the options. However, this broke a selftest in t/t7600-merge.sh which expected to have --no-ff completely override the --squash option, so that

 git merge --squash --no-ff <branch>

fast-forwards, and makes a merge commit; now it prepares a squash commit. Combining --squash with --no-ff doesn't seem to make sense though, so the test is adapted to test --no-ff without the preceding --squash.

The unexpected behavior was reported by John Goerzen through
 http://bing.sdebian.org/468568
Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 git-merge.sh     |   13 +++++++------
 t/t7600-merge.sh |    1 +
 2 files changed, 8 insertions(+), 6 deletions(-)
Show changes to 2 files +8 −6

git-merge.sh, t/t7600-merge.sh

diff --git a/git-merge.sh b/git-merge.sh
index 1c123a3..2a5c456 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -37,6 +37,7 @@ use_strategies=
 
 allow_fast_forward=t
 allow_trivial_merge=t
+squash= no_commit=
 
 dropsave() {
 	rm -f -- "$GIT_DIR/MERGE_HEAD" "$GIT_DIR/MERGE_MSG" \
@@ -152,17 +153,17 @@ parse_config () {
 		--summary)
 			show_diffstat=t ;;
 		--squash)
-			allow_fast_forward=t squash=t no_commit=t ;;
+			squash=t no_commit=t ;;
 		--no-squash)
-			allow_fast_forward=t squash= no_commit= ;;
+			squash= no_commit= ;;
 		--commit)
-			allow_fast_forward=t squash= no_commit= ;;
+			no_commit= ;;
 		--no-commit)
-			allow_fast_forward=t squash= no_commit=t ;;
+			no_commit=t ;;
 		--ff)
-			allow_fast_forward=t squash= no_commit= ;;
+			allow_fast_forward=t ;;
 		--no-ff)
-			allow_fast_forward=false squash= no_commit= ;;
+			allow_fast_forward=f ;;
 		-s|--strategy)
 			shift
 			case " $all_strategies " in
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 50c51c8..085f55f 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -419,6 +419,7 @@ test_debug 'gitk --all'
 
 test_expect_success 'merge c0 with c1 (no-ff)' '
 	git reset --hard c0 &&
+	git config branch.master.mergeoptions "" &&
 	test_tick &&
 	git merge --no-ff c1 &&
 	verify_merge file result.1 &&

-- 
1.5.4.3
Junio C Hamano· Mar 2, 2008, 23:50 UTC · re: Gerrit Pape · lore

Re: [PATCH/RFC] git-merge.sh: better handling of combined --squash,--no-ff,--no-commit options

Gerrit Pape <pape@smarden.org> writes:
Show 14 quoted lines
> git-merge used to use either the --squash,--no-squash, --no-ff,--ff,
> --no-commit,--commit option, whichever came last in the command line.
> This lead to some un-intuitive behavior, having
>
>  git merge --no-commit --no-ff <branch>
>
> actually commit the merge.  Now git-merge respects --no-commit together
> with --no-ff, as well as other combinations of the options.  However,
> this broke a selftest in t/t7600-merge.sh which expected to have --no-ff
> completely override the --squash option, so that
>
>  git merge --squash --no-ff <branch>
>
> fast-forwards, and makes a merge commit; now it prepares a squash ...

Both make sense when they make sense (i.e. if you and the other side are not fast-forward nor up-to-date and need a real merge).

> ...  Combining --squash with --no-ff doesn't seem to make sense

Yeah, I think forbidding this combination would make much more sense. The former asks there be _no_ merge (the user does not want to have a merge ever), while the other one asks to create a merge even when there is no need to (the user does want a merge).

Are there other combinations that we should forbid?
Gerrit Pape· Mar 3, 2008, 09:22 UTC · re: Junio C Hamano · lore

[PATCH] git-merge.sh: better handling of combined --squash,--no-ff,--no-commit options

git-merge used to use either the --squash,--no-squash, --no-ff,--ff, --no-commit,--commit option, whichever came last in the command line. This lead to some un-intuitive behavior, having

 git merge --no-commit --no-ff <branch>

actually commit the merge. Now git-merge respects --no-commit together with --no-ff, as well as other combinations of the options. However, this broke a selftest in t/t7600-merge.sh which expected to have --no-ff completely override the --squash option, so that

 git merge --squash --no-ff <branch>

fast-forwards, and makes a merge commit; combining --squash with --no-ff doesn't really make sense though, and is now refused by git-merge. The test is adapted to test --no-ff without the preceding --squash, and another test is added to make sure the --squash --no-ff combination is refused.

The unexpected behavior was reported by John Goerzen through
 http://bing.sdebian.org/468568
Signed-off-by: Gerrit Pape <pape@smarden.org>
---
On Sun, Mar 02, 2008 at 03:50:46PM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> Gerrit Pape <pape@smarden.org> writes:
> > ...  Combining --squash with --no-ff doesn't seem to make sense
> Yeah, I think forbidding this combination would make much more sense.  The
> former asks there be _no_ merge (the user does not want to have a merge
> ever), while the other one asks to create a merge even when there is no
> need to (the user does want a merge).
Okay.
> Are there other combinations that we should forbid?

I don't think so, it's just --squash --no-ff, and maybe --squash --ff, but --ff is the default anyway.

The combination --squash --commit doesn't work with the current implementation though, but it might make sense to have the squash committed automatically?

 git-merge.sh     |   17 +++++++++++------
 t/t7600-merge.sh |    6 ++++++
 2 files changed, 17 insertions(+), 6 deletions(-)
Show changes to 2 files +17 −6

git-merge.sh, t/t7600-merge.sh

diff --git a/git-merge.sh b/git-merge.sh
index 1c123a3..03cd398 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -37,6 +37,7 @@ use_strategies=
 
 allow_fast_forward=t
 allow_trivial_merge=t
+squash= no_commit=
 
 dropsave() {
 	rm -f -- "$GIT_DIR/MERGE_HEAD" "$GIT_DIR/MERGE_MSG" \
@@ -152,17 +153,21 @@ parse_config () {
 		--summary)
 			show_diffstat=t ;;
 		--squash)
-			allow_fast_forward=t squash=t no_commit=t ;;
+			test "$allow_fast_forward" = t ||
+				die "You cannot combine --squash with --no-ff."
+			squash=t no_commit=t ;;
 		--no-squash)
-			allow_fast_forward=t squash= no_commit= ;;
+			squash= no_commit= ;;
 		--commit)
-			allow_fast_forward=t squash= no_commit= ;;
+			no_commit= ;;
 		--no-commit)
-			allow_fast_forward=t squash= no_commit=t ;;
+			no_commit=t ;;
 		--ff)
-			allow_fast_forward=t squash= no_commit= ;;
+			allow_fast_forward=t ;;
 		--no-ff)
-			allow_fast_forward=false squash= no_commit= ;;
+			test "$squash" != t ||
+				die "You cannot combine --squash with --no-ff."
+			allow_fast_forward=f ;;
 		-s|--strategy)
 			shift
 			case " $all_strategies " in
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 50c51c8..5d16628 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -419,6 +419,7 @@ test_debug 'gitk --all'
 
 test_expect_success 'merge c0 with c1 (no-ff)' '
 	git reset --hard c0 &&
+	git config branch.master.mergeoptions "" &&
 	test_tick &&
 	git merge --no-ff c1 &&
 	verify_merge file result.1 &&
@@ -427,6 +428,11 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'combining --squash and --no-ff is refused' '
+	test_must_fail git merge --squash --no-ff c1 &&
+	test_must_fail git merge --no-ff --squash c1
+'
+
 test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
 	git reset --hard c0 &&
 	git config branch.master.mergeoptions "--no-ff" &&

-- 
1.5.4.3

← back to recent threads