Skip to content

Commit e7b120c

Browse files
gitstergitster
committed
Merge branch 'kh/replay-invalid-onto-advance'
Improve the error message when a bad argument is given to the `--onto` option of "git replay". Test coverage of "git replay" has been improved. * kh/replay-invalid-onto-advance: t3650: add more regression tests for failure conditions replay: die if we cannot parse object replay: improve code comment and die message replay: die descriptively when invalid commit-ish is given replay: find *onto only after testing for ref name replay: remove dead code and rearrange
2 parents 24c43fb + 56b77a6 commit e7b120c

File tree

2 files changed

+79
-62
lines changed

2 files changed

+79
-62
lines changed

‎builtin/replay.c‎

Lines changed: 25 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,16 @@ static const char *short_commit_name(struct repository *repo,
3333
DEFAULT_ABBREV);
3434
}
3535

36-
static struct commit *peel_committish(struct repository *repo, const char *name)
36+
static struct commit *peel_committish(struct repository *repo,
37+
const char *name,
38+
const char *mode)
3739
{
3840
struct object *obj;
3941
struct object_id oid;
4042

4143
if (repo_get_oid(repo, name, &oid))
42-
return NULL;
43-
obj = parse_object(repo, &oid);
44+
die(_("'%s' is not a valid commit-ish for %s"), name, mode);
45+
obj = parse_object_or_die(repo, &oid, name);
4446
return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
4547
OBJ_COMMIT);
4648
}
@@ -162,12 +164,12 @@ static void get_ref_information(struct repository *repo,
162164
}
163165
}
164166

165-
static void determine_replay_mode(struct repository *repo,
166-
struct rev_cmdline_info *cmd_info,
167-
const char *onto_name,
168-
char **advance_name,
169-
struct commit **onto,
170-
struct strset **update_refs)
167+
static void set_up_replay_mode(struct repository *repo,
168+
struct rev_cmdline_info *cmd_info,
169+
const char *onto_name,
170+
char **advance_name,
171+
struct commit **onto,
172+
struct strset **update_refs)
171173
{
172174
struct ref_info rinfo;
173175

@@ -178,69 +180,30 @@ static void determine_replay_mode(struct repository *repo,
178180
die_for_incompatible_opt2(!!onto_name, "--onto",
179181
!!*advance_name, "--advance");
180182
if (onto_name) {
181-
*onto = peel_committish(repo, onto_name);
183+
*onto = peel_committish(repo, onto_name, "--onto");
182184
if (rinfo.positive_refexprs <
183185
strset_get_size(&rinfo.positive_refs))
184186
die(_("all positive revisions given must be references"));
185-
} else if (*advance_name) {
187+
*update_refs = xcalloc(1, sizeof(**update_refs));
188+
**update_refs = rinfo.positive_refs;
189+
memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
190+
} else {
186191
struct object_id oid;
187192
char *fullname = NULL;
188193

189-
*onto = peel_committish(repo, *advance_name);
194+
if (!*advance_name)
195+
BUG("expected either onto_name or *advance_name in this function");
196+
190197
if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),
191198
&oid, &fullname, 0) == 1) {
192199
free(*advance_name);
193200
*advance_name = fullname;
194201
} else {
195202
die(_("argument to --advance must be a reference"));
196203
}
204+
*onto = peel_committish(repo, *advance_name, "--advance");
197205
if (rinfo.positive_refexprs > 1)
198206
die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
199-
} else {
200-
int positive_refs_complete = (
201-
rinfo.positive_refexprs ==
202-
strset_get_size(&rinfo.positive_refs));
203-
int negative_refs_complete = (
204-
rinfo.negative_refexprs ==
205-
strset_get_size(&rinfo.negative_refs));
206-
/*
207-
* We need either positive_refs_complete or
208-
* negative_refs_complete, but not both.
209-
*/
210-
if (rinfo.negative_refexprs > 0 &&
211-
positive_refs_complete == negative_refs_complete)
212-
die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
213-
if (negative_refs_complete) {
214-
struct hashmap_iter iter;
215-
struct strmap_entry *entry;
216-
const char *last_key = NULL;
217-
218-
if (rinfo.negative_refexprs == 0)
219-
die(_("all positive revisions given must be references"));
220-
else if (rinfo.negative_refexprs > 1)
221-
die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
222-
else if (rinfo.positive_refexprs > 1)
223-
die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));
224-
225-
/* Only one entry, but we have to loop to get it */
226-
strset_for_each_entry(&rinfo.negative_refs,
227-
&iter, entry) {
228-
last_key = entry->key;
229-
}
230-
231-
free(*advance_name);
232-
*advance_name = xstrdup_or_null(last_key);
233-
} else { /* positive_refs_complete */
234-
if (rinfo.negative_refexprs > 1)
235-
die(_("cannot implicitly determine correct base for --onto"));
236-
if (rinfo.negative_refexprs == 1)
237-
*onto = rinfo.onto;
238-
}
239-
}
240-
if (!*advance_name) {
241-
*update_refs = xcalloc(1, sizeof(**update_refs));
242-
**update_refs = rinfo.positive_refs;
243-
memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
244207
}
245208
strset_clear(&rinfo.negative_refs);
246209
strset_clear(&rinfo.positive_refs);
@@ -451,11 +414,11 @@ int cmd_replay(int argc,
451414
revs.simplify_history = 0;
452415
}
453416

