Skip to content

Commit 9a3e3ca

Browse files
LukeShugitster
LukeShu
authored and
gitster
committed
subtree: be stricter about validating flags
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 49470cd commit 9a3e3ca

File tree

2 files changed

+175
-25
lines changed

2 files changed

+175
-25
lines changed

‎contrib/subtree/git-subtree.sh

Lines changed: 64 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -44,17 +44,6 @@ squash merge subtree changes as a single commit
4444
m,message= use the given message as the commit message for the merge commit
4545
"
4646

47-
arg_debug=
48-
arg_command=
49-
arg_prefix=
50-
arg_split_branch=
51-
arg_split_onto=
52-
arg_split_rejoin=
53-
arg_split_ignore_joins=
54-
arg_split_annotate=
55-
arg_addmerge_squash=
56-
arg_addmerge_message=
57-
5847
indent=0
5948

6049
# Usage: debug [MSG...]
@@ -106,10 +95,61 @@ main () {
10695
then
10796
set -- -h
10897
fi
109-
eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
98+
set_args="$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
99+
eval "$set_args"
110100
. git-sh-setup
111101
require_work_tree
112102

103+
# First figure out the command and whether we use --rejoin, so
104+
# that we can provide more helpful validation when we do the
105+
# "real" flag parsing.
106+
arg_split_rejoin=
107+
allow_split=
108+
allow_addmerge=
109+
while test $# -gt 0
110+
do
111+
opt="$1"
112+
shift
113+
case "$opt" in
114+
--annotate|-b|-P|-m|--onto)
115+
shift
116+
;;
117+
--rejoin)
118+
arg_split_rejoin=1
119+
;;
120+
--no-rejoin)
121+
arg_split_rejoin=
122+
;;
123+
--)
124+
break
125+
;;
126+
esac
127+
done
128+
arg_command=$1
129+
case "$arg_command" in
130+
add|merge|pull)
131+
allow_addmerge=1
132+
;;
133+
split|push)
134+
allow_split=1
135+
allow_addmerge=$arg_split_rejoin
136+
;;
137+
*)
138+
die "Unknown command '$arg_command'"
139+
;;
140+
esac
141+
# Reset the arguments array for "real" flag parsing.
142+
eval "$set_args"
143+
144+
# Begin "real" flag parsing.
145+
arg_debug=
146+
arg_prefix=
147+
arg_split_branch=
148+
arg_split_onto=
149+
arg_split_ignore_joins=
150+
arg_split_annotate=
151+
arg_addmerge_squash=
152+
arg_addmerge_message=
113153
while test $# -gt 0
114154
do
115155
opt="$1"
@@ -123,13 +163,16 @@ main () {
123163
arg_debug=1
124164
;;
125165
--annotate)
166+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
126167
arg_split_annotate="$1"
127168
shift
128169
;;
129170
--no-annotate)
171+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
130172
arg_split_annotate=
131173
;;
132174
-b)
175+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
133176
arg_split_branch="$1"
134177
shift
135178
;;
@@ -138,35 +181,42 @@ main () {
138181
shift
139182
;;
140183
-m)
184+
test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
141185
arg_addmerge_message="$1"
142186
shift
143187
;;
144188
--no-prefix)
145189
arg_prefix=
146190
;;
147191
--onto)
192+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
148193
arg_split_onto="$1"
149194
shift
150195
;;
151196
--no-onto)
197+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
152198
arg_split_onto=
153199
;;
154200
--rejoin)
155-
arg_split_rejoin=1
201+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
156202
;;
157203
--no-rejoin)
158-
arg_split_rejoin=
204+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
159205
;;
160206
--ignore-joins)
207+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
161208
arg_split_ignore_joins=1
162209
;;
163210
--no-ignore-joins)
211+
test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
164212
arg_split_ignore_joins=
165213
;;
166214
--squash)
215+
test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
167216
arg_addmerge_squash=1
168217
;;
169218
--no-squash)
219+
test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
170220
arg_addmerge_squash=
171221
;;
172222
--)
@@ -177,19 +227,8 @@ main () {
177227
;;
178228
esac
179229
done
180-
181-
arg_command="$1"
182230
shift
183231

184-
case "$arg_command" in
185-
add|merge|pull|split|push)
186-
:
187-
;;
188-
*)
189-
die "Unknown command '$arg_command'"
190-
;;
191-
esac
192-
193232
if test -z "$arg_prefix"
194233
then
195234
die "You must provide the --prefix option."

