{"thread":{"id":"59782","subject":"[PATCH] cherry-pick: refuse cherry-pick sequence if index is dirty","startedAt":"2023-05-23T08:35:32Z","lastAt":"2023-09-06T05:02:52Z","messageCount":8,"participants":["Tao Klerks via GitGitGadget","Tao Klerks","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"477661","messageId":"pull.1535.git.1684830767336.gitgitgadget@gmail.com","threadId":"59782","inReplyTo":null,"subject":"[PATCH] cherry-pick: refuse cherry-pick sequence if index is dirty","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-23T08:32:47Z","receivedAt":"2023-05-23T08:35:32Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nCherry-pick, like merge or rebase, refuses to run when there are changes\nin the index. However, if a cherry-pick sequence is requested, this\nrefusal happens \"too late\": when the cherry-pick sequence has already\nstarted, and an \"--abort\" or \"--quit\" is needed to resume normal\noperation.\n\nNormally, when an operation is \"in-progress\" and you want to go back to\nwhere you were before, \"--abort\" is the right thing to run. If you run\n\"git cherry-pick --abort\" in this specific situation, however, your\nstaged changes are destroyed as part of the abort! Generally speaking,\nthe abort process assumes any changes in the index are part of the\noperation to be aborted.\n\nAdd an earlier check in the cherry-pick sequence process to ensure that\nthe index is clean, reusing the already-generalized method used for\nrebase. Also add a test.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    cherry-pick: refuse cherry-pick sequence if index is dirty\n    \n    I encountered this data-loss bug (performing normally-safe operations\n    results in loss of local staged changes) while exploring\n    largely-unrelated changes as per thread\n    CAPMMpogFHnX2YPA4VmffmA0pku=43CQJ8iebCOkFm4ravBVTeg@mail.gmail.com on\n    \"git checkout --force\", and on the possibility of supporting same-commit\n    switch while preserving merge metadata.\n    \n    I believe it is best handled as a standalone issue, it looks like a\n    simple bugfix to me, hence this separate patch.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1535%2FTaoK%2Ftao-cherry-pick-sequence-safety-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1535/TaoK/tao-cherry-pick-sequence-safety-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1535\n\n builtin/revert.c                |  2 +-\n sequencer.c                     | 14 +++++++++-----\n sequencer.h                     |  3 ++-\n t/t3510-cherry-pick-sequence.sh | 10 ++++++++++\n 4 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0240ec8593b..91ebba38eaa 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -224,7 +224,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\treturn sequencer_rollback(the_repository, opts);\n \tif (cmd == 's')\n \t\treturn sequencer_skip(the_repository, opts);\n-\treturn sequencer_pick_revisions(the_repository, opts);\n+\treturn sequencer_pick_revisions(the_repository, opts, me);\n }\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\ndiff --git a/sequencer.c b/sequencer.c\nindex b553b49fbb6..9abea3b97fc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3162,7 +3162,7 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,\n \treturn 0;\n }\n \n-static int create_seq_dir(struct repository *r)\n+static int create_seq_dir(struct repository *r, const char *requested_action)\n {\n \tenum replay_action action;\n \tconst char *in_progress_error = NULL;\n@@ -3194,6 +3194,9 @@ static int create_seq_dir(struct repository *r)\n \t\t\t\tadvise_skip ? \"--skip | \" : \"\");\n \t\treturn -1;\n \t}\n+\tif (require_clean_work_tree(r, requested_action,\n+\t\t\t\t    _(\"Please commit or stash them.\"), 1, 1))\n+\t\treturn -1;\n \tif (mkdir(git_path_seq_dir(), 0777) < 0)\n \t\treturn error_errno(_(\"could not create sequencer directory '%s'\"),\n \t\t\t\t   git_path_seq_dir());\n@@ -5169,7 +5172,8 @@ static int single_pick(struct repository *r,\n }\n \n int sequencer_pick_revisions(struct repository *r,\n-\t\t\t     struct replay_opts *opts)\n+\t\t\t     struct replay_opts *opts,\n+\t\t\t     const char *action)\n {\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n \tstruct object_id oid;\n@@ -5223,12 +5227,12 @@ int sequencer_pick_revisions(struct repository *r,\n \n \t/*\n \t * Start a new cherry-pick/ revert sequence; but\n-\t * first, make sure that an existing one isn't in\n-\t * progress\n+\t * first, make sure that the index is clean and that\n+\t * an existing one isn't in progress.\n \t */\n \n \tif (walk_revs_populate_todo(&todo_list, opts) ||\n-\t\t\tcreate_seq_dir(r) < 0)\n+\t\t\tcreate_seq_dir(r, action) < 0)\n \t\treturn -1;\n \tif (repo_get_oid(r, \"HEAD\", &oid) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"can't revert as initial commit\"));\ndiff --git a/sequencer.h b/sequencer.h\nindex 913a0f652d9..1b39325c52c 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -159,7 +159,8 @@ void todo_list_filter_update_refs(struct repository *r,\n /* Call this to setup defaults before parsing command line options */\n void sequencer_init_config(struct replay_opts *opts);\n int sequencer_pick_revisions(struct repository *repo,\n-\t\t\t     struct replay_opts *opts);\n+\t\t\t     struct replay_opts *opts,\n+\t\t\t     const char *action);\n int sequencer_continue(struct repository *repo, struct replay_opts *opts);\n int sequencer_rollback(struct repository *repo, struct replay_opts *opts);\n int sequencer_skip(struct repository *repo, struct replay_opts *opts);\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 3b0fa66c33d..e8f4138bf89 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -47,6 +47,16 @@ test_expect_success 'cherry-pick persists data on failure' '\n \ttest_path_is_file .git/sequencer/opts\n '\n \n+test_expect_success 'cherry-pick sequence refuses to run on dirty index' '\n+\tpristine_detach initial &&\n+\ttouch localindexchange &&\n+\tgit add localindexchange &&\n+\techo picking &&\n+\ttest_must_fail git cherry-pick initial..picked &&\n+\ttest_path_is_missing .git/sequencer &&\n+\ttest_must_fail git cherry-pick --abort\n+'\n+\n test_expect_success 'cherry-pick mid-cherry-pick-sequence' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick base..anotherpick &&\n\nbase-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680\n-- \ngitgitgadget\n"},{"id":"477666","messageId":"CAPMMpojV1Ts=OKM0FbBHU6=EB5RKNxHucX-8VQmYoQBNefKpqQ@mail.gmail.com","threadId":"59782","inReplyTo":"pull.1535.git.1684830767336.gitgitgadget@gmail.com","subject":"Re: [PATCH] cherry-pick: refuse cherry-pick sequence if index is dirty","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-23T16:01:25Z","receivedAt":"2023-05-23T16:01:45Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, May 23, 2023 at 10:32 AM Tao Klerks via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Tao Klerks <tao@klerks.biz>\n>\n> Cherry-pick, like merge or rebase, refuses to run when there are changes\n> in the index. However, if a cherry-pick sequence is requested, this\n> refusal happens \"too late\": when the cherry-pick sequence has already\n> started, and an \"--abort\" or \"--quit\" is needed to resume normal\n> operation.\n>\n> Normally, when an operation is \"in-progress\" and you want to go back to\n> where you were before, \"--abort\" is the right thing to run. If you run\n> \"git cherry-pick --abort\" in this specific situation, however, your\n> staged changes are destroyed as part of the abort! Generally speaking,\n> the abort process assumes any changes in the index are part of the\n> operation to be aborted.\n>\n> Add an earlier check in the cherry-pick sequence process to ensure that\n> the index is clean, reusing the already-generalized method used for\n> rebase. Also add a test.\n>\n> Signed-off-by: Tao Klerks <tao@klerks.biz>\n> ---\n\nMy apologies for the premature submission: I've now realized I used\nthe wrong existing check. \"git rebase\" checks for a clean *worktree*\n(ignoring untracked files), and that is what I reused here. What git\nmerge and git cherry-pick check for, and what I should have added a\ncheck for here, is a clean *index*.\n\nThe current implementation of this patch is far too restrictive. It\ndoesn't break any tests (and maybe I should add one now that I know),\nbut it's doing the wrong thing.\n\nTao\n"},{"id":"477682","messageId":"xmqqjzwyh9tp.fsf@gitster.g","threadId":"59782","inReplyTo":"CAPMMpojV1Ts=OKM0FbBHU6=EB5RKNxHucX-8VQmYoQBNefKpqQ@mail.gmail.com","subject":"Re: [PATCH] cherry-pick: refuse cherry-pick sequence if index is dirty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-24T00:06:26Z","receivedAt":"2023-05-24T00:06:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n> The current implementation of this patch is far too restrictive. It\n> doesn't break any tests (and maybe I should add one now that I know),\n> but it's doing the wrong thing.\n\nI am ambivalent.  What do we want to see in a multi-pick sequence\nthat is different from rebase?  A single-step cherry-pick can fail\nsafely before it touches the index or the working tree files, but if\ntwo-step cherry-pick, whose first step succeeds, finds that it\ncannot safely carry out its second step without clobbering the local\nchanges made to the working tree files, what should happen?  Are we\nOK if we stopped in the state just after the first step has already\nbeen done?\n\nMy (tentative) answer to that question is \"yes\", but the recovery\noptions of \"cherry-pick\" may want to work differently from what we\nhave seen them traditionally do.  Namely, the user accepts that the\nfirst step is already done, and stopping \"cherry-pick\", be it called\n\"--abort\" or something else, should just remove the sequencer state\nand behave as if the single-pick cherry-pick on the first step only\nhas just finished and leave such a state in the index and the\nworking tree.  If that is what we are going to do, then it would\nmake sense to adopt the same safety semantics we use for \"git merge\"\nand \"git checkout\" to ensure only that the index is clean, relying\non the unpack-trees machinery that stops before clobbering a locally\nmodified working tree files.  But if we are to aim for \"all-or-none\"\nsemantics people expect from aborting \"git rebase\", I suspect that\nit would be way too complicated to allow random changes in the\nworking tree files that we may only discover to be problems after\nstarting the sequence of replaying commits one-by-one, and \"too\nrestrictive\" check may be justified.  To put it differently, if it\nis too restrictive for multi-pick, then we would want to loosen it\nfor \"git rebase\" as well, as the issues are likely to be the same.\n\nThanks.\n\n\n\n\n\n"},{"id":"477685","messageId":"CAPMMpoic_+RATwS46=Bd2K4+D_5yEw9RQFGR075Bs4aQJUjtsQ@mail.gmail.com","threadId":"59782","inReplyTo":"xmqqjzwyh9tp.fsf@gitster.g","subject":"Re: [PATCH] cherry-pick: refuse cherry-pick sequence if index is dirty","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-24T09:33:49Z","receivedAt":"2023-05-24T09:34:09Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, May 24, 2023 at 2:06 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Tao Klerks <tao@klerks.biz> writes:\n>\n> > The current implementation of this patch is far too restrictive. It\n> > doesn't break any tests (and maybe I should add one now that I know),\n> > but it's doing the wrong thing.\n>\n> I am ambivalent.  What do we want to see in a multi-pick sequence\n> that is different from rebase?\n\nI would argue there are primarily three things that are different:\n1. The checkout of the new base (and checkout of the original in an \"--abort\")\n2. The support for and/or more-common expectation of \"messing\" with\ncommits as you go, eg squash, edit\n3. The (partial) support for rebasing/recreating merge commits\n\nI'm not sure to what extent any of these justify having tighter\nrestrictions on when we allow a rebase to start, though.\n\n> A single-step cherry-pick can fail\n> safely before it touches the index or the working tree files, but if\n> two-step cherry-pick, whose first step succeeds, finds that it\n> cannot safely carry out its second step without clobbering the local\n> changes made to the working tree files, what should happen?  Are we\n> OK if we stopped in the state just after the first step has already\n> been done?\n\nThis is the current behavior: it stops before the specific pick that\nis going to affect local unstaged changes, or if there are *any*\nstaged changes (in which case it stops as it's about to do the first\npick - the first time this check runs). The reasoning for this\nbehavior, as I understand it, is that the \"--abort\" strategy,\nintending to \"undo whatever I started doing here, including a conflict\nresolution\", resets the index. So as long as there is nothing you want\nto keep in the index, and as long as we know that any previous picks\nhaven't impacted any files with unstaged changes, we're good.\n\nThe bug that I want to fix is that we only end up checking whether\nthere are changes in the index *after* we've already committed to\nresetting the index upon later \"--abort\". It's a kind of catch-22:\nwe've detected that aborting would destroy your work, so we leave you\nin a state where the most obvious thing to do is abort, so we destroy\nyour work... Of course, if you understand what's going on you can\nchoose to \"--quit\" and *not* lose your work... but this is completely\nantithetical to the general intent of \"--abort\".\n\nThere's another, smaller flaw here I think, common to Merge,\nSingle-Cherry-Pick, and Sequence-Cherry-Pick, which is that *if* you\nstart with unstaged changes, and you end up in a conflict resolution\nor \"--no-commit\" pause, and you then \"git add\" your unstaged changes\nduring that pause/resolution, and you *then* later \"--abort\"... then\nyour originally-unstaged changes are destroyed by the \"--abort\" - so\nit has *not* taken you back to where you were before the operation\nstarted. This is, to me as a user, non-obvious, and could potentially\nlead to data loss. The only way I see to fix that, is to have *all* of\nthese operations refuse to operate on dirty worktrees altogether -\nlike rebase already does.\n\nI suspect this level of \"strictness\" would be welcome to newcomers,\nand less welcome to existing experienced users.\n\n>\n> My (tentative) answer to that question is \"yes\", but the recovery\n> options of \"cherry-pick\" may want to work differently from what we\n> have seen them traditionally do.  Namely, the user accepts that the\n> first step is already done, and stopping \"cherry-pick\", be it called\n> \"--abort\" or something else, should just remove the sequencer state\n> and behave as if the single-pick cherry-pick on the first step only\n> has just finished and leave such a state in the index and the\n> working tree.\n\nThis behavior exists, and is called \"--quit\", right?\n\nThe semantics as I understand it are:\n--quit: I know what I'm doing, just remove any \"ongoing operation\"\nmetadata and let me work with the current index and worktree.\n--abort: This was a bad idea, please take me back to where I was\nbefore I started this operation (without losing any work I had\nongoing, pls!)\n\n>  If that is what we are going to do, then it would\n> make sense to adopt the same safety semantics we use for \"git merge\"\n> and \"git checkout\" to ensure only that the index is clean, relying\n> on the unpack-trees machinery that stops before clobbering a locally\n> modified working tree files.\n\nYep\n\n> But if we are to aim for \"all-or-none\"\n> semantics people expect from aborting \"git rebase\", I suspect that\n> it would be way too complicated to allow random changes in the\n> working tree files that we may only discover to be problems after\n> starting the sequence of replaying commits one-by-one, and \"too\n> restrictive\" check may be justified.\n\nI don't think I understand this argument. If we want to support both\nsets of semantics, then that's exactly what \"--quit\" and \"--abort\"\nachieve, right? (as long as we check for the dirty index *before*\ncommitting to destroying the index in case of \"--abort\")\n\n>  To put it differently, if it\n> is too restrictive for multi-pick, then we would want to loosen it\n> for \"git rebase\" as well, as the issues are likely to be the same.\n\nMy argument for only changing \"sequence-cherry-pick\" here, and having\nit (continue to) use the index-safety-only semantics of\nsingle-cherry-pick and merge, is that *this is not a change in\ncabability* - it is only a bugfix. Switching to the worktree-safety\nsemantics of rebase would be a substantial change in behavior beyond\nthe bugfix.\n\nI, personally, would prefer to see the worktree-safety semantics of\nrebase be used in *all* these operations, so I could no longer shoot\nmyself in the foot by starting a merge, accidentally staging some\npreviously-unstaged changes during conflict resolution, and then\nlosing those changes by \"--abort\"ing. But I expect that this kind of\nchange would need to be behind a config option of some sort, trading\noff safety against low friction.\n\nI could imagine a setting like \"core.OperationWorktreeSafety\", with\nsettings \"default\" (current behavior - rebase disallows dirty\nworktrees, the others disallow dirty index), \"strict\" (all behave like\ncurrent rebase) and \"lax\" (all behave like merge).\n\nAs discussed elsewhere, I would also like to (have an option to) treat\nuntracked files as \"worktree dirtiness\"/unstaged changes in exactly\nthe same way as changes to tracked files - but that's another topic :)\n\nI'll prepare a v2 with index-safety-only for sequence-cherry-pick for\nnow, please let me know if a (better-named)\n\"core.OperationWorktreeSafety\" option is something that you'd be\ninterested in / that would make sense to you.\n\nThanks!\n"},{"id":"477763","messageId":"pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com","threadId":"59782","inReplyTo":"pull.1535.git.1684830767336.gitgitgadget@gmail.com","subject":"[PATCH v2] cherry-pick: refuse cherry-pick sequence if index is dirty","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-28T09:08:08Z","receivedAt":"2023-05-28T09:11:09Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nCherry-pick, like merge or rebase, refuses to run when there are changes\nin the index. However, if a cherry-pick sequence is requested, this\nrefusal happens \"too late\": when the cherry-pick sequence has already\nstarted, and an \"--abort\" or \"--quit\" is needed to resume normal\noperation.\n\nNormally, when an operation is \"in-progress\" and you want to go back to\nwhere you were before, \"--abort\" is the right thing to run. If you run\n\"git cherry-pick --abort\" in this specific situation, however, your\nstaged changes are destroyed as part of the abort! Generally speaking,\nthe abort process assumes any changes in the index are part of the\noperation to be aborted.\n\nAdd an earlier check in the cherry-pick sequence process to ensure that\nthe index is clean, introducing a new general \"quit if index dirty\" function\nderived from the existing worktree-level function used in rebase and pull.\nAlso add a test.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    cherry-pick: refuse cherry-pick sequence if index is dirty\n    \n    V2: In the first version I had reused the require_clean_work_tree()\n    function, which meant I accidentally tightened the requirements for\n    sequence cherry-pick, from \"no staged changes\" to \"no staged or\n    uncommitted changes\".\n    \n    I've now introduced a new function require_clean_index() reusing the\n    existing code.\n    \n    In the thread, Junio suggested it might make sense to make the\n    requirements for sequence-cherry-pick the same as rebase - either to\n    tighten them as I accidentally did before, or relax rebase to match the\n    existing behavior of merge and cherry-pick. I am not sure the right way\n    to make such changes would be, I think it might warrant a new\n    configuration option, but I definitely think it is beyond the scope of\n    this simple bugfix.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1535%2FTaoK%2Ftao-cherry-pick-sequence-safety-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1535/TaoK/tao-cherry-pick-sequence-safety-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1535\n\nRange-diff vs v1:\n\n 1:  ed225309835 ! 1:  be9a592a34a cherry-pick: refuse cherry-pick sequence if index is dirty\n     @@ Commit message\n          operation to be aborted.\n      \n          Add an earlier check in the cherry-pick sequence process to ensure that\n     -    the index is clean, reusing the already-generalized method used for\n     -    rebase. Also add a test.\n     +    the index is clean, introducing a new general \"quit if index dirty\" function\n     +    derived from the existing worktree-level function used in rebase and pull.\n     +    Also add a test.\n      \n          Signed-off-by: Tao Klerks <tao@klerks.biz>\n      \n     - ## builtin/revert.c ##\n     -@@ builtin/revert.c: static int run_sequencer(int argc, const char **argv, const char *prefix,\n     - \t\treturn sequencer_rollback(the_repository, opts);\n     - \tif (cmd == 's')\n     - \t\treturn sequencer_skip(the_repository, opts);\n     --\treturn sequencer_pick_revisions(the_repository, opts);\n     -+\treturn sequencer_pick_revisions(the_repository, opts, me);\n     - }\n     - \n     - int cmd_revert(int argc, const char **argv, const char *prefix)\n     -\n       ## sequencer.c ##\n      @@ sequencer.c: static int walk_revs_populate_todo(struct todo_list *todo_list,\n       \treturn 0;\n       }\n       \n      -static int create_seq_dir(struct repository *r)\n     -+static int create_seq_dir(struct repository *r, const char *requested_action)\n     ++static const char *cherry_pick_action_name(enum replay_action action) {\n     ++\tswitch (action) {\n     ++\tcase REPLAY_REVERT:\n     ++\t\treturn \"revert\";\n     ++\t\tbreak;\n     ++\tcase REPLAY_PICK:\n     ++\t\treturn \"cherry-pick\";\n     ++\t\tbreak;\n     ++\tdefault:\n     ++\t\tBUG(\"unexpected action in cherry_pick_action_name\");\n     ++\t}\n     ++}\n     ++\n     ++static int create_seq_dir(struct repository *r, enum replay_action requested_action)\n       {\n     - \tenum replay_action action;\n     +-\tenum replay_action action;\n     ++\tenum replay_action in_progress_action;\n     ++\tconst char *in_progress_action_name = NULL;\n       \tconst char *in_progress_error = NULL;\n     -@@ sequencer.c: static int create_seq_dir(struct repository *r)\n     + \tconst char *in_progress_advice = NULL;\n     ++\tconst char *requested_action_name = NULL;\n     + \tunsigned int advise_skip =\n     + \t\trefs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\") ||\n     + \t\trefs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\");\n     + \n     +-\tif (!sequencer_get_last_command(r, &action)) {\n     +-\t\tswitch (action) {\n     +-\t\tcase REPLAY_REVERT:\n     +-\t\t\tin_progress_error = _(\"revert is already in progress\");\n     +-\t\t\tin_progress_advice =\n     +-\t\t\t_(\"try \\\"git revert (--continue | %s--abort | --quit)\\\"\");\n     +-\t\t\tbreak;\n     +-\t\tcase REPLAY_PICK:\n     +-\t\t\tin_progress_error = _(\"cherry-pick is already in progress\");\n     +-\t\t\tin_progress_advice =\n     +-\t\t\t_(\"try \\\"git cherry-pick (--continue | %s--abort | --quit)\\\"\");\n     +-\t\t\tbreak;\n     +-\t\tdefault:\n     +-\t\t\tBUG(\"unexpected action in create_seq_dir\");\n     +-\t\t}\n     ++\tif (!sequencer_get_last_command(r, &in_progress_action)) {\n     ++\t\tin_progress_action_name = cherry_pick_action_name(in_progress_action);\n     ++\t\tin_progress_error = _(\"%s is already in progress\");\n     ++\t\tin_progress_advice =\n     ++\t\t_(\"try \\\"git %s (--continue | %s--abort | --quit)\\\"\");\n     + \t}\n     + \tif (in_progress_error) {\n     +-\t\terror(\"%s\", in_progress_error);\n     ++\t\terror(in_progress_error, in_progress_action_name);\n     + \t\tif (advice_enabled(ADVICE_SEQUENCER_IN_USE))\n     + \t\t\tadvise(in_progress_advice,\n     ++\t\t\t\tin_progress_action_name,\n       \t\t\t\tadvise_skip ? \"--skip | \" : \"\");\n       \t\treturn -1;\n       \t}\n     -+\tif (require_clean_work_tree(r, requested_action,\n     ++\trequested_action_name = cherry_pick_action_name(requested_action);\n     ++\tif (require_clean_index(r, requested_action_name,\n      +\t\t\t\t    _(\"Please commit or stash them.\"), 1, 1))\n      +\t\treturn -1;\n       \tif (mkdir(git_path_seq_dir(), 0777) < 0)\n       \t\treturn error_errno(_(\"could not create sequencer directory '%s'\"),\n       \t\t\t\t   git_path_seq_dir());\n     -@@ sequencer.c: static int single_pick(struct repository *r,\n     - }\n     - \n     - int sequencer_pick_revisions(struct repository *r,\n     --\t\t\t     struct replay_opts *opts)\n     -+\t\t\t     struct replay_opts *opts,\n     -+\t\t\t     const char *action)\n     - {\n     - \tstruct todo_list todo_list = TODO_LIST_INIT;\n     - \tstruct object_id oid;\n      @@ sequencer.c: int sequencer_pick_revisions(struct repository *r,\n       \n       \t/*\n     @@ sequencer.c: int sequencer_pick_revisions(struct repository *r,\n      +\t * first, make sure that the index is clean and that\n      +\t * an existing one isn't in progress.\n       \t */\n     - \n     +-\n       \tif (walk_revs_populate_todo(&todo_list, opts) ||\n      -\t\t\tcreate_seq_dir(r) < 0)\n     -+\t\t\tcreate_seq_dir(r, action) < 0)\n     ++\t\t\tcreate_seq_dir(r, opts->action) < 0)\n       \t\treturn -1;\n       \tif (repo_get_oid(r, \"HEAD\", &oid) && (opts->action == REPLAY_REVERT))\n       \t\treturn error(_(\"can't revert as initial commit\"));\n      \n     - ## sequencer.h ##\n     -@@ sequencer.h: void todo_list_filter_update_refs(struct repository *r,\n     - /* Call this to setup defaults before parsing command line options */\n     - void sequencer_init_config(struct replay_opts *opts);\n     - int sequencer_pick_revisions(struct repository *repo,\n     --\t\t\t     struct replay_opts *opts);\n     -+\t\t\t     struct replay_opts *opts,\n     -+\t\t\t     const char *action);\n     - int sequencer_continue(struct repository *repo, struct replay_opts *opts);\n     - int sequencer_rollback(struct repository *repo, struct replay_opts *opts);\n     - int sequencer_skip(struct repository *repo, struct replay_opts *opts);\n     -\n       ## t/t3510-cherry-pick-sequence.sh ##\n      @@ t/t3510-cherry-pick-sequence.sh: test_expect_success 'cherry-pick persists data on failure' '\n       \ttest_path_is_file .git/sequencer/opts\n     @@ t/t3510-cherry-pick-sequence.sh: test_expect_success 'cherry-pick persists data\n       test_expect_success 'cherry-pick mid-cherry-pick-sequence' '\n       \tpristine_detach initial &&\n       \ttest_must_fail git cherry-pick base..anotherpick &&\n     +\n     + ## wt-status.c ##\n     +@@ wt-status.c: int has_uncommitted_changes(struct repository *r,\n     + \treturn result;\n     + }\n     + \n     +-/**\n     +- * If the work tree has unstaged or uncommitted changes, dies with the\n     +- * appropriate message.\n     +- */\n     +-int require_clean_work_tree(struct repository *r,\n     +-\t\t\t    const char *action,\n     +-\t\t\t    const char *hint,\n     +-\t\t\t    int ignore_submodules,\n     +-\t\t\t    int gently)\n     ++static int require_clean_index_or_work_tree(struct repository *r,\n     ++\t\t\t\t     const char *action,\n     ++\t\t\t\t     const char *hint,\n     ++\t\t\t\t     int ignore_submodules,\n     ++\t\t\t\t     int check_index_only,\n     ++\t\t\t\t     int gently)\n     + {\n     + \tstruct lock_file lock_file = LOCK_INIT;\n     + \tint err = 0, fd;\n     +@@ wt-status.c: int require_clean_work_tree(struct repository *r,\n     + \t\trepo_update_index_if_able(r, &lock_file);\n     + \trollback_lock_file(&lock_file);\n     + \n     +-\tif (has_unstaged_changes(r, ignore_submodules)) {\n     +-\t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n     +-\t\terror(_(\"cannot %s: You have unstaged changes.\"), _(action));\n     +-\t\terr = 1;\n     ++\tif (!check_index_only) {\n     ++\t\tif (has_unstaged_changes(r, ignore_submodules)) {\n     ++\t\t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n     ++\t\t\terror(_(\"cannot %s: You have unstaged changes.\"), _(action));\n     ++\t\t\terr = 1;\n     ++\t\t}\n     + \t}\n     + \n     + \tif (has_uncommitted_changes(r, ignore_submodules)) {\n     +@@ wt-status.c: int require_clean_work_tree(struct repository *r,\n     + \n     + \treturn err;\n     + }\n     ++\n     ++/**\n     ++ * If the work tree has unstaged or uncommitted changes, dies with the\n     ++ * appropriate message.\n     ++ */\n     ++int require_clean_work_tree(struct repository *r,\n     ++\t\t\t    const char *action,\n     ++\t\t\t    const char *hint,\n     ++\t\t\t    int ignore_submodules,\n     ++\t\t\t    int gently)\n     ++{\n     ++\treturn require_clean_index_or_work_tree(r,\n     ++\t\t\t\t\t\taction,\n     ++\t\t\t\t\t\thint,\n     ++\t\t\t\t\t\tignore_submodules,\n     ++\t\t\t\t\t\t0,\n     ++\t\t\t\t\t\tgently);\n     ++}\n     ++\n     ++/**\n     ++ * If the work tree has uncommitted changes, dies with the appropriate\n     ++ * message.\n     ++ */\n     ++int require_clean_index(struct repository *r,\n     ++\t\t\tconst char *action,\n     ++\t\t\tconst char *hint,\n     ++\t\t\tint ignore_submodules,\n     ++\t\t\tint gently)\n     ++{\n     ++\treturn require_clean_index_or_work_tree(r,\n     ++\t\t\t\t\t\taction,\n     ++\t\t\t\t\t\thint,\n     ++\t\t\t\t\t\tignore_submodules,\n     ++\t\t\t\t\t\t1,\n     ++\t\t\t\t\t\tgently);\n     ++}\n     +\n     + ## wt-status.h ##\n     +@@ wt-status.h: int require_clean_work_tree(struct repository *repo,\n     + \t\t\t    const char *hint,\n     + \t\t\t    int ignore_submodules,\n     + \t\t\t    int gently);\n     ++int require_clean_index(struct repository *repo,\n     ++\t\t\t    const char *action,\n     ++\t\t\t    const char *hint,\n     ++\t\t\t    int ignore_submodules,\n     ++\t\t\t    int gently);\n     + \n     + #endif /* STATUS_H */\n\n\n sequencer.c                     | 53 ++++++++++++++++------------\n t/t3510-cherry-pick-sequence.sh | 10 ++++++\n wt-status.c                     | 61 ++++++++++++++++++++++++++-------\n wt-status.h                     |  5 +++\n 4 files changed, 94 insertions(+), 35 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b553b49fbb6..ea1c34045d3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3162,38 +3162,48 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,\n \treturn 0;\n }\n \n-static int create_seq_dir(struct repository *r)\n+static const char *cherry_pick_action_name(enum replay_action action) {\n+\tswitch (action) {\n+\tcase REPLAY_REVERT:\n+\t\treturn \"revert\";\n+\t\tbreak;\n+\tcase REPLAY_PICK:\n+\t\treturn \"cherry-pick\";\n+\t\tbreak;\n+\tdefault:\n+\t\tBUG(\"unexpected action in cherry_pick_action_name\");\n+\t}\n+}\n+\n+static int create_seq_dir(struct repository *r, enum replay_action requested_action)\n {\n-\tenum replay_action action;\n+\tenum replay_action in_progress_action;\n+\tconst char *in_progress_action_name = NULL;\n \tconst char *in_progress_error = NULL;\n \tconst char *in_progress_advice = NULL;\n+\tconst char *requested_action_name = NULL;\n \tunsigned int advise_skip =\n \t\trefs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\") ||\n \t\trefs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\");\n \n-\tif (!sequencer_get_last_command(r, &action)) {\n-\t\tswitch (action) {\n-\t\tcase REPLAY_REVERT:\n-\t\t\tin_progress_error = _(\"revert is already in progress\");\n-\t\t\tin_progress_advice =\n-\t\t\t_(\"try \\\"git revert (--continue | %s--abort | --quit)\\\"\");\n-\t\t\tbreak;\n-\t\tcase REPLAY_PICK:\n-\t\t\tin_progress_error = _(\"cherry-pick is already in progress\");\n-\t\t\tin_progress_advice =\n-\t\t\t_(\"try \\\"git cherry-pick (--continue | %s--abort | --quit)\\\"\");\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tBUG(\"unexpected action in create_seq_dir\");\n-\t\t}\n+\tif (!sequencer_get_last_command(r, &in_progress_action)) {\n+\t\tin_progress_action_name = cherry_pick_action_name(in_progress_action);\n+\t\tin_progress_error = _(\"%s is already in progress\");\n+\t\tin_progress_advice =\n+\t\t_(\"try \\\"git %s (--continue | %s--abort | --quit)\\\"\");\n \t}\n \tif (in_progress_error) {\n-\t\terror(\"%s\", in_progress_error);\n+\t\terror(in_progress_error, in_progress_action_name);\n \t\tif (advice_enabled(ADVICE_SEQUENCER_IN_USE))\n \t\t\tadvise(in_progress_advice,\n+\t\t\t\tin_progress_action_name,\n \t\t\t\tadvise_skip ? \"--skip | \" : \"\");\n \t\treturn -1;\n \t}\n+\trequested_action_name = cherry_pick_action_name(requested_action);\n+\tif (require_clean_index(r, requested_action_name,\n+\t\t\t\t    _(\"Please commit or stash them.\"), 1, 1))\n+\t\treturn -1;\n \tif (mkdir(git_path_seq_dir(), 0777) < 0)\n \t\treturn error_errno(_(\"could not create sequencer directory '%s'\"),\n \t\t\t\t   git_path_seq_dir());\n@@ -5223,12 +5233,11 @@ int sequencer_pick_revisions(struct repository *r,\n \n \t/*\n \t * Start a new cherry-pick/ revert sequence; but\n-\t * first, make sure that an existing one isn't in\n-\t * progress\n+\t * first, make sure that the index is clean and that\n+\t * an existing one isn't in progress.\n \t */\n-\n \tif (walk_revs_populate_todo(&todo_list, opts) ||\n-\t\t\tcreate_seq_dir(r) < 0)\n+\t\t\tcreate_seq_dir(r, opts->action) < 0)\n \t\treturn -1;\n \tif (repo_get_oid(r, \"HEAD\", &oid) && (opts->action == REPLAY_REVERT))\n \t\treturn error(_(\"can't revert as initial commit\"));\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 3b0fa66c33d..e8f4138bf89 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -47,6 +47,16 @@ test_expect_success 'cherry-pick persists data on failure' '\n \ttest_path_is_file .git/sequencer/opts\n '\n \n+test_expect_success 'cherry-pick sequence refuses to run on dirty index' '\n+\tpristine_detach initial &&\n+\ttouch localindexchange &&\n+\tgit add localindexchange &&\n+\techo picking &&\n+\ttest_must_fail git cherry-pick initial..picked &&\n+\ttest_path_is_missing .git/sequencer &&\n+\ttest_must_fail git cherry-pick --abort\n+'\n+\n test_expect_success 'cherry-pick mid-cherry-pick-sequence' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick base..anotherpick &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 068b76ef6d9..e6ecb3fa606 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2616,15 +2616,12 @@ int has_uncommitted_changes(struct repository *r,\n \treturn result;\n }\n \n-/**\n- * If the work tree has unstaged or uncommitted changes, dies with the\n- * appropriate message.\n- */\n-int require_clean_work_tree(struct repository *r,\n-\t\t\t    const char *action,\n-\t\t\t    const char *hint,\n-\t\t\t    int ignore_submodules,\n-\t\t\t    int gently)\n+static int require_clean_index_or_work_tree(struct repository *r,\n+\t\t\t\t     const char *action,\n+\t\t\t\t     const char *hint,\n+\t\t\t\t     int ignore_submodules,\n+\t\t\t\t     int check_index_only,\n+\t\t\t\t     int gently)\n {\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint err = 0, fd;\n@@ -2635,10 +2632,12 @@ int require_clean_work_tree(struct repository *r,\n \t\trepo_update_index_if_able(r, &lock_file);\n \trollback_lock_file(&lock_file);\n \n-\tif (has_unstaged_changes(r, ignore_submodules)) {\n-\t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n-\t\terror(_(\"cannot %s: You have unstaged changes.\"), _(action));\n-\t\terr = 1;\n+\tif (!check_index_only) {\n+\t\tif (has_unstaged_changes(r, ignore_submodules)) {\n+\t\t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n+\t\t\terror(_(\"cannot %s: You have unstaged changes.\"), _(action));\n+\t\t\terr = 1;\n+\t\t}\n \t}\n \n \tif (has_uncommitted_changes(r, ignore_submodules)) {\n@@ -2659,3 +2658,39 @@ int require_clean_work_tree(struct repository *r,\n \n \treturn err;\n }\n+\n+/**\n+ * If the work tree has unstaged or uncommitted changes, dies with the\n+ * appropriate message.\n+ */\n+int require_clean_work_tree(struct repository *r,\n+\t\t\t    const char *action,\n+\t\t\t    const char *hint,\n+\t\t\t    int ignore_submodules,\n+\t\t\t    int gently)\n+{\n+\treturn require_clean_index_or_work_tree(r,\n+\t\t\t\t\t\taction,\n+\t\t\t\t\t\thint,\n+\t\t\t\t\t\tignore_submodules,\n+\t\t\t\t\t\t0,\n+\t\t\t\t\t\tgently);\n+}\n+\n+/**\n+ * If the work tree has uncommitted changes, dies with the appropriate\n+ * message.\n+ */\n+int require_clean_index(struct repository *r,\n+\t\t\tconst char *action,\n+\t\t\tconst char *hint,\n+\t\t\tint ignore_submodules,\n+\t\t\tint gently)\n+{\n+\treturn require_clean_index_or_work_tree(r,\n+\t\t\t\t\t\taction,\n+\t\t\t\t\t\thint,\n+\t\t\t\t\t\tignore_submodules,\n+\t\t\t\t\t\t1,\n+\t\t\t\t\t\tgently);\n+}\ndiff --git a/wt-status.h b/wt-status.h\nindex ab9cc9d8f03..9f424d7c16c 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -181,5 +181,10 @@ int require_clean_work_tree(struct repository *repo,\n \t\t\t    const char *hint,\n \t\t\t    int ignore_submodules,\n \t\t\t    int gently);\n+int require_clean_index(struct repository *repo,\n+\t\t\t    const char *action,\n+\t\t\t    const char *hint,\n+\t\t\t    int ignore_submodules,\n+\t\t\t    int gently);\n \n #endif /* STATUS_H */\n\nbase-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680\n-- \ngitgitgadget\n"},{"id":"477800","messageId":"2bd15b46-c6a4-5b97-9d0f-cb605a4627c1@gmail.com","threadId":"59782","inReplyTo":"CAPMMpoic_+RATwS46=Bd2K4+D_5yEw9RQFGR075Bs4aQJUjtsQ@mail.gmail.com","subject":"Re: [PATCH] cherry-pick: refuse cherry-pick sequence if index is dirty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-30T13:01:25Z","receivedAt":"2023-05-30T13:02:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/05/2023 10:33, Tao Klerks wrote:\n> On Wed, May 24, 2023 at 2:06 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Tao Klerks <tao@klerks.biz> writes:\n>>\n>>> The current implementation of this patch is far too restrictive. It\n>>> doesn't break any tests (and maybe I should add one now that I know),\n>>> but it's doing the wrong thing.\n>>\n>> I am ambivalent.  What do we want to see in a multi-pick sequence\n>> that is different from rebase?\n> \n> I would argue there are primarily three things that are different:\n> 1. The checkout of the new base (and checkout of the original in an \"--abort\")\n> 2. The support for and/or more-common expectation of \"messing\" with\n> commits as you go, eg squash, edit\n> 3. The (partial) support for rebasing/recreating merge commits\n> \n> I'm not sure to what extent any of these justify having tighter\n> restrictions on when we allow a rebase to start, though.\n> \n>> A single-step cherry-pick can fail\n>> safely before it touches the index or the working tree files, but if\n>> two-step cherry-pick, whose first step succeeds, finds that it\n>> cannot safely carry out its second step without clobbering the local\n>> changes made to the working tree files, what should happen?  Are we\n>> OK if we stopped in the state just after the first step has already\n>> been done?\n> \n> This is the current behavior: it stops before the specific pick that\n> is going to affect local unstaged changes, or if there are *any*\n> staged changes (in which case it stops as it's about to do the first\n> pick - the first time this check runs). The reasoning for this\n> behavior, as I understand it, is that the \"--abort\" strategy,\n> intending to \"undo whatever I started doing here, including a conflict\n> resolution\", resets the index. So as long as there is nothing you want\n> to keep in the index, and as long as we know that any previous picks\n> haven't impacted any files with unstaged changes, we're good.\n\n\"cherry-pick --abort\" uses \"reset --merge\" so is slightly more \ncomplicated than just resetting the index.\n\n> The bug that I want to fix is that we only end up checking whether\n> there are changes in the index *after* we've already committed to\n> resetting the index upon later \"--abort\". It's a kind of catch-22:\n> we've detected that aborting would destroy your work, so we leave you\n> in a state where the most obvious thing to do is abort, so we destroy\n> your work... Of course, if you understand what's going on you can\n> choose to \"--quit\" and *not* lose your work... but this is completely\n> antithetical to the general intent of \"--abort\".\n> \n> There's another, smaller flaw here I think, common to Merge,\n> Single-Cherry-Pick, and Sequence-Cherry-Pick, which is that *if* you\n> start with unstaged changes, and you end up in a conflict resolution\n> or \"--no-commit\" pause, and you then \"git add\" your unstaged changes\n> during that pause/resolution, and you *then* later \"--abort\"... then\n> your originally-unstaged changes are destroyed by the \"--abort\" - so\n> it has *not* taken you back to where you were before the operation\n> started. This is, to me as a user, non-obvious, and could potentially\n> lead to data loss. The only way I see to fix that, is to have *all* of\n> these operations refuse to operate on dirty worktrees altogether -\n> like rebase already does.\n> \n> I suspect this level of \"strictness\" would be welcome to newcomers,\n> and less welcome to existing experienced users.\n\nIndeed, being able to cherry-pick when the worktree is dirty is \noccasionally useful but it is confusing when \"cherry-pick --abort\" does \nnot restore the pre-cherry-pick state (and as Junio says below it would \nbe complicated to preserve the initial changes when aborting).\n\n>> My (tentative) answer to that question is \"yes\", but the recovery\n>> options of \"cherry-pick\" may want to work differently from what we\n>> have seen them traditionally do.  Namely, the user accepts that the\n>> first step is already done, and stopping \"cherry-pick\", be it called\n>> \"--abort\" or something else, should just remove the sequencer state\n>> and behave as if the single-pick cherry-pick on the first step only\n>> has just finished and leave such a state in the index and the\n>> working tree.\n> \n> This behavior exists, and is called \"--quit\", right?\n\nI think so though I'm not sure what what \"--quit\" does to CHERRY_PICK_HEAD.\n\n> The semantics as I understand it are:\n> --quit: I know what I'm doing, just remove any \"ongoing operation\"\n> metadata and let me work with the current index and worktree.\n> --abort: This was a bad idea, please take me back to where I was\n> before I started this operation (without losing any work I had\n> ongoing, pls!)\n> \n>>   If that is what we are going to do, then it would\n>> make sense to adopt the same safety semantics we use for \"git merge\"\n>> and \"git checkout\" to ensure only that the index is clean, relying\n>> on the unpack-trees machinery that stops before clobbering a locally\n>> modified working tree files.\n> \n> Yep\n> \n>> But if we are to aim for \"all-or-none\"\n>> semantics people expect from aborting \"git rebase\", I suspect that\n>> it would be way too complicated to allow random changes in the\n>> working tree files that we may only discover to be problems after\n>> starting the sequence of replaying commits one-by-one, and \"too\n>> restrictive\" check may be justified.\n >\n> I don't think I understand this argument. If we want to support both\n> sets of semantics, then that's exactly what \"--quit\" and \"--abort\"\n> achieve, right? (as long as we check for the dirty index *before*\n> committing to destroying the index in case of \"--abort\")\n> \n>>   To put it differently, if it\n>> is too restrictive for multi-pick, then we would want to loosen it\n>> for \"git rebase\" as well, as the issues are likely to be the same.\n> \n> My argument for only changing \"sequence-cherry-pick\" here, and having\n> it (continue to) use the index-safety-only semantics of\n> single-cherry-pick and merge, is that *this is not a change in\n> cabability* - it is only a bugfix. Switching to the worktree-safety\n> semantics of rebase would be a substantial change in behavior beyond\n> the bugfix.\n> \n> I, personally, would prefer to see the worktree-safety semantics of\n> rebase be used in *all* these operations, so I could no longer shoot\n> myself in the foot by starting a merge, accidentally staging some\n> previously-unstaged changes during conflict resolution, and then\n> losing those changes by \"--abort\"ing. But I expect that this kind of\n> change would need to be behind a config option of some sort, trading\n> off safety against low friction.\n> \n> I could imagine a setting like \"core.OperationWorktreeSafety\", with\n> settings \"default\" (current behavior - rebase disallows dirty\n> worktrees, the others disallow dirty index), \"strict\" (all behave like\n> current rebase) and \"lax\" (all behave like merge).\n> \n> As discussed elsewhere, I would also like to (have an option to) treat\n> untracked files as \"worktree dirtiness\"/unstaged changes in exactly\n> the same way as changes to tracked files - but that's another topic :)\n> \n> I'll prepare a v2 with index-safety-only for sequence-cherry-pick for\n> now, please let me know if a (better-named)\n> \"core.OperationWorktreeSafety\" option is something that you'd be\n> interested in / that would make sense to you.\n\nI think it's worth considering a config option. One problem though is \nthat if the user has to actively enable it to get greater safety it wont \nbe much help to new users who don't know about it and if it is on by \ndefault we'll be breaking someone's existing workflow.\n\nBest Wishes\n\nPhillip\n> Thanks!\n\n"},{"id":"477801","messageId":"999f12b2-38d6-f446-e763-4985116ad37d@gmail.com","threadId":"59782","inReplyTo":"pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] cherry-pick: refuse cherry-pick sequence if index is dirty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-30T14:16:06Z","receivedAt":"2023-05-30T14:16:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Tao\n\nOn 28/05/2023 10:08, Tao Klerks via GitGitGadget wrote:\n> From: Tao Klerks <tao@klerks.biz>\n> \n> Cherry-pick, like merge or rebase, refuses to run when there are changes\n> in the index. However, if a cherry-pick sequence is requested, this\n> refusal happens \"too late\": when the cherry-pick sequence has already\n> started, and an \"--abort\" or \"--quit\" is needed to resume normal\n> operation.\n> \n> Normally, when an operation is \"in-progress\" and you want to go back to\n> where you were before, \"--abort\" is the right thing to run. If you run\n> \"git cherry-pick --abort\" in this specific situation, however, your\n> staged changes are destroyed as part of the abort! Generally speaking,\n> the abort process assumes any changes in the index are part of the\n> operation to be aborted.\n> \n> Add an earlier check in the cherry-pick sequence process to ensure that\n> the index is clean, introducing a new general \"quit if index dirty\" function\n> derived from the existing worktree-level function used in rebase and pull.\n> Also add a test.\n\nThanks for working on this, I think it useful to have this added safety \ncheck.\n>   sequencer.c                     | 53 ++++++++++++++++------------\n>   t/t3510-cherry-pick-sequence.sh | 10 ++++++\n>   wt-status.c                     | 61 ++++++++++++++++++++++++++-------\n>   wt-status.h                     |  5 +++\n>   4 files changed, 94 insertions(+), 35 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index b553b49fbb6..ea1c34045d3 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3162,38 +3162,48 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,\n>   \treturn 0;\n>   }\n>   \n> -static int create_seq_dir(struct repository *r)\n> +static const char *cherry_pick_action_name(enum replay_action action) {\n> +\tswitch (action) {\n> +\tcase REPLAY_REVERT:\n> +\t\treturn \"revert\";\n> +\t\tbreak;\n> +\tcase REPLAY_PICK:\n> +\t\treturn \"cherry-pick\";\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tBUG(\"unexpected action in cherry_pick_action_name\");\n> +\t}\n> +}\n> +\n> +static int create_seq_dir(struct repository *r, enum replay_action requested_action)\n>   {\n> -\tenum replay_action action;\n> +\tenum replay_action in_progress_action;\n> +\tconst char *in_progress_action_name = NULL;\n>   \tconst char *in_progress_error = NULL;\n>   \tconst char *in_progress_advice = NULL;\n> +\tconst char *requested_action_name = NULL;\n>   \tunsigned int advise_skip =\n>   \t\trefs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\") ||\n>   \t\trefs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\");\n>   \n> -\tif (!sequencer_get_last_command(r, &action)) {\n> -\t\tswitch (action) {\n> -\t\tcase REPLAY_REVERT:\n> -\t\t\tin_progress_error = _(\"revert is already in progress\");\n> -\t\t\tin_progress_advice =\n> -\t\t\t_(\"try \\\"git revert (--continue | %s--abort | --quit)\\\"\");\n> -\t\t\tbreak;\n> -\t\tcase REPLAY_PICK:\n> -\t\t\tin_progress_error = _(\"cherry-pick is already in progress\");\n> -\t\t\tin_progress_advice =\n> -\t\t\t_(\"try \\\"git cherry-pick (--continue | %s--abort | --quit)\\\"\");\n> -\t\t\tbreak;\n> -\t\tdefault:\n> -\t\t\tBUG(\"unexpected action in create_seq_dir\");\n> -\t\t}\n> +\tif (!sequencer_get_last_command(r, &in_progress_action)) {\n> +\t\tin_progress_action_name = cherry_pick_action_name(in_progress_action);\n> +\t\tin_progress_error = _(\"%s is already in progress\");\n> +\t\tin_progress_advice =\n> +\t\t_(\"try \\\"git %s (--continue | %s--abort | --quit)\\\"\");\n>   \t}\n>   \tif (in_progress_error) {\n> -\t\terror(\"%s\", in_progress_error);\n> +\t\terror(in_progress_error, in_progress_action_name);\n>   \t\tif (advice_enabled(ADVICE_SEQUENCER_IN_USE))\n>   \t\t\tadvise(in_progress_advice,\n> +\t\t\t\tin_progress_action_name,\n>   \t\t\t\tadvise_skip ? \"--skip | \" : \"\");\n>   \t\treturn -1;\n>   \t}\n\nI found the changes up to this point a bit confusing. Maybe I've missed \nsomething but I don't think they are really related to fixing the bug \ndescribed in the commit message. As such they're a distraction from the \n\"real\" fix.\n\n\n> +\trequested_action_name = cherry_pick_action_name(requested_action);\n\nWe already have the function action_name() so I don't think we need to \nadd cherry_pick_action_name(). Also the name of the new function is \nconfusing as it may return \"revert\".\n\n> +\tif (require_clean_index(r, requested_action_name,\n> +\t\t\t\t    _(\"Please commit or stash them.\"), 1, 1))\n\nHow does this interact with \"--no-commit\"? I think the check that you \nrefer to in the commit message is in do_pick_commit() where we have\n\n\tif (opts->no_commit) {\n\t\t/*\n\t\t * We do not intend to commit immediately.  We just want to\n\t\t * merge the differences in, so let's compute the tree\n\t\t * that represents the \"current\" state for the merge machinery\n\t\t * to work on.\n\t\t */\n\t\tif (write_index_as_tree(&head, r->index, r->index_file, 0, NULL))\n\t\t\treturn error(_(\"your index file is unmerged.\"));\n\t} else {\n\t\tunborn = repo_get_oid(r, \"HEAD\", &head);\n\t\t/* Do we want to generate a root commit? */\n\t\tif (is_pick_or_similar(command) && opts->have_squash_onto &&\n\t\t    oideq(&head, &opts->squash_onto)) {\n\t\t\tif (is_fixup(command))\n\t\t\t\treturn error(_(\"cannot fixup root commit\"));\n\t\t\tflags |= CREATE_ROOT_COMMIT;\n\t\t\tunborn = 1;\n\t\t} else if (unborn)\n\t\t\toidcpy(&head, the_hash_algo->empty_tree);\n\t\tif (index_differs_from(r, unborn ? empty_tree_oid_hex() : \"HEAD\",\n\t\t\t\t       NULL, 0))\n\t\t\treturn error_dirty_index(r, opts);\n\t}\n\nI think it would be simpler to reuse the existing check by extracting \nthe \"else\" clause above into a separate function in sequencer.c and call \nit here guarded by \"if (!opts->no_commit)\" as well as in that \"else\" \nclause in do_pick_commit()\n\nBest Wishes\n\nPhillip\n\n> +\t\treturn -1;\n>   \tif (mkdir(git_path_seq_dir(), 0777) < 0)\n>   \t\treturn error_errno(_(\"could not create sequencer directory '%s'\"),\n>   \t\t\t\t   git_path_seq_dir());\n> @@ -5223,12 +5233,11 @@ int sequencer_pick_revisions(struct repository *r,\n>   \n>   \t/*\n>   \t * Start a new cherry-pick/ revert sequence; but\n> -\t * first, make sure that an existing one isn't in\n> -\t * progress\n> +\t * first, make sure that the index is clean and that\n> +\t * an existing one isn't in progress.\n>   \t */\n> -\n>   \tif (walk_revs_populate_todo(&todo_list, opts) ||\n> -\t\t\tcreate_seq_dir(r) < 0)\n> +\t\t\tcreate_seq_dir(r, opts->action) < 0)\n>   \t\treturn -1;\n>   \tif (repo_get_oid(r, \"HEAD\", &oid) && (opts->action == REPLAY_REVERT))\n>   \t\treturn error(_(\"can't revert as initial commit\"));\n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> index 3b0fa66c33d..e8f4138bf89 100755\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -47,6 +47,16 @@ test_expect_success 'cherry-pick persists data on failure' '\n>   \ttest_path_is_file .git/sequencer/opts\n>   '\n>   \n> +test_expect_success 'cherry-pick sequence refuses to run on dirty index' '\n> +\tpristine_detach initial &&\n> +\ttouch localindexchange &&\n> +\tgit add localindexchange &&\n> +\techo picking &&\n> +\ttest_must_fail git cherry-pick initial..picked &&\n> +\ttest_path_is_missing .git/sequencer &&\n> +\ttest_must_fail git cherry-pick --abort\n> +'\n> +\n>   test_expect_success 'cherry-pick mid-cherry-pick-sequence' '\n>   \tpristine_detach initial &&\n>   \ttest_must_fail git cherry-pick base..anotherpick &&\n> diff --git a/wt-status.c b/wt-status.c\n> index 068b76ef6d9..e6ecb3fa606 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -2616,15 +2616,12 @@ int has_uncommitted_changes(struct repository *r,\n>   \treturn result;\n>   }\n>   \n> -/**\n> - * If the work tree has unstaged or uncommitted changes, dies with the\n> - * appropriate message.\n> - */\n> -int require_clean_work_tree(struct repository *r,\n> -\t\t\t    const char *action,\n> -\t\t\t    const char *hint,\n> -\t\t\t    int ignore_submodules,\n> -\t\t\t    int gently)\n> +static int require_clean_index_or_work_tree(struct repository *r,\n> +\t\t\t\t     const char *action,\n> +\t\t\t\t     const char *hint,\n> +\t\t\t\t     int ignore_submodules,\n> +\t\t\t\t     int check_index_only,\n> +\t\t\t\t     int gently)\n>   {\n>   \tstruct lock_file lock_file = LOCK_INIT;\n>   \tint err = 0, fd;\n> @@ -2635,10 +2632,12 @@ int require_clean_work_tree(struct repository *r,\n>   \t\trepo_update_index_if_able(r, &lock_file);\n>   \trollback_lock_file(&lock_file);\n>   \n> -\tif (has_unstaged_changes(r, ignore_submodules)) {\n> -\t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n> -\t\terror(_(\"cannot %s: You have unstaged changes.\"), _(action));\n> -\t\terr = 1;\n> +\tif (!check_index_only) {\n> +\t\tif (has_unstaged_changes(r, ignore_submodules)) {\n> +\t\t\t/* TRANSLATORS: the action is e.g. \"pull with rebase\" */\n> +\t\t\terror(_(\"cannot %s: You have unstaged changes.\"), _(action));\n> +\t\t\terr = 1;\n> +\t\t}\n>   \t}\n>   \n>   \tif (has_uncommitted_changes(r, ignore_submodules)) {\n> @@ -2659,3 +2658,39 @@ int require_clean_work_tree(struct repository *r,\n>   \n>   \treturn err;\n>   }\n> +\n> +/**\n> + * If the work tree has unstaged or uncommitted changes, dies with the\n> + * appropriate message.\n> + */\n> +int require_clean_work_tree(struct repository *r,\n> +\t\t\t    const char *action,\n> +\t\t\t    const char *hint,\n> +\t\t\t    int ignore_submodules,\n> +\t\t\t    int gently)\n> +{\n> +\treturn require_clean_index_or_work_tree(r,\n> +\t\t\t\t\t\taction,\n> +\t\t\t\t\t\thint,\n> +\t\t\t\t\t\tignore_submodules,\n> +\t\t\t\t\t\t0,\n> +\t\t\t\t\t\tgently);\n> +}\n> +\n> +/**\n> + * If the work tree has uncommitted changes, dies with the appropriate\n> + * message.\n> + */\n> +int require_clean_index(struct repository *r,\n> +\t\t\tconst char *action,\n> +\t\t\tconst char *hint,\n> +\t\t\tint ignore_submodules,\n> +\t\t\tint gently)\n> +{\n> +\treturn require_clean_index_or_work_tree(r,\n> +\t\t\t\t\t\taction,\n> +\t\t\t\t\t\thint,\n> +\t\t\t\t\t\tignore_submodules,\n> +\t\t\t\t\t\t1,\n> +\t\t\t\t\t\tgently);\n> +}\n> diff --git a/wt-status.h b/wt-status.h\n> index ab9cc9d8f03..9f424d7c16c 100644\n> --- a/wt-status.h\n> +++ b/wt-status.h\n> @@ -181,5 +181,10 @@ int require_clean_work_tree(struct repository *repo,\n>   \t\t\t    const char *hint,\n>   \t\t\t    int ignore_submodules,\n>   \t\t\t    int gently);\n> +int require_clean_index(struct repository *repo,\n> +\t\t\t    const char *action,\n> +\t\t\t    const char *hint,\n> +\t\t\t    int ignore_submodules,\n> +\t\t\t    int gently);\n>   \n>   #endif /* STATUS_H */\n> \n> base-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680\n\n"},{"id":"481426","messageId":"CAPMMpoj8udDuDferkaRfoKDV6EHMVO6fH3_GE9SUN51VKbwvJA@mail.gmail.com","threadId":"59782","inReplyTo":"999f12b2-38d6-f446-e763-4985116ad37d@gmail.com","subject":"Re: [PATCH v2] cherry-pick: refuse cherry-pick sequence if index is dirty","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-09-06T05:02:39Z","receivedAt":"2023-09-06T05:02:52Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, May 30, 2023 at 4:16 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> Hi Tao\n>\n> On 28/05/2023 10:08, Tao Klerks via GitGitGadget wrote:\n> > From: Tao Klerks <tao@klerks.biz>\n> >\n> <SNIP>\n>\n> I found the changes up to this point a bit confusing. Maybe I've missed\n> something but I don't think they are really related to fixing the bug\n> described in the commit message. As such they're a distraction from the\n> \"real\" fix.\n>\n\nUnderstood, thanks - *if* I kept them, they should be in a separate\n\"prep refactor\" commit.\n\nThe reason I did all this was just that I needed to build a new\nmessage that displayed the correct cherry-pick action name - something\nthat was, in the existing code, done by repeating the entire advice\nmessage. I didn't want to do the same, and if I was going add what I\nneeded to construct the message more dynamically I figured I should\nupdate the existing repetition-based approach.\n\n>\n> > +     requested_action_name = cherry_pick_action_name(requested_action);\n>\n> We already have the function action_name() so I don't think we need to\n> add cherry_pick_action_name().\n\nThe reason I had added a new one was that action_name() also supported\n\"REPLAY_INTERACTIVE_REBASE\", which should not be an option in the\ncodepath that I was refactoring. I wanted to retain the existing\n\"defensiveness\", but that clearly got in the way of both brevity and\nclarity.\n\n> Also the name of the new function is\n> confusing as it may return \"revert\".\n\nYeah, the name was supposed to reflect the context (\"cherry-pick logic\nwhich also covers revert, as opposed to rebase which also uses\nsequencer but is a substantially separate flow\"), rather than the\noutput value.\n\n>\n> > +     if (require_clean_index(r, requested_action_name,\n> > +                                 _(\"Please commit or stash them.\"), 1, 1))\n>\n> How does this interact with \"--no-commit\"? I think the check that you\n> refer to in the commit message is in do_pick_commit() where we have\n>\n>         if (opts->no_commit) {\n>                 /*\n>                  * We do not intend to commit immediately.  We just want to\n>                  * merge the differences in, so let's compute the tree\n>                  * that represents the \"current\" state for the merge machinery\n>                  * to work on.\n>                  */\n>                 if (write_index_as_tree(&head, r->index, r->index_file, 0, NULL))\n>                         return error(_(\"your index file is unmerged.\"));\n>         } else {\n>                 unborn = repo_get_oid(r, \"HEAD\", &head);\n>                 /* Do we want to generate a root commit? */\n>                 if (is_pick_or_similar(command) && opts->have_squash_onto &&\n>                     oideq(&head, &opts->squash_onto)) {\n>                         if (is_fixup(command))\n>                                 return error(_(\"cannot fixup root commit\"));\n>                         flags |= CREATE_ROOT_COMMIT;\n>                         unborn = 1;\n>                 } else if (unborn)\n>                         oidcpy(&head, the_hash_algo->empty_tree);\n>                 if (index_differs_from(r, unborn ? empty_tree_oid_hex() : \"HEAD\",\n>                                        NULL, 0))\n>                         return error_dirty_index(r, opts);\n>         }\n>\n> I think it would be simpler to reuse the existing check by extracting\n> the \"else\" clause above into a separate function in sequencer.c and call\n> it here guarded by \"if (!opts->no_commit)\" as well as in that \"else\"\n> clause in do_pick_commit()\n\nThat sounds very plausible.\n\nI will (very belatedly) have a go, and submit another version sometime soon.\n\nThanks so much for taking the time to review, and my apologies for the\nmonths-later context revival!\n"}]}