Skip to content

Prevent fixup from amending unreachable commits #2277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
GontuSujithReddy wants to merge 1 commit into from
Closed

Prevent fixup from amending unreachable commits #2277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 172 additions & 24 deletions sequencer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,19 @@ static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
* command is processed, this file is deleted.
*/
static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
/*
* When a commit before a fixup/squash could not be applied, this file
* contains the object name of that commit. The following fixup/squash
* command must not be applied to the previous commit instead.
*/
static GIT_PATH_FUNC(rebase_path_fixup_target, "rebase-merge/fixup-target")
/*
* When a pick conflicts before a fixup/squash, this file contains HEAD
* before the pick was attempted. It lets us tell whether the pick was
* actually committed when the rebase is continued.
*/
static GIT_PATH_FUNC(rebase_path_fixup_target_head,
"rebase-merge/fixup-target-head")
/*
* When we stop at a given patch via the "edit" command, this file contains
* the commit object name of the corresponding patch.
Expand Down Expand Up @@ -5085,6 +5098,24 @@ static int pick_one_commit(struct repository *r,
enum pick_result pick_res;
struct todo_item *item = todo_list->items + todo_list->current;
const char *arg = todo_item_get_arg(todo_list, item);
struct strbuf target = STRBUF_INIT;

if (is_fixup(item->command) &&
file_exists(rebase_path_fixup_target())) {
if (!read_oneliner(&target, rebase_path_fixup_target(),
READ_ONELINER_SKIP_IF_EMPTY)) {
*reschedule = 1;
strbuf_release(&target);
return error(_("could not read '%s'"),
rebase_path_fixup_target());
}
error(_("cannot %s %s: commit %s was not applied"),
command_to_string(item->command),
short_commit_name(r, item->commit), target.buf);
strbuf_release(&target);
*reschedule = 1;
return -1;
}

pick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
check_todo);
Expand All @@ -5097,6 +5128,18 @@ static int pick_one_commit(struct repository *r,
default:
return 0;
}
if (pick_res == PICK_RESULT_CONFLICTS && !is_fixup(item->command)) {
struct object_id head;
const char *head_hex;

if (repo_get_oid(r, "HEAD", &head))
oidclr(&head, r->hash_algo);
head_hex = oid_to_hex(&head);
if (write_message(head_hex, strlen(head_hex),
rebase_path_fixup_target_head(), 1))
return error_errno(_("could not write '%s'"),
rebase_path_fixup_target_head());
}

if (pick_res == PICK_RESULT_ERROR) {
/* Reschedule */
Expand All @@ -5108,6 +5151,16 @@ static int pick_one_commit(struct repository *r,
int to_amend = pick_res != PICK_RESULT_CONFLICTS &&
pick_res != PICK_RESULT_DROPPED;

if (pick_res == PICK_RESULT_OK)
unlink(rebase_path_fixup_target());
else if (pick_res == PICK_RESULT_DROPPED) {
const char *oid = oid_to_hex(&item->commit->object.oid);
if (write_message(oid, strlen(oid),
rebase_path_fixup_target(), 0))
return error_errno(_("could not write '%s'"),
rebase_path_fixup_target());
}

/*
* NEEDSWORK: Do not record the commit as rewritten when
* continuing if it was dropped. Does it even make sense
Expand All @@ -5123,10 +5176,19 @@ static int pick_one_commit(struct repository *r,
return error_with_patch(r, commit, arg, item->arg_len, opts,
res, to_amend);
} else if (pick_res == PICK_RESULT_OK) {
if (!is_fixup(item->command))
unlink(rebase_path_fixup_target());
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
return 0;
} else if (pick_res == PICK_RESULT_DROPPED) {
if (!is_fixup(item->command)) {
const char *oid = oid_to_hex(&item->commit->object.oid);
if (write_message(oid, strlen(oid),
rebase_path_fixup_target(), 0))
return error_errno(_("could not write '%s'"),
rebase_path_fixup_target());
}
if (is_final_fixup(todo_list))
flush_rewritten_pending();
return 0;
Expand Down Expand Up @@ -5242,6 +5304,8 @@ static int pick_commits(struct repository *r,
} else if (item->command == TODO_RESET) {
if ((res = do_reset(r, arg, item->arg_len, opts)))
reschedule = 1;
else
unlink(rebase_path_fixup_target());
} else if (item->command == TODO_MERGE) {
if ((res = do_merge(r, item->commit, arg, item->arg_len,
item->flags, &check_todo, opts)) < 0)
Expand All @@ -5254,6 +5318,8 @@ static int pick_commits(struct repository *r,
return error_with_patch(r, item->commit,
arg, item->arg_len,
opts, res, 0);
if (!res)
unlink(rebase_path_fixup_target());
} else if (item->command == TODO_UPDATE_REF) {
struct strbuf ref = STRBUF_INIT;
strbuf_add(&ref, arg, item->arg_len);
Expand Down Expand Up @@ -5442,31 +5508,54 @@ static int commit_staged_changes(struct repository *r,
goto out;
}

if (file_exists(rebase_path_amend())) {
struct object_id head, to_amend;

if (repo_get_oid(r, "HEAD", &head)) {
ret = error(_("cannot amend non-existing commit"));
goto out;
if (file_exists(rebase_path_amend())) {
struct object_id head, to_amend;

if (repo_get_oid(r, "HEAD", &head)) {
ret = error(_("cannot amend non-existing commit"));
goto out;
}

if (!read_oneliner(&rev, rebase_path_amend(), 0)) {
ret = error(_("invalid file: '%s'"), rebase_path_amend());
goto out;
}

if (get_oid_hex(rev.buf, &to_amend)) {
ret = error(_("invalid contents: '%s'"),
rebase_path_amend());
goto out;
}

if (is_clean && !oideq(&head, &to_amend)) {
struct commit *head_commit = lookup_commit(r, &head);
struct commit *amend_commit = lookup_commit(r, &to_amend);
struct commit_list *list = NULL;

commit_list_insert(amend_commit, &list);

if (!head_commit || !amend_commit ||
!repo_is_descendant_of(r, head_commit, list)) {
commit_list_free(list);
ret = error(_("fixup target %s is missing from HEAD history; "
"refusing to continue"), oid_to_hex(&to_amend));
goto out;
}
commit_list_free(list);
}

if (!is_clean && !oideq(&head, &to_amend)) {
ret = error(_("\nYou have uncommitted changes in your "
"working tree. Please, commit them\n"
"first and then run 'git rebase "
"--continue' again."));
goto out;
}


if (!read_oneliner(&rev, rebase_path_amend(), 0)) {
ret = error(_("invalid file: '%s'"), rebase_path_amend());
goto out;
}

if (get_oid_hex(rev.buf, &to_amend)) {
ret = error(_("invalid contents: '%s'"),
rebase_path_amend());
goto out;
}
if (!is_clean && !oideq(&head, &to_amend)) {
ret = error(_("\nYou have uncommitted changes in your "
"working tree. Please, commit them\n"
"first and then run 'git rebase "
"--continue' again."));
goto out;
}


/*
* When skipping a failed fixup/squash, we need to edit the
* commit message, the current fixup list and count, and if it
Expand Down Expand Up @@ -5622,10 +5711,64 @@ static int commit_staged_changes(struct repository *r,
return ret;
}

/*
* A conflicted pick before a fixup/squash may have been skipped by the user
* (or become empty after resolving the conflicts). In that case, remember
* the missing target so the fixup/squash is not applied to the wrong commit.
* Return 1 if the pick was skipped, 0 if it was committed, and -1 on error.
*/
static int check_fixup_target(struct repository *r)
{
struct strbuf head_buf = STRBUF_INIT, stopped_buf = STRBUF_INIT;
struct object_id expected_head, actual_head, stopped;
const char *head_path = rebase_path_fixup_target_head();
const char *stopped_path = rebase_path_stopped_sha();
int ret = -1;

if (!file_exists(head_path))
return 0;

if (!read_oneliner(&head_buf, head_path, READ_ONELINER_SKIP_IF_EMPTY) ||
get_oid_hex(head_buf.buf, &expected_head)) {
error(_("invalid contents: '%s'"), head_path);
goto out;
}

if (repo_get_oid(r, "HEAD", &actual_head))
oidclr(&actual_head, r->hash_algo);

if (!oideq(&expected_head, &actual_head)) {
/* The commit was resolved and created, so the target is present. */
unlink(head_path);
unlink(rebase_path_fixup_target());
ret = 0;
goto out;
}

if (!read_oneliner(&stopped_buf, stopped_path,
READ_ONELINER_SKIP_IF_EMPTY) ||
get_oid_hex(stopped_buf.buf, &stopped)) {
error(_("invalid contents: '%s'"), stopped_path);
goto out;
}

if (write_message(stopped_buf.buf, stopped_buf.len,
rebase_path_fixup_target(), 1)) {
goto out;
}
unlink(head_path);
ret = 1;

out:
strbuf_release(&head_buf);
strbuf_release(&stopped_buf);
return ret;
}

int sequencer_continue(struct repository *r, struct replay_opts *opts)
{
struct todo_list todo_list = TODO_LIST_INIT;
int res;
int res, skipped_fixup_target = 0;

if (read_and_refresh_cache(r, opts))
return -1;
Expand All @@ -5648,6 +5791,10 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
res = -1;
goto release_todo_list;
}
if ((skipped_fixup_target = check_fixup_target(r)) < 0) {
res = -1;
goto release_todo_list;
}
} else if (!file_exists(get_todo_path(opts)))
return continue_single_pick(r, opts);
else if ((res = read_populate_todo(r, &todo_list, opts)))
Expand All @@ -5671,7 +5818,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
struct strbuf buf = STRBUF_INIT;
struct object_id oid;

