{"thread":{"id":"54473","subject":"[RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","startedAt":"2020-10-21T06:25:02Z","lastAt":"2020-10-21T20:53:29Z","messageCount":8,"participants":["Edmundo Carmona Antoranz","Taylor Blau","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"408037","messageId":"20201021062430.2029566-1-eantoranz@gmail.com","threadId":"54473","inReplyTo":null,"subject":"[RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-10-21T06:24:30Z","receivedAt":"2020-10-21T06:25:02Z","isPatch":false,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Currently, if 'git cherry-pick --no-commit' is run _and the cherry-pick\noperation is successful_, the metadata from the original revision is lost and\nto git it's like a cherry-pick operation is not taking place at all. Hence,\nwe can't wrap up the cherry-pick operation by calling\n'git cherry-pick --continue'.\n\nWith this patch, we allow sequencer to save the metadata from the original\ncherry-pick operation so that 'git cherry-pick --continue' can be called.\n\nSigned-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>\n---\n sequencer.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 00acb12496..c1ccbe0faf 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2022,9 +2022,8 @@ static int do_pick_commit(struct repository *r,\n \t * However, if the merge did not even start, then we don't want to\n \t * write it at all.\n \t */\n-\tif ((command == TODO_PICK || command == TODO_REWORD ||\n-\t     command == TODO_EDIT) && !opts->no_commit &&\n-\t    (res == 0 || res == 1) &&\n+\tif ((command == TODO_PICK || command == TODO_REWORD || command == TODO_EDIT)\n+\t\t&& ((res == 0 && opts->no_commit) || (res == 1 && !opts->no_commit)) &&\n \t    update_ref(NULL, \"CHERRY_PICK_HEAD\", &commit->object.oid, NULL,\n \t\t       REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))\n \t\tres = -1;\n-- \n2.28.0\n\n"},{"id":"408038","messageId":"CAOc6eta3xMMH_m3+Apiy7uY94MECLRGiV2y=kNmCzwh0MS=36g@mail.gmail.com","threadId":"54473","inReplyTo":"20201021062430.2029566-1-eantoranz@gmail.com","subject":"Re: [RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-10-21T06:31:05Z","receivedAt":"2020-10-21T06:31:18Z","isPatch":false,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Wed, Oct 21, 2020 at 12:24 AM Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n>\n>\n> With this patch, we allow sequencer to save the metadata from the original\n> cherry-pick operation so that 'git cherry-pick --continue' can be called.\n>\nI would expect 'git cherry-pick --no-commit' to save the metadata from\nthe revision cherry-picked if the operation is successful so to allow\nrunning 'git cherry-pick --continue' afterwards, however that's not\nthe case. I was wondering if this is done on purpose. The patch in the\nRFC is not in any way my final proposal (or even a real proposal) but\njust a little first step so that I can gather information so that I\ncan adjust the code accordingly from all the feedback... _if_ an\nadjustment is needed, of course. I don't know how --no-commit would\nhave to play out if multiple revisions are asked to be processed by\nsequencer, for example.\n"},{"id":"408067","messageId":"20201021161242.GA111581@nand.local","threadId":"54473","inReplyTo":"20201021062430.2029566-1-eantoranz@gmail.com","subject":"Re: [RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-21T16:12:42Z","receivedAt":"2020-10-21T16:12:50Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Edmundo,\n\nOn Wed, Oct 21, 2020 at 12:24:30AM -0600, Edmundo Carmona Antoranz wrote:\n> Currently, if 'git cherry-pick --no-commit' is run _and the cherry-pick\n> operation is successful_, the metadata from the original revision is lost and\n> to git it's like a cherry-pick operation is not taking place at all. Hence,\n> we can't wrap up the cherry-pick operation by calling\n> 'git cherry-pick --continue'.\n\nInteresting.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 00acb12496..c1ccbe0faf 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2022,9 +2022,8 @@ static int do_pick_commit(struct repository *r,\n>  \t * However, if the merge did not even start, then we don't want to\n>  \t * write it at all.\n>  \t */\n> -\tif ((command == TODO_PICK || command == TODO_REWORD ||\n> -\t     command == TODO_EDIT) && !opts->no_commit &&\n> -\t    (res == 0 || res == 1) &&\n> +\tif ((command == TODO_PICK || command == TODO_REWORD || command == TODO_EDIT)\n> +\t\t&& ((res == 0 && opts->no_commit) || (res == 1 && !opts->no_commit)) &&\n>  \t    update_ref(NULL, \"CHERRY_PICK_HEAD\", &commit->object.oid, NULL,\n>  \t\t       REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))\n>  \t\tres = -1;\n> --\n> 2.28.0\n\nHmm. I'm a little confused after reading your patch below. Why does (res\n== 0) change to (res == 0 && opts->no_commit)? Wouldn't we still want to\nupdate our CHERRY_PICK_HEAD even if \"res == 0 && !opts->no_commit\"?\n\nEven still, this patch as it is seems to fail a number of tests. You can\nrun the tests yourself by running \"make && make test\", and there is more\ninformation about that in t/README.\n\nThanks,\nTaylor\n"},{"id":"408079","messageId":"CAOc6etawWz526w-NPU00bvwRt5_dnZEq0Gk0L_2N3NPG3tAGYA@mail.gmail.com","threadId":"54473","inReplyTo":"20201021161242.GA111581@nand.local","subject":"Re: [RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-10-21T18:04:52Z","receivedAt":"2020-10-21T18:05:05Z","isPatch":false,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"I think my previous reply was rejected so let me try again (I\napologize if you get a duplicate):\n\nOn Wed, Oct 21, 2020 at 10:12 AM Taylor Blau <me@ttaylorr.com> wrote:\n> Hmm. I'm a little confused after reading your patch below. Why does (res\n> == 0) change to (res == 0 && opts->no_commit)? Wouldn't we still want to\n> update our CHERRY_PICK_HEAD even if \"res == 0 && !opts->no_commit\"?\n\nMy _guess_ (haven't sat down to study the code that much) is that if\n\"res == 0 && !opts->no_commit\", then commit will wrap up in a normal\nfashion and then there's actually no need for this file to be updated.\nYou can see that the same thing is done when reverting (preexisting\ncode), a few lines after where the patch is applied (if I remember\ncorrectly).\n\n>\n> Even still, this patch as it is seems to fail a number of tests. You can\n> run the tests yourself by running \"make && make test\", and there is more\n> information about that in t/README.\n\nWill actually look at that just so that I can have a better idea of\nwhat the _current_ behavior is expected from using the flag. Looking\nforward to more comments\n\n>\n> Thanks,\n> Taylor\n\nThank you!\n"},{"id":"408087","messageId":"xmqqv9f37476.fsf@gitster.c.googlers.com","threadId":"54473","inReplyTo":"20201021062430.2029566-1-eantoranz@gmail.com","subject":"Re: [RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-21T19:46:05Z","receivedAt":"2020-10-21T19:46:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:\n\n> Currently, if 'git cherry-pick --no-commit' is run _and the cherry-pick\n> operation is successful_, the metadata from the original revision is lost and\n> to git it's like a cherry-pick operation is not taking place at all. Hence,\n> we can't wrap up the cherry-pick operation by calling\n> 'git cherry-pick --continue'.\n\nThis cuts both ways, though.\n\nI often use the \"--no-commit\" form of the command as a better\nversion of the 'git show $that_commit | git apply --index' pipeline,\nand what I'll do starting from the working tree that I prepare that\nway is often *not* to commit it exactly or even use any data from\nthe original commit.\n\nSo a change like this would make the use of the command for my\nusecase more cumbersome---now it leaves cruft behind, so I need to\nclean it up later, but with what?  \"cherry-pick --abort\" would\ntry to muck with the index and the working tree, but that is not\ndefinitely what I want.\n\nSo, personally, I am fairly negative on this line of change.  If the\nuser says upfront \"--no-commit\", then user does not want a commit,\nso why should we even allow \"--continue\"?\n\nBefore dismissing the idea totally, let's see what potential use\ncases this change _could_ benefit, and see if there are already ways\nto satisfy these use cases without making this change.  For example,\nif the user wants to examine the result before actually \"committing\"\nto move the target branch forward with this change, keeping it an\noption to back out if the result of cherry-picking turns out to be\nbad, the \"--no-commit first, examine, and --continue or --abort\"\nsequence may help such a workflow.\n\nBut the user can already do so without this change:\n\n    $ git checkout target_branch^0 ;#detach\n    $ git cherry-pick source_branch\n    ... examine the result ...\n    ... and if it is satisfactory ...\n    $ git checkout -B target_branch\n    ... or if it is not, then discard ...\n    $ git checkout target_branch\n\n> With this patch, we allow sequencer to save the metadata from the original\n> cherry-pick operation so that 'git cherry-pick --continue' can be called.\n\n"},{"id":"408094","messageId":"xmqqa6wf71mv.fsf@gitster.c.googlers.com","threadId":"54473","inReplyTo":"xmqqv9f37476.fsf@gitster.c.googlers.com","subject":"Re: [RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-21T20:41:28Z","receivedAt":"2020-10-21T20:41:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Before dismissing the idea totally, let's see what potential use\n> cases this change _could_ benefit, and see if there are already ways\n> to satisfy these use cases without making this change.  For example,\n> if the user wants to examine the result before actually \"committing\"\n> to move the target branch forward with this change, keeping it an\n> option to back out if the result of cherry-picking turns out to be\n> bad, the \"--no-commit first, examine, and --continue or --abort\"\n> sequence may help such a workflow.\n>\n> But the user can already do so without this change:\n>\n>     $ git checkout target_branch^0 ;#detach\n>     $ git cherry-pick source_branch\n>     ... examine the result ...\n>     ... and if it is satisfactory ...\n>     $ git checkout -B target_branch\n>     ... or if it is not, then discard ...\n>     $ git checkout target_branch\n>\n>> With this patch, we allow sequencer to save the metadata from the original\n>> cherry-pick operation so that 'git cherry-pick --continue' can be called.\n\nAnd if the user wants to do the inspection while on the target\nbranch, \"commit -c/-C\" is the perfect choice.\n\n    $ git cherry-pick --no-commit source_branch\n    ... examine the result ...\n    ... and if it is satisfactory ...\n    $ git commit -c source_branch\n    ... or if it is not, then discard ...\n    $ git reset --hard\n\n"},{"id":"408097","messageId":"CAOc6etbSbg0rDrqu__MASTEdz2FhnhxtQLn6gvhBZL8BLz+AQQ@mail.gmail.com","threadId":"54473","inReplyTo":"xmqqv9f37476.fsf@gitster.c.googlers.com","subject":"Re: [RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-10-21T20:45:13Z","receivedAt":"2020-10-21T20:45:28Z","isPatch":false,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Wed, Oct 21, 2020 at 1:46 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> So, personally, I am fairly negative on this line of change.  If the\n> user says upfront \"--no-commit\", then user does not want a commit,\n> so why should we even allow \"--continue\"?\n\nThis is the way merge --no-commit works, actually. You run\n--no-commit, then you run merge --continue to wrap it up and that's\nwhy to me it had always felt a bit off in terms of consistency of the\nUI.\n\n>\n> Before dismissing the idea totally, let's see what potential use\n> cases this change _could_ benefit, and see if there are already ways\n> to satisfy these use cases without making this change.  For example,\n> if the user wants to examine the result before actually \"committing\"\n> to move the target branch forward with this change, keeping it an\n> option to back out if the result of cherry-picking turns out to be\n> bad, the \"--no-commit first, examine, and --continue or --abort\"\n> sequence may help such a workflow.\n\nCase in point, I am working on a script that corrects EOL format of\nfiles in a branch. When I started developing it, I was trusting that I\ncould run cherry-pick --no-commit so that I could then check the exit\ncode of git to see if there had been problems or not in the operation.\nIf there were, I would correct the EOL format of the files. Then,\nsuccessful cherry-pick or not, I would run cherry-pick --no-commit and\nit would work.... but that was when I realized that cherry-pick\n--no-commit is not keeping the information. Given that --no-commit\n_forgets_ the information from the revision, I have to run a normal\ncherry-pick. If it fails, I correct the EOL format and then I run\ncherry-pick --continue. It is noty like a game changer, you know... I\ncan live with it.... how many people have complained about it, anyway?\nI just find it inconsistent in terms of how cherry-pick/merge\n--no-commit behave.\n\nhttps://github.com/eantoranz/conflict_book/blob/0385d240dda962512b215781039ed42ec2f12ec2/scripts/correct_eol.sh#L197\n\nAnyway, thanks for all your feedback and let's see if there are more comments.\n\nBR\n\n\n>\n> But the user can already do so without this change:\n>\n>     $ git checkout target_branch^0 ;#detach\n>     $ git cherry-pick source_branch\n>     ... examine the result ...\n>     ... and if it is satisfactory ...\n>     $ git checkout -B target_branch\n>     ... or if it is not, then discard ...\n>     $ git checkout target_branch\n>\n> > With this patch, we allow sequencer to save the metadata from the original\n> > cherry-pick operation so that 'git cherry-pick --continue' can be called.\n>\n"},{"id":"408100","messageId":"CAOc6eta+ZiqDSh30X8SDmcPE2M3OuHBt+zx5vQY6pwWr1DYWFw@mail.gmail.com","threadId":"54473","inReplyTo":"CAOc6etbSbg0rDrqu__MASTEdz2FhnhxtQLn6gvhBZL8BLz+AQQ@mail.gmail.com","subject":"Re: [RFC] sequencer: allow metadata to be saved if using cherry-pick --no-commit","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-10-21T20:53:15Z","receivedAt":"2020-10-21T20:53:29Z","isPatch":false,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Wed, Oct 21, 2020 at 2:45 PM Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n> If there were, I would correct the EOL format of the files. Then,\n> successful cherry-pick or not, I would run cherry-pick --no-commit and\n> it would work.... but that was when I realized that cherry-pick\n\nI meant success cherry-pick or not, I would run cherry-pick --continue\n"}]}