BUG: "cherry-pick A..B || git reset --hard OTHER"
I was burned a few times with this in the past few years, but it did not irritate me often enough that I didn't write it down. But I think this is serious enough that deserves attention from those who were involved.
A short reproduction recipe, using objects from git.git that are publicly available and stable, shows how bad it is.
$ git checkout v2.9.3^0
$ git cherry-pick 0582a34f52..a94bb68397
... see conflict, decide to give up backporting to
... such an old fork point.
... the git-prompt gives "|CHERRY-PICKING" correctly. $ git reset --hard v2.10.2^0
... the git-prompt no longer says "|CHERRY-PICKING" $ edit && git commit -m "prelim work for backporting"
[detached HEAD cc5a6a9219] prelim work for backporting $ git cherry-pick 0582a34f52..a94bb68397
error: a cherry-pick or revert is already in progress
hint: try "git cherry-pick (--continue | --quit | --abort)"
fatal: cherry-pick failed $ git cherry-pick --abort
... we come back to v2.9.3^0, losing the new commit!The above shows two bugs.
(1) The third invocation of "cherry-pick" with "--abort" to get rid of the state from the unfinished cherry-pick we did previously is necessary, but the command does not notice that we resetted to a new branch AND we even did some other work there. This loses end-user's work.
"git cherry-pick --abort" should learn from "git am --abort" that has an extra safety to deal with the above workflow. The state from the unfinished "am" is removed, but the head is not rewound to avoid losing end-user's work.
You can try by replacing two instances of
$ git cherry-pick 0582a34f52..a94bb68397
with
$ git format-patch --stdout 0582a34f52..a94bb68397 | git am
in the above sequence, and conclude with "git am--abort" to see how much more pleasant and safe "git am --abort" is.
(2) The bug in "cherry-pick --abort" is made worse because the git-completion script seems to be unaware of $GIT_DIR/sequencer and stops saying "|CHERRY-PICKING" after the step to switch to a different state with "git reset --hard v2.10.2^0". If the prompt showed it after "git reset", the end user would have thought twice before starting the "prelim work".
Thanks.
Quoted 4 lines · Junio C Hamano
I was burned a few times with this in the past few years, but it did not irritate me often enough that I didn't write it down. But I think this is serious enough that deserves attention from those who were involved.
Yeah, I agree that we should do something about it.
Quoted 39 lines
A short reproduction recipe, using objects from git.git that are publicly available and stable, shows how bad it is.
$ git checkout v2.9.3^0$ git cherry-pick 0582a34f52..a94bb68397 ... see conflict, decide to give up backporting to ... such an old fork point. ... the git-prompt gives "|CHERRY-PICKING" correctly.$ git reset --hard v2.10.2^0 ... the git-prompt no longer says "|CHERRY-PICKING"$ edit && git commit -m "prelim work for backporting" [detached HEAD cc5a6a9219] prelim work for backporting$ git cherry-pick 0582a34f52..a94bb68397 error: a cherry-pick or revert is already in progress hint: try "git cherry-pick (--continue | --quit | --abort)" fatal: cherry-pick failed$ git cherry-pick --abort ... we come back to v2.9.3^0, losing the new commit!The above shows two bugs.
(1) The third invocation of "cherry-pick" with "--abort" to get rid of the state from the unfinished cherry-pick we did previously is necessary, but the command does not notice that we resetted to a new branch AND we even did some other work there. This loses end-user's work.
"git cherry-pick --abort" should learn from "git am --abort" that has an extra safety to deal with the above workflow. The state from the unfinished "am" is removed, but the head is not rewound to avoid losing end-user's work.
You can try by replacing two instances of
Quoted 8 lines
$ git cherry-pick
0582a34f52..a94bb68397with
$ git format-patch --stdout
0582a34f52..a94bb68397| git amin the above sequence, and conclude with "git am--abort" to see how much more pleasant and safe "git am --abort" is.
Ok I will try to take a look at that next week.
Quoted 6 lines
(2) The bug in "cherry-pick --abort" is made worse because the git-completion script seems to be unaware of $GIT_DIR/sequencer and stops saying "|CHERRY-PICKING" after the step to switch to a different state with "git reset --hard v2.10.2^0". If the prompt showed it after "git reset", the end user would have thought twice before starting the "prelim work".
I am not used to hacking the prompt or completion scripts, so I would appreciate if someone else could do it.
Thanks,
Christian.
Hi,
Quoted 21 lines · Junio C Hamano
I was burned a few times with this in the past few years, but it did not irritate me often enough that I didn't write it down. But I think this is serious enough that deserves attention from those who were involved.
A short reproduction recipe, using objects from git.git that are publicly available and stable, shows how bad it is.
$ git checkout v2.9.3^0$ git cherry-pick 0582a34f52..a94bb68397 ... see conflict, decide to give up backporting to ... such an old fork point. ... the git-prompt gives "|CHERRY-PICKING" correctly.$ git reset --hard v2.10.2^0 ... the git-prompt no longer says "|CHERRY-PICKING"$ edit && git commit -m "prelim work for backporting" [detached HEAD cc5a6a9219] prelim work for backporting
Quoted 7 lines
$ git cherry-pick 0582a34f52..a94bb68397 error: a cherry-pick or revert is already in progress hint: try "git cherry-pick (--continue | --quit | --abort)" fatal: cherry-pick failed$ git cherry-pick --abort ... we come back to v2.9.3^0, losing the new commit!
Apart from the git-prompt bug: isn't this a user error? I think "git cherry-pick --quit"[1] would be the right thing to do, not --abort.
On the other hand, one (as a user) could also expect that "git reset --hard" also resets sequencer-related states (and that is what the git-prompt suggests), but that would probably break a lot of scripts ;)
[1] By the way: git cherry-pick --quit, git rebase --forget ... different wording for the same thing makes things unintuitive.
Quoted 13 lines
(1) The third invocation of "cherry-pick" with "--abort" to get rid of the state from the unfinished cherry-pick we did previously is necessary, but the command does not notice that we resetted to a new branch AND we even did some other work there. This loses end-user's work.
"git cherry-pick --abort" should learn from "git am --abort" that has an extra safety to deal with the above workflow. The state from the unfinished "am" is removed, but the head is not rewound to avoid losing end-user's work.
You can try by replacing two instances of
Quoted 8 lines
$ git cherry-pick
0582a34f52..a94bb68397with
$ git format-patch --stdout
0582a34f52..a94bb68397| git amin the above sequence, and conclude with "git am--abort" to see how much more pleasant and safe "git am --abort" is.
Definitely. I'd volunteer to add that safety guard. (But (2) remains.)
~Stephan
Quoted 2 lines · Stephan Beyer
[1] By the way: git cherry-pick --quit, git rebase --forget ... different wording for the same thing makes things unintuitive.
It is not too late to STOP "--forget" from getting added to "rebase" and give it a better name.
Having said that, I have a feeling that these options do not have to exist; isn't their presence just a symptom that the "--abort" for the command misbehaves? Isn't the reason why there is no need for "am --quit" because its "--abort" behaves more sensibly?
Hi,
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · Stephan Beyer
[1] By the way: git cherry-pick --quit, git rebase --forget ... different wording for the same thing makes things unintuitive.
It is not too late to STOP "--forget" from getting added to "rebase" and give it a better name.
Oh. ;) I am not sure. I personally think that --forget is a better name than --quit because when I hear --quit I tend to look into the manual page first to check if there are weird side effects (and then the manual page says that it "forgets" ;D).
So I'd rather favor adding --forget to cherry-pick/revert instead... or this:
Quoted 4 lines
Having said that, I have a feeling that these options do not have to exist; isn't their presence just a symptom that the "--abort" for the command misbehaves? Isn't the reason why there is no need for "am --quit" because its "--abort" behaves more sensibly?
You're probably right. I have no other use-case in mind than "oh I forgot that I was rebasing... now just abort that and don't bother me further (i.e. please don't bring me back)"
~Stephan
Quoted 10 lines · Stephan Beyer
Hi,
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · Stephan Beyer
[1] By the way: git cherry-pick --quit, git rebase --forget ... different wording for the same thing makes things unintuitive.
It is not too late to STOP "--forget" from getting added to "rebase" and give it a better name.
Sorry I didn't know about --quit (and it has been there since 2011, I guess I'm just not big sequencer user).
Quoted 1 line
Oh. ;) I am not sure. I personally think that --forget is a better name
Yeah, I was stuck with the name --destroy for many months and was very happy the day I found --forget, which does not imply any destructive side effects and is distinct enough from --abort to not confuse people.
Quoted 5 lines
than --quit because when I hear --quit I tend to look into the manual page first to check if there are weird side effects (and then the manual page says that it "forgets" ;D).
So I'd rather favor adding --forget to cherry-pick/revert instead... or this:
Signature · 1 line
Duy
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · Stephan Beyer
[1] By the way: git cherry-pick --quit, git rebase --forget ... different wording for the same thing makes things unintuitive.
It is not too late to STOP "--forget" from getting added to "rebase" and give it a better name.
Having the same operation with different names only increases git reputation of bad/inconsistent UI. Either forget is renamed to quit, or vice versa. I prefer forget, but the decision is yours and the community's. So I'm sending two patches to rename in either direction. You can pick one.
Signature · 1 line
Duy
PATCH 1/1 rebase: rename --forget to be consistent with sequencer
--- Documentation/git-rebase.txt | 4 ++-- contrib/completion/git-completion.bash | 4 ++-- git-rebase.sh | 6 +++--- t/t3407-rebase-abort.sh | 8 ++++---- 4 files changed, 11 insertions(+), 11 deletions(-)
Documentation/git-rebase.txt +2 -2
@@ -12,7 +12,7 @@ SYNOPSIS [<upstream> [<branch>]] 'git rebase' [-i | --interactive] [options] [--exec <cmd>] [--onto <newbase>] --root [<branch>] -'git rebase' --continue | --skip | --abort | --forget | --edit-todo +'git rebase' --continue | --skip | --abort | --quit | --edit-todo DESCRIPTION -----------
@@ -252,7 +252,7 @@ leave out at most one of A and B, in which case it defaults to HEAD. will be reset to where it was when the rebase operation was started. ---forget:: +--quit:: Abort the rebase operation but HEAD is not reset back to the original branch. The index and working tree are also left unchanged as a result.
contrib/completion/git-completion.bash +2 -2
@@ -1670,10 +1670,10 @@ _git_rebase () { local dir="$(__gitdir)" if [ -f "$dir"/rebase-merge/interactive ]; then - __gitcomp "--continue --skip --abort --forget --edit-todo" + __gitcomp "--continue --skip --abort --quit --edit-todo" return elif [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then - __gitcomp "--continue --skip --abort --forget" + __gitcomp "--continue --skip --abort --quit" return fi __git_complete_strategy && return
git-rebase.sh +3 -3
@@ -43,7 +43,7 @@ continue! continue abort! abort and check out the original branch skip! skip current patch and continue edit-todo! edit the todo list during an interactive rebase -forget! abort but keep HEAD where it is +quit! abort but keep HEAD where it is " . git-sh-setup . git-sh-i18n
@@ -240,7 +240,7 @@ do --verify) ok_to_skip_pre_rebase= ;; - --continue|--skip|--abort|--forget|--edit-todo) + --continue|--skip|--abort|--quit|--edit-todo) test $total_argc -eq 2 || usage action=${1##--} ;;
@@ -403,7 +403,7 @@ abort) finish_rebase exit ;; -forget) +quit) exec rm -rf "$state_dir" ;; edit-todo)
t/t3407-rebase-abort.sh +4 -4
@@ -99,26 +99,26 @@ testrebase() { testrebase "" .git/rebase-apply testrebase " --merge" .git/rebase-merge -test_expect_success 'rebase --forget' ' +test_expect_success 'rebase --quit' ' cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase master && test_path_is_dir .git/rebase-apply && head_before=$(git rev-parse HEAD) && - git rebase --forget && + git rebase --quit && test $(git rev-parse HEAD) = $head_before && test ! -d .git/rebase-apply ' -test_expect_success 'rebase --merge --forget' ' +test_expect_success 'rebase --merge --quit' ' cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase --merge master && test_path_is_dir .git/rebase-merge && head_before=$(git rev-parse HEAD) && - git rebase --forget && + git rebase --quit && test $(git rev-parse HEAD) = $head_before && test ! -d .git/rebase-merge '
Signature · 1 line
2.8.2.524.g6ff3d78
PATCH 1/1 revert, cherry-pick: rename --quit to be consistent with rebase
The old --quit remains supported, just hidden away.
--- Documentation/git-cherry-pick.txt | 2 +- Documentation/git-revert.txt | 2 +- Documentation/sequencer.txt | 2 +- builtin/revert.c | 7 +++++-- contrib/completion/git-completion.bash | 4 ++-- sequencer.c | 2 +- t/t3510-cherry-pick-sequence.sh | 14 +++++++------- t/t3511-cherry-pick-x.sh | 2 +- 8 files changed, 19 insertions(+), 16 deletions(-)
Documentation/git-cherry-pick.txt +1 -1
@@ -11,7 +11,7 @@ SYNOPSIS 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] [-S[<keyid>]] <commit>... 'git cherry-pick' --continue -'git cherry-pick' --quit +'git cherry-pick' --forget 'git cherry-pick' --abort DESCRIPTION
Documentation/git-revert.txt +1 -1
@@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] [-S[<keyid>]] <commit>... 'git revert' --continue -'git revert' --quit +'git revert' --forget 'git revert' --abort DESCRIPTION
Documentation/sequencer.txt +1 -1
@@ -3,7 +3,7 @@ '.git/sequencer'. Can be used to continue after resolving conflicts in a failed cherry-pick or revert. ---quit:: +--forget:: Forget about the current operation in progress. Can be used to clear the sequencer state after a failed cherry-pick or revert.
builtin/revert.c +5 -2
@@ -77,7 +77,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)const char *me =action_name (opts);int cmd =0 ;struct option options[] = { -OPT_CMDMODE (0 ,"quit" , &cmd,N_ ("end revert or cherry-pick sequence" ),'q' ), +OPT_CMDMODE (0 ,"forget" , &cmd,N_ ("end revert or cherry-pick sequence" ),'q' ), + {OPTION_CMDMODE ,0 ,"quit" , &cmd,NULL , +N_ ("end revert or cherry-pick sequence" ), +PARSE_OPT_NOARG |PARSE_OPT_NONEG |PARSE_OPT_HIDDEN ,NULL ,'q' },OPT_CMDMODE (0 ,"continue" , &cmd,N_ ("resume revert or cherry-pick sequence" ),'c' ),OPT_CMDMODE (0 ,"abort" , &cmd,N_ ("cancel revert or cherry-pick sequence" ),'a' ),OPT_BOOL ('n' ,"no-commit" , &opts->no_commit,N_ ("don't automatically commit" )),
@@ -134,7 +137,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)if (opts->subcommand !=REPLAY_NONE ) {char *this_operation;if (opts->subcommand ==REPLAY_REMOVE_STATE ) - this_operation ="--quit" ; + this_operation ="--forget" ;else if (opts->subcommand ==REPLAY_CONTINUE ) this_operation ="--continue" ;else {
contrib/completion/git-completion.bash +2 -2
@@ -1047,7 +1047,7 @@ _git_cherry_pick () { local dir="$(__gitdir)" if [ -f "$dir"/CHERRY_PICK_HEAD ]; then - __gitcomp "--continue --quit --abort" + __gitcomp "--continue --forget --abort" return fi case "$cur" in
@@ -2303,7 +2303,7 @@ _git_revert () { local dir="$(__gitdir)" if [ -f "$dir"/REVERT_HEAD ]; then - __gitcomp "--continue --quit --abort" + __gitcomp "--continue --forget --abort" return fi case "$cur" in
sequencer.c +1 -1
@@ -812,7 +812,7 @@ static int create_seq_dir(void) {if (file_exists (git_path_seq_dir ())) {error (_ ("a cherry-pick or revert is already in progress" )); -advise (_ ("try \"git cherry-pick (--continue | --quit | --abort)\"" )); +advise (_ ("try \"git cherry-pick (--continue | --forget | --abort)\"" ));return -1 ; }else if (mkdir (git_path_seq_dir (),0777 ) <0 )
t/t3510-cherry-pick-sequence.sh +7 -7
@@ -18,7 +18,7 @@ test_description='Test cherry-pick continuation features _r10='\1\1\1\1\1\1\1\1\1\1' pristine_detach () { - git cherry-pick --quit && + git cherry-pick --forget && git checkout -f "$1^0" && git read-tree -u --reset HEAD && git clean -d -f -f -q -x
@@ -89,9 +89,9 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' ' test_path_is_missing .git/sequencer ' -test_expect_success '--quit does not complain when no cherry-pick is in progress' ' +test_expect_success '--forget does not complain when no cherry-pick is in progress' ' pristine_detach initial && - git cherry-pick --quit + git cherry-pick --forget ' test_expect_success '--abort requires cherry-pick in progress' '
@@ -99,14 +99,14 @@ test_expect_success '--abort requires cherry-pick in progress' ' test_must_fail git cherry-pick --abort ' -test_expect_success '--quit cleans up sequencer state' ' +test_expect_success '--forget cleans up sequencer state' ' pristine_detach initial && test_expect_code 1 git cherry-pick base..picked && - git cherry-pick --quit && + git cherry-pick --forget && test_path_is_missing .git/sequencer ' -test_expect_success '--quit keeps HEAD and conflicted index intact' ' +test_expect_success '--forget keeps HEAD and conflicted index intact' ' pristine_detach initial && cat >expect <<-\EOF && OBJID
@@ -116,7 +116,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' ' :000000 100644 OBJID OBJID A unrelated EOF test_expect_code 1 git cherry-pick base..picked && - git cherry-pick --quit && + git cherry-pick --forget && test_path_is_missing .git/sequencer && test_must_fail git update-index --refresh && {
t/t3511-cherry-pick-x.sh +1 -1
@@ -5,7 +5,7 @@ test_description='Test cherry-pick -x and -s' . ./test-lib.sh pristine_detach () { - git cherry-pick --quit && + git cherry-pick --forget && git checkout -f "$1^0" && git read-tree -u --reset HEAD && git clean -d -f -f -q -x
Signature · 1 line
2.8.2.524.g6ff3d78
Quoted 6 lines · Duy Nguyen
Quoted 5 lines · Junio C Hamano
Quoted 2 lines · Stephan Beyer
[1] By the way: git cherry-pick --quit, git rebase --forget ... different wording for the same thing makes things unintuitive.
Quoted 8 lines
Quoted 2 lines
It is not too late to STOP "--forget" from getting added to "rebase" and give it a better name.
Having the same operation with different names only increases git reputation of bad/inconsistent UI. Either forget is renamed to quit, or vice versa. I prefer forget, but the decision is yours and the community's. So I'm sending two patches to rename in either direction. You can pick one.
I actually was advocating to remove both by making --abort saner. With an updated --abort that behaves saner, is "rebase --forget" still necessary?
Hi Junio,
Quoted 10 lines · Junio C Hamano
Quoted 5 lines · Duy Nguyen
Having the same operation with different names only increases git reputation of bad/inconsistent UI. Either forget is renamed to quit, or vice versa. I prefer forget, but the decision is yours and the community's. So I'm sending two patches to rename in either direction. You can pick one.
I actually was advocating to remove both by making --abort saner. With an updated --abort that behaves saner, is "rebase --forget" still necessary?
A quick change in t3407 of the "rebase --forget" test to use "rebase --abort" failed. That's because it checks the use-case of forgetting/aborting without changing the HEAD. So --abort makes a rollback, --forget just keeps the current head. I am not sure if that tested use-case is a real use-case though.
A quick change in the pristine_detach function in t3510 and t3511 from "cherry-pick --quit" to "cherry-pick --abort" works when one ignores the return value of "cherry-pick --abort". The "--quit" is used here to ensure a clean cherry-pick state, and --quit always succeeds, even if no cherry-pick is in progress. That may be a real use-case somehow that could also be used for "rebase --forget"
t3510 also shows another use-case for --quit: the title says it all:
"cherry-pick --quit" to "cherry-pick --abort"
With this additional information, I'd vote to keep --quit/--forget and just make it consistent.
~Stephan
Quoted 2 lines · Stephan Beyer
t3510 also shows another use-case for --quit: the title says it all:
"cherry-pick --quit" to "cherry-pick --abort"
I should've read what I actually pasted.
I wanted to paste: '--quit keeps HEAD and conflicted index intact'
Sorry for making no sense ;)
Quoted 2 lines
With this additional information, I'd vote to keep --quit/--forget and just make it consistent.
Now!
~Stephan
Quoted 14 lines · Stephan Beyer
Hi Junio,
Quoted 10 lines · Junio C Hamano
Quoted 5 lines · Duy Nguyen
Having the same operation with different names only increases git reputation of bad/inconsistent UI. Either forget is renamed to quit, or vice versa. I prefer forget, but the decision is yours and the community's. So I'm sending two patches to rename in either direction. You can pick one.
I actually was advocating to remove both by making --abort saner. With an updated --abort that behaves saner, is "rebase --forget" still necessary?
Quoted 5 lines
A quick change in t3407 of the "rebase --forget" test to use "rebase --abort" failed. That's because it checks the use-case of forgetting/aborting without changing the HEAD. So --abort makes a rollback, --forget just keeps the current head. I am not sure if that tested use-case is a real use-case though.
It is. I wanted something like this for years but "rm -rf /path/to/.git/rebase*" was not as bad when there were no linked worktrees.
rebase and cherry-pick/revert are not exactly in the same situation. When cherry-pick/revert in "continue/abort" mode, there's usually some conflicted files and it's easy to notice.
But an interactive rebase could stop at some commit with clean worktree (the 'edit' command). Then I could even add some more commits on top. I don't see how 'rebase --abort' can know my intention in this case, whether I tried (with some new commits) and failed, and want to revert/abort the whole thing, moving HEAD back to the original; or whether I forgot I was in the middle of rebase and started to do something else, and --abort needs to keep HEAD where it is.
Signature · 1 line
Duy
Quoted 11 lines · Duy Nguyen
rebase and cherry-pick/revert are not exactly in the same situation. When cherry-pick/revert in "continue/abort" mode, there's usually some conflicted files and it's easy to notice.
But an interactive rebase could stop at some commit with clean worktree (the 'edit' command). Then I could even add some more commits on top. I don't see how 'rebase --abort' can know my intention in this case, whether I tried (with some new commits) and failed, and want to revert/abort the whole thing, moving HEAD back to the original; or whether I forgot I was in the middle of rebase and started to do something else, and --abort needs to keep HEAD where it is.
OK.
Hi,
Quoted 19 lines · Stephan Beyer
Hi,
Quoted 15 lines · Junio C Hamano
(1) The third invocation of "cherry-pick" with "--abort" to get rid of the state from the unfinished cherry-pick we did previously is necessary, but the command does not notice that we resetted to a new branch AND we even did some other work there. This loses end-user's work.
"git cherry-pick --abort" should learn from "git am --abort" that has an extra safety to deal with the above workflow. The state from the unfinished "am" is removed, but the head is not rewound to avoid losing end-user's work.
You can try by replacing two instances of
$ git cherry-pick
0582a34f52..a94bb68397
Quoted 7 lines
Quoted 6 lines
with
$ git format-patch --stdout
0582a34f52..a94bb68397| git amin the above sequence, and conclude with "git am--abort" to see how much more pleasant and safe "git am --abort" is.
Definitely. I'd volunteer to add that safety guard. (But (2) remains.)
That would be great if you could take care of that!
Thanks,
Christian.
PATCH 1/5 am: Fix filename in safe_to_abort() error message
In reply to Junio C Hamano---
Okay let's give it a try. Some minor things that I found are also in this patchset (patch 01, 02 and 05). The branch can also be found on
https://github.com/sbeyer/git/commits/sequencer-abort-safety
builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/am.c +1 -1
@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)if (read_state_file (&sb, state,"abort-safety" ,1 ) >0 ) {if (get_oid_hex (sb.buf, &abort_safety)) -die (_ ("could not parse %s" ),am_path (state,"abort_safety" )); +die (_ ("could not parse %s" ),am_path (state,"abort-safety" )); }else oidclr (&abort_safety);
Signature · 1 line
2.11.0.27.g4eed97c
PATCH 2/5 am: Change safe_to_abort()'s not rewinding error into a warning
The error message tells the user that something went terribly wrong and the --abort could not be performed. But the --abort is performed, only without rewinding. By simply changing the error into a warning, we indicate the user that she must not try something like "git am --abort --force", instead she just has to check the HEAD.
--- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/am.c +1 -1
@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)if (!oidcmp (&head, &abort_safety))return 1 ; -error (_ ("You seem to have moved HEAD since the last 'am' failure.\n" +warning (_ ("You seem to have moved HEAD since the last 'am' failure.\n" "Not rewinding to ORIG_HEAD" ));return 0 ;
Signature · 1 line
2.11.0.27.g4eed97c
PATCH 3/5 Add test that cherry-pick --abort does not unsafely change HEAD
In reply to Stephan Beyer--- t/t3510-cherry-pick-sequence.sh | 10 ++++++++++ 1 file changed, 10 insertions(+)
t/t3510-cherry-pick-sequence.sh +10 -0
@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' +test_expect_success '--abort does not unsafely change HEAD' ' + pristine_detach initial && + test_must_fail git cherry-pick picked anotherpick && + git reset --hard base && + test_must_fail git cherry-pick picked anotherpick && + git cherry-pick --abort 2>actual && + test_i18ngrep "You seem to have moved HEAD" actual && + test_cmp_rev base HEAD +' + test_expect_success 'cherry-pick --abort to cancel multiple revert' ' pristine_detach anotherpick && test_expect_code 1 git revert base..picked &&
Signature · 1 line
2.11.0.27.g4eed97c
PATCH 4/5 Make sequencer abort safer
In reply to Stephan BeyerIn contrast to "git am --abort", a sequencer abort did not check whether the current HEAD is the one that is expected. This can lead to loss of work (when not spotted and resolved using reflog before the garbage collector chimes in).
This behavior is now changed by mimicking "git am --abort":
the abortion is done but HEAD is not changed when the current HEAD is not the expected HEAD.
A new file "sequencer/current" is added to save the expected HEAD.
The new behavior is only active when --abort is invoked on multiple picks. The problem does not occur for the single-pick case because it is handled differently.
--- sequencer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
sequencer.c +49 -0
@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")/* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)return -1 ; } +static void update_curr_file () +{ +struct object_id head; + +/* Do nothing on a single-pick */ +if (!file_exists (git_path_seq_dir ())) +return ; + +if (!get_oid ("HEAD" , &head)) +write_file (git_path_curr_file (),"%s" ,oid_to_hex (&head)); +else +write_file (git_path_curr_file (),"%s" ,"" ); +} +static int fast_forward_to (const unsigned char *to,const unsigned char *from,int unborn,struct replay_opts *opts) {
@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,strbuf_release (&sb);strbuf_release (&err);ref_transaction_free (transaction); +update_curr_file ();return 0 ; }
@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, leave:free_message (commit, &msg); +update_curr_file ();return res; }
@@ -1132,9 +1149,34 @@ static int save_head(const char *head)return 0 ; } +static int rollback_is_safe () +{ +struct strbuf sb =STRBUF_INIT ; +struct object_id expected_head, actual_head; + +if (strbuf_read_file (&sb,git_path_curr_file (),0 ) >=0 ) { +strbuf_trim (&sb); +if (get_oid_hex (sb.buf, &expected_head)) { +strbuf_release (&sb); +die (_ ("could not parse %s" ),git_path_curr_file ()); + } +strbuf_release (&sb); + } +else if (errno ==ENOENT ) +oidclr (&expected_head); +else +die_errno (_ ("could not read '%s'" ),git_path_curr_file ()); + +if (get_oid ("HEAD" , &actual_head)) +oidclr (&actual_head); + +return !oidcmp (&actual_head, &expected_head); +} +static int reset_for_rollback (const unsigned char *sha1) {const char *argv[4 ];/* reset --merge <arg> + NULL */ + argv[0 ] ="reset" ; argv[1 ] ="--merge" ; argv[2 ] =sha1_to_hex (sha1);
@@ -1189,6 +1231,12 @@ int sequencer_rollback(struct replay_opts *opts)error (_ ("cannot abort from a branch yet to be born" )); goto fail; } + +if (!rollback_is_safe ()) { +/* Do not error, just do not rollback */ +warning (_ ("You seem to have moved HEAD. " +"Not rewinding, check your HEAD!" )); + }else if (reset_for_rollback (sha1)) goto fail;strbuf_release (&buf);
@@ -1393,6 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)return -1 ;if (save_opts (opts))return -1 ; +update_curr_file (); res =pick_commits (&todo_list, opts);todo_list_release (&todo_list);return res;
Signature · 1 line
2.11.0.27.g4eed97c
Re: [PATCH 4/5] Make sequencer abort safer
Hi,
sequencer.c GIT_PATH_FUNC(git_path_seq_dir, "sequencer") · line 30
Quoted 9 lines · Stephan Beyer
sequencer.c +1
-0@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
Is it required by law to have a four-letter infix, or can we have a nicer variable name (e.g. git_path_current_file)?
Ciao,
Dscho
Re: [PATCH 4/5] Make sequencer abort safer
Quoted 14 lines · Johannes Schindelin
sequencer.c GIT_PATH_FUNC(git_path_seq_dir, "sequencer") · line 30
Quoted 9 lines · Stephan Beyer
sequencer.c +1
-0@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")Is it required by law to have a four-letter infix, or can we have a nicer variable name (e.g.
git_path_current_file)?
I agree with you that, as other git_path_*_file variables match the actual name on the filesystem, this one should too, together with the update_curr_file() function.
By the way, this step seems to be a fix to an existing problem, and the new test added in 3/5 seems to be a demonstration of the issue. If that is the case, shouldn't the new test initially expect failure and updated by this step to expect success?
I'll queue this on top of step 4/5 as "SQUASH???" as usual. The other SQUASH??? that must come after 3/5 for t3510 should be trivial (the reverse of what appears here).
Thanks.
sequencer.c | 22 +++++++++++----------- t/t3510-cherry-pick-sequence.sh | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-)
sequencer.c +11 -11
@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") -static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current") +static GIT_PATH_FUNC(git_path_current_file, "sequencer/current")/* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -311,7 +311,7 @@ static int error_dirty_index(struct replay_opts *opts)return -1 ; } -static void update_curr_file () +static void update_current_file (void ) {struct object_id head;
@@ -320,9 +320,9 @@ static void update_curr_file()return ;if (!get_oid ("HEAD" , &head)) -write_file (git_path_curr_file (),"%s" ,oid_to_hex (&head)); +write_file (git_path_current_file (),"%s" ,oid_to_hex (&head));else -write_file (git_path_curr_file (),"%s" ,"" ); +write_file (git_path_current_file (),"%s" ,"" ); } static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -354,7 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,strbuf_release (&sb);strbuf_release (&err);ref_transaction_free (transaction); -update_curr_file (); +update_current_file ();return 0 ; }
@@ -829,7 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, leave:free_message (commit, &msg); -update_curr_file (); +update_current_file ();return res; }
@@ -1149,23 +1149,23 @@ static int save_head(const char *head)return 0 ; } -static int rollback_is_safe () +static int rollback_is_safe (void ) {struct strbuf sb =STRBUF_INIT ;struct object_id expected_head, actual_head; -if (strbuf_read_file (&sb,git_path_curr_file (),0 ) >=0 ) { +if (strbuf_read_file (&sb,git_path_current_file (),0 ) >=0 ) {strbuf_trim (&sb);if (get_oid_hex (sb.buf, &expected_head)) {strbuf_release (&sb); -die (_ ("could not parse %s" ),git_path_curr_file ()); +die (_ ("could not parse %s" ),git_path_current_file ()); }strbuf_release (&sb); }else if (errno ==ENOENT )oidclr (&expected_head);else -die_errno (_ ("could not read '%s'" ),git_path_curr_file ()); +die_errno (_ ("could not read '%s'" ),git_path_current_file ());if (get_oid ("HEAD" , &actual_head))oidclr (&actual_head);
@@ -1441,7 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)return -1 ;if (save_opts (opts))return -1 ; -update_curr_file (); +update_current_file (); res =pick_commits (&todo_list, opts);todo_list_release (&todo_list);return res;
t/t3510-cherry-pick-sequence.sh +1 -1
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' -test_expect_failure '--abort does not unsafely change HEAD' ' +test_expect_success '--abort does not unsafely change HEAD' ' pristine_detach initial && test_must_fail git cherry-pick picked anotherpick && git reset --hard base &&
Re: [PATCH 4/5] Make sequencer abort safer
Hi,
I'm a little afraid of feeding Parkinson's law of triviality here, but... ;)
Quoted 17 lines · Junio C Hamano
Quoted 14 lines · Johannes Schindelin
sequencer.c GIT_PATH_FUNC(git_path_seq_dir, "sequencer") · line 30
Quoted 9 lines · Stephan Beyer
sequencer.c +1
-0@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")Is it required by law to have a four-letter infix, or can we have a nicer variable name (e.g.
git_path_current_file)?
Quoted 3 lines
I agree with you that, as other git_path_*_file variables match the actual name on the filesystem, this one should too, together with the
update_curr_file()function.
I totally agree with that (and I don't know why I used "curr", probably just because it looked consistent and good...).
However:
sequencer.c static int error_dirty_index(struct replay_opts *opts) · line 314
Quoted 2 lines
sequencer.c +1
-1-static void update_curr_file () +static void update_current_file (void )
This function name could lead to the impression that there is some current file (defined by a global state or whatever) that is updated.
So I'd rather rename the *file* to one of
* sequencer/abort-safety (consistent to am, describes its purpose)
* sequencer/safety (shorter, still describes the purpose)
* sequencer/current-head (describes what it contains)
* sequencer/last (a four-letter word, not totally unambiguous though)
Quoted 4 lines
By the way, this step seems to be a fix to an existing problem, and the new test added in 3/5 seems to be a demonstration of the issue. If that is the case, shouldn't the new test initially expect failure and updated by this step to expect success?
That's usually a matter of taste that I sometimes also discuss with colleagues in other projects... However, for the git test suite with its "known breakage" behavior, your recommendation is surely the best way to do it (aside from introducing the test and the fix in one commit... but that does not show in the history that there actually was that breakage)
~Stephan
Re: [PATCH 4/5] Make sequencer abort safer
Quoted 8 lines · Stephan Beyer
However:
Review
Quoted 2 lines
(quoted diff) +1
-1-static void update_curr_file() +static void update_current_file(void)This function name could lead to the impression that there is some current file (defined by a global state or whatever) that is updated.
Quoted 6 lines
So I'd rather rename the *file* to one of
* sequencer/abort-safety (consistent to am, describes its purpose)
* sequencer/safety (shorter, still describes the purpose)
* sequencer/current-head (describes what it contains)
* sequencer/last (a four-letter word, not totally unambiguous though)
OK, so here is a patch that needs to be squashed further on top of 4/5. I just picked the first one on your list ;-)
Thanks.
sequencer.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
sequencer.c +10 -10
@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") -static GIT_PATH_FUNC(git_path_current_file, "sequencer/current") +static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")/* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -306,7 +306,7 @@ static int error_dirty_index(struct replay_opts *opts)return -1 ; } -static void update_current_file (void ) +static void update_abort_safety_file (void ) {struct object_id head;
@@ -315,9 +315,9 @@ static void update_current_file(void)return ;if (!get_oid ("HEAD" , &head)) -write_file (git_path_current_file (),"%s" ,oid_to_hex (&head)); +write_file (git_path_abort_safety_file (),"%s" ,oid_to_hex (&head));else -write_file (git_path_current_file (),"%s" ,"" ); +write_file (git_path_abort_safety_file (),"%s" ,"" ); } static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -349,7 +349,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,strbuf_release (&sb);strbuf_release (&err);ref_transaction_free (transaction); -update_current_file (); +update_abort_safety_file ();return 0 ; }
@@ -824,7 +824,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, leave:free_message (commit, &msg); -update_current_file (); +update_abort_safety_file ();return res; }
@@ -1149,18 +1149,18 @@ static int rollback_is_safe(void)struct strbuf sb =STRBUF_INIT ;struct object_id expected_head, actual_head; -if (strbuf_read_file (&sb,git_path_current_file (),0 ) >=0 ) { +if (strbuf_read_file (&sb,git_path_abort_safety_file (),0 ) >=0 ) {strbuf_trim (&sb);if (get_oid_hex (sb.buf, &expected_head)) {strbuf_release (&sb); -die (_ ("could not parse %s" ),git_path_current_file ()); +die (_ ("could not parse %s" ),git_path_abort_safety_file ()); }strbuf_release (&sb); }else if (errno ==ENOENT )oidclr (&expected_head);else -die_errno (_ ("could not read '%s'" ),git_path_current_file ()); +die_errno (_ ("could not read '%s'" ),git_path_abort_safety_file ());if (get_oid ("HEAD" , &actual_head))oidclr (&actual_head);
@@ -1436,7 +1436,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)return -1 ;if (save_opts (opts))return -1 ; -update_current_file (); +update_abort_safety_file (); res =pick_commits (&todo_list, opts);todo_list_release (&todo_list);return res;
PATCH v2 1/5 am: Fix filename in safe_to_abort() error message
--- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/am.c +1 -1
@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)if (read_state_file (&sb, state,"abort-safety" ,1 ) >0 ) {if (get_oid_hex (sb.buf, &abort_safety)) -die (_ ("could not parse %s" ),am_path (state,"abort_safety" )); +die (_ ("could not parse %s" ),am_path (state,"abort-safety" )); }else oidclr (&abort_safety);
Signature · 1 line
2.11.0.27.g74d6bea
PATCH v2 2/5 am: Change safe_to_abort()'s not rewinding error into a warning
The error message tells the user that something went terribly wrong and the --abort could not be performed. But the --abort is performed, only without rewinding. By simply changing the error into a warning, we indicate the user that she must not try something like "git am --abort --force", instead she just has to check the HEAD.
--- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/am.c +1 -1
@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)if (!oidcmp (&head, &abort_safety))return 1 ; -error (_ ("You seem to have moved HEAD since the last 'am' failure.\n" +warning (_ ("You seem to have moved HEAD since the last 'am' failure.\n" "Not rewinding to ORIG_HEAD" ));return 0 ;
Signature · 1 line
2.11.0.27.g74d6bea
PATCH v2 3/5 Add test that cherry-pick --abort does not unsafely change HEAD
In reply to Stephan BeyerThe test expects failure because it is a current breakage reported by Junio C Hamano.
--- t/t3510-cherry-pick-sequence.sh | 10 ++++++++++ 1 file changed, 10 insertions(+)
t/t3510-cherry-pick-sequence.sh +10 -0
@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' +test_expect_failure '--abort does not unsafely change HEAD' ' + pristine_detach initial && + test_must_fail git cherry-pick picked anotherpick && + git reset --hard base && + test_must_fail git cherry-pick picked anotherpick && + git cherry-pick --abort 2>actual && + test_i18ngrep "You seem to have moved HEAD" actual && + test_cmp_rev base HEAD +' + test_expect_success 'cherry-pick --abort to cancel multiple revert' ' pristine_detach anotherpick && test_expect_code 1 git revert base..picked &&
Signature · 1 line
2.11.0.27.g74d6bea
PATCH v2 4/5 Make sequencer abort safer
In reply to Stephan BeyerIn contrast to "git am --abort", a sequencer abort did not check whether the current HEAD is the one that is expected. This can lead to loss of work (when not spotted and resolved using reflog before the garbage collector chimes in).
This behavior is now changed by mimicking "git am --abort":
the abortion is done but HEAD is not changed when the current HEAD is not the expected HEAD.
A new file "sequencer/current" is added to save the expected HEAD.
The new behavior is only active when --abort is invoked on multiple picks. The problem does not occur for the single-pick case because it is handled differently.
--- sequencer.c | 48 +++++++++++++++++++++++++++++++++++++++++ t/t3510-cherry-pick-sequence.sh | 2 +- 2 files changed, 49 insertions(+), 1 deletion(-)
sequencer.c +48 -0
@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") +static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")/* * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)return -1 ; } +static void update_abort_safety_file (void ) +{ +struct object_id head; + +/* Do nothing on a single-pick */ +if (!file_exists (git_path_seq_dir ())) +return ; + +if (!get_oid ("HEAD" , &head)) +write_file (git_path_abort_safety_file (),"%s" ,oid_to_hex (&head)); +else +write_file (git_path_abort_safety_file (),"%s" ,"" ); +} +static int fast_forward_to (const unsigned char *to,const unsigned char *from,int unborn,struct replay_opts *opts) {
@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,strbuf_release (&sb);strbuf_release (&err);ref_transaction_free (transaction); +update_abort_safety_file ();return 0 ; }
@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, leave:free_message (commit, &msg); +update_abort_safety_file ();return res; }
@@ -1132,6 +1149,30 @@ static int save_head(const char *head)return 0 ; } +static int rollback_is_safe (void ) +{ +struct strbuf sb =STRBUF_INIT ; +struct object_id expected_head, actual_head; + +if (strbuf_read_file (&sb,git_path_abort_safety_file (),0 ) >=0 ) { +strbuf_trim (&sb); +if (get_oid_hex (sb.buf, &expected_head)) { +strbuf_release (&sb); +die (_ ("could not parse %s" ),git_path_abort_safety_file ()); + } +strbuf_release (&sb); + } +else if (errno ==ENOENT ) +oidclr (&expected_head); +else +die_errno (_ ("could not read '%s'" ),git_path_abort_safety_file ()); + +if (get_oid ("HEAD" , &actual_head)) +oidclr (&actual_head); + +return !oidcmp (&actual_head, &expected_head); +} +static int reset_for_rollback (const unsigned char *sha1) {const char *argv[4 ];/* reset --merge <arg> + NULL */
@@ -1189,6 +1230,12 @@ int sequencer_rollback(struct replay_opts *opts)error (_ ("cannot abort from a branch yet to be born" )); goto fail; } + +if (!rollback_is_safe ()) { +/* Do not error, just do not rollback */ +warning (_ ("You seem to have moved HEAD. " +"Not rewinding, check your HEAD!" )); + }else if (reset_for_rollback (sha1)) goto fail;strbuf_release (&buf);
@@ -1393,6 +1440,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)return -1 ;if (save_opts (opts))return -1 ; +update_abort_safety_file (); res =pick_commits (&todo_list, opts);todo_list_release (&todo_list);return res;
t/t3510-cherry-pick-sequence.sh +1 -1
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' ' git diff-index --exit-code HEAD ' -test_expect_failure '--abort does not unsafely change HEAD' ' +test_expect_success '--abort does not unsafely change HEAD' ' pristine_detach initial && test_must_fail git cherry-pick picked anotherpick && git reset --hard base &&
Signature · 1 line
2.11.0.27.g74d6bea
Re: [PATCH v2 4/5] Make sequencer abort safer
On Fri, Dec 9, 2016 at 8:01 PM, Stephan Beyer <[email protected]> wrote:
[...]
sequencer.c static int save_head(const char *head) · line 1164
Quoted 13 lines
sequencer.c +13
-0+static int rollback_is_safe (void ) +{ +struct strbuf sb =STRBUF_INIT ; +struct object_id expected_head, actual_head; + +if (strbuf_read_file (&sb,git_path_abort_safety_file (),0 ) >=0 ) { +strbuf_trim (&sb); +if (get_oid_hex (sb.buf, &expected_head)) { +strbuf_release (&sb); +die (_ ("could not parse %s" ),git_path_abort_safety_file ()); + } +strbuf_release (&sb); + }
Maybe the following is a bit simpler:
if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
int res;
strbuf_trim(&sb);
res = get_oid_hex(sb.buf, &expected_head);
strbuf_release(&sb);
if (res)
die(_("could not parse %s"), git_path_abort_safety_file());
}Thanks,
Christian.
Re: [PATCH v2 4/5] Make sequencer abort safer
Quoted 16 lines · Christian Couder
sequencer.c static int save_head(const char *head) · line 1164
Quoted 13 lines
sequencer.c +13
-0+static int rollback_is_safe (void ) +{ +struct strbuf sb =STRBUF_INIT ; +struct object_id expected_head, actual_head; + +if (strbuf_read_file (&sb,git_path_abort_safety_file (),0 ) >=0 ) { +strbuf_trim (&sb); +if (get_oid_hex (sb.buf, &expected_head)) { +strbuf_release (&sb); +die (_ ("could not parse %s" ),git_path_abort_safety_file ()); + } +strbuf_release (&sb); + }Maybe the following is a bit simpler:
Quoted 8 lines
if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
int res;
strbuf_trim(&sb);
res = get_oid_hex(sb.buf, &expected_head);
strbuf_release(&sb);
if (res)
die(_("could not parse %s"), git_path_abort_safety_file());
}Is there any point in calling strbuf_release() if we're about to die anyway? I could see it if it were "return error()", but it's normal in our code base for die() to be abrupt.
-Peff
Re: [PATCH v2 4/5] Make sequencer abort safer
Quoted 27 lines · Jeff King
Quoted 16 lines · Christian Couder
sequencer.c static int save_head(const char *head) · line 1164
Quoted 13 lines
sequencer.c +13
-0+static int rollback_is_safe (void ) +{ +struct strbuf sb =STRBUF_INIT ; +struct object_id expected_head, actual_head; + +if (strbuf_read_file (&sb,git_path_abort_safety_file (),0 ) >=0 ) { +strbuf_trim (&sb); +if (get_oid_hex (sb.buf, &expected_head)) { +strbuf_release (&sb); +die (_ ("could not parse %s" ),git_path_abort_safety_file ()); + } +strbuf_release (&sb); + }Maybe the following is a bit simpler:
Quoted 8 lines
if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) { int res; strbuf_trim(&sb); res = get_oid_hex(sb.buf, &expected_head); strbuf_release(&sb); if (res) die(_("could not parse %s"), git_path_abort_safety_file()); }
Quoted 3 lines
Is there any point in calling
strbuf_release()if we're about to die anyway? I could see it if it were "returnerror()", but it's normal in our code base fordie()to be abrupt.
The point is that someone "libifies" the function some day; then "die()" becomes "return error()" almost automatically. Chances are high that the resulting memory leak is forgotten. That's one of the reasons why I like being strict about memory leaks.
However, I cannot tell if mine or Christian's variant is really "simpler" (with whatever measure) and I also don't care much.
~Stephan
PATCH v2 5/5 sequencer: Remove useless get_dir() function
In reply to Stephan BeyerThis function is used only once, for the removal of the directory. It is not used for the creation of the directory nor anywhere else.
--- sequencer.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
sequencer.c +1 -6
@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)return 0 ; } -static const char *get_dir (const struct replay_opts *opts) -{ -return git_path_seq_dir (); -} -static const char *get_todo_path (const struct replay_opts *opts) {return git_path_todo_file ();
@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)free (opts->xopts[i]);free (opts->xopts); -strbuf_addf (&dir,"%s" ,get_dir (opts)); +strbuf_addf (&dir,"%s" ,git_path_seq_dir ());remove_dir_recursively (&dir,0 );strbuf_release (&dir);
Signature · 1 line
2.11.0.27.g74d6bea
PATCH 5/5 sequencer: Remove useless get_dir() function
In reply to Stephan BeyerThis function is used only once, for the removal of the directory. It is not used for the creation of the directory nor anywhere else.
--- sequencer.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
sequencer.c +1 -6
@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)return 0 ; } -static const char *get_dir (const struct replay_opts *opts) -{ -return git_path_seq_dir (); -} -static const char *get_todo_path (const struct replay_opts *opts) {return git_path_todo_file ();
@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)free (opts->xopts[i]);free (opts->xopts); -strbuf_addf (&dir,"%s" ,get_dir (opts)); +strbuf_addf (&dir,"%s" ,git_path_seq_dir ());remove_dir_recursively (&dir,0 );strbuf_release (&dir);
Signature · 1 line
2.11.0.27.g4eed97c
Re: [PATCH 1/5] am: Fix filename in safe_to_abort() error message
In reply to Stephan BeyerHi Stephan,
builtin/am.c static int safe_to_abort(const struct am_state *state) · line 2127
Quoted 10 lines · Stephan Beyer
builtin/am.c +1
-1@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)if (read_state_file (&sb, state,"abort-safety" ,1 ) >0 ) {if (get_oid_hex (sb.buf, &abort_safety)) -die (_ ("could not parse %s" ),am_path (state,"abort_safety" )); +die (_ ("could not parse %s" ),am_path (state,"abort-safety" ));
Ah, this is obviously correct. Sorry for the oversight.
Quoted 5 lines
builtin/am.c +0
-1}else oidclr (&abort_safety); --2.11.0.27.g4eed97c
Thanks,
Paul