if (read_oneliner(&buf, rebase_path_stopped_sha(),
if (!skipped_fixup_target &&
read_oneliner(&buf, rebase_path_stopped_sha(),
READ_ONELINER_SKIP_IF_EMPTY) &&
!get_oid_hex(buf.buf, &oid))
record_in_rewritten(&oid, peek_command(&todo_list, 0));
Expand Down
51 changes: 48 additions & 3 deletions t/t3415-rebase-autosquash.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,9 +465,7 @@ test_expect_success 'abort last squash' '
rebase -ki --autosquash HEAD~4 &&
: do not finish the squash, but resolve it manually &&
git commit --allow-empty --amend -m edited-first &&
git rebase --skip &&
git show >actual &&
test_grep ! first actual
test_must_fail git rebase --skip
'

test_expect_success 'fixup a fixup' '
Expand Down Expand Up @@ -510,4 +508,51 @@ test_expect_success 'pick and fixup respect commit.cleanup' '
test_commit_message HEAD -m "something"
'

test_expect_success 'autosquash refuses to fix up a target skipped after conflict' '
test_when_finished "test_might_fail git rebase --abort" &&
git reset --hard base &&
test_commit target conflict-file target &&
target=$(git rev-parse HEAD) &&
test_commit middle middle-file middle &&
echo fixup >fixup-file &&
git add fixup-file &&
test_tick &&
git commit -m "fixup! target" &&
topic=$(git rev-parse HEAD) &&
git checkout -B autosquash-empty-conflict-upstream "$target^" &&
test_commit upstream conflict-file upstream &&
git checkout -B autosquash-empty-conflict-topic "$topic" &&
test_must_fail env GIT_SEQUENCE_EDITOR=: \
git rebase -i --autosquash \
--onto autosquash-empty-conflict-upstream "$target^" >actual 2>&1 &&
echo upstream >conflict-file &&
git add conflict-file &&
test_must_fail git rebase --continue >actual 2>&1 &&
test_grep "was not applied" actual &&
test_cmp_rev autosquash-empty-conflict-upstream HEAD &&
git rebase --abort
'

test_expect_success 'autosquash refuses to fix up a target already upstream' '
test_when_finished "test_might_fail git rebase --abort" &&
git reset --hard base &&
test_commit target-upstream upstream-file target &&
target=$(git rev-parse HEAD) &&
test_commit middle-upstream middle-file middle &&
echo fixup >fixup-file &&
git add fixup-file &&
test_tick &&
git commit -m "fixup! target-upstream" &&
topic=$(git rev-parse HEAD) &&
git checkout -B autosquash-already-upstream "$target^" &&
test_commit upstream-copy upstream-file target &&
git checkout -B autosquash-already-upstream-topic "$topic" &&
test_must_fail env GIT_SEQUENCE_EDITOR=: \
git rebase -i --autosquash --reapply-cherry-picks --empty=drop \
--onto autosquash-already-upstream "$target^" >actual 2>&1 &&
test_grep "was not applied" actual &&
test_cmp_rev autosquash-already-upstream HEAD &&
git rebase --abort
'

test_done
Loading