Skip to content

Commit c012943

Browse files
gitstergitster
committed
Merge branch 'rr/revert-cherry-pick'
* rr/revert-cherry-pick: t3502, t3510: clarify cherry-pick -m failure t3510 (cherry-pick-sequencer): use exit status revert: simplify getting commit subject in format_todo() revert: tolerate extra spaces, tabs in insn sheet revert: make commit subjects in insn sheet optional revert: free msg in format_todo()
2 parents 6fee20d + bf71009 commit c012943

File tree

3 files changed

+90
-49
lines changed

3 files changed

+90
-49
lines changed

‎builtin/revert.c

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -700,44 +700,47 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
700700
struct replay_opts *opts)
701701
{
702702
struct commit_list *cur = NULL;
703-
struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
704703
const char *sha1_abbrev = NULL;
705704
const char *action_str = opts->action == REVERT ? "revert" : "pick";
705+
const char *subject;
706+
int subject_len;
706707

707708
for (cur = todo_list; cur; cur = cur->next) {
708709
sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
709-
if (get_message(cur->item, &msg))
710-
return error(_("Cannot get commit message for %s"), sha1_abbrev);
711-
strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
710+
subject_len = find_commit_subject(cur->item->buffer, &subject);
711+
strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
712+
subject_len, subject);
712713
}
713714
return 0;
714715
}
715716

