diff mbox series

[30/30] subtree: be stricter about validating flags

Message ID 20210423194230.1388945-31-lukeshu@lukeshu.com (mailing list archive)
State Superseded
Headers show
Series subtree: clean up, improve UX | expand

Commit Message

Luke Shumaker April 23, 2021, 7:42 p.m. UTC 
From: Luke Shumaker <lukeshu@datawire.io>

Don't silently ignore a flag that's invalid for a given subcommand.  The
user expected it to do something; we should tell the user that they are
mistaken, instead of surprising the user.

It could be argued that this change might break existing users.  I'd
argue that those existing users are already broken, and they just don't
know it.  Let them know that they're broken.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 contrib/subtree/git-subtree.sh     |  89 ++++++++++++++++-------
 contrib/subtree/t/t7900-subtree.sh | 111 +++++++++++++++++++++++++++++
 2 files changed, 175 insertions(+), 25 deletions(-)

Comments

Danny Lin April 25, 2021, 2:55 a.m. UTC | #1 
>
> From: Luke Shumaker <lukeshu@datawire.io>
>
> Don't silently ignore a flag that's invalid for a given subcommand.  The
> user expected it to do something; we should tell the user that they are
> mistaken, instead of surprising the user.
>
> It could be argued that this change might break existing users.  I'd
> argue that those existing users are already broken, and they just don't
> know it.  Let them know that they're broken.
>
> Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
> ......

Thank you for the recent works, Luke.

I'd like to know if you are aware or working on the issue that branches
before "git subtree add --squash" are pushed via "git subtree push",
which has been reported by me at Tue, 16 Jul 2019 02:30:44 +0800, as I
haven't seen an obvious fix about it in the recent patches in a quick
glance.

Below is the original report (with a slight modification):

-----------------------------------------------------------------------

Say we have a repository with "master" and "sub" branches like below:

    A---B---C---D---E---F  master
               /
          X---Y

    X---Y  sub

where commits A and B include changes in subdirectory "sub", commit C
removes subdirectory "sub", commit D is generated by "git subtree add
-P sub Y", and commits E and F also include changes in subdirectory
"sub".

We'd get this if we run "git subtree push -P sub . sub" on master at F:

    X---Y---E'--F'  sub (after push)

On the other hand, if we have simliar trees except that commit D is
generated by "git subtree add --squash -P sub Y":

    A---B---C---D---E---F  master
               /
              Y'

    X---Y  sub

We'd expect to get this after we run "git subtree push -P sub . sub" on
master at F:

    X---Y---E'--F'  sub (after push, expected)

But actually we get this (in Git 2.22.0):

    A---B---C---D'--E'--F'  sub (after push, actual)
               /
          X---Y

This seems to be a side effect of 2.7.0 -> 2.7.1 in which a change is
made to include merged branches in "git subtree push", but mistakenly
causes branches before "git subtree add --squash" be included.



This bug is especially obvious if we simply run `git subtree push`
after a `git subtree add --squash`:

If master is at commit D, which is generated by
`git subtree add --squash -P sub Y`:

    A---B---C---D  master
               /
              Y'

    X---Y  sub

After running `git subtree push -P sub . sub`, we get:

    A---B---C---D'  sub (after push, actual)
               /
          X---Y

While the sub branch should not change at all!

This bug, seemingly introduced by 933cfeb90b5d03b4096db6d60494a6eedea25d03
critically makes --squash approach for `git subtree` almost unusable.
Luke Shumaker April 26, 2021, 5:35 p.m. UTC | #2 
On Sat, 24 Apr 2021 20:55:17 -0600,
Danny Lin wrote:
> Thank you for the recent works, Luke.
> 
> I'd like to know if you are aware or working on the issue that branches
> before "git subtree add --squash" are pushed via "git subtree push",
> which has been reported by me at Tue, 16 Jul 2019 02:30:44 +0800, as I
> haven't seen an obvious fix about it in the recent patches in a quick
> glance.

I'm not sure if I was aware of it before, but I'll be sure to keep it
in mind when I'm updating my (yet to be submitted) fixes to the
'split' algorithm.

Thanks for the additional test-case!
diff mbox series

Patch

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7361d8de3f..5073d82b2e 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -30,17 +30,6 @@  m,message=    use the given message as the commit message for the merge commit
 
 PATH=$(git --exec-path):$PATH
 