‎contrib/subtree/t/t7900-subtree.sh

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@ test_create_commit () (
3333
git commit -m "$commit" || error "Could not commit"
3434
)
3535

36+
test_wrong_flag() {
37+
test_must_fail "$@" >out 2>err &&
38+
test_must_be_empty out &&
39+
grep "flag does not make sense with" err
40+
}
41+
3642
last_commit_subject () {
3743
git log --pretty=format:%s -1
3844
}
@@ -72,6 +78,22 @@ test_expect_success 'no pull from non-existent subtree' '
7278
)
7379
'
7480

81+
test_expect_success 'add rejects flags for split' '
82+
subtree_test_create_repo "$test_count" &&
83+
subtree_test_create_repo "$test_count/sub proj" &&
84+
test_create_commit "$test_count" main1 &&
85+
test_create_commit "$test_count/sub proj" sub1 &&
86+
(
87+
cd "$test_count" &&
88+
git fetch ./"sub proj" HEAD &&
89+
test_wrong_flag git subtree add --prefix="sub dir" --annotate=foo FETCH_HEAD &&
90+
test_wrong_flag git subtree add --prefix="sub dir" --branch=foo FETCH_HEAD &&
91+
test_wrong_flag git subtree add --prefix="sub dir" --ignore-joins FETCH_HEAD &&
92+
test_wrong_flag git subtree add --prefix="sub dir" --onto=foo FETCH_HEAD &&
93+
test_wrong_flag git subtree add --prefix="sub dir" --rejoin FETCH_HEAD
94+
)
95+
'
96+
7597
test_expect_success 'add subproj as subtree into sub dir/ with --prefix' '
7698
subtree_test_create_repo "$test_count" &&
7799
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
128150
# Tests for 'git subtree merge'
129151
#
130152

153+
test_expect_success 'merge rejects flags for split' '
154+
subtree_test_create_repo "$test_count" &&
155+
subtree_test_create_repo "$test_count/sub proj" &&
156+
test_create_commit "$test_count" main1 &&
157+
test_create_commit "$test_count/sub proj" sub1 &&
158+
(
159+
cd "$test_count" &&
160+
git fetch ./"sub proj" HEAD &&
161+
git subtree add --prefix="sub dir" FETCH_HEAD
162+
) &&
163+
test_create_commit "$test_count/sub proj" sub2 &&
164+
(
165+
cd "$test_count" &&
166+
git fetch ./"sub proj" HEAD &&
167+
test_wrong_flag git subtree merge --prefix="sub dir" --annotate=foo FETCH_HEAD &&
168+
test_wrong_flag git subtree merge --prefix="sub dir" --branch=foo FETCH_HEAD &&
169+
test_wrong_flag git subtree merge --prefix="sub dir" --ignore-joins FETCH_HEAD &&
170+
test_wrong_flag git subtree merge --prefix="sub dir" --onto=foo FETCH_HEAD &&
171+
test_wrong_flag git subtree merge --prefix="sub dir" --rejoin FETCH_HEAD
172+
)
173+
'
174+
131175
test_expect_success 'merge new subproj history into sub dir/ with --prefix' '
132176
subtree_test_create_repo "$test_count" &&
133177
subtree_test_create_repo "$test_count/sub proj" &&
@@ -262,6 +306,30 @@ test_expect_success 'split requires path given by option --prefix must exist' '
262306
)
263307
'
264308