716-
static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
717+
static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
717718
{
718719
unsigned char commit_sha1[20];
719-
char sha1_abbrev[40];
720720
enum replay_action action;
721-
int insn_len = 0;
722-
char *p, *q;
721+
char *end_of_object_name;
722+
int saved, status, padding;
723723

724-
if (!prefixcmp(start, "pick ")) {
724+
if (!prefixcmp(bol, "pick")) {
725725
action = CHERRY_PICK;
726-
insn_len = strlen("pick");
727-
p = start + insn_len + 1;
728-
} else if (!prefixcmp(start, "revert ")) {
726+
bol += strlen("pick");
727+
} else if (!prefixcmp(bol, "revert")) {
729728
action = REVERT;
730-
insn_len = strlen("revert");
731-
p = start + insn_len + 1;
729+
bol += strlen("revert");
732730
} else
733731
return NULL;
734732

735-
q = strchr(p, ' ');
736-
if (!q)
733+
/* Eat up extra spaces/ tabs before object name */
734+
padding = strspn(bol, " \t");
735+
if (!padding)
737736
return NULL;
738-
q++;
737+
bol += padding;
739738

740-
strlcpy(sha1_abbrev, p, q - p);
739+
end_of_object_name = bol + strcspn(bol, " \t\n");
740+
saved = *end_of_object_name;
741+
*end_of_object_name = '\0';
742+
status = get_sha1(bol, commit_sha1);
743+
*end_of_object_name = saved;
741744

742745
/*
743746
* Verify that the action matches up with the one in
@@ -750,7 +753,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
750753
return NULL;
751754
}
752755

753-
if (get_sha1(sha1_abbrev, commit_sha1) < 0)
756+
if (status < 0)
754757
return NULL;
755758

756759
return lookup_commit_reference(commit_sha1);
@@ -765,13 +768,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
765768
int i;
766769

767770
for (i = 1; *p; i++) {
768-
commit = parse_insn_line(p, opts);
771+
char *eol = strchrnul(p, '\n');
772+
commit = parse_insn_line(p, eol, opts);
769773
if (!commit)
770774
return error(_("Could not parse line %d."), i);
771775
next = commit_list_append(commit, next);
772-
p = strchrnul(p, '\n');
773-
if (*p)
774-
p++;
776+
p = *eol ? eol + 1 : eol;
775777
}
776778
if (!*todo_list)
777779
return error(_("No commits parsed."));

‎t/t3502-cherry-pick-merge.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ test_expect_success 'cherry-pick a non-merge with -m should fail' '
3535
3636
git reset --hard &&
3737
git checkout a^0 &&
38-
test_must_fail git cherry-pick -m 1 b &&
38+
test_expect_code 128 git cherry-pick -m 1 b &&
3939
git diff --exit-code a --
4040
4141
'

‎t/t3510-cherry-pick-sequence.sh

Lines changed: 64 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ test_description='Test cherry-pick continuation features
1414

1515
. ./test-lib.sh
1616

17+
# Repeat first match 10 times
18+
_r10='\1\1\1\1\1\1\1\1\1\1'
19+
1720
pristine_detach () {
1821
git cherry-pick --quit &&
1922
git checkout -f "$1^0" &&
@@ -43,7 +46,7 @@ test_expect_success setup '
4346

4447
test_expect_success 'cherry-pick persists data on failure' '
4548
pristine_detach initial &&
46-
test_must_fail git cherry-pick -s base..anotherpick &&
49+
test_expect_code 1 git cherry-pick -s base..anotherpick &&
4750
test_path_is_dir .git/sequencer &&
4851
test_path_is_file .git/sequencer/head &&
4952
test_path_is_file .git/sequencer/todo &&
@@ -64,7 +67,7 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
6467

6568
test_expect_success 'cherry-pick persists opts correctly' '
6669
pristine_detach initial &&
67-
test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
70+
test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
6871
test_path_is_dir .git/sequencer &&
6972
test_path_is_file .git/sequencer/head &&
7073
test_path_is_file .git/sequencer/todo &&
@@ -104,7 +107,7 @@ test_expect_success '--abort requires cherry-pick in progress' '
104107

105108
test_expect_success '--quit cleans up sequencer state' '
106109
pristine_detach initial &&
107-
test_must_fail git cherry-pick base..picked &&
110+
test_expect_code 1 git cherry-pick base..picked &&
108111
git cherry-pick --quit &&
109112
test_path_is_missing .git/sequencer
110113
'
@@ -118,7 +121,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
118121
:000000 100644 OBJID OBJID A foo
119122
:000000 100644 OBJID OBJID A unrelated
120123
EOF
121-
test_must_fail git cherry-pick base..picked &&
124+
test_expect_code 1 git cherry-pick base..picked &&
122125
git cherry-pick --quit &&
123126
test_path_is_missing .git/sequencer &&
124127
test_must_fail git update-index --refresh &&
@@ -132,7 +135,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
132135

133136
test_expect_success '--abort to cancel multiple cherry-pick' '
134137
pristine_detach initial &&
135-
test_must_fail git cherry-pick base..anotherpick &&
138+
test_expect_code 1 git cherry-pick base..anotherpick &&
136139
git cherry-pick --abort &&
137140
test_path_is_missing .git/sequencer &&
138141
test_cmp_rev initial HEAD &&
@@ -142,7 +145,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' '
142145

143146
test_expect_success '--abort to cancel single cherry-pick' '
144147
pristine_detach initial &&
145-
test_must_fail git cherry-pick picked &&
148+
test_expect_code 1 git cherry-pick picked &&
146149
git cherry-pick --abort &&
147150
test_path_is_missing .git/sequencer &&
148151
test_cmp_rev initial HEAD &&
@@ -152,7 +155,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
152155

153156
test_expect_success 'cherry-pick --abort to cancel multiple revert' '
154157
pristine_detach anotherpick &&
155-
test_must_fail git revert base..picked &&
158+
test_expect_code 1 git revert base..picked &&
156159
git cherry-pick --abort &&
157160
test_path_is_missing .git/sequencer &&
158161
test_cmp_rev anotherpick HEAD &&
@@ -162,15 +165,15 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' '
162165

163166
test_expect_success 'revert --abort works, too' '
164167
pristine_detach anotherpick &&
165-
test_must_fail git revert base..picked &&
168+
test_expect_code 1 git revert base..picked &&
166169
git revert --abort &&
167170
test_path_is_missing .git/sequencer &&
168171
test_cmp_rev anotherpick HEAD
169172
'
170173

171174
test_expect_success '--abort to cancel single revert' '
172175
pristine_detach anotherpick &&
173-
test_must_fail git revert picked &&
176+
test_expect_code 1 git revert picked &&
174177
git revert --abort &&
175178
test_path_is_missing .git/sequencer &&
176179
test_cmp_rev anotherpick HEAD &&
@@ -181,7 +184,7 @@ test_expect_success '--abort to cancel single revert' '
181184
test_expect_success '--abort keeps unrelated change, easy case' '
182185
pristine_detach unrelatedpick &&
183186
echo changed >expect &&
184-
test_must_fail git cherry-pick picked..yetanotherpick &&
187+
test_expect_code 1 git cherry-pick picked..yetanotherpick &&
185188
echo changed >unrelated &&
186189
git cherry-pick --abort &&
187190
test_cmp expect unrelated
@@ -190,7 +193,7 @@ test_expect_success '--abort keeps unrelated change, easy case' '
190193
test_expect_success '--abort refuses to clobber unrelated change, harder case' '
191194
pristine_detach initial &&
192195
echo changed >expect &&
193-
test_must_fail git cherry-pick base..anotherpick &&
196+
test_expect_code 1 git cherry-pick base..anotherpick &&
194197
echo changed >unrelated &&
195198
test_must_fail git cherry-pick --abort &&
196199
test_cmp expect unrelated &&
@@ -205,7 +208,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '
205208

206209
test_expect_success 'cherry-pick still writes sequencer state when one commit is left' '
207210
pristine_detach initial &&
208-
test_must_fail git cherry-pick base..picked &&
211+
test_expect_code 1 git cherry-pick base..picked &&
209212
test_path_is_dir .git/sequencer &&
210213
echo "resolved" >foo &&
211214
git add foo &&
@@ -229,7 +232,7 @@ test_expect_success 'cherry-pick still writes sequencer state when one commit is
229232

230233
test_expect_success '--abort after last commit in sequence' '
231234
pristine_detach initial &&
232-
test_must_fail git cherry-pick base..picked &&
235+
test_expect_code 1 git cherry-pick base..picked &&
233236
git cherry-pick --abort &&
234237
test_path_is_missing .git/sequencer &&
235238
test_cmp_rev initial HEAD &&
@@ -239,22 +242,22 @@ test_expect_success '--abort after last commit in sequence' '
239242

240243
test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '
241244
pristine_detach initial &&
242-
test_must_fail git cherry-pick base..anotherpick &&
245+
test_expect_code 1 git cherry-pick base..anotherpick &&
243246
test-chmtime -v +0 .git/sequencer >expect &&
244-
test_must_fail git cherry-pick unrelatedpick &&
247+
test_expect_code 128 git cherry-pick unrelatedpick &&
245248
test-chmtime -v +0 .git/sequencer >actual &&
246249
test_cmp expect actual
247250
'
248251

249252
test_expect_success '--continue complains when no cherry-pick is in progress' '
250253
pristine_detach initial &&
251-
test_must_fail git cherry-pick --continue
254+
test_expect_code 128 git cherry-pick --continue
252255
'
253256

254257
test_expect_success '--continue complains when there are unresolved conflicts' '
255258
pristine_detach initial &&
256-
test_must_fail git cherry-pick base..anotherpick &&
257-
test_must_fail git cherry-pick --continue
259+
test_expect_code 1 git cherry-pick base..anotherpick &&
260+
test_expect_code 128 git cherry-pick --continue
258261
'
259262

260263
test_expect_success '--continue of single cherry-pick' '
@@ -318,7 +321,7 @@ test_expect_success '--continue after resolving conflicts' '
318321

319322
test_expect_success '--continue after resolving conflicts and committing' '
320323
pristine_detach initial &&
321-
test_must_fail git cherry-pick base..anotherpick &&
324+
test_expect_code 1 git cherry-pick base..anotherpick &&
322325
echo "c" >foo &&
323326
git add foo &&
324327
git commit &&
@@ -368,7 +371,7 @@ test_expect_success 'follow advice and skip nil patch' '
368371

369372
test_expect_success '--continue respects opts' '
370373
pristine_detach initial &&
371-
test_must_fail git cherry-pick -x base..anotherpick &&
374+
test_expect_code 1 git cherry-pick -x base..anotherpick &&
372375
echo "c" >foo &&
373376
git add foo &&
374377
git commit &&
@@ -409,7 +412,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
409412

410413
test_expect_success '--signoff is not automatically propagated to resolved conflict' '
411414
pristine_detach initial &&
412-
test_must_fail git cherry-pick --signoff base..anotherpick &&
415+
test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
413416
echo "c" >foo &&
414417
git add foo &&
415418
git commit &&
@@ -453,29 +456,65 @@ test_expect_success 'sign-off needs to be reaffirmed after conflict resolution,
453456

454457
test_expect_success 'malformed instruction sheet 1' '
455458
pristine_detach initial &&
456-
test_must_fail git cherry-pick base..anotherpick &&
459+
test_expect_code 1 git cherry-pick base..anotherpick &&
457460
echo "resolved" >foo &&
458461
git add foo &&
459462
git commit &&
460463
sed "s/pick /pick/" .git/sequencer/todo >new_sheet &&
461464
cp new_sheet .git/sequencer/todo &&
462-
test_must_fail git cherry-pick --continue
465+
test_expect_code 128 git cherry-pick --continue
463466
'
464467

465468
test_expect_success 'malformed instruction sheet 2' '
466469
pristine_detach initial &&
467-
test_must_fail git cherry-pick base..anotherpick &&
470+
test_expect_code 1 git cherry-pick base..anotherpick &&
468471
echo "resolved" >foo &&
469472
git add foo &&
470473
git commit &&
471474
sed "s/pick/revert/" .git/sequencer/todo >new_sheet &&
472475
cp new_sheet .git/sequencer/todo &&
473-
test_must_fail git cherry-pick --continue
476+
test_expect_code 128 git cherry-pick --continue
474477
'
475478

476479
test_expect_success 'empty commit set' '
477480
pristine_detach initial &&
478481
test_expect_code 128 git cherry-pick base..base
479482
'
480483

484+
test_expect_success 'malformed instruction sheet 3' '
485+
pristine_detach initial &&
486+
test_expect_code 1 git cherry-pick base..anotherpick &&
487+
echo "resolved" >foo &&
488+
git add foo &&
489+
git commit &&
490+
sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
491+
cp new_sheet .git/sequencer/todo &&
492+
test_expect_code 128 git cherry-pick --continue
493+
'
494+
495+
test_expect_success 'instruction sheet, fat-fingers version' '
496+
pristine_detach initial &&
497+
test_expect_code 1 git cherry-pick base..anotherpick &&
498+
echo "c" >foo &&
499+
git add foo &&
500+
git commit &&
501+
sed "s/pick \([0-9a-f]*\)/pick \1 /" .git/sequencer/todo >new_sheet &&
502+
cp new_sheet .git/sequencer/todo &&
503+
git cherry-pick --continue
504+
'
505+
506+
test_expect_success 'commit descriptions in insn sheet are optional' '
507+
pristine_detach initial &&
508+
test_expect_code 1 git cherry-pick base..anotherpick &&
509+
echo "c" >foo &&
510+
git add foo &&
511+
git commit &&
512+
cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
513+
cp new_sheet .git/sequencer/todo &&
514+
git cherry-pick --continue &&
515+
test_path_is_missing .git/sequencer &&
516+
git rev-list HEAD >commits &&
517+
test_line_count = 4 commits
518+
'
519+
481520
test_done

0 commit comments

Comments
 (0)