-arg_debug=
-arg_command=
-arg_prefix=
-arg_split_branch=
-arg_split_onto=
-arg_split_rejoin=
-arg_split_ignore_joins=
-arg_split_annotate=
-arg_addmerge_squash=
-arg_addmerge_message=
-
 indent=0
 
 # Usage: debug [MSG...]
@@ -77,10 +66,61 @@  main () {
 	then
 		set -- -h
 	fi
-	eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
+	set_args="$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
+	eval "$set_args"
 	. git-sh-setup
 	require_work_tree
 
+	# First figure out the command and whether we use --rejoin, so
+	# that we can provide more helpful validation when we do the
+	# "real" flag parsing.
+	arg_split_rejoin=
+	allow_split=
+	allow_addmerge=
+	while test $# -gt 0
+	do
+		opt="$1"
+		shift
+		case "$opt" in
+			--annotate|-b|-P|-m|--onto)
+				shift
+				;;
+			--rejoin)
+				arg_split_rejoin=1
+				;;
+			--no-rejoin)
+				arg_split_rejoin=
+				;;
+			--)
+				break
+				;;
+		esac
+	done
+	arg_command=$1
+	case "$arg_command" in
+	add|merge|pull)
+		allow_addmerge=1
+		;;
+	split|push)
+		allow_split=1
+		allow_addmerge=$arg_split_rejoin
+		;;
+	*)
+		die "Unknown command '$arg_command'"
+		;;
+	esac
+	# Reset the arguments array for "real" flag parsing.
+	eval "$set_args"
+
+	# Begin "real" flag parsing.
+	arg_debug=
+	arg_prefix=
+	arg_split_branch=
+	arg_split_onto=
+	arg_split_ignore_joins=
+	arg_split_annotate=
+	arg_addmerge_squash=
+	arg_addmerge_message=
 	while test $# -gt 0
 	do
 		opt="$1"
@@ -94,13 +134,16 @@  main () {
 			arg_debug=1
 			;;
 		--annotate)
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_split_annotate="$1"
 			shift
 			;;
 		--no-annotate)
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_split_annotate=
 			;;
 		-b)
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_split_branch="$1"
 			shift
 			;;
@@ -109,6 +152,7 @@  main () {
 			shift
 			;;
 		-m)
+			test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_addmerge_message="$1"
 			shift
 			;;
@@ -116,28 +160,34 @@  main () {
 			arg_prefix=
 			;;
 		--onto)
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_split_onto="$1"
 			shift
 			;;
 		--no-onto)
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_split_onto=
 			;;
 		--rejoin)
-			arg_split_rejoin=1
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			;;
 		--no-rejoin)
-			arg_split_rejoin=
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			;;
 		--ignore-joins)
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_split_ignore_joins=1
 			;;
 		--no-ignore-joins)
+			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_split_ignore_joins=
 			;;
 		--squash)
+			test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_addmerge_squash=1
 			;;
 		--no-squash)
+			test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
 			arg_addmerge_squash=
 			;;
 		--)
@@ -148,19 +198,8 @@  main () {
 			;;
 		esac
 	done
-
-	arg_command="$1"
 	shift
 
-	case "$arg_command" in
-	add|merge|pull|split|push)
-		:
-		;;
-	*)
-		die "Unknown command '$arg_command'"
-		;;
-	esac
-
 	if test -z "$arg_prefix"
 	then
 		die "You must provide the --prefix option."
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index fb85eed5fc..f83c491a61 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -33,6 +33,12 @@  test_create_commit () (
 	git commit -m "$commit" || error "Could not commit"
 )
 
+test_wrong_flag() {
+	test_must_fail "$@" >out 2>err &&
+	test_must_be_empty out &&
+	grep "flag does not make sense with" err
+}
+
 last_commit_subject () {
 	git log --pretty=format:%s -1
 }
@@ -72,6 +78,22 @@  test_expect_success 'no pull from non-existent subtree' '
 	)
 '
 
+test_expect_success 'add rejects flags for split' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		test_wrong_flag git subtree add --prefix="sub dir" --annotate=foo FETCH_HEAD &&
+		test_wrong_flag git subtree add --prefix="sub dir" --branch=foo FETCH_HEAD &&
+		test_wrong_flag git subtree add --prefix="sub dir" --ignore-joins FETCH_HEAD &&
+		test_wrong_flag git subtree add --prefix="sub dir" --onto=foo FETCH_HEAD &&
+		test_wrong_flag git subtree add --prefix="sub dir" --rejoin FETCH_HEAD
+	)
+'
+
 test_expect_success 'add subproj as subtree into sub dir/ with --prefix' '
 	subtree_test_create_repo "$test_count" &&
 	subtree_test_create_repo "$test_count/sub proj" &&