309+
test_expect_success 'split rejects flags for add' '
310+
subtree_test_create_repo "$test_count" &&
311+
subtree_test_create_repo "$test_count/sub proj" &&
312+
test_create_commit "$test_count" main1 &&
313+
test_create_commit "$test_count/sub proj" sub1 &&
314+
(
315+
cd "$test_count" &&
316+
git fetch ./"sub proj" HEAD &&
317+
git subtree add --prefix="sub dir" FETCH_HEAD
318+
) &&
319+
test_create_commit "$test_count" "sub dir"/main-sub1 &&
320+
test_create_commit "$test_count" main2 &&
321+
test_create_commit "$test_count/sub proj" sub2 &&
322+
test_create_commit "$test_count" "sub dir"/main-sub2 &&
323+
(
324+
cd "$test_count" &&
325+
git fetch ./"sub proj" HEAD &&
326+
git subtree merge --prefix="sub dir" FETCH_HEAD &&
327+
split_hash=$(git subtree split --prefix="sub dir" --annotate="*") &&
328+
test_wrong_flag git subtree split --prefix="sub dir" --squash &&
329+
test_wrong_flag git subtree split --prefix="sub dir" --message=foo
330+
)
331+
'
332+
265333
test_expect_success 'split sub dir/ with --rejoin' '
266334
subtree_test_create_repo "$test_count" &&
267335
subtree_test_create_repo "$test_count/sub proj" &&
@@ -542,6 +610,26 @@ test_expect_success 'pull basic operation' '
542610
)
543611
'
544612

613+
test_expect_success 'pull rejects flags for split' '
614+
subtree_test_create_repo "$test_count" &&
615+
subtree_test_create_repo "$test_count/sub proj" &&
616+
test_create_commit "$test_count" main1 &&
617+
test_create_commit "$test_count/sub proj" sub1 &&
618+
(
619+
cd "$test_count" &&
620+
git fetch ./"sub proj" HEAD &&
621+
git subtree add --prefix="sub dir" FETCH_HEAD
622+
) &&
623+
test_create_commit "$test_count/sub proj" sub2 &&
624+
(
625+
test_must_fail git subtree pull --prefix="sub dir" --annotate=foo ./"sub proj" HEAD &&
626+
test_must_fail git subtree pull --prefix="sub dir" --branch=foo ./"sub proj" HEAD &&
627+
test_must_fail git subtree pull --prefix="sub dir" --ignore-joins ./"sub proj" HEAD &&
628+
test_must_fail git subtree pull --prefix="sub dir" --onto=foo ./"sub proj" HEAD &&
629+
test_must_fail git subtree pull --prefix="sub dir" --rejoin ./"sub proj" HEAD
630+
)
631+
'
632+
545633
#
546634
# Tests for 'git subtree push'
547635
#
@@ -584,6 +672,29 @@ test_expect_success 'push requires path given by option --prefix must exist' '
584672
)
585673
'
586674

675+
test_expect_success 'push rejects flags for add' '
676+
subtree_test_create_repo "$test_count" &&
677+
subtree_test_create_repo "$test_count/sub proj" &&
678+
test_create_commit "$test_count" main1 &&
679+
test_create_commit "$test_count/sub proj" sub1 &&
680+
(
681+
cd "$test_count" &&
682+
git fetch ./"sub proj" HEAD &&
683+
git subtree add --prefix="sub dir" FETCH_HEAD
684+
) &&
685+
test_create_commit "$test_count" "sub dir"/main-sub1 &&
686+
test_create_commit "$test_count" main2 &&
687+
test_create_commit "$test_count/sub proj" sub2 &&
688+
test_create_commit "$test_count" "sub dir"/main-sub2 &&
689+
(
690+
cd "$test_count" &&
691+
git fetch ./"sub proj" HEAD &&
692+
git subtree merge --prefix="sub dir" FETCH_HEAD &&
693+
test_wrong_flag git subtree split --prefix="sub dir" --squash ./"sub proj" from-mainline &&
694+
test_wrong_flag git subtree split --prefix="sub dir" --message=foo ./"sub proj" from-mainline
695+
)
696+
'
697+
587698
test_expect_success 'push basic operation' '
588699
subtree_test_create_repo "$test_count" &&
589700
subtree_test_create_repo "$test_count/sub proj" &&

0 commit comments

Comments
 (0)