Re: [PATCH 4/5] Make sequencer abort safer
Stephan Beyerwrites: > However: > >> -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) 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(-) diff --git a/sequencer.c b/sequencer.c index 874aaa4cd4..3ac4cb8d3b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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", )) - write_file(git_path_current_file(), "%s", oid_to_hex()); + write_file(git_path_abort_safety_file(), "%s", oid_to_hex()); 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(); strbuf_release(); 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, ); - 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(, git_path_current_file(), 0) >= 0) { + if (strbuf_read_file(, git_path_abort_safety_file(), 0) >= 0) { strbuf_trim(); if (get_oid_hex(sb.buf, _head)) { strbuf_release(); - die(_("could not parse %s"), git_path_current_file()); + die(_("could not parse %s"), git_path_abort_safety_file()); } strbuf_release(); } else if (errno == ENOENT) oidclr(_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", _head)) oidclr(_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(_list, opts); todo_list_release(_list); return res;
Re: [PATCH 4/5] Make sequencer abort safer
Hi, I'm a little afraid of feeding Parkinson's law of triviality here, but... ;) On 12/08/2016 06:27 PM, Junio C Hamano wrote: > Johannes Schindelinwrites: > >> On Wed, 7 Dec 2016, Stephan Beyer wrote: >> >>> diff --git a/sequencer.c b/sequencer.c >>> index 30b10ba14..c9b560ac1 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -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. I totally agree with that (and I don't know why I used "curr", probably just because it looked consistent and good...). However: > -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) > 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
Johannes Schindelinwrites: > On Wed, 7 Dec 2016, Stephan Beyer wrote: > >> diff --git a/sequencer.c b/sequencer.c >> index 30b10ba14..c9b560ac1 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -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(-) diff --git a/sequencer.c b/sequencer.c index c9b560ac15..ce04377f8e 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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", )) - write_file(git_path_curr_file(), "%s", oid_to_hex()); + write_file(git_path_current_file(), "%s", oid_to_hex()); 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(); strbuf_release(); 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, ); - 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(, git_path_curr_file(), 0) >= 0) { + if (strbuf_read_file(, git_path_current_file(), 0) >= 0) { strbuf_trim(); if (get_oid_hex(sb.buf, _head)) { strbuf_release(); - die(_("could not parse %s"), git_path_curr_file()); + die(_("could not parse %s"), git_path_current_file()); } strbuf_release(); } else if (errno == ENOENT) oidclr(_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", _head)) oidclr(_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(_list, opts); todo_list_release(_list); return res; diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index efcd4fc485..372307c21b 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -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
Re: [PATCH 4/5] Make sequencer abort safer
Hi, On Wed, 7 Dec 2016, Stephan Beyer wrote: > diff --git a/sequencer.c b/sequencer.c > index 30b10ba14..c9b560ac1 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -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
[PATCH 4/5] Make sequencer abort safer
In 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. Signed-off-by: Stephan Beyer--- sequencer.c | 49 + 1 file changed, 49 insertions(+) diff --git a/sequencer.c b/sequencer.c index 30b10ba14..c9b560ac1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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", )) + write_file(git_path_curr_file(), "%s", oid_to_hex()); + 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(); strbuf_release(); 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, ); + 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(, git_path_curr_file(), 0) >= 0) { + strbuf_trim(); + if (get_oid_hex(sb.buf, _head)) { + strbuf_release(); + die(_("could not parse %s"), git_path_curr_file()); + } + strbuf_release(); + } + else if (errno == ENOENT) + oidclr(_head); + else + die_errno(_("could not read '%s'"), git_path_curr_file()); + + if (get_oid("HEAD", _head)) + oidclr(_head); + + return !oidcmp(_head, _head); +} + static int reset_for_rollback(const unsigned char *sha1) { const char *argv[4];/* reset --merge + 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(); @@ -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(_list, opts); todo_list_release(_list); return res; -- 2.11.0.27.g4eed97c