| Message ID | 20190329163009.493-2-phillip.wood123@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | A couple of cherry-pick related fixes | expand |
Phillip Wood <phillip.wood123@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When cherry-picking or reverting a sequence of commits and if the final > pick/revert has conflicts and the user uses `git commit` to commit the > conflict resolution and does not run `git cherry-pick --continue` then > the sequencer state is left behind. This can cause problems later. In my > case I cherry-picked a sequence of commits the last one of which I > committed with `git commit` after resolving some conflicts, then a while > later, on a different branch I aborted a revert which rewound my HEAD to > the end of the cherry-pick sequence on the previous branch. I've certainly seen this myself. Do you use command line prompt support to remind you of the operation in progress? I do, and I have a suspicion that it did not help me in this situation by ceasing to tell me that I have leftover state files after a manual commit of the final step that conflicted and gave control back to me. And detecting that we are finishing the last step and making sure that the state files are removed does sound like the right approach to fix it. > diff --git a/branch.c b/branch.c > index 28b81a7e02..9ed60081c1 100644 > --- a/branch.c > +++ b/branch.c > @@ -5,6 +5,7 @@ > #include "refs.h" > #include "refspec.h" > #include "remote.h" > +#include "sequencer.h" > #include "commit.h" > #include "worktree.h" > > @@ -339,8 +340,10 @@ void create_branch(struct repository *r, > > void remove_branch_state(struct repository *r) > { > - unlink(git_path_cherry_pick_head(r)); > - unlink(git_path_revert_head(r)); > + if (!unlink(git_path_cherry_pick_head(r))) > + sequencer_post_commit_cleanup(); > + if (!unlink(git_path_revert_head(r))) > + sequencer_post_commit_cleanup(); This and the same one in builtin/commit.c feels a bit iffy. If we had CHERRY_PICK_HEAD or REVERT_HEAD and attempted to remove one or the other, whether the removal succeeds or fails (perhaps a virus scanner on Windows had the file open while we tried to unlink it, causing the unlink() to fail), don't we want the clean-up to happen? > @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > if (amend && !no_post_rewrite) { > commit_post_rewrite(the_repository, current_head, &oid); > } > + This is an unrelated change. > if (!quiet) { > unsigned int flags = 0; > > diff --git a/sequencer.c b/sequencer.c > index 0db410d590..028699209f 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path) > return len; > } > > +void sequencer_post_commit_cleanup(void) > +{ > + struct replay_opts opts = REPLAY_OPTS_INIT; > + struct strbuf buf = STRBUF_INIT; > + const char *eol; > + const char *todo_path = git_path_todo_file(); > + > + if (strbuf_read_file(&buf, todo_path, 0) < 0) { > + if (errno == ENOENT) { > + return; > + } else { > + error_errno("unable to open '%s'", todo_path); > + return; > + } > + } > + /* If there is only one line then we are done */ > + eol = strchr(buf.buf, '\n'); > + if (!eol || !eol[1]) > + sequencer_remove_state(&opts); > + > + strbuf_release(&buf); > +} I find this helper doing a bit too much and a bit too little at the same time. To reduce the iffiness I mentioned earlier, the callers would behefit to have a helper that - notices the need to remove CHERRY_PICK_HEAD or REVERT_HEAD, and returns without doing anything if there is no need; - remove the *_HEAD file. - detect if we have dealt with the last step, and returns without doing any more thing if there are more to do; - remove the state files. IOW, replace the existing series of two unlink() calls with a single call to the helper. On the other hand, the bulk of hand-rolled logic to determine if we have processed the last step is better done in another helper function that helps this helper, i.e. void sequencer_post_commit_cleanup(struct repo *r) { int need_cleanup = 0; if (file_exists(git_path_cherry_pick_head(r)) { unlink(git_path_cherry_pick_head(r)); need_cleanup = 1; } if (file_exists(git_path_revert_head(r)) { unlink(git_path_revert_head(r)); need_cleanup = 1; } if (!need_cleanup) return; if (!have_finished_the_last_pick()) return; sequencer_remove_state(&opts); } as that makes it easier to follow the logic of what is going on at this level, while at the same time making the logic in the have_finished_the_last_pick() helper easier to read by giving it a meaningful name.
On Fri, Mar 29, 2019 at 11:32 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When cherry-picking or reverting a sequence of commits and if the final > pick/revert has conflicts and the user uses `git commit` to commit the > conflict resolution and does not run `git cherry-pick --continue` then > the sequencer state is left behind. This can cause problems later. In my > case I cherry-picked a sequence of commits the last one of which I > committed with `git commit` after resolving some conflicts, then a while > later, on a different branch I aborted a revert which rewound my HEAD to > the end of the cherry-pick sequence on the previous branch. Avoid this > potential problem by removing the sequencer state if we're committing or > resetting the final pick in a sequence. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > branch.c | 7 +++++-- > builtin/commit.c | 7 +++++-- > sequencer.c | 23 +++++++++++++++++++++++ > sequencer.h | 1 + > t/t3507-cherry-pick-conflict.sh | 19 +++++++++++++++++++ > 5 files changed, 53 insertions(+), 4 deletions(-) > > diff --git a/branch.c b/branch.c > index 28b81a7e02..9ed60081c1 100644 > --- a/branch.c > +++ b/branch.c > @@ -5,6 +5,7 @@ > #include "refs.h" > #include "refspec.h" > #include "remote.h" > +#include "sequencer.h" > #include "commit.h" > #include "worktree.h" > > @@ -339,8 +340,10 @@ void create_branch(struct repository *r, > > void remove_branch_state(struct repository *r) This function is also called in git-am, git-rebase and git-checkout. While the first two should not be affected, git-checkout can be executed while we're in the middle of a cherry-pick or revert. I guess that's ok because git-checkout is basically the same as git-reset in this case? > { > - unlink(git_path_cherry_pick_head(r)); > - unlink(git_path_revert_head(r)); > + if (!unlink(git_path_cherry_pick_head(r))) > + sequencer_post_commit_cleanup(); > + if (!unlink(git_path_revert_head(r))) > + sequencer_post_commit_cleanup(); > unlink(git_path_merge_head(r)); > unlink(git_path_merge_rr(r)); > unlink(git_path_merge_msg(r)); > diff --git a/builtin/commit.c b/builtin/commit.c > index 2986553d5f..422b7d62a5 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1657,8 +1657,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > die("%s", err.buf); > } > > - unlink(git_path_cherry_pick_head(the_repository)); > - unlink(git_path_revert_head(the_repository)); > + if (!unlink(git_path_cherry_pick_head(the_repository))) > + sequencer_post_commit_cleanup(); > + if (!unlink(git_path_revert_head(the_repository))) > + sequencer_post_commit_cleanup(); > unlink(git_path_merge_head(the_repository)); > unlink(git_path_merge_msg(the_repository)); > unlink(git_path_merge_mode(the_repository)); > @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > if (amend && !no_post_rewrite) { > commit_post_rewrite(the_repository, current_head, &oid); > } > + > if (!quiet) { > unsigned int flags = 0; > > diff --git a/sequencer.c b/sequencer.c > index 0db410d590..028699209f 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path) > return len; > } > > +void sequencer_post_commit_cleanup(void) > +{ > + struct replay_opts opts = REPLAY_OPTS_INIT; > + struct strbuf buf = STRBUF_INIT; > + const char *eol; > + const char *todo_path = git_path_todo_file(); > + > + if (strbuf_read_file(&buf, todo_path, 0) < 0) { > + if (errno == ENOENT) { > + return; > + } else { > + error_errno("unable to open '%s'", todo_path); _() the string to make it translatable. > + return; > + } > + } > + /* If there is only one line then we are done */ > + eol = strchr(buf.buf, '\n'); > + if (!eol || !eol[1]) > + sequencer_remove_state(&opts); Should we say something to let the user know cherry-pick/revert is finished? (unless --quiet is specified) > + > + strbuf_release(&buf); > +} > + > static int read_populate_todo(struct repository *r, > struct todo_list *todo_list, > struct replay_opts *opts)
Hi Junio On 01/04/2019 09:28, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> When cherry-picking or reverting a sequence of commits and if the final >> pick/revert has conflicts and the user uses `git commit` to commit the >> conflict resolution and does not run `git cherry-pick --continue` then >> the sequencer state is left behind. This can cause problems later. In my >> case I cherry-picked a sequence of commits the last one of which I >> committed with `git commit` after resolving some conflicts, then a while >> later, on a different branch I aborted a revert which rewound my HEAD to >> the end of the cherry-pick sequence on the previous branch. > > I've certainly seen this myself. Do you use command line prompt > support to remind you of the operation in progress? I do, and I > have a suspicion that it did not help me in this situation by > ceasing to tell me that I have leftover state files after a manual > commit of the final step that conflicted and gave control back to > me. Same here, the prompt I use just checks for the presence of CHERRY_PICK_HEAD which disappears after committing. > And detecting that we are finishing the last step and making sure > that the state files are removed does sound like the right approach > to fix it. > >> diff --git a/branch.c b/branch.c >> index 28b81a7e02..9ed60081c1 100644 >> --- a/branch.c >> +++ b/branch.c >> @@ -5,6 +5,7 @@ >> #include "refs.h" >> #include "refspec.h" >> #include "remote.h" >> +#include "sequencer.h" >> #include "commit.h" >> #include "worktree.h" >> >> @@ -339,8 +340,10 @@ void create_branch(struct repository *r, >> >> void remove_branch_state(struct repository *r) >> { >> - unlink(git_path_cherry_pick_head(r)); >> - unlink(git_path_revert_head(r)); >> + if (!unlink(git_path_cherry_pick_head(r))) >> + sequencer_post_commit_cleanup(); >> + if (!unlink(git_path_revert_head(r))) >> + sequencer_post_commit_cleanup(); > > This and the same one in builtin/commit.c feels a bit iffy. If we > had CHERRY_PICK_HEAD or REVERT_HEAD and attempted to remove one or > the other, whether the removal succeeds or fails (perhaps a virus > scanner on Windows had the file open while we tried to unlink it, > causing the unlink() to fail), don't we want the clean-up to happen? Good point, I could add '|| errno == ENOENT' to the if or just use file_exists() as you suggest below >> @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >> if (amend && !no_post_rewrite) { >> commit_post_rewrite(the_repository, current_head, &oid); >> } >> + > > This is an unrelated change. Oops I'm not sure where that came from >> if (!quiet) { >> unsigned int flags = 0; >> >> diff --git a/sequencer.c b/sequencer.c >> index 0db410d590..028699209f 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path) >> return len; >> } >> >> +void sequencer_post_commit_cleanup(void) >> +{ >> + struct replay_opts opts = REPLAY_OPTS_INIT; >> + struct strbuf buf = STRBUF_INIT; >> + const char *eol; >> + const char *todo_path = git_path_todo_file(); >> + >> + if (strbuf_read_file(&buf, todo_path, 0) < 0) { >> + if (errno == ENOENT) { >> + return; >> + } else { >> + error_errno("unable to open '%s'", todo_path); >> + return; >> + } >> + } >> + /* If there is only one line then we are done */ >> + eol = strchr(buf.buf, '\n'); >> + if (!eol || !eol[1]) >> + sequencer_remove_state(&opts); >> + >> + strbuf_release(&buf); >> +} > > I find this helper doing a bit too much and a bit too little at the > same time. To reduce the iffiness I mentioned earlier, the callers > would behefit to have a helper that > > - notices the need to remove CHERRY_PICK_HEAD or REVERT_HEAD, and > returns without doing anything if there is no need; > > - remove the *_HEAD file. > > - detect if we have dealt with the last step, and returns without > doing any more thing if there are more to do; > > - remove the state files. > > IOW, replace the existing series of two unlink() calls with a single > call to the helper. > > On the other hand, the bulk of hand-rolled logic to determine if we > have processed the last step is better done in another helper > function that helps this helper, i.e. > > void sequencer_post_commit_cleanup(struct repo *r) > { > int need_cleanup = 0; > > if (file_exists(git_path_cherry_pick_head(r)) { > unlink(git_path_cherry_pick_head(r)); > need_cleanup = 1; > } > if (file_exists(git_path_revert_head(r)) { > unlink(git_path_revert_head(r)); > need_cleanup = 1; > } > if (!need_cleanup) > return; > if (!have_finished_the_last_pick()) > return; > sequencer_remove_state(&opts); > } > > as that makes it easier to follow the logic of what is going on at > this level, while at the same time making the logic in the > have_finished_the_last_pick() helper easier to read by giving it a > meaningful name. Thanks that definitely improves things, I'll send a re-roll Best Wishes Phillip
On 01/04/2019 11:09, Duy Nguyen wrote: > On Fri, Mar 29, 2019 at 11:32 PM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> When cherry-picking or reverting a sequence of commits and if the final >> pick/revert has conflicts and the user uses `git commit` to commit the >> conflict resolution and does not run `git cherry-pick --continue` then >> the sequencer state is left behind. This can cause problems later. In my >> case I cherry-picked a sequence of commits the last one of which I >> committed with `git commit` after resolving some conflicts, then a while >> later, on a different branch I aborted a revert which rewound my HEAD to >> the end of the cherry-pick sequence on the previous branch. Avoid this >> potential problem by removing the sequencer state if we're committing or >> resetting the final pick in a sequence. >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> branch.c | 7 +++++-- >> builtin/commit.c | 7 +++++-- >> sequencer.c | 23 +++++++++++++++++++++++ >> sequencer.h | 1 + >> t/t3507-cherry-pick-conflict.sh | 19 +++++++++++++++++++ >> 5 files changed, 53 insertions(+), 4 deletions(-) >> >> diff --git a/branch.c b/branch.c >> index 28b81a7e02..9ed60081c1 100644 >> --- a/branch.c >> +++ b/branch.c >> @@ -5,6 +5,7 @@ >> #include "refs.h" >> #include "refspec.h" >> #include "remote.h" >> +#include "sequencer.h" >> #include "commit.h" >> #include "worktree.h" >> >> @@ -339,8 +340,10 @@ void create_branch(struct repository *r, >> >> void remove_branch_state(struct repository *r) > > This function is also called in git-am, git-rebase and git-checkout. > While the first two should not be affected, git-checkout can be > executed while we're in the middle of a cherry-pick or revert. I guess > that's ok because git-checkout is basically the same as git-reset in > this case? Yes that's right >> { >> - unlink(git_path_cherry_pick_head(r)); >> - unlink(git_path_revert_head(r)); >> + if (!unlink(git_path_cherry_pick_head(r))) >> + sequencer_post_commit_cleanup(); >> + if (!unlink(git_path_revert_head(r))) >> + sequencer_post_commit_cleanup(); >> unlink(git_path_merge_head(r)); >> unlink(git_path_merge_rr(r)); >> unlink(git_path_merge_msg(r)); >> diff --git a/builtin/commit.c b/builtin/commit.c >> index 2986553d5f..422b7d62a5 100644 >> --- a/builtin/commit.c >> +++ b/builtin/commit.c >> @@ -1657,8 +1657,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >> die("%s", err.buf); >> } >> >> - unlink(git_path_cherry_pick_head(the_repository)); >> - unlink(git_path_revert_head(the_repository)); >> + if (!unlink(git_path_cherry_pick_head(the_repository))) >> + sequencer_post_commit_cleanup(); >> + if (!unlink(git_path_revert_head(the_repository))) >> + sequencer_post_commit_cleanup(); >> unlink(git_path_merge_head(the_repository)); >> unlink(git_path_merge_msg(the_repository)); >> unlink(git_path_merge_mode(the_repository)); >> @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >> if (amend && !no_post_rewrite) { >> commit_post_rewrite(the_repository, current_head, &oid); >> } >> + >> if (!quiet) { >> unsigned int flags = 0; >> >> diff --git a/sequencer.c b/sequencer.c >> index 0db410d590..028699209f 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path) >> return len; >> } >> >> +void sequencer_post_commit_cleanup(void) >> +{ >> + struct replay_opts opts = REPLAY_OPTS_INIT; >> + struct strbuf buf = STRBUF_INIT; >> + const char *eol; >> + const char *todo_path = git_path_todo_file(); >> + >> + if (strbuf_read_file(&buf, todo_path, 0) < 0) { >> + if (errno == ENOENT) { >> + return; >> + } else { >> + error_errno("unable to open '%s'", todo_path); > > _() the string to make it translatable. Well spotted, thanks >> + return; >> + } >> + } >> + /* If there is only one line then we are done */ >> + eol = strchr(buf.buf, '\n'); >> + if (!eol || !eol[1]) >> + sequencer_remove_state(&opts); > > Should we say something to let the user know cherry-pick/revert is > finished? (unless --quiet is specified) I'd not thought of that, at the moment we don't say anything about removing CHERRY_PICK_HEAD etc when they are removed by reset or checkout, I'm not sure this is much different to those cases - but maybe they should be printing some feedback as well. Best Wishes Phillip >> + >> + strbuf_release(&buf); >> +} >> + >> static int read_populate_todo(struct repository *r, >> struct todo_list *todo_list, >> struct replay_opts *opts)
diff --git a/branch.c b/branch.c index 28b81a7e02..9ed60081c1 100644 --- a/branch.c +++ b/branch.c @@ -5,6 +5,7 @@ #include "refs.h" #include "refspec.h" #include "remote.h" +#include "sequencer.h" #include "commit.h" #include "worktree.h" @@ -339,8 +340,10 @@ void create_branch(struct repository *r, void remove_branch_state(struct repository *r) { - unlink(git_path_cherry_pick_head(r)); - unlink(git_path_revert_head(r)); + if (!unlink(git_path_cherry_pick_head(r))) + sequencer_post_commit_cleanup(); + if (!unlink(git_path_revert_head(r))) + sequencer_post_commit_cleanup(); unlink(git_path_merge_head(r)); unlink(git_path_merge_rr(r)); unlink(git_path_merge_msg(r)); diff --git a/builtin/commit.c b/builtin/commit.c index 2986553d5f..422b7d62a5 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1657,8 +1657,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) die("%s", err.buf); } - unlink(git_path_cherry_pick_head(the_repository)); - unlink(git_path_revert_head(the_repository)); + if (!unlink(git_path_cherry_pick_head(the_repository))) + sequencer_post_commit_cleanup(); + if (!unlink(git_path_revert_head(the_repository))) + sequencer_post_commit_cleanup(); unlink(git_path_merge_head(the_repository)); unlink(git_path_merge_msg(the_repository)); unlink(git_path_merge_mode(the_repository)); @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (amend && !no_post_rewrite) { commit_post_rewrite(the_repository, current_head, &oid); } + if (!quiet) { unsigned int flags = 0; diff --git a/sequencer.c b/sequencer.c index 0db410d590..028699209f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path) return len; } +void sequencer_post_commit_cleanup(void) +{ + struct replay_opts opts = REPLAY_OPTS_INIT; + struct strbuf buf = STRBUF_INIT; + const char *eol; + const char *todo_path = git_path_todo_file(); + + if (strbuf_read_file(&buf, todo_path, 0) < 0) { + if (errno == ENOENT) { + return; + } else { + error_errno("unable to open '%s'", todo_path); + return; + } + } + /* If there is only one line then we are done */ + eol = strchr(buf.buf, '\n'); + if (!eol || !eol[1]) + sequencer_remove_state(&opts); + + strbuf_release(&buf); +} + static int read_populate_todo(struct repository *r, struct todo_list *todo_list, struct replay_opts *opts) diff --git a/sequencer.h b/sequencer.h index 4d505b3590..43548295a1 100644 --- a/sequencer.h +++ b/sequencer.h @@ -144,3 +144,4 @@ int read_author_script(const char *path, char **name, char **email, char **date, void parse_strategy_opts(struct replay_opts *opts, char *raw_opts); int write_basic_state(struct replay_opts *opts, const char *head_name, const char *onto, const char *orig_head); +void sequencer_post_commit_cleanup(void); diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 0db166152a..69e6389e69 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -156,6 +156,25 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' ' test_must_fail git rev-parse --verify CHERRY_PICK_HEAD ' +test_expect_success 'successful final commit clears sequencer state' ' + pristine_detach initial && + + test_must_fail git cherry-pick base picked-signed && + echo resolved >foo && + test_path_is_file .git/sequencer/todo && + git commit -a && + test_must_fail test_path_exists .git/sequencer +' + +test_expect_success 'reset after final pick clears sequencer state' ' + pristine_detach initial && + + test_must_fail git cherry-pick base picked-signed && + echo resolved >foo && + test_path_is_file .git/sequencer/todo && + git reset && + test_must_fail test_path_exists .git/sequencer +' test_expect_success 'failed cherry-pick produces dirty index' ' pristine_detach initial &&