{"thread":{"id":"27524","subject":"[RFC PATCH] revert: Implement --abort processing","startedAt":"2011-06-01T16:07:26Z","lastAt":"2011-06-02T16:41:42Z","messageCount":6,"participants":["Ramkumar Ramachandra","Sverre Rabbelier","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"169121","messageId":"1306944446-11031-1-git-send-email-artagnon@gmail.com","threadId":"27524","inReplyTo":null,"subject":"[RFC PATCH] revert: Implement --abort processing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-01T16:07:26Z","receivedAt":"2011-06-01T16:07:26Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"To abort, perform a \"rerere clear\" and \"reset --hard\" to the ref\nspecified by the HEAD file introduced earlier in the series.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n I want some feedback on this -- am I doing this right? Should I be\n re-implementing \"reset --hard\" like this?\n\n builtin/revert.c                |   84 +++++++++++++++++++++++++++++++++++++--\n t/t3510-cherry-pick-sequence.sh |   27 ++++++++++++\n 2 files changed, 107 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8eee4d5..ae85df7 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -12,6 +12,7 @@\n #include \"revision.h\"\n #include \"rerere.h\"\n #include \"merge-recursive.h\"\n+#include \"unpack-trees.h\"\n #include \"refs.h\"\n #include \"dir.h\"\n \n@@ -208,8 +209,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\tNULL);\n \n \t/* Remove these when the options are actually implemented */\n-\tif (opts->abort_oper)\n-\t\tdie(\"--abort is not implemented yet\");\n \tif (opts->skip_oper)\n \t\tdie(\"--skip is not implemented yet\");\n \tif (opts->continue_oper)\n@@ -647,6 +646,44 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)\n \trollback_lock_file(&index_lock);\n }\n \n+static int hard_reset_to_ref(unsigned char *sha1)\n+{\n+\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tstruct string_list merge_rr = STRING_LIST_INIT_DUP;\n+\tstruct unpack_trees_options unpack_tree_opts;\n+\tstruct tree_desc desc;\n+\tint fd;\n+\n+\t/* rerere clear */\n+\tsetup_rerere(&merge_rr, 0);\n+\trerere_clear(&merge_rr);\n+\n+\t/* Clean worktree, index */\n+\tread_cache_unmerged();\n+\n+\tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n+\tunpack_tree_opts.head_idx = 1;\n+\tunpack_tree_opts.src_index = &the_index;\n+\tunpack_tree_opts.dst_index = &the_index;\n+\tunpack_tree_opts.fn = oneway_merge;\n+\tunpack_tree_opts.merge = 1;\n+\tunpack_tree_opts.update = 1;\n+\tunpack_tree_opts.reset = 1;\n+\n+\tfd = hold_locked_index(lock, 1);\n+\tfill_tree_descriptor(&desc, sha1);\n+\tif (unpack_trees(1, &desc, &unpack_tree_opts) < 0)\n+\t\treturn error (_(\"Failed to unpack trees.\"));\n+\tif (write_index(&the_index, fd) ||\n+\t\tcommit_locked_index(lock))\n+\t\treturn error(_(\"Could not write index.\"));\n+\n+\t/* Update the HEAD ref */\n+\tif (update_ref(\"updating HEAD\", \"HEAD\", sha1, NULL, 0, MSG_ON_ERR))\n+\t\treturn error(_(\"Unable to update HEAD\"));\n+\treturn 0;\n+}\n+\n static void format_todo(struct strbuf *buf, struct commit_list *list,\n \t\t\tstruct replay_opts *opts)\n {\n@@ -743,6 +780,45 @@ static int pick_commits(struct replay_opts *opts)\n \treturn cleanup_sequencer_dir();\n }\n \n+static int process_continuation(struct replay_opts *opts)\n+{\n+\tunsigned char sha1[20];\n+\tchar head[40];\n+\tint fd;\n+\n+\tif (opts->abort_oper) {\n+\t\t/* First, read the HEAD_FILE */\n+\t\tif (!file_exists(HEAD_FILE))\n+\t\t\tgoto error;\n+\t\tfd = open(HEAD_FILE, O_RDONLY, 0666);\n+\t\tif (fd < 0)\n+\t\t\treturn error(_(\"Could not open '%s' for reading: %s\"),\n+\t\t\t\tHEAD_FILE, strerror(errno));\n+\t\tif (read_in_full(fd, head, 40) < 40 || get_sha1_hex(head, sha1) < 0) {\n+\t\t\tclose(fd);\n+\t\t\treturn error(_(\"Corrupt '%s': %s\"), HEAD_FILE, strerror(errno));\n+\t\t}\n+\t\tclose(fd);\n+\n+\t\tif (hard_reset_to_ref(sha1) < 0)\n+\t\t\t/* Error already reported */\n+\t\t\treturn -1;\n+\t\treturn cleanup_sequencer_dir();\n+\t}\n+\telse if (opts->skip_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t}\n+\telse if (opts->continue_oper) {\n+\t\tif (!file_exists(TODO_FILE))\n+\t\t\tgoto error;\n+\t}\n+\n+\treturn pick_commits(opts);\n+error:\n+\treturn error(_(\"No %s in progress\"), me);\n+}\n+\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tint res;\n@@ -755,7 +831,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tme = \"revert\";\n \tparse_args(argc, argv, &opts);\n-\tres = pick_commits(&opts);\n+\tres = process_continuation(&opts);\n \tif (res > 0)\n \t\t/* Exit status from conflict */\n \t\treturn res;\n@@ -775,7 +851,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tme = \"cherry-pick\";\n \tparse_args(argc, argv, &opts);\n-\tres = pick_commits(&opts);\n+\tres = process_continuation(&opts);\n \tif (res > 0)\n \t\treturn res;\n \tif (res < 0)\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex a2e1888..7e22898 100644\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -36,4 +36,31 @@ test_expect_success 'cherry-pick cleans up sequencer directory upon success' '\n \ttest_must_fail test -d .git/sequencer\n '\n \n+test_expect_success '--abort complains when no cherry-pick is in progress' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --abort >actual 2>&1 &&\n+\techo \"error: No cherry-pick in progress\" >expect &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success '--abort restores HEAD after failed cherry-pick' '\n+\tpristine_detach initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\ttest_must_fail git cherry-pick base..picked &&\n+\tgit cherry-pick --abort &&\n+\tnewhead=$(git rev-parse HEAD) &&\n+\ttest \"$head\" = \"$newhead\"\n+'\n+\n+test_expect_success '--abort resets worktree and index after failed cherry-pick' '\n+\tpristine_detach initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\ttest_must_fail git cherry-pick base..picked &&\n+\tgit add foo &&\n+\tgit cherry-pick --abort &&\n+\tgit update-index -q --ignore-submodules --refresh &&\n+\tgit diff-files --quiet --ignore-submodules &&\n+\tgit diff-index --cached --quiet --ignore-submodules HEAD --\n+'\n+\n test_done\n-- \n1.7.5.GIT\n"},{"id":"169124","messageId":"BANLkTinDvtmU1+xNencddOH-Lj+d+mhasg@mail.gmail.com","threadId":"27524","inReplyTo":"1306944446-11031-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] revert: Implement --abort processing","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-01T16:24:18Z","receivedAt":"2011-06-01T16:24:18Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Wed, Jun 1, 2011 at 11:07, Ramkumar Ramachandra <artagnon@gmail.com> wrote:\n> To abort, perform a \"rerere clear\" and \"reset --hard\" to the ref\n> specified by the HEAD file introduced earlier in the series.\n\nIsn't this the kind of usecase we added 'git reset --merge' for?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169133","messageId":"7vfwnth2pp.fsf@alter.siamese.dyndns.org","threadId":"27524","inReplyTo":"1306944446-11031-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] revert: Implement --abort processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-01T17:38:10Z","receivedAt":"2011-06-01T17:38:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> To abort, perform a \"rerere clear\" and \"reset --hard\" to the ref\n> specified by the HEAD file introduced earlier in the series.\n\nWhat is the scenario this new feature targets? Is it to recover from this?\n\n    % git revert $that_single_faulty_commit\n    ... oops conflicted, let's try if I can resolve ...\n    % edit $conflicted_files\n    ... yuck, I managed to resolve two paths but not other three ...\n    ... let's give up for now ...\n\nIn that scenario, a recovery may involve \"git reset --hard\", but clearing\nthe rerere record for what you have already resolved may or may not be.  I\nwould imagine that the above \"give up _for now_\" would be concluded with:\n\n    % git add $files_i_managed_to_resolve_this_round\n    % git rerere\n    $ git reset --hard\n\nEven if you dropped the unconditional \"rerere clear\" from the patch, I am\nnot sure what this new feature buys us. Some people would want the rerere\ncache cleared, some others don't. \"revert --abort\" will forever be to\naborting revert and restoring some but not all the parts of the operation\nthe user wants to be undone, as you cannot satisfy everybody.  So I am a\nbit puzzled why you thought this was even a good idea to begin with.\n\n\"reset --hard\" has established semantics people already understand what it\ndoes. I would rather see us let them use that, without confusing them with\n\"what are the differences between the two?\" unnecessarily.\n"},{"id":"169138","messageId":"20110601190006.GB9730@elie","threadId":"27524","inReplyTo":"1306944446-11031-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] revert: Implement --abort processing","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-01T19:00:06Z","receivedAt":"2011-06-01T19:00:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> To abort, perform a \"rerere clear\" and \"reset --hard\" to the ref\n> specified by the HEAD file introduced earlier in the series.\n\nThe above doesn't explain why, so it makes it harder to answer:\n\n>  Should I be\n>  re-implementing \"reset --hard\" like this?\n\nSo I'll try to fill in the blanks.  Wouldn't it be better to call\n\"reset --merge\" code?  That is what \"git merge --abort\" does.\n\nLet's consider an example.\n\nSuppose I have run \"git cherry-pick foo\" and run into a conflict.  Now\nI start to fix things up the way I like, but I suddenly realize that\nthe cause was cherry-picking the wrong commit; it's better to start\nover and do that.  So I try to abort.\n\nI have some changes to files that did not participate in the automatic\ncherry-pick:\n\n 1. for unrelated reasons, I bumped the version number in the Makefile\n as a reminder not to forget later, without commiting it or marking\n with \"git add\";\n\n 2. I (manually) moved a declaration to a different header file to\n reflect differences between the codebase at the time of foo^ and HEAD,\n to get it to compile.  Which works, so I mark it with \"git add\" for\n incorporation into the corrected cherry-pick commit.\n\nWith \"git reset --merge\", (1) is left alone, while (2) is backed out,\nunmerged entries are of course clobbered, and hazy cases in which I\nmake some changes, \"git add\", and then make more changes without \"git\nadd\" cause the operation to error out.  It would be nicer if git could\nread my mind, but at first glance this seems like an okay second-best.\n\nAre there other considerations or situations that would lead to a\ndifferent conclusion?  (Not a rhetorical question.)\n\nHope that helps,\nJonathan\n"},{"id":"169194","messageId":"BANLkTikgY_Kc6gu_iTtpiiayo9e=DPxOLw@mail.gmail.com","threadId":"27524","inReplyTo":"20110601190006.GB9730@elie","subject":"Re: [RFC PATCH] revert: Implement --abort processing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-06-02T13:03:42Z","receivedAt":"2011-06-02T13:03:42Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Sverre, Junio and Jonathan,\n\nSverre Rabbelier writes:\n> Isn't this the kind of usecase we added 'git reset --merge' for?\n\nYes, I can see that now.\n\nJunio Hamano writes:\n> Even if you dropped the unconditional \"rerere clear\" from the patch, I am\n> not sure what this new feature buys us. Some people would want the rerere\n> cache cleared, some others don't. \"revert --abort\" will forever be to\n> aborting revert and restoring some but not all the parts of the operation\n> the user wants to be undone, as you cannot satisfy everybody.  So I am a\n> bit puzzled why you thought this was even a good idea to begin with.\n\nIt's actually very specific to the way I work/ think -- I would have\nexpected an abort to go back in time, and make it look like the\noperation wasn't performed in the first place. My normal workflow: I\nmake my changes, create a \"fixup!\" commit, abort, and cherry-pick that\ncommit from my reflog. Yes, I use \"reset --hard\" a lot, and yes, it's\na very powerful hammer.\n\nI see now that this probably doesn't fit everyone's usecase. So the\nchanges I propose are:\n1. Don't rerere clear. We can probably document this fact somewhere,\nand hint the user about this during the time of abort.\n2. Use reset --merge as Sverre suggested.\n\nI'll think about this workflow and post a patch soon.\n\nJonathan Nieder writes:\n> I have some changes to files that did not participate in the automatic\n> cherry-pick:\n>\n>  1. for unrelated reasons, I bumped the version number in the Makefile\n>  as a reminder not to forget later, without commiting it or marking\n>  with \"git add\";\n>\n>  2. I (manually) moved a declaration to a different header file to\n>  reflect differences between the codebase at the time of foo^ and HEAD,\n>  to get it to compile.  Which works, so I mark it with \"git add\" for\n>  incorporation into the corrected cherry-pick commit.\n>\n> With \"git reset --merge\", (1) is left alone, while (2) is backed out,\n> unmerged entries are of course clobbered, and hazy cases in which I\n> make some changes, \"git add\", and then make more changes without \"git\n> add\" cause the operation to error out.  It would be nicer if git could\n> read my mind, but at first glance this seems like an okay second-best.\n\nThanks for the excellent explanation. I'll think about this workflow\nfor a while before posting another iteration of this patch.\n\n-- Ram\n"},{"id":"169205","messageId":"7vboygdw3d.fsf@alter.siamese.dyndns.org","threadId":"27524","inReplyTo":"BANLkTikgY_Kc6gu_iTtpiiayo9e=DPxOLw@mail.gmail.com","subject":"Re: [RFC PATCH] revert: Implement --abort processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-02T16:41:42Z","receivedAt":"2011-06-02T16:41:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> I see now that this probably doesn't fit everyone's usecase. So the\n> changes I propose are:\n> 1. Don't rerere clear. We can probably document this fact somewhere,\n> and hint the user about this during the time of abort.\n> 2. Use reset --merge as Sverre suggested.\n>\n> I'll think about this workflow and post a patch soon.\n\nPlease do the former but I would advice you not to do the latter.\n\nIf you do not clear rerere then you are not serving the original audience\nyou wanted to serve, and if you do, you are hurting people when they do\nwant to keep rerere. You cannot win either way.\n\nThe users are better off being exposed to \"reset --merge\" than kept\nunaware of the concepts necessary to do what they want hidden behind\n\"revert --abort\" that has fuzzy semantics that restores some but not all,\nwith the definition of \"some\" being something we happened to decide here,\nwhich can never match what the user in every situation would want.\n"}]}