{"thread":{"id":"60625","subject":"[PATCH 1/1] revert/cherry-pick: add --show-current-patch option","startedAt":"2023-12-18T12:10:57Z","lastAt":"2023-12-21T16:33:00Z","messageCount":5,"participants":["Michael Lohmann","Phillip Wood","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"485775","messageId":"20231218121048.68290-1-mi.al.lohmann@gmail.com","threadId":"60625","inReplyTo":null,"subject":"[PATCH 0/1] revert/cherry-pick: add --show-current-patch option","fromName":"Michael Lohmann","fromEmail":"mial.lohmann@gmail.com","sentAt":"2023-12-18T12:10:47Z","receivedAt":"2023-12-18T12:10:57Z","isPatch":true,"sender":{"key":"mial.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Hi,\nI am a lead developer of a small team and quite often I have to\ncherry-pick commits (and sometimes also revert them). When\ncherry-picking multiple commits at once and there is a merge conflict it\nsometimes can be hard to understand what the current patch is trying to\ndo in order to resolve the conflict properly. With `rebase` there is\n`--show-current-patch` and since that is quite helpful I would suggest\nto also add this flag also to `cherry-pick` and `revert`.\n\nSince this is my first contribution to git I am not exactly sure where\nthe best place for this functionality is. From my initial understanding\nthere are two places where to put the actual invocation of the `show`:\n- Duplicate the code (with the needed adaptations) of builtin/rebase.c\n  in builtin/revert.c\n- Create a central function that shows the respective `*_HEAD` depending\n  on the current `action`.\n\nIn this first draft I went with the second option, since I felt that it\nreduces code duplication and the sequencer already has the action enum\nwith exactly those three cases. On the other hand I don’t really have a\ngood understanding of the role that this `sequencer` should play and if\nthis adds additional coupling that is unwanted. My current impression\nis, that this would be the right place, since this looks to be the core\nof the commands where a user can apply a sequence of commits and in my\nopinion even if additional actions would be added, they could also fail\nand so it would be good to add the `--show-current-patch` option to that\none as well.\n\nSide note: my only C(++) experience was ~10 years ago and only for a\nsingle university course, so my perspective is much more from a general\narchitecture point of view than based on any C experience, let alone in\nthis code base and so I would be very grateful for criticism!\n\n\nSide note: The check for the `REBASE_HEAD` would not be necessary, since\nthat is already taken care of in the builtin/rebase.c before.\nNevertheless I opted for this check, because I would much rather require\nthe same preconditions no matter from where I call this function. The\nwhole argument parsing / option struct are very different between rebase\nand revert. Maybe it would make sense to align them a bit further?\nInitial observations: `rebase_options->type` is functionally similar to\n`replay_opts->action` (as in \"what general action am I performing? -\ninteractive rebase / cherry-pick / revert / ...\") whereas\n`rebase_options->action` is not part of the `replay_opts` struct at all.\nInstead the role is taken over in builtin/revert.c by `int cmd = 0;`.\nI am preparing a patch converting this to an enum, so that there are\nno random chars that have to be kept in sync manually in different\nplaces, or is that a design decision?\n\nI looked through the mailing list archive and did not find anything\nrelated on this topic. The only slightly related thread I could find was\nin [1] by Elijah Newren and that one was talking about a separate\npossible feature and how to get certain information if CHERRY_PICK_HEAD\nand REVERT_HEAD were to be replaced by a different construct. I hope I\ndid not miss something...\n\nCheers\nMichael\n\n[1]:\nhttps://lore.kernel.org/git/CABPp-BGd-W8T7EsvKYyjdi3=mfSTJ8zM-uzVsFnh1AWyV2wEzQ@mail.gmail.com\n\nMichael Lohmann (1):\n  revert/cherry-pick: add --show-current-patch option\n\n Documentation/git-cherry-pick.txt      |  2 +-\n Documentation/git-revert.txt           |  2 +-\n Documentation/sequencer.txt            |  5 +++++\n builtin/rebase.c                       |  7 ++----\n builtin/revert.c                       |  9 ++++++--\n contrib/completion/git-completion.bash |  2 +-\n sequencer.c                            | 24 +++++++++++++++++++++\n sequencer.h                            |  2 ++\n t/t3507-cherry-pick-conflict.sh        | 30 ++++++++++++++++++++++++++\n 9 files changed, 73 insertions(+), 10 deletions(-)\n\n-- \n2.43.0.77.gff6ea8bb74\n\n"},{"id":"485774","messageId":"20231218121048.68290-2-mi.al.lohmann@gmail.com","threadId":"60625","inReplyTo":"20231218121048.68290-1-mi.al.lohmann@gmail.com","subject":"[PATCH 1/1] revert/cherry-pick: add --show-current-patch option","fromName":"Michael Lohmann","fromEmail":"mial.lohmann@gmail.com","sentAt":"2023-12-18T12:10:48Z","receivedAt":"2023-12-18T12:10:58Z","isPatch":true,"sender":{"key":"mial.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"This aligns the interface to the rebase one and allows for an easier way\nof figuring out how to resolve conflicts if commits fail to apply\n(especially when reverting/cherry-picking multiple commits at the same\ntime)\n\nSigned-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>\n---\n Documentation/git-cherry-pick.txt      |  2 +-\n Documentation/git-revert.txt           |  2 +-\n Documentation/sequencer.txt            |  5 +++++\n builtin/rebase.c                       |  7 ++----\n builtin/revert.c                       |  9 ++++++--\n contrib/completion/git-completion.bash |  2 +-\n sequencer.c                            | 24 +++++++++++++++++++++\n sequencer.h                            |  2 ++\n t/t3507-cherry-pick-conflict.sh        | 30 ++++++++++++++++++++++++++\n 9 files changed, 73 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex fdcad3d200..af41903fe7 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n [verse]\n 'git cherry-pick' [--edit] [-n] [-m <parent-number>] [-s] [-x] [--ff]\n \t\t  [-S[<keyid>]] <commit>...\n-'git cherry-pick' (--continue | --skip | --abort | --quit)\n+'git cherry-pick' (--continue | --skip | --abort | --quit | --show-current-patch)\n \n DESCRIPTION\n -----------\ndiff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\nindex cbe0208834..5bd2ecf35a 100644\n--- a/Documentation/git-revert.txt\n+++ b/Documentation/git-revert.txt\n@@ -9,7 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git revert' [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>...\n-'git revert' (--continue | --skip | --abort | --quit)\n+'git revert' (--continue | --skip | --abort | --quit | --show-current-patch)\n \n DESCRIPTION\n -----------\ndiff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt\nindex 3bceb56474..e9394761bc 100644\n--- a/Documentation/sequencer.txt\n+++ b/Documentation/sequencer.txt\n@@ -12,5 +12,10 @@\n \tto clear the sequencer state after a failed cherry-pick or\n \trevert.\n \n+--show-current-patch::\n+\tShow the current patch when a revert or cherry-pick is\n+\tstopped because of conflicts. This is the equivalent of\n+\t`git show REVERT_HEAD` or `git show CHERRY_PICK_HEAD`.\n+\n --abort::\n \tCancel the operation and return to the pre-sequence state.\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 9f8192e0a5..8ad3cf3e90 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -360,12 +360,9 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n \t\tret = edit_todo_file(flags);\n \t\tbreak;\n \tcase ACTION_SHOW_CURRENT_PATCH: {\n-\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n-\n-\t\tcmd.git_cmd = 1;\n-\t\tstrvec_pushl(&cmd.args, \"show\", \"REBASE_HEAD\", \"--\", NULL);\n-\t\tret = run_command(&cmd);\n+\t\tstruct replay_opts replay_opts = get_replay_opts(opts);\n \n+\t\tret = sequencer_show_current_patch(the_repository, &replay_opts);\n \t\tbreak;\n \t}\n \tdefault:\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e6f9a1ad26..cbcd9fdc23 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -24,14 +24,14 @@\n \n static const char * const revert_usage[] = {\n \tN_(\"git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>...\"),\n-\tN_(\"git revert (--continue | --skip | --abort | --quit)\"),\n+\tN_(\"git revert (--continue | --skip | --abort | --quit | --show-current-patch)\"),\n \tNULL\n };\n \n static const char * const cherry_pick_usage[] = {\n \tN_(\"git cherry-pick [--edit] [-n] [-m <parent-number>] [-s] [-x] [--ff]\\n\"\n \t   \"                [-S[<keyid>]] <commit>...\"),\n-\tN_(\"git cherry-pick (--continue | --skip | --abort | --quit)\"),\n+\tN_(\"git cherry-pick (--continue | --skip | --abort | --quit | --show-current-patch)\"),\n \tNULL\n };\n \n@@ -93,6 +93,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), 'c'),\n \t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), 'a'),\n \t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), 's'),\n+\t\tOPT_CMDMODE(0, \"show-current-patch\", &cmd, N_(\"show the patch file being reverted or cherry-picked\"), 'p'),\n \t\tOPT_CLEANUP(&cleanup_arg),\n \t\tOPT_BOOL('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n@@ -154,6 +155,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\t\tthis_operation = \"--continue\";\n \t\telse if (cmd == 's')\n \t\t\tthis_operation = \"--skip\";\n+\t\telse if (cmd == 'p')\n+\t\t\tthis_operation = \"--show-current-patch\";\n \t\telse {\n \t\t\tassert(cmd == 'a');\n \t\t\tthis_operation = \"--abort\";\n@@ -224,6 +227,8 @@ 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+\tif (cmd == 'p')\n+\t\treturn sequencer_show_current_patch(the_repository, opts);\n \treturn sequencer_pick_revisions(the_repository, opts);\n }\n \ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 13a39ebd2e..b740b7d48c 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1618,7 +1618,7 @@ _git_checkout ()\n \tesac\n }\n \n-__git_sequencer_inprogress_options=\"--continue --quit --abort --skip\"\n+__git_sequencer_inprogress_options=\"--continue --quit --abort --skip --show-current-patch\"\n \n __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options\n \ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed..3f6f9ad75c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3417,6 +3417,30 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)\n \treturn -1;\n }\n \n+int sequencer_show_current_patch(struct repository *r, struct replay_opts *opts)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tcmd.git_cmd = 1;\n+\tswitch (opts->action) {\n+\tcase REPLAY_REVERT:\n+\t\tif (!refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n+\t\t\tdie(_(\"No revert in progress?\"));\n+\t\tstrvec_pushl(&cmd.args, \"show\", \"REVERT_HEAD\", \"--\", NULL);\n+\t\tbreak;\n+\tcase REPLAY_PICK:\n+\t\tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\"))\n+\t\t\tdie(_(\"No cherry-pick in progress?\"));\n+\t\tstrvec_pushl(&cmd.args, \"show\", \"CHERRY_PICK_HEAD\", \"--\", NULL);\n+\t\tbreak;\n+\tcase REPLAY_INTERACTIVE_REBASE:\n+\t\tif (!refs_ref_exists(get_main_ref_store(r), \"REBASE_HEAD\"))\n+\t\t\tdie(_(\"No rebase in progress?\"));\n+\t\tstrvec_pushl(&cmd.args, \"show\", \"REBASE_HEAD\", \"--\", NULL);\n+\t\tbreak;\n+\t}\n+\treturn run_command(&cmd);\n+}\n+\n static int save_todo(struct todo_list *todo_list, struct replay_opts *opts,\n \t\t     int reschedule)\n {\ndiff --git a/sequencer.h b/sequencer.h\nindex 913a0f652d..e20cb8bc56 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -162,6 +162,8 @@ int sequencer_pick_revisions(struct repository *repo,\n \t\t\t     struct replay_opts *opts);\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_show_current_patch(struct repository *repo,\n+\t\t\t\t struct replay_opts *opts);\n int sequencer_skip(struct repository *repo, struct replay_opts *opts);\n void replay_opts_release(struct replay_opts *opts);\n int sequencer_remove_state(struct replay_opts *opts);\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex c88d597b12..4f50d287a6 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -566,6 +566,36 @@ test_expect_success 'cherry-pick preserves sparse-checkout' '\n \ttest_grep ! \"Changes not staged for commit:\" actual\n '\n \n+test_expect_success 'cherry-pick --show-current-patch fails if no cherry-pick in progress' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --show-current-patch\n+'\n+\n+test_expect_success 'cherry-pick --show-current-patch describes patch that failed to apply' '\n+\ttest_when_finished \"git cherry-pick --abort || :\" &&\n+\tpristine_detach initial &&\n+\tgit show picked >expected &&\n+\n+\ttest_must_fail git cherry-pick picked &&\n+\n+\tgit cherry-pick --show-current-patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'revert --show-current-patch fails if no revert in progress' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git revert --show-current-patch\n+'\n+\n+test_expect_success 'revert --show-current-patch describes patch that failed to apply' '\n+\ttest_when_finished \"git revert --abort || :\" &&\n+\tpristine_detach initial &&\n+\tgit show picked >expected &&\n+\ttest_must_fail git revert picked &&\n+\tgit revert --show-current-patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'cherry-pick --continue remembers --keep-redundant-commits' '\n \ttest_when_finished \"git cherry-pick --abort || :\" &&\n \tpristine_detach initial &&\n-- \n2.43.0.77.gff6ea8bb74\n\n"},{"id":"485785","messageId":"42ff6b11-f991-4a6d-ad68-ca8c5a3cd735@gmail.com","threadId":"60625","inReplyTo":"20231218121048.68290-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH 0/1] revert/cherry-pick: add --show-current-patch option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-18T16:42:52Z","receivedAt":"2023-12-18T16:42:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michael\n\nOn 18/12/2023 12:10, Michael Lohmann wrote:\n> Hi,\n> I am a lead developer of a small team and quite often I have to\n> cherry-pick commits (and sometimes also revert them). When\n> cherry-picking multiple commits at once and there is a merge conflict it\n> sometimes can be hard to understand what the current patch is trying to\n> do in order to resolve the conflict properly. With `rebase` there is\n> `--show-current-patch` and since that is quite helpful I would suggest\n> to also add this flag also to `cherry-pick` and `revert`.\n\nThanks for bringing this up I agree it can be very helpful to look at \nthe original commit when resolving cherry-pick and revert conflicts. I'm \nin two minds about this change though - I wonder if it'd be better to \nimprove the documentation for CHERRY_PICK_HEAD and REVERT_HEAD and tell \nusers to run \"git show CHERRY_PICK_HEAD\" instead. I think the main \nreason we have a \"--show-current-patch\" option for \"rebase\" is that \nthere are two different implementations of that command and the \npatched-based one of them does not support REBASE_HEAD. That reasoning \ndoes not apply to \"cherry-pick\" and \"revert\" and \"--show-current-patch\" \nsuggests a patch-based implementation which is also not the case for \nthese commands.\n\nBest Wishes\n\nPhillip\n\n> Since this is my first contribution to git I am not exactly sure where\n> the best place for this functionality is. From my initial understanding\n> there are two places where to put the actual invocation of the `show`:\n> - Duplicate the code (with the needed adaptations) of builtin/rebase.c\n>    in builtin/revert.c\n> - Create a central function that shows the respective `*_HEAD` depending\n>    on the current `action`.\n> \n> In this first draft I went with the second option, since I felt that it\n> reduces code duplication and the sequencer already has the action enum\n> with exactly those three cases. On the other hand I don’t really have a\n> good understanding of the role that this `sequencer` should play and if\n> this adds additional coupling that is unwanted. My current impression\n> is, that this would be the right place, since this looks to be the core\n> of the commands where a user can apply a sequence of commits and in my\n> opinion even if additional actions would be added, they could also fail\n> and so it would be good to add the `--show-current-patch` option to that\n> one as well.\n> \n> Side note: my only C(++) experience was ~10 years ago and only for a\n> single university course, so my perspective is much more from a general\n> architecture point of view than based on any C experience, let alone in\n> this code base and so I would be very grateful for criticism!\n> \n> \n> Side note: The check for the `REBASE_HEAD` would not be necessary, since\n> that is already taken care of in the builtin/rebase.c before.\n> Nevertheless I opted for this check, because I would much rather require\n> the same preconditions no matter from where I call this function. The\n> whole argument parsing / option struct are very different between rebase\n> and revert. Maybe it would make sense to align them a bit further?\n> Initial observations: `rebase_options->type` is functionally similar to\n> `replay_opts->action` (as in \"what general action am I performing? -\n> interactive rebase / cherry-pick / revert / ...\") whereas\n> `rebase_options->action` is not part of the `replay_opts` struct at all.\n> Instead the role is taken over in builtin/revert.c by `int cmd = 0;`.\n> I am preparing a patch converting this to an enum, so that there are\n> no random chars that have to be kept in sync manually in different\n> places, or is that a design decision?\n> \n> I looked through the mailing list archive and did not find anything\n> related on this topic. The only slightly related thread I could find was\n> in [1] by Elijah Newren and that one was talking about a separate\n> possible feature and how to get certain information if CHERRY_PICK_HEAD\n> and REVERT_HEAD were to be replaced by a different construct. I hope I\n> did not miss something...\n> \n> Cheers\n> Michael\n> \n> [1]:\n> https://lore.kernel.org/git/CABPp-BGd-W8T7EsvKYyjdi3=mfSTJ8zM-uzVsFnh1AWyV2wEzQ@mail.gmail.com\n> \n> Michael Lohmann (1):\n>    revert/cherry-pick: add --show-current-patch option\n> \n>   Documentation/git-cherry-pick.txt      |  2 +-\n>   Documentation/git-revert.txt           |  2 +-\n>   Documentation/sequencer.txt            |  5 +++++\n>   builtin/rebase.c                       |  7 ++----\n>   builtin/revert.c                       |  9 ++++++--\n>   contrib/completion/git-completion.bash |  2 +-\n>   sequencer.c                            | 24 +++++++++++++++++++++\n>   sequencer.h                            |  2 ++\n>   t/t3507-cherry-pick-conflict.sh        | 30 ++++++++++++++++++++++++++\n>   9 files changed, 73 insertions(+), 10 deletions(-)\n> \n"},{"id":"485835","messageId":"20231220065141.7599-1-mi.al.lohmann@gmail.com","threadId":"60625","inReplyTo":"42ff6b11-f991-4a6d-ad68-ca8c5a3cd735@gmail.com","subject":"Re: Re: [PATCH 0/1] revert/cherry-pick: add --show-current-patch option","fromName":"Michael Lohmann","fromEmail":"mial.lohmann@gmail.com","sentAt":"2023-12-20T06:51:41Z","receivedAt":"2023-12-20T06:52:09Z","isPatch":true,"sender":{"key":"mial.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Hi Phillip\n\nOn 18/12/2023 16:42, Phillip Wood wrote:\n> Thanks for bringing this up I agree it can be very helpful to look at\n> the original commit when resolving cherry-pick and revert conflicts.\n> I'm in two minds about this change though - I wonder if it'd be better\n> to improve the documentation for CHERRY_PICK_HEAD and REVERT_HEAD and\n> tell users to run \"git show CHERRY_PICK_HEAD\" instead. I think the\n> main reason we have a \"--show-current-patch\" option for \"rebase\" is\n> that there are two different implementations of that command and the\n> patched-based one of them does not support REBASE_HEAD. That reasoning\n> does not apply to \"cherry-pick\" and \"revert\" and\n> \"--show-current-patch\" suggests a patch-based implementation which is\n> also not the case for these commands.\n\nI appreciate the urge of limiting the interface to the minimum needed\nand not to duplicate functionality that already exists. On the other\nhand, this would\na) grant the user the same experience, not having to wonder about\nimplementation details such as different backends for rebase, but not\nfor revert/cherry-pick and\nb) (I know it is more indicative of me, but:) when I am looking for a\nfeature in software and I look into the respective man page I tend to\nfocus first on the synopsis instead of reading the whole page (or\nsometimes I even just rely on the shell autocompletion for\ndiscoverability).\n\nSo yes, mentioning REVERT_HEAD and CHERRY_PICK_HEAD in the respective\ndocs would technically be sufficient, but I don't think it is as\ndiscoverable to an average user (who does not know about the details of\nall the existing pseudo refs) as a toplevel action would be. But an\nassessment of the pros and cons is not on me to decide.\n\nI have to be honest: I have troubles distinguishing a \"patch\" and a\n\"diff\", the latter of which `git show <commit>` shows according to the\ndocumentation (\"For commits it shows the log message and textual\ndiff.\"), though my understanding was that a patch is a diff + context\nlines, which is what `git show` actually shows... I think this is\nprobably why I don't feel so strong about the potential loose usage of\nthe word here.\n\nAlso the documentation of cherry-pick already uses the word \"patch\" in a\n(according to my understanding from a technical perspective) sloppy (but\nfrom a layman's point of view probably nevertheless helpful) way:\n\n> The following sequence attempts to backport a patch, bails out because\n> the code the patch applies to has changed too much, and then tries\n> again, this time exercising more care about matching up context lines.\n> \n> ------------\n> $ git cherry-pick topic^             <1>\n> $ git diff                           <2>\n> $ git cherry-pick --abort            <3>\n> $ git cherry-pick -Xpatience topic^  <4>\n> ------------\n> <1> apply the change that would be shown by `git show topic^`.\n>     In this example, the patch does not apply cleanly, so\n>     information about the conflict is written to the index and\n>     working tree and no new commit results.\n\nShould that also be rephrased?\n\n\nOut of curiosity: The following from the rebase docs seems to imply that\nthe apply backend will probably be removed in the future:\n> --apply\n>           Use applying strategies to rebase (calling git-am\n>           internally). This option may become a no-op in the future\n>           once the merge backend handles everything the apply one\n>           does.\n\nBut I would expect the `rebase --show-current-patch` still to be\nworking. Would that only be a legacy compatibility flag and instead also\nfor rebases the recommended option would be to run\n`git show REBASE_HEAD`?\n\nBest Wishes\n\nMichael\n"},{"id":"485932","messageId":"cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com","threadId":"60625","inReplyTo":"20231220065141.7599-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH 0/1] revert/cherry-pick: add --show-current-patch option","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-21T16:32:57Z","receivedAt":"2023-12-21T16:33:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michael\n\nOn 20/12/2023 06:51, Michael Lohmann wrote:\n> Hi Phillip\n> \n> On 18/12/2023 16:42, Phillip Wood wrote:\n>> Thanks for bringing this up I agree it can be very helpful to look at\n>> the original commit when resolving cherry-pick and revert conflicts.\n\nAs an aside I find it useful is to do a kind of range-diff before \ncommitting the conflict resolution. Unfortunately one cannot use \"git \nrange-diff\" because the conflict resolution is not yet committed. \nInstead I use\n\n     diff <(git diff CHERRY_PICK_HEAD^-) <(git diff HEAD)\n\nin practice it is helpful to pipe the diffs through sed to delete the \n\"index\" lines and normalize the hunk headers.\n\n>> I'm in two minds about this change though - I wonder if it'd be better\n>> to improve the documentation for CHERRY_PICK_HEAD and REVERT_HEAD and\n>> tell users to run \"git show CHERRY_PICK_HEAD\" instead. I think the\n>> main reason we have a \"--show-current-patch\" option for \"rebase\" is\n>> that there are two different implementations of that command and the\n>> patched-based one of them does not support REBASE_HEAD. That reasoning\n>> does not apply to \"cherry-pick\" and \"revert\" and\n>> \"--show-current-patch\" suggests a patch-based implementation which is\n>> also not the case for these commands.\n> \n> I appreciate the urge of limiting the interface to the minimum needed\n> and not to duplicate functionality that already exists. On the other\n> hand, this would\n> a) grant the user the same experience, not having to wonder about\n> implementation details such as different backends for rebase, but not\n> for revert/cherry-pick and\n> b) (I know it is more indicative of me, but:) when I am looking for a\n> feature in software and I look into the respective man page I tend to\n> focus first on the synopsis instead of reading the whole page (or\n> sometimes I even just rely on the shell autocompletion for\n> discoverability).\n> \n> So yes, mentioning REVERT_HEAD and CHERRY_PICK_HEAD in the respective\n> docs would technically be sufficient, but I don't think it is as\n> discoverable to an average user (who does not know about the details of\n> all the existing pseudo refs) as a toplevel action would be. But an\n> assessment of the pros and cons is not on me to decide.\n\nTo make the psuedo refs discoverable we should certainly be mentioning \nthem in the section about resolving conflicts. I haven't checked what \nthe docs say at the moment but a worked example showing how to inspect \nthe conflicts and the original changes would be helpful I think. That \ndoes assume that the user actually reads the section about resolving \nconflicts rather than just scanning the available command line options \nthough.\n\n> I have to be honest: I have troubles distinguishing a \"patch\" and a\n> \"diff\", the latter of which `git show <commit>` shows according to the\n> documentation (\"For commits it shows the log message and textual\n> diff.\"), though my understanding was that a patch is a diff + context\n> lines, which is what `git show` actually shows... I think this is\n> probably why I don't feel so strong about the potential loose usage of\n> the word here.\n\nI think for the purposes of this discussion \"patch\" and \"diff\" are \nlargely interchangeable (a \"patch\" is essentially a \"diff\" with a commit \nmessage). Maybe I'm overthinking it but the reason I'm not very keen on \n\"--show-current-patch\" (in addition to the \"duplicate functionality\" \nargument you mention above) is that cherry-pick and revert do not work \nby applying patches (or diffs) but use a 3-way merge instead. I think \n--show-current-patch first appeared as an option to \"git am\" which makes \nsense as that command is all about applying patches.\n\nI'd be interested to hear what other people think about whether it makes \n\"--show-current-patch\" make sense for other commands.\n\n> Also the documentation of cherry-pick already uses the word \"patch\" in a\n> (according to my understanding from a technical perspective) sloppy (but\n> from a layman's point of view probably nevertheless helpful) way:\n> \n>> The following sequence attempts to backport a patch, bails out because\n>> the code the patch applies to has changed too much, and then tries\n>> again, this time exercising more care about matching up context lines.\n>>\n>> ------------\n>> $ git cherry-pick topic^             <1>\n>> $ git diff                           <2>\n>> $ git cherry-pick --abort            <3>\n>> $ git cherry-pick -Xpatience topic^  <4>\n>> ------------\n>> <1> apply the change that would be shown by `git show topic^`.\n>>      In this example, the patch does not apply cleanly, so\n>>      information about the conflict is written to the index and\n>>      working tree and no new commit results.\n> \n> Should that also be rephrased?\n\nIt would certainly be more accurate for the first paragraph to say \nsomething like\n\n     The following sequence tries to backport a commit. It bails out\n     because the code modified by the commit has conflicting changes in\n     the current branch.\n\nThe bit about exercising more care about matching up context lines is \nmoot these days as the default merge strategy is \"ort\" which uses the \nhistogram diff algorithm to do just that so commands <3> & <4> should \nnot be needed.\n\n> \n> Out of curiosity: The following from the rebase docs seems to imply that\n> the apply backend will probably be removed in the future:\n>> --apply\n>>            Use applying strategies to rebase (calling git-am\n>>            internally). This option may become a no-op in the future\n>>            once the merge backend handles everything the apply one\n>>            does.\n> \n> But I would expect the `rebase --show-current-patch` still to be\n> working. Would that only be a legacy compatibility flag and instead also\n> for rebases the recommended option would be to run\n> `git show REBASE_HEAD`?\n\nThe long term goal is to remove the apply backend but I don't think \nanyone is actively working on it at the moment. We'd certainly need to \nkeep the --show-current-patch option for backwards compatibility.\n\nI'll be off the list for the next couple of weeks but I'll be sure to \ncatch up with this thread in the New Year\n\nBest Wishes\n\nPhillip\n"}]}