{"thread":{"id":"48095","subject":"[PATCH 0/3] rebase --keep-empty/--root fixes","startedAt":"2018-03-20T10:03:32Z","lastAt":"2018-03-29T18:22:21Z","messageCount":10,"participants":["Phillip Wood","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"342304","messageId":"20180320100315.15261-1-phillip.wood@talktalk.net","threadId":"48095","inReplyTo":null,"subject":"[PATCH 0/3] rebase --keep-empty/--root fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-03-20T10:03:12Z","receivedAt":"2018-03-20T10:03:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThese patches are based on top of maint. The first two are a reworking\nof \"[PATCH 2/2] rebase --root -k: fix when root commit is empty\" [1]\n\n[1] https://public-inbox.org/git/20180314111127.14217-2-phillip.wood@talktalk.net/\n\nPhillip Wood (3):\n  rebase --root: stop assuming squash_onto is unset\n  rebase -i --keep-empty: don't prune empty commits\n  rebase: respect --no-keep-empty\n\n git-rebase.sh                     |  4 ++++\n sequencer.c                       | 18 +++++++++++-------\n t/t3421-rebase-topology-linear.sh |  2 +-\n 3 files changed, 16 insertions(+), 8 deletions(-)\n\n-- \n2.16.2\n\n"},{"id":"342305","messageId":"20180320100315.15261-4-phillip.wood@talktalk.net","threadId":"48095","inReplyTo":"20180320100315.15261-1-phillip.wood@talktalk.net","subject":"[PATCH 3/3] rebase: respect --no-keep-empty","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-03-20T10:03:15Z","receivedAt":"2018-03-20T10:03:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n$OPT_SPEC has always allowed --no-keep-empty so lets start handling\nit.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n git-rebase.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 8b1b892d72..37b8f13971 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -263,6 +263,9 @@ do\n \t--keep-empty)\n \t\tkeep_empty=yes\n \t\t;;\n+\t--no-keep-empty)\n+\t\tkeep_empty=\n+\t\t;;\n \t--preserve-merges)\n \t\tpreserve_merges=t\n \t\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n-- \n2.16.2\n\n"},{"id":"342306","messageId":"20180320100315.15261-3-phillip.wood@talktalk.net","threadId":"48095","inReplyTo":"20180320100315.15261-1-phillip.wood@talktalk.net","subject":"[PATCH 2/3] rebase -i --keep-empty: don't prune empty commits","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-03-20T10:03:14Z","receivedAt":"2018-03-20T10:03:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf there are empty commits on the left hand side of $upstream...HEAD\nthen the empty commits on the right hand side that we want to keep are\npruned by --cherry-pick. Fix this by using --cherry-mark instead of\n--cherry-pick and keeping the commits that are empty or are not marked\nas cherry-picks.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                       | 18 +++++++++++-------\n t/t3421-rebase-topology-linear.sh |  2 +-\n 2 files changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4d3f60594c..96ff812c8d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2470,7 +2470,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,\n \tinit_revisions(&revs, NULL);\n \trevs.verbose_header = 1;\n \trevs.max_parents = 1;\n-\trevs.cherry_pick = 1;\n+\trevs.cherry_mark = 1;\n \trevs.limited = 1;\n \trevs.reverse = 1;\n \trevs.right_only = 1;\n@@ -2495,14 +2495,18 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,\n \t\treturn error(_(\"make_script: error preparing revisions\"));\n \n \twhile ((commit = get_revision(&revs))) {\n+\t\tint is_empty  = is_original_commit_empty(commit);\n+\n \t\tstrbuf_reset(&buf);\n-\t\tif (!keep_empty && is_original_commit_empty(commit))\n+\t\tif (!keep_empty && is_empty)\n \t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n-\t\tstrbuf_addf(&buf, \"%s %s \", insn,\n-\t\t\t    oid_to_hex(&commit->object.oid));\n-\t\tpretty_print_commit(&pp, commit, &buf);\n-\t\tstrbuf_addch(&buf, '\\n');\n-\t\tfputs(buf.buf, out);\n+\t\tif (is_empty || !(commit->object.flags & PATCHSAME)) {\n+\t\t\tstrbuf_addf(&buf, \"%s %s \", insn,\n+\t\t\t\t    oid_to_hex(&commit->object.oid));\n+\t\t\tpretty_print_commit(&pp, commit, &buf);\n+\t\t\tstrbuf_addch(&buf, '\\n');\n+\t\t\tfputs(buf.buf, out);\n+\t\t}\n \t}\n \tstrbuf_release(&buf);\n \treturn 0;\ndiff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh\nindex 68fe2003ef..52fc6885e5 100755\n--- a/t/t3421-rebase-topology-linear.sh\n+++ b/t/t3421-rebase-topology-linear.sh\n@@ -215,7 +215,7 @@ test_run_rebase () {\n }\n test_run_rebase success ''\n test_run_rebase failure -m\n-test_run_rebase failure -i\n+test_run_rebase success -i\n test_run_rebase failure -p\n \n #       m\n-- \n2.16.2\n\n"},{"id":"342307","messageId":"20180320100315.15261-2-phillip.wood@talktalk.net","threadId":"48095","inReplyTo":"20180320100315.15261-1-phillip.wood@talktalk.net","subject":"[PATCH 1/3] rebase --root: stop assuming squash_onto is unset","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-03-20T10:03:13Z","receivedAt":"2018-03-20T10:03:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the user set the environment variable 'squash_onto', the 'rebase'\ncommand would erroneously assume that the user passed the option\n'--root'.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n git-rebase.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex fd72a35c65..8b1b892d72 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -60,6 +60,7 @@ $(gettext 'Resolve all conflicts manually, mark them as resolved with\n You can instead skip this commit: run \"git rebase --skip\".\n To abort and get back to the state before \"git rebase\", run \"git rebase --abort\".')\n \"\n+squash_onto=\n unset onto\n unset restrict_revision\n cmd=\n-- \n2.16.2\n\n"},{"id":"342322","messageId":"nycvar.QRO.7.76.6.1803201630380.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48095","inReplyTo":"20180320100315.15261-3-phillip.wood@talktalk.net","subject":"Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-20T15:33:46Z","receivedAt":"2018-03-20T15:34:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 20 Mar 2018, Phillip Wood wrote:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 4d3f60594c..96ff812c8d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2470,7 +2470,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,\n>  \tinit_revisions(&revs, NULL);\n>  \trevs.verbose_header = 1;\n>  \trevs.max_parents = 1;\n> -\trevs.cherry_pick = 1;\n> +\trevs.cherry_mark = 1;\n\nThis will conflict with my --recreate-merges patch series, but in a good\nway.\n\n> @@ -2495,14 +2495,18 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,\n>  \t\treturn error(_(\"make_script: error preparing revisions\"));\n>  \n>  \twhile ((commit = get_revision(&revs))) {\n> +\t\tint is_empty  = is_original_commit_empty(commit);\n> +\n>  \t\tstrbuf_reset(&buf);\n> -\t\tif (!keep_empty && is_original_commit_empty(commit))\n> +\t\tif (!keep_empty && is_empty)\n>  \t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n> -\t\tstrbuf_addf(&buf, \"%s %s \", insn,\n> -\t\t\t    oid_to_hex(&commit->object.oid));\n> -\t\tpretty_print_commit(&pp, commit, &buf);\n> -\t\tstrbuf_addch(&buf, '\\n');\n> -\t\tfputs(buf.buf, out);\n> +\t\tif (is_empty || !(commit->object.flags & PATCHSAME)) {\n\nMay I suggest inverting the logic here, to make the code more obvious and\nalso to avoid indenting the block even further?\n\n\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n\t\t\tcontinue;\n\n> +\t\t\tstrbuf_addf(&buf, \"%s %s \", insn,\n> +\t\t\t\t    oid_to_hex(&commit->object.oid));\n> +\t\t\tpretty_print_commit(&pp, commit, &buf);\n> +\t\t\tstrbuf_addch(&buf, '\\n');\n> +\t\t\tfputs(buf.buf, out);\n> +\t\t}\n>  \t}\n>  \tstrbuf_release(&buf);\n>  \treturn 0;\n\nThanks,\nDscho\n"},{"id":"342323","messageId":"nycvar.QRO.7.76.6.1803201635580.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48095","inReplyTo":"nycvar.QRO.7.76.6.1803201634260.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH 0/3] rebase --keep-empty/--root fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-20T15:36:22Z","receivedAt":"2018-03-20T15:36:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"... and now also Cc:ing the list and Junio...\n\n\nOn Tue, 20 Mar 2018, Johannes Schindelin wrote:\n\n> Hi Phillip,\n> \n> On Tue, 20 Mar 2018, Phillip Wood wrote:\n> \n> > Phillip Wood (3):\n> >   rebase --root: stop assuming squash_onto is unset\n> >   rebase -i --keep-empty: don't prune empty commits\n> >   rebase: respect --no-keep-empty\n> \n> Those patches look good. I offered a suggestion to 2/3 to avoid indenting\n> a code block, but I would also be okay to leave it as-is.\n> \n> Thanks,\n> Dscho\n> \n"},{"id":"342343","messageId":"xmqqfu4uslar.fsf@gitster-ct.c.googlers.com","threadId":"48095","inReplyTo":"nycvar.QRO.7.76.6.1803201630380.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-20T17:34:36Z","receivedAt":"2018-03-20T17:34:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> +\t\tif (!keep_empty && is_empty)\n>>  \t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n\nWe are not trying to preserve an empty one, and have found an empty\none, so we comment it out, and then...\n\n>> +\t\tif (is_empty || !(commit->object.flags & PATCHSAME)) {\n>\n> May I suggest inverting the logic here, to make the code more obvious and\n> also to avoid indenting the block even further?\n>\n> \t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> \t\t\tcontinue;\n\n... if a non-empty one that already appears in the upstream, we do\nnot do anything to it.  There is no room for keep-empty or lack of\nit to affect what happens to these commits.\n\nOtherwise the insn is emitted for the commit.\n\n>> +\t\t\tstrbuf_addf(&buf, \"%s %s \", insn,\n>> +\t\t\t\t    oid_to_hex(&commit->object.oid));\n>> +\t\t\tpretty_print_commit(&pp, commit, &buf);\n>> +\t\t\tstrbuf_addch(&buf, '\\n');\n>> +\t\t\tfputs(buf.buf, out);\n>> +\t\t}\n\nI tend to agree that the suggested structure is easier to follow\nthan Phillip's version.\n\nBut I wonder if this is even easier to follow.  It makes it even\nmore clear that patchsame commits that are not empty are discarded\nunconditionally.\n\n\twhile ((commit = get_revision(&revs))) {\n\t\tint is_empty  = is_original_commit_empty(commit);\n\t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n\t\t\tcontinue;\n\t\tstrbuf_reset(&buf);\n\t\tif (!keep_empty && is_empty)\n\t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n\t\tstrbuf_addf(&buf, \"%s %s \", insn,\n\t\t\t    oid_to_hex(&commit->object.oid));\n\t\tpretty_print_commit(&pp, commit, &buf);\n\t\tstrbuf_addch(&buf, '\\n');\n\t\tfputs(buf.buf, out);\n\t}\n\nOr did I screw up the rewrite?\n"},{"id":"342352","messageId":"68c15e00-139d-b45c-bc76-cf1c99d2f1e4@talktalk.net","threadId":"48095","inReplyTo":"xmqqfu4uslar.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-03-20T18:39:10Z","receivedAt":"2018-03-20T18:39:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/03/18 17:34, Junio C Hamano wrote:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n>>> +\t\tif (!keep_empty && is_empty)\n>>>  \t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n> \n> We are not trying to preserve an empty one, and have found an empty\n> one, so we comment it out, and then...\n> \n>>> +\t\tif (is_empty || !(commit->object.flags & PATCHSAME)) {\n>>\n>> May I suggest inverting the logic here, to make the code more obvious and\n>> also to avoid indenting the block even further?\n>>\n>> \t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n>> \t\t\tcontinue;\n> \n> ... if a non-empty one that already appears in the upstream, we do\n> not do anything to it.  There is no room for keep-empty or lack of\n> it to affect what happens to these commits.\n> \n> Otherwise the insn is emitted for the commit.\n> \n>>> +\t\t\tstrbuf_addf(&buf, \"%s %s \", insn,\n>>> +\t\t\t\t    oid_to_hex(&commit->object.oid));\n>>> +\t\t\tpretty_print_commit(&pp, commit, &buf);\n>>> +\t\t\tstrbuf_addch(&buf, '\\n');\n>>> +\t\t\tfputs(buf.buf, out);\n>>> +\t\t}\n> \n> I tend to agree that the suggested structure is easier to follow\n> than Phillip's version.\n> \n> But I wonder if this is even easier to follow.  It makes it even\n> more clear that patchsame commits that are not empty are discarded\n> unconditionally.\n> \n> \twhile ((commit = get_revision(&revs))) {\n> \t\tint is_empty  = is_original_commit_empty(commit);\n> \t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> \t\t\tcontinue;\n> \t\tstrbuf_reset(&buf);\n> \t\tif (!keep_empty && is_empty)\n> \t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n> \t\tstrbuf_addf(&buf, \"%s %s \", insn,\n> \t\t\t    oid_to_hex(&commit->object.oid));\n> \t\tpretty_print_commit(&pp, commit, &buf);\n> \t\tstrbuf_addch(&buf, '\\n');\n> \t\tfputs(buf.buf, out);\n> \t}\n> \n> Or did I screw up the rewrite?\n> \nI've not tested it but I think it's OK, I agree that it is clearer than\nmy version\n\nBest Wishes\n\nPhillip\n"},{"id":"342480","messageId":"nycvar.QRO.7.76.6.1803212336420.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48095","inReplyTo":"xmqqfu4uslar.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-21T22:38:21Z","receivedAt":"2018-03-21T22:38:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 20 Mar 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> +\t\tif (!keep_empty && is_empty)\n> >>  \t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n> \n> We are not trying to preserve an empty one, and have found an empty\n> one, so we comment it out, and then...\n> \n> >> +\t\tif (is_empty || !(commit->object.flags & PATCHSAME)) {\n> >\n> > May I suggest inverting the logic here, to make the code more obvious and\n> > also to avoid indenting the block even further?\n> >\n> > \t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> > \t\t\tcontinue;\n> \n> ... if a non-empty one that already appears in the upstream, we do\n> not do anything to it.  There is no room for keep-empty or lack of\n> it to affect what happens to these commits.\n> \n> Otherwise the insn is emitted for the commit.\n> \n> >> +\t\t\tstrbuf_addf(&buf, \"%s %s \", insn,\n> >> +\t\t\t\t    oid_to_hex(&commit->object.oid));\n> >> +\t\t\tpretty_print_commit(&pp, commit, &buf);\n> >> +\t\t\tstrbuf_addch(&buf, '\\n');\n> >> +\t\t\tfputs(buf.buf, out);\n> >> +\t\t}\n> \n> I tend to agree that the suggested structure is easier to follow\n> than Phillip's version.\n> \n> But I wonder if this is even easier to follow.  It makes it even\n> more clear that patchsame commits that are not empty are discarded\n> unconditionally.\n> \n> \twhile ((commit = get_revision(&revs))) {\n> \t\tint is_empty  = is_original_commit_empty(commit);\n> \t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n> \t\t\tcontinue;\n> \t\tstrbuf_reset(&buf);\n> \t\tif (!keep_empty && is_empty)\n> \t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n> \t\tstrbuf_addf(&buf, \"%s %s \", insn,\n> \t\t\t    oid_to_hex(&commit->object.oid));\n> \t\tpretty_print_commit(&pp, commit, &buf);\n> \t\tstrbuf_addch(&buf, '\\n');\n> \t\tfputs(buf.buf, out);\n> \t}\n> \n> Or did I screw up the rewrite?\n\nThis looks correct. And the postimage is easier to follow than the one of\nmy suggested change.\n\nMy version is easier to review on the mailing list, of course, as it\nminimizes the diff... ;-)\n\nCiao,\nDscho\n"},{"id":"343356","messageId":"xmqq605e7o5a.fsf@gitster-ct.c.googlers.com","threadId":"48095","inReplyTo":"nycvar.QRO.7.76.6.1803212336420.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-29T18:05:53Z","receivedAt":"2018-03-29T18:22:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> But I wonder if this is even easier to follow.  It makes it even\n>> more clear that patchsame commits that are not empty are discarded\n>> unconditionally.\n>> \n>> \twhile ((commit = get_revision(&revs))) {\n>> \t\tint is_empty  = is_original_commit_empty(commit);\n>> \t\tif (!is_empty && (commit->object.flags & PATCHSAME))\n>> \t\t\tcontinue;\n>> \t\tstrbuf_reset(&buf);\n>> \t\tif (!keep_empty && is_empty)\n>> \t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n>> \t\tstrbuf_addf(&buf, \"%s %s \", insn,\n>> \t\t\t    oid_to_hex(&commit->object.oid));\n>> \t\tpretty_print_commit(&pp, commit, &buf);\n>> \t\tstrbuf_addch(&buf, '\\n');\n>> \t\tfputs(buf.buf, out);\n>> \t}\n>> \n>> Or did I screw up the rewrite?\n>\n> This looks correct. And the postimage is easier to follow than the one of\n> my suggested change.\n\nOK, let's squash this in and rebuild both pw/rebase-keep-empty-fixes\nand also pw/rebase-signoff that builds on this topic, so that they\ncan be advanced to 'next'.\n\n"}]}