@@ -128,6 +150,28 @@  test_expect_success 'add subproj as subtree into sub dir/ with --squash and --pr
 # Tests for 'git subtree merge'
 #
 
+test_expect_success 'merge rejects flags for split' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" FETCH_HEAD
+	) &&
+	test_create_commit "$test_count/sub proj" sub2 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		test_wrong_flag git subtree merge --prefix="sub dir" --annotate=foo FETCH_HEAD &&
+		test_wrong_flag git subtree merge --prefix="sub dir" --branch=foo FETCH_HEAD &&
+		test_wrong_flag git subtree merge --prefix="sub dir" --ignore-joins FETCH_HEAD &&
+		test_wrong_flag git subtree merge --prefix="sub dir" --onto=foo FETCH_HEAD &&
+		test_wrong_flag git subtree merge --prefix="sub dir" --rejoin FETCH_HEAD
+	)
+'
+
 test_expect_success 'merge new subproj history into sub dir/ with --prefix' '
 	subtree_test_create_repo "$test_count" &&
 	subtree_test_create_repo "$test_count/sub proj" &&
@@ -262,6 +306,30 @@  test_expect_success 'split requires path given by option --prefix must exist' '
 	)
 '
 
+test_expect_success 'split rejects flags for add' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" FETCH_HEAD
+	) &&
+	test_create_commit "$test_count" "sub dir"/main-sub1 &&
+	test_create_commit "$test_count" main2 &&
+	test_create_commit "$test_count/sub proj" sub2 &&
+	test_create_commit "$test_count" "sub dir"/main-sub2 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree merge --prefix="sub dir" FETCH_HEAD &&
+		split_hash=$(git subtree split --prefix="sub dir" --annotate="*") &&
+		test_wrong_flag git subtree split --prefix="sub dir" --squash &&
+		test_wrong_flag git subtree split --prefix="sub dir" --message=foo
+	)
+'
+
 test_expect_success 'split sub dir/ with --rejoin' '
 	subtree_test_create_repo "$test_count" &&
 	subtree_test_create_repo "$test_count/sub proj" &&
@@ -521,6 +589,26 @@  test_expect_success 'pull basic operation' '
 	)
 '
 
+test_expect_success 'pull rejects flags for split' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" FETCH_HEAD
+	) &&
+	test_create_commit "$test_count/sub proj" sub2 &&
+	(
+		test_must_fail git subtree pull --prefix="sub dir" --annotate=foo ./"sub proj" HEAD &&
+		test_must_fail git subtree pull --prefix="sub dir" --branch=foo ./"sub proj" HEAD &&
+		test_must_fail git subtree pull --prefix="sub dir" --ignore-joins ./"sub proj" HEAD &&
+		test_must_fail git subtree pull --prefix="sub dir" --onto=foo ./"sub proj" HEAD &&
+		test_must_fail git subtree pull --prefix="sub dir" --rejoin ./"sub proj" HEAD
+	)
+'
+
 #
 # Tests for 'git subtree push'
 #
@@ -563,6 +651,29 @@  test_expect_success 'push requires path given by option --prefix must exist' '
 	)
 '
 
+test_expect_success 'push rejects flags for add' '
+	subtree_test_create_repo "$test_count" &&
+	subtree_test_create_repo "$test_count/sub proj" &&
+	test_create_commit "$test_count" main1 &&
+	test_create_commit "$test_count/sub proj" sub1 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree add --prefix="sub dir" FETCH_HEAD
+	) &&
+	test_create_commit "$test_count" "sub dir"/main-sub1 &&
+	test_create_commit "$test_count" main2 &&
+	test_create_commit "$test_count/sub proj" sub2 &&
+	test_create_commit "$test_count" "sub dir"/main-sub2 &&
+	(
+		cd "$test_count" &&
+		git fetch ./"sub proj" HEAD &&
+		git subtree merge --prefix="sub dir" FETCH_HEAD &&
+		test_wrong_flag git subtree split --prefix="sub dir" --squash ./"sub proj" from-mainline &&
+		test_wrong_flag git subtree split --prefix="sub dir" --message=foo ./"sub proj" from-mainline
+	)
+'
+
 test_expect_success 'push basic operation' '
 	subtree_test_create_repo "$test_count" &&
 	subtree_test_create_repo "$test_count/sub proj" &&