454-
determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
455-
&onto, &update_refs);
417+
set_up_replay_mode(repo, &revs.cmdline,
418+
onto_name, &advance_name,
419+
&onto, &update_refs);
456420

457-
if (!onto) /* FIXME: Should handle replaying down to root commit */
458-
die("Replaying down to root commit is not supported yet!");
421+
/* FIXME: Should allow replaying commits with the first as a root commit */
459422

460423
/* Build reflog message */
461424
if (advance_name_opt)
@@ -491,7 +454,7 @@ int cmd_replay(int argc,
491454
int hr;
492455

493456
if (!commit->parents)
494-
die(_("replaying down to root commit is not supported yet!"));
457+
die(_("replaying down from root commit is not supported yet!"));
495458
if (commit->parents->next)
496459
die(_("replaying merge commits is not supported yet!"));
497460

‎t/t3650-replay-basics.sh‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,13 @@ test_expect_success 'setup' '
4343
test_commit L &&
4444
test_commit M &&
4545
46+
git switch --detach topic4 &&
47+
test_commit N &&
48+
test_commit O &&
49+
git switch -c topic-with-merge topic4 &&
50+
test_merge P O --no-ff &&
51+
git switch main &&
52+
4653
git switch -c conflict B &&
4754
test_commit C.conflict C.t conflict
4855
'
@@ -51,6 +58,53 @@ test_expect_success 'setup bare' '
5158
git clone --bare . bare
5259
'
5360

61+
test_expect_success 'argument to --advance must be a reference' '
62+
echo "fatal: argument to --advance must be a reference" >expect &&
63+
oid=$(git rev-parse main) &&
64+
test_must_fail git replay --advance=$oid topic1..topic2 2>actual &&
65+
test_cmp expect actual
66+
'
67+
68+
test_expect_success '--onto with invalid commit-ish' '
69+
printf "fatal: ${SQ}refs/not-valid${SQ} is not " >expect &&
70+
printf "a valid commit-ish for --onto\n" >>expect &&
71+
test_must_fail git replay --onto=refs/not-valid topic1..topic2 2>actual &&
72+
test_cmp expect actual
73+
'
74+
75+
test_expect_success 'option --onto or --advance is mandatory' '
76+
echo "error: option --onto or --advance is mandatory" >expect &&
77+
test_might_fail git replay -h >>expect &&
78+
test_must_fail git replay topic1..topic2 2>actual &&
79+
test_cmp expect actual
80+
'
81+
82+
test_expect_success 'no base or negative ref gives no-replaying down to root error' '
83+
echo "fatal: replaying down from root commit is not supported yet!" >expect &&
84+
test_must_fail git replay --onto=topic1 topic2 2>actual &&
85+
test_cmp expect actual
86+
'
87+
88+
test_expect_success 'options --advance and --contained cannot be used together' '
89+
printf "fatal: options ${SQ}--advance${SQ} " >expect &&
90+
printf "and ${SQ}--contained${SQ} cannot be used together\n" >>expect &&
91+
test_must_fail git replay --advance=main --contained \
92+
topic1..topic2 2>actual &&
93+
test_cmp expect actual
94+
'
95+
96+
test_expect_success 'cannot advance target ... ordering would be ill-defined' '
97+
echo "fatal: cannot advance target with multiple sources because ordering would be ill-defined" >expect &&
98+
test_must_fail git replay --advance=main main topic1 topic2 2>actual &&
99+
test_cmp expect actual
100+
'
101+
102+
test_expect_success 'replaying merge commits is not supported yet' '
103+
echo "fatal: replaying merge commits is not supported yet!" >expect &&
104+
test_must_fail git replay --advance=main main..topic-with-merge 2>actual &&
105+
test_cmp expect actual
106+
'
107+
54108
test_expect_success 'using replay to rebase two branches, one on top of other' '
55109
git replay --ref-action=print --onto main topic1..topic2 >result &&
56110

0 commit comments

Comments
 (0)