Skip to content

Commit 2ddddc2

Browse files
gitstergitster
committed
Merge branch 'ag/edit-todo-drop-check' into pu
Allow the rebase.missingCommitsCheck configuration to kick in when "rebase --edit-todo" and "rebase --continue" restarts the procedure. Broken. cf. <[email protected]> * ag/edit-todo-drop-check: rebase-interactive: warn if commit is dropped with `rebase --edit-todo' sequencer: move check_todo_list_from_file() to rebase-interactive.c
2 parents b652833 + 5a5445d commit 2ddddc2

5 files changed

+214
-49
lines changed

‎rebase-interactive.c

Lines changed: 76 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,13 @@
55
#include "strbuf.h"
66
#include "commit-slab.h"
77
#include "config.h"
8+
#include "dir.h"
9+
10+
static const char edit_todo_list_advice[] =
11+
N_("You can fix this with 'git rebase --edit-todo' "
12+
"and then run 'git rebase --continue'.\n"
13+
"Or you can abort the rebase with 'git rebase"
14+
" --abort'.\n");
815

916
enum missing_commit_check_level {
1017
MISSING_COMMIT_CHECK_IGNORE = 0,
@@ -86,21 +93,24 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
8693
struct todo_list *new_todo, const char *shortrevisions,
8794
const char *shortonto, unsigned flags)
8895
{
89-
const char *todo_file = rebase_path_todo();
96+
const char *todo_file = rebase_path_todo(),
97+
*todo_backup = rebase_path_todo_backup();
9098
unsigned initial = shortrevisions && shortonto;
99+
int incorrect = 0;
91100

92101
/* If the user is editing the todo list, we first try to parse
93102
* it. If there is an error, we do not return, because the user
94103
* might want to fix it in the first place. */
95104
if (!initial)
96-
todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
105+
incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
106+
file_exists(rebase_path_dropped());
97107

98108
if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
99109
-1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
100110
return error_errno(_("could not write '%s'"), todo_file);
101111

102-
if (initial &&
103-
todo_list_write_to_file(r, todo_list, rebase_path_todo_backup(),
112+
if (!incorrect &&
113+
todo_list_write_to_file(r, todo_list, todo_backup,
104114
shortrevisions, shortonto, -1,
105115
(flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
106116
return error(_("could not write '%s'."), rebase_path_todo_backup());
@@ -112,10 +122,23 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
112122
if (initial && new_todo->buf.len == 0)
113123
return -3;
114124

115-
/* For the initial edit, the todo list gets parsed in
116-
* complete_action(). */
117-
if (!initial)
118-
return todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo);
125+
if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
126+
fprintf(stderr, _(edit_todo_list_advice));
127+
return -4;
128+
}
129+
130+
if (incorrect) {
131+
if (todo_list_check_against_backup(r, new_todo)) {
132+
write_file(rebase_path_dropped(), "");
133+
return -4;
134+
}
135+
136+
if (incorrect > 0)
137+
unlink(rebase_path_dropped());
138+
} else if (todo_list_check(todo_list, new_todo)) {
139+
write_file(rebase_path_dropped(), "");
140+
return -4;
141+
}
119142

120143
return 0;
121144
}
@@ -180,7 +203,52 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
180203
"the level of warnings.\n"
181204
"The possible behaviours are: ignore, warn, error.\n\n"));
182205

206+
fprintf(stderr, _(edit_todo_list_advice));
207+
183208
leave_check:
184209
clear_commit_seen(&commit_seen);
185210
return res;
186211
}
212+
213+
int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)
214+
{
215+
struct todo_list backup = TODO_LIST_INIT;
216+
int res = 0;
217+
218+
if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
219+
todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
220+
res = todo_list_check(&backup, todo_list);
221+
}
222+
223+
todo_list_release(&backup);
224+
return res;
225+
}
226+
227+
int check_todo_list_from_file(struct repository *r)
228+
{
229+
struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
230+
int res = 0;
231+
232+
if (strbuf_read_file(&new_todo.buf, rebase_path_todo(), 0) < 0) {
233+
res = error(_("could not read '%s'."), rebase_path_todo());
234+
goto out;
235+
}
236+
237+
if (strbuf_read_file(&old_todo.buf, rebase_path_todo_backup(), 0) < 0) {
238+
res = error(_("could not read '%s'."), rebase_path_todo_backup());
239+
goto out;
240+
}
241+
242+
res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
243+
if (!res)
244+
res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
245+
if (res)
246+
fprintf(stderr, _(edit_todo_list_advice));
247+
if (!res)
248+
res = todo_list_check(&old_todo, &new_todo);
249+
out:
250+
todo_list_release(&old_todo);
251+
todo_list_release(&new_todo);
252+
253+
return res;
254+
}

‎rebase-interactive.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,11 @@ void append_todo_help(int command_count,
1111
int edit_todo_list(struct repository *r, struct todo_list *todo_list,
1212
struct todo_list *new_todo, const char *shortrevisions,
1313
const char *shortonto, unsigned flags);
14+
1415
int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
16+
int todo_list_check_against_backup(struct repository *r,
17+
struct todo_list *todo_list);
18+
19+
int check_todo_list_from_file(struct repository *r);
1520

1621
#endif

‎sequencer.c

Lines changed: 11 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
5858
GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
5959
GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup")
6060

61+
GIT_PATH_FUNC(rebase_path_dropped, "rebase-merge/dropped")
62+
6163
/*
6264
* The rebase command lines that have already been processed. A line
6365
* is moved here when it is first handled, before any associated user
@@ -4312,6 +4314,14 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
43124314
if (is_rebase_i(opts)) {
43134315
if ((res = read_populate_todo(r, &todo_list, opts)))
43144316
goto release_todo_list;
4317+
4318+
if (file_exists(rebase_path_dropped())) {
4319+
if ((res = todo_list_check_against_backup(r, &todo_list)))
4320+
goto release_todo_list;
4321+
4322+
unlink(rebase_path_dropped());
4323+
}
4324+
43154325
if (commit_staged_changes(r, opts, &todo_list)) {
43164326
res = -1;
43174327
goto release_todo_list;
@@ -5052,41 +5062,6 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
50525062
return res;
50535063
}
50545064

5055-
static const char edit_todo_list_advice[] =
5056-
N_("You can fix this with 'git rebase --edit-todo' "
5057-
"and then run 'git rebase --continue'.\n"
5058-
"Or you can abort the rebase with 'git rebase"
5059-
" --abort'.\n");
5060-
5061-
int check_todo_list_from_file(struct repository *r)
5062-
{
5063-
struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
5064-
int res = 0;
5065-
5066-
if (strbuf_read_file_or_whine(&new_todo.buf, rebase_path_todo()) < 0) {
5067-
res = -1;
5068-
goto out;
5069-
}
5070-
5071-
if (strbuf_read_file_or_whine(&old_todo.buf, rebase_path_todo_backup()) < 0) {
5072-
res = -1;
5073-
goto out;
5074-
}
5075-
5076-
res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
5077-
if (!res)
5078-
res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
5079-
if (!res)
5080-
res = todo_list_check(&old_todo, &new_todo);
5081-
if (res)
5082-
fprintf(stderr, _(edit_todo_list_advice));
5083-
out:
5084-
todo_list_release(&old_todo);
5085-
todo_list_release(&new_todo);
5086-
5087-
return res;
5088-
}
5089-
50905065
/* skip picking commits whose parents are unchanged */
50915066
static int skip_unnecessary_picks(struct repository *r,
50925067
struct todo_list *todo_list,
@@ -5184,11 +5159,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
51845159
todo_list_release(&new_todo);
51855160

51865161
return error(_("nothing to do"));
5187-
}
5188-
5189-
if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
5190-
todo_list_check(todo_list, &new_todo)) {
5191-
fprintf(stderr, _(edit_todo_list_advice));
5162+
} else if (res == -4) {
51925163
checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
51935164
todo_list_release(&new_todo);
51945165

‎sequencer.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ struct repository;
1111
const char *git_path_commit_editmsg(void);
1212
const char *rebase_path_todo(void);
1313
const char *rebase_path_todo_backup(void);
14+
const char *rebase_path_dropped(void);
1415

1516
#define APPEND_SIGNOFF_DEDUP (1u << 0)
1617

@@ -156,7 +157,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
156157

157158
void todo_list_add_exec_commands(struct todo_list *todo_list,
158159
struct string_list *commands);
159-
int check_todo_list_from_file(struct repository *r);
160160
int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
161161
const char *shortrevisions, const char *onto_name,
162162
struct commit *onto, const char *orig_head, struct string_list *commands,

‎t/t3404-rebase-interactive.sh

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1469,6 +1469,127 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
14691469
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
14701470
'
14711471

1472+
test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' '
1473+
test_config rebase.missingCommitsCheck ignore &&
1474+
rebase_setup_and_clean missing-commit &&
1475+
(
1476+
set_fake_editor &&
1477+
FAKE_LINES="break 1 2 3 4 5" git rebase -i --root &&
1478+
FAKE_LINES="1 2 3 4" git rebase --edit-todo &&
1479+
git rebase --continue 2>actual
1480+
) &&
1481+
test D = $(git cat-file commit HEAD | sed -ne \$p) &&
1482+
test_i18ngrep \
1483+
"Successfully rebased and updated refs/heads/missing-commit" \
1484+
actual
1485+
'
1486+
1487+
test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
1488+
cat >expect <<-EOF &&
1489+
error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
1490+
Warning: some commits may have been dropped accidentally.
1491+
Dropped commits (newer to older):
1492+
- $(git rev-list --pretty=oneline --abbrev-commit -1 master)
1493+
- $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
1494+
To avoid this message, use "drop" to explicitly remove a commit.
1495+
EOF
1496+
head -n4 expect >expect.2 &&
1497+
tail -n1 expect >>expect.2 &&
1498+
tail -n4 expect.2 >expect.3 &&
1499+
test_config rebase.missingCommitsCheck warn &&
1500+
rebase_setup_and_clean missing-commit &&
1501+
(
1502+
set_fake_editor &&
1503+
test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
1504+
git rebase -i --root &&
1505+
cp .git/rebase-merge/git-rebase-todo.backup orig &&
1506+
FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 &&
1507+
head -n6 actual.2 >actual &&
1508+
test_i18ncmp expect actual &&
1509+
cp orig .git/rebase-merge/git-rebase-todo &&
1510+
FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
1511+
head -n4 actual.2 >actual &&
1512+
test_i18ncmp expect.3 actual &&
1513+
git rebase --continue 2>actual
1514+
) &&
1515+
test D = $(git cat-file commit HEAD | sed -ne \$p) &&
1516+
test_i18ngrep \
1517+
"Successfully rebased and updated refs/heads/missing-commit" \
1518+
actual
1519+
'
1520+
1521+
test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
1522+
cat >expect <<-EOF &&
1523+
error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
1524+
Warning: some commits may have been dropped accidentally.
1525+
Dropped commits (newer to older):
1526+
- $(git rev-list --pretty=oneline --abbrev-commit -1 master)
1527+
- $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
1528+
To avoid this message, use "drop" to explicitly remove a commit.
1529+
1530+
Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
1531+
The possible behaviours are: ignore, warn, error.
1532+
1533+
You can fix this with '\''git rebase --edit-todo'\'' and then run '\''git rebase --continue'\''.
1534+
Or you can abort the rebase with '\''git rebase --abort'\''.
1535+
EOF
1536+
tail -n11 expect >expect.2 &&
1537+
head -n3 expect.2 >expect.3 &&
1538+
tail -n7 expect.2 >>expect.3 &&
1539+
test_config rebase.missingCommitsCheck error &&
1540+
rebase_setup_and_clean missing-commit &&
1541+
(
1542+
set_fake_editor &&
1543+
test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
1544+
git rebase -i --root &&
1545+
cp .git/rebase-merge/git-rebase-todo.backup orig &&
1546+
test_must_fail env FAKE_LINES="2 3 4" \
1547+
git rebase --edit-todo 2>actual &&
1548+
test_i18ncmp expect actual &&
1549+
test_must_fail git rebase --continue 2>actual &&
1550+
test_i18ncmp expect.2 actual &&
1551+
test_must_fail git rebase --edit-todo &&
1552+
cp orig .git/rebase-merge/git-rebase-todo &&
1553+
test_must_fail env FAKE_LINES="1 2 3 4" \
1554+
git rebase --edit-todo 2>actual &&
1555+
test_i18ncmp expect.3 actual &&
1556+
test_must_fail git rebase --continue 2>actual &&
1557+
test_i18ncmp expect.3 actual &&
1558+
cp orig .git/rebase-merge/git-rebase-todo &&
1559+
FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
1560+
git rebase --continue 2>actual
1561+
) &&
1562+
test D = $(git cat-file commit HEAD | sed -ne \$p) &&
1563+
test_i18ngrep \
1564+
"Successfully rebased and updated refs/heads/missing-commit" \
1565+
actual
1566+
'
1567+
1568+
test_expect_success 'rebase.missingCommitsCheck = error after resolving conflicts' '
1569+
test_config rebase.missingCommitsCheck error &&
1570+
(
1571+
set_fake_editor &&
1572+
FAKE_LINES="drop 1 break 2 3 4" git rebase -i A E
1573+
) &&
1574+
git rebase --edit-todo &&
1575+
test_must_fail git rebase --continue &&
1576+
echo x >file1 &&
1577+
git add file1 &&
1578+
git rebase --continue
1579+
'
1580+
1581+
test_expect_success 'rebase.missingCommitsCheck = error when editing for a second time' '
1582+
test_config rebase.missingCommitsCheck error &&
1583+
(
1584+
set_fake_editor &&
1585+
FAKE_LINES="1 break 2 3" git rebase -i A D &&
1586+
cp .git/rebase-merge/git-rebase-todo todo &&
1587+
test_must_fail env FAKE_LINES=2 git rebase --edit-todo &&
1588+
GIT_SEQUENCE_EDITOR="cp todo" git rebase --edit-todo &&
1589+
git rebase --continue
1590+
)
1591+
'
1592+
14721593
test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
14731594
rebase_setup_and_clean abbrevcmd &&
14741595
test_commit "first" file1.txt "first line" first &&

0 commit comments

Comments
 (0)