{"thread":{"id":"48123","subject":"[RFC PATCH v4] rebase-interactive","startedAt":"2018-03-23T04:40:06Z","lastAt":"2018-03-27T10:03:33Z","messageCount":42,"participants":["Wink Saville","Eric Sunshine","Johannes Schindelin","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"342557","messageId":"cover.1521779249.git.wink@saville.com","threadId":"48123","inReplyTo":null,"subject":"[RFC PATCH v4] rebase-interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T04:39:51Z","receivedAt":"2018-03-23T04:40:06Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"This is v4 of the first 2 patches of \"[RFC PATCH vV n/9] rebase-interactive\",\nlooking forward to any additional comments.\n\n\nWink Saville (2):\n rebase-interactive: Simplify pick_on_preserving_merges\n rebase: Update invocation of rebase dot-sourced scripts\n\n git-rebase--am.sh | 11 -----------\n git-rebase--interactive.sh | 28 +++++++---------------------\n git-rebase--merge.sh | 11 -----------\n git-rebase.sh | 2 ++\n 4 files changed, 9 insertions(+), 43 deletions(-)\n\n-- \n2.16.2\n\n"},{"id":"342558","messageId":"c49af83f3ca3d180cbd101d62eccc3b021373d9b.1521779249.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521779249.git.wink@saville.com","subject":"[RFC PATCH v4] rebase-interactive: Simplify pick_on_preserving_merges","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T04:39:52Z","receivedAt":"2018-03-23T04:40:08Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"Use compound if statement instead of nested if statements to\nsimplify pick_on_preserving_merges.\n\nSigned-off-by: Wink Saville \nReviewed-by: Junio C Hamano \n---\n git-rebase--interactive.sh | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 331c8dfea..561e2660e 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -307,17 +307,14 @@ pick_one_preserving_merges () {\n \tesac\n \tsha1=$(git rev-parse $sha1)\n \n-\tif test -f \"$state_dir\"/current-commit\n+\tif test -f \"$state_dir\"/current-commit && test \"$fast_forward\" = t\n \tthen\n-\t\tif test \"$fast_forward\" = t\n-\t\tthen\n-\t\t\twhile read current_commit\n-\t\t\tdo\n-\t\t\t\tgit rev-parse HEAD > \"$rewritten\"/$current_commit\n-\t\t\tdone <\"$state_dir\"/current-commit\n-\t\t\trm \"$state_dir\"/current-commit ||\n-\t\t\t\tdie \"$(gettext \"Cannot write current commit's replacement sha1\")\"\n-\t\tfi\n+\t\twhile read current_commit\n+\t\tdo\n+\t\t\tgit rev-parse HEAD > \"$rewritten\"/$current_commit\n+\t\tdone <\"$state_dir\"/current-commit\n+\t\trm \"$state_dir\"/current-commit ||\n+\t\t\tdie \"$(gettext \"Cannot write current commit's replacement sha1\")\"\n \tfi\n \n \techo $sha1 >> \"$state_dir\"/current-commit\n-- \n2.16.2\n\n"},{"id":"342559","messageId":"ed4cfdc9f31b920eae5055c3b080e2ca5b2f6e42.1521779249.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521779249.git.wink@saville.com","subject":"[RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T04:39:53Z","receivedAt":"2018-03-23T04:40:10Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"The backend scriptlets for \"git rebase\" were structured in a\nbit unusual way for historical reasons. Originally, it was\ndesigned in such a way that dot-sourcing them from \"git\nrebase\" would be sufficient to invoke the specific backend.\n\nWhen it was discovered that some shell implementations\n(e.g. FreeBSD 9.x) misbehaved when exiting with a \"return\"\nis executed at the top level of a dot-sourced script (the\noriginal was expecting that the control returns to the next\ncommand in \"git rebase\" after dot-sourcing the scriptlet).\n\nTo fix this issue the whole body of git-rebase--$backend.sh\nwas made into a shell function git_rebase__$backend and then\nthe last statement of the scriptlet would invoke the function.\n\nHere the call is moved to \"git rebase\" side, instead of at the\nend of each scriptlet. This give us a more normal arrangement\nwhere the scriptlet function library and allows multiple functions\nto be implemented in a scriptlet.\n\nSigned-off-by: Wink Saville \nReviewed-by: Junio C Hamano \nReviewed-by: Eric Sunsine \n---\n git-rebase--am.sh | 11 -----------\n git-rebase--interactive.sh | 11 -----------\n git-rebase--merge.sh | 11 -----------\n git-rebase.sh | 2 ++\n 4 files changed, 2 insertions(+), 33 deletions(-)\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex be3f06892..e5fd6101d 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -4,15 +4,6 @@\n # Copyright (c) 2010 Junio C Hamano.\n #\n \n-# The whole contents of this file is run by dot-sourcing it from\n-# inside a shell function. It used to be that \"return\"s we see\n-# below were not inside any function, and expected to return\n-# to the function that dot-sourced us.\n-#\n-# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a\n-# construct and continue to run the statements that follow such a \"return\".\n-# As a work-around, we introduce an extra layer of a function\n-# here, and immediately call it after defining it.\n git_rebase__am () {\n \n case \"$action\" in\n@@ -105,5 +96,3 @@ fi\n move_to_original_branch\n \n }\n-# ... and then we call the whole thing.\n-git_rebase__am\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 561e2660e..213d75f43 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -740,15 +740,6 @@ get_missing_commit_check_level () {\n \tprintf '%s' \"$check_level\" | tr 'A-Z' 'a-z'\n }\n \n-# The whole contents of this file is run by dot-sourcing it from\n-# inside a shell function. It used to be that \"return\"s we see\n-# below were not inside any function, and expected to return\n-# to the function that dot-sourced us.\n-#\n-# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a\n-# construct and continue to run the statements that follow such a \"return\".\n-# As a work-around, we introduce an extra layer of a function\n-# here, and immediately call it after defining it.\n git_rebase__interactive () {\n \n case \"$action\" in\n@@ -1029,5 +1020,3 @@ fi\n do_rest\n \n }\n-# ... and then we call the whole thing.\n-git_rebase__interactive\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex ceb715453..685f48ca4 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -104,15 +104,6 @@ finish_rb_merge () {\n \tsay All done.\n }\n \n-# The whole contents of this file is run by dot-sourcing it from\n-# inside a shell function. It used to be that \"return\"s we see\n-# below were not inside any function, and expected to return\n-# to the function that dot-sourced us.\n-#\n-# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a\n-# construct and continue to run the statements that follow such a \"return\".\n-# As a work-around, we introduce an extra layer of a function\n-# here, and immediately call it after defining it.\n git_rebase__merge () {\n \n case \"$action\" in\n@@ -171,5 +162,3 @@ done\n finish_rb_merge\n \n }\n-# ... and then we call the whole thing.\n-git_rebase__merge\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex a1f6e5de6..4595a316a 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -196,7 +196,9 @@ run_specific_rebase () {\n \t\texport GIT_EDITOR\n \t\tautosquash=\n \tfi\n+\t# Source the code and invoke it\n \t. git-rebase--$type\n+\tgit_rebase__$type\n \tret=$?\n \tif test $ret -eq 0\n \tthen\n-- \n2.16.2\n\n"},{"id":"342562","messageId":"CAPig+cQG16AhLPMeOFAw1GF81oXivFSDHvQ5B8kX20YGAT_BxQ@mail.gmail.com","threadId":"48123","inReplyTo":"ed4cfdc9f31b920eae5055c3b080e2ca5b2f6e42.1521779249.git.wink@saville.com","subject":"Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-23T06:26:55Z","receivedAt":"2018-03-23T06:27:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for splitting these changes into smaller, more manageable\nchunks. A couple non-code comments below apply to both patches in this\n2-patch series even though I'm responding only to this patch. (The\nactual code changes in the other patch looked fine and the patch was\neasily digested.)\n\nOn Fri, Mar 23, 2018 at 12:39 AM, Wink Saville wrote:\n> rebase: Update invocation of rebase dot-sourced scripts\n\nNit: On this project, the summary line is not capitalized, so: s/Update/update/\n\n> The backend scriptlets for \"git rebase\" were structured in a\n\nOn this project, commit messages are written in imperative mood. The\ncommit message Junio suggested[1] said \"are structured\", which makes\nfor a better imperative mood fit.\n\n> bit unusual way for historical reasons. Originally, it was\n> designed in such a way that dot-sourcing them from \"git\n> rebase\" would be sufficient to invoke the specific backend.\n>\n> When it was discovered that some shell implementations\n> (e.g. FreeBSD 9.x) misbehaved when exiting with a \"return\"\n> is executed at the top level of a dot-sourced script (the\n> original was expecting that the control returns to the next\n> command in \"git rebase\" after dot-sourcing the scriptlet).\n\nECANTPARSE: This paragraph is grammatically corrupt.\n\n? \"When {something}...\" but then what?\n\n? \"...when exiting with a \"return\" is executed\"\n\n> To fix this issue the whole body of git-rebase--$backend.sh\n> was made into a shell function git_rebase__$backend and then\n> the last statement of the scriptlet would invoke the function.\n\nJunio's proposed commit message[1] called this a \"workaround\", not a\n\"fix\", and, indeed, \"workaround\" better characterizes that change. If\nanything, _this_ patch is a (more correct) \"fix\" for that workaround.\n\n> Here the call is moved to \"git rebase\" side, instead of at the\n\nJunio's version, using imperative mood, said \"Move the call...\".\n\n> end of each scriptlet. This give us a more normal arrangement\n> where the scriptlet function library and allows multiple functions\n> to be implemented in a scriptlet.\n\nECANTPARSE: Grammatically corrupt.\n\n? \"where the ... library and allows...\"\n\nOverall, Junio's proposed message followed project practice\n(imperative mood) more closely and felt somewhat more coherent\n(despite the run-on sentence in the first paragraph and the apparent\nincorrect explanation of top-level \"return\" misbehavior -- the in-code\ncomment says top-level \"return\" was essentially a no-op in broken\nshells, whereas he said it exited the shell).\n\nPerhaps the following re-write addresses the above concerns:\n\n Due to historical reasons, the backend scriptlets for \"git rebase\"\n are structured a bit unusually. As originally designed,\n dot-sourcing them from \"git rebase\" was sufficient to invoke the\n specific backend.\n\n However, it was later discovered that some shell implementations\n (e.g. FreeBSD 9.x) misbehaved by continuing to execute statements\n following a top-level \"return\" rather than returning control to\n the next statement in \"git rebase\" after dot-sourcing the\n scriptlet. To work around this shortcoming, the whole body of\n git-rebase--$backend.sh was made into a shell function\n git_rebase__$backend, and then the very last line of the scriptlet\n called that function.\n\n A more normal architecture is for a dot-sourced scriptlet merely\n to define functions (thus acting as a function library), and for\n those functions to be called by the script doing the dot-sourcing.\n Migrate to this arrangement by moving the git_rebase__$backend\n call from the end of a scriptlet into \"git rebase\" itself.\n\n While at it, remove the large comment block from each scriptlet\n explaining this historic anomaly since it serves no purpose under\n the new normalized architecture in which a scriptlet is merely a\n function library.\n\n> Signed-off-by: Wink Saville \n> Reviewed-by: Junio C Hamano \n> Reviewed-by: Eric Sunsine \n\nDespite its name, on this project, a Reviewed-by: does not mean merely\nthat a person looked at and commented on a patch. Rather, it is a way\nfor a person to say \"I have studied and understood the patch and feel\nthat it is ready for inclusion in the project.\" Reviewed-by:'s are\ntherefore always given explicitly by the reviewer and Junio adds them\nto a patch when queuing. (Reviewed-by:'s are not always given, though,\neven when a reviewer has not found problems with a patch. For\ninstance, even if I review and comment on this or subsequent patches,\nI will not give a Reviewed-by: since I'm not an area expert, thus\nwouldn't feel comfortable stating that the patch is correct.)\n\nConsequently, these Reviewed-by: lines should be dropped. (You can, on\nthe other hand, add Helped-by:'s when appropriate.)\n\nThe patch itself makes sense and seems straightforward. See one minor\ncomment below...\n\n[1]: https://public-inbox.org/git/xmqqefkbltxv.fsf@gitster-ct.c.googlers.com/\n\n> ---\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index a1f6e5de6..4595a316a 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -196,7 +196,9 @@ run_specific_rebase () {\n> export GIT_EDITOR\n> autosquash=\n> fi\n> + # Source the code and invoke it\n> . git-rebase--$type\n> + git_rebase__$type\n\nThe new comment merely repeats what the two lines of code themselves\nalready state clearly, thus the comment is an unnecessary distraction;\nit does not aid in understanding the code.\n"},{"id":"342602","messageId":"nycvar.QRO.7.76.6.1803231810220.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48123","inReplyTo":"c49af83f3ca3d180cbd101d62eccc3b021373d9b.1521779249.git.wink@saville.com","subject":"Re: [RFC PATCH v4] rebase-interactive: Simplify pick_on_preserving_merges","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-23T17:10:42Z","receivedAt":"2018-03-23T17:10:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Wink,\n\nOn Thu, 22 Mar 2018, Wink Saville wrote:\n\n> Use compound if statement instead of nested if statements to\n> simplify pick_on_preserving_merges.\n> \n> Signed-off-by: Wink Saville \n> Reviewed-by: Junio C Hamano \n> ---\n> git-rebase--interactive.sh | 17 +++++++----------\n\nThe patch is obviously correct.\n\nThanks,\nJohannes\n"},{"id":"342604","messageId":"nycvar.QRO.7.76.6.1803231811530.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48123","inReplyTo":"ed4cfdc9f31b920eae5055c3b080e2ca5b2f6e42.1521779249.git.wink@saville.com","subject":"Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-23T17:12:10Z","receivedAt":"2018-03-23T17:12:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Wink,\n\nOn Thu, 22 Mar 2018, Wink Saville wrote:\n\n> The backend scriptlets for \"git rebase\" were structured in a\n> bit unusual way for historical reasons. Originally, it was\n> designed in such a way that dot-sourcing them from \"git\n> rebase\" would be sufficient to invoke the specific backend.\n> \n> When it was discovered that some shell implementations\n> (e.g. FreeBSD 9.x) misbehaved when exiting with a \"return\"\n> is executed at the top level of a dot-sourced script (the\n> original was expecting that the control returns to the next\n> command in \"git rebase\" after dot-sourcing the scriptlet).\n> \n> To fix this issue the whole body of git-rebase--$backend.sh\n> was made into a shell function git_rebase__$backend and then\n> the last statement of the scriptlet would invoke the function.\n> \n> Here the call is moved to \"git rebase\" side, instead of at the\n> end of each scriptlet. This give us a more normal arrangement\n> where the scriptlet function library and allows multiple functions\n> to be implemented in a scriptlet.\n> \n> Signed-off-by: Wink Saville \n> Reviewed-by: Junio C Hamano \n> Reviewed-by: Eric Sunsine \n> ---\n> git-rebase--am.sh | 11 -----------\n> git-rebase--interactive.sh | 11 -----------\n> git-rebase--merge.sh | 11 -----------\n> git-rebase.sh | 2 ++\n\nThe patch makes sense to me.\n\nThanks,\nJohannes\n"},{"id":"342667","messageId":"CAKk8isrxTmryumw5EFVcPxx9wUKA=pB3VxvH9VaHPLRraa=4=g@mail.gmail.com","threadId":"48123","inReplyTo":"nycvar.QRO.7.76.6.1803231811530.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T19:06:16Z","receivedAt":"2018-03-23T19:06:44Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"On Fri, Mar 23, 2018 at 10:12 AM, Johannes Schindelin\n wrote:\n> Hi Wink,\n>\n> On Thu, 22 Mar 2018, Wink Saville wrote:\n>\n>> The backend scriptlets for \"git rebase\" were structured in a\n>> bit unusual way for historical reasons. Originally, it was\n>> designed in such a way that dot-sourcing them from \"git\n>> rebase\" would be sufficient to invoke the specific backend.\n>>\n>> When it was discovered that some shell implementations\n>> (e.g. FreeBSD 9.x) misbehaved when exiting with a \"return\"\n>> is executed at the top level of a dot-sourced script (the\n>> original was expecting that the control returns to the next\n>> command in \"git rebase\" after dot-sourcing the scriptlet).\n>>\n>> To fix this issue the whole body of git-rebase--$backend.sh\n>> was made into a shell function git_rebase__$backend and then\n>> the last statement of the scriptlet would invoke the function.\n>>\n>> Here the call is moved to \"git rebase\" side, instead of at the\n>> end of each scriptlet. This give us a more normal arrangement\n>> where the scriptlet function library and allows multiple functions\n>> to be implemented in a scriptlet.\n>>\n>> Signed-off-by: Wink Saville \n>> Reviewed-by: Junio C Hamano \n>> Reviewed-by: Eric Sunsine \n>> ---\n>> git-rebase--am.sh | 11 -----------\n>> git-rebase--interactive.sh | 11 -----------\n>> git-rebase--merge.sh | 11 -----------\n>> git-rebase.sh | 2 ++\n>\n> The patch makes sense to me.\n>\n> Thanks,\n> Johannes\n\nJunio, Eric and Johannes, thanks for the help!!!\n\nI've created v5 with the two patches, what is the suggested\nformat-patch/send-email command(s)?\n\nHere is one possibility:\n\ngit format-patch --cover-letter --rfc --thread -v 5\n--to=git@vger.kernel.org --cc=sunshine@sunshineco.com\n--cc=Johannes.Schindelin@gmx.de -o patches/v5 master..v5-2\n\nIf this was the first version then the above would seem to be a\nreasonable choice.\nBut this is version 5 and maybe I don't need --cover-letter which, I\nthink means I\ndon't want to use --thread. If that's the case should I add --in-reply-to? But\nthat leads to the question. from which message should I get the Message-Id?\n\nMore likely I'm totally wrong and should do something completely different,\nadvice appreciated.\n\n-- Wink\n"},{"id":"342680","messageId":"xmqqfu4qikhp.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"CAKk8isrxTmryumw5EFVcPxx9wUKA=pB3VxvH9VaHPLRraa=4=g@mail.gmail.com","subject":"Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T20:51:14Z","receivedAt":"2018-03-23T20:51:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wink Saville writes:\n\n> Here is one possibility:\n>\n> git format-patch --cover-letter --rfc --thread -v 5\n> --to=git@vger.kernel.org --cc=sunshine@sunshineco.com\n> --cc=Johannes.Schindelin@gmx.de -o patches/v5 master..v5-2\n\nSounds sensible.\n\n> If this was the first version then the above would seem to be a\n> reasonable choice.\n\nMy personal preference (both as a reviewer and an occasional\nmulti-patch series submitter) is to use a cover letter for a larger\nseries (e.g. more than 3-5 patches), regardless of the iteration.\nIn fact, a submitter tends to have _more_ things to say in the cover\nletter for v2 and subsequent iteration than the original iteration.\n\nThe motivation behind the series may not change so greatly but will\nbe refined as iterations go on, and you want help those who missed\nthe earlier iteration understand what you are doing with the updated\ncover letter. Also cover letter is the ideal place to outline where\nto find older iterations and their discussion and summarize what\nchanged since these earlier attempts in this round.\n\n> But this is version 5 and maybe I don't need --cover-letter which, I\n> think means I\n> don't want to use --thread. If that's the case should I add --in-reply-to? But\n> that leads to the question. from which message should I get the Message-Id?\n\nThe most typical practice I've seen around here is that v5's cover\nis made in-reply-to v4's cover.\n\n"},{"id":"342681","messageId":"xmqqbmfeik0i.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"CAPig+cQG16AhLPMeOFAw1GF81oXivFSDHvQ5B8kX20YGAT_BxQ@mail.gmail.com","subject":"Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T21:01:33Z","receivedAt":"2018-03-23T21:01:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> When it was discovered that some shell implementations\n> ...\n> ECANTPARSE: This paragraph is grammatically corrupt.\n>\n> ECANTPARSE: Grammatically corrupt.\n> ...\n> (despite the run-on sentence in the first paragraph and the apparent\n> incorrect explanation of top-level \"return\" misbehavior -- the in-code\n> comment says top-level \"return\" was essentially a no-op in broken\n> shells, whereas he said it exited the shell).\n\nMy bad, almost entirely. Sorry.\n"},{"id":"342682","messageId":"CAKk8isqwm4Fibm1cFfSUm+g7JckOrfDwjvsTBgmktei5w+N0xQ@mail.gmail.com","threadId":"48123","inReplyTo":"xmqqfu4qikhp.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:05:40Z","receivedAt":"2018-03-23T21:06:08Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"On Fri, Mar 23, 2018 at 1:51 PM, Junio C Hamano wrote:\n> Wink Saville writes:\n>\n>> Here is one possibility:\n>>\n>> git format-patch --cover-letter --rfc --thread -v 5\n>> --to=git@vger.kernel.org --cc=sunshine@sunshineco.com\n>> --cc=Johannes.Schindelin@gmx.de -o patches/v5 master..v5-2\n>\n> Sounds sensible.\n>\n>> If this was the first version then the above would seem to be a\n>> reasonable choice.\n>\n> My personal preference (both as a reviewer and an occasional\n> multi-patch series submitter) is to use a cover letter for a larger\n> series (e.g. more than 3-5 patches), regardless of the iteration.\n> In fact, a submitter tends to have _more_ things to say in the cover\n> letter for v2 and subsequent iteration than the original iteration.\n>\n> The motivation behind the series may not change so greatly but will\n> be refined as iterations go on, and you want help those who missed\n> the earlier iteration understand what you are doing with the updated\n> cover letter. Also cover letter is the ideal place to outline where\n> to find older iterations and their discussion and summarize what\n> changed since these earlier attempts in this round.\n>\n>> But this is version 5 and maybe I don't need --cover-letter which, I\n>> think means I\n>> don't want to use --thread. If that's the case should I add --in-reply-to? But\n>> that leads to the question. from which message should I get the Message-Id?\n>\n> The most typical practice I've seen around here is that v5's cover\n> is made in-reply-to v4's cover.\n>\n\nMake sense\n"},{"id":"342684","messageId":"CAPig+cT=0-+zgyGP7NEL3FFrc9bTDe9JLugoBqiFo5BtJq=2PQ@mail.gmail.com","threadId":"48123","inReplyTo":"xmqqbmfeik0i.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-23T21:18:19Z","receivedAt":"2018-03-23T21:18:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 23, 2018 at 5:01 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> (despite the run-on sentence in the first paragraph and the apparent\n>> incorrect explanation of top-level \"return\" misbehavior -- the in-code\n>> comment says top-level \"return\" was essentially a no-op in broken\n>> shells, whereas he said it exited the shell).\n>\n> My bad, almost entirely. Sorry.\n\nNo apology necessary. That minor error aside, your proposed commit\nmessage gave just the right amount of detail for a person (me) not at\nall familiar with the topic to be able to understand it fully and\nintuit the patch content before even reading the patch proper. That's\na good commit message.\n"},{"id":"342685","messageId":"cover.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521779249.git.wink@saville.com","subject":"[RFC PATCH v5 0/8] rebase-interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:21Z","receivedAt":"2018-03-23T21:25:49Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"Reworked patch 1 so that all of the backend scriptlets\nused by git-rebase use a normal function style invocation.\n\nMerged the previous patch 2 and 3 have been squashed which\nprovides reviewers a little easier time to detect any changes\nduring extraction of the functions.\n\nWink Saville (8):\n rebase-interactive: simplify pick_on_preserving_merges\n rebase: update invocation of rebase dot-sourced scripts\n Indent function git_rebase__interactive\n Extract functions out of git_rebase__interactive\n Add and use git_rebase__interactive__preserve_merges\n Remove unused code paths from git_rebase__interactive\n Remove unused code paths from git_rebase__interactive__preserve_merges\n Remove merges_option and a blank line\n\n git-rebase--am.sh | 11 --\n git-rebase--interactive.sh | 407 ++++++++++++++++++++++++---------------------\n git-rebase--merge.sh | 11 --\n git-rebase.sh | 1 +\n 4 files changed, 216 insertions(+), 214 deletions(-)\n\n-- \n2.16.2\n\n"},{"id":"342686","messageId":"221ad09edfa7df5e28718b677b5084bb4167d567.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"[RFC PATCH v5 1/8] rebase-interactive: simplify pick_on_preserving_merges","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:22Z","receivedAt":"2018-03-23T21:25:52Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"Use compound if statement instead of nested if statements to\nsimplify pick_on_preserving_merges.\n\nSigned-off-by: Wink Saville \n---\n git-rebase--interactive.sh | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 331c8dfea..561e2660e 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -307,17 +307,14 @@ pick_one_preserving_merges () {\n \tesac\n \tsha1=$(git rev-parse $sha1)\n \n-\tif test -f \"$state_dir\"/current-commit\n+\tif test -f \"$state_dir\"/current-commit && test \"$fast_forward\" = t\n \tthen\n-\t\tif test \"$fast_forward\" = t\n-\t\tthen\n-\t\t\twhile read current_commit\n-\t\t\tdo\n-\t\t\t\tgit rev-parse HEAD > \"$rewritten\"/$current_commit\n-\t\t\tdone <\"$state_dir\"/current-commit\n-\t\t\trm \"$state_dir\"/current-commit ||\n-\t\t\t\tdie \"$(gettext \"Cannot write current commit's replacement sha1\")\"\n-\t\tfi\n+\t\twhile read current_commit\n+\t\tdo\n+\t\t\tgit rev-parse HEAD > \"$rewritten\"/$current_commit\n+\t\tdone <\"$state_dir\"/current-commit\n+\t\trm \"$state_dir\"/current-commit ||\n+\t\t\tdie \"$(gettext \"Cannot write current commit's replacement sha1\")\"\n \tfi\n \n \techo $sha1 >> \"$state_dir\"/current-commit\n-- \n2.16.2\n\n"},{"id":"342687","messageId":"693aa1c256cd7d4a22a5ac7ca5fbea386210ce49.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"[RFC PATCH v5 2/8] rebase: update invocation of rebase dot-sourced scripts","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:23Z","receivedAt":"2018-03-23T21:25:55Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"Due to historical reasons, the backend scriptlets for \"git rebase\"\nare structured a bit unusually. As originally designed,\ndot-sourcing them from \"git rebase\" was sufficient to invoke the\nspecific backend.\n\nHowever, it was later discovered that some shell implementations\n(e.g. FreeBSD 9.x) misbehaved by continuing to execute statements\nfollowing a top-level \"return\" rather than returning control to\nthe next statement in \"git rebase\" after dot-sourcing the\nscriptlet. To work around this shortcoming, the whole body of\ngit-rebase--$backend.sh was made into a shell function\ngit_rebase__$backend, and then the very last line of the scriptlet\ncalled that function.\n\nA more normal architecture is for a dot-sourced scriptlet merely\nto define functions (thus acting as a function library), and for\nthose functions to be called by the script doing the dot-sourcing.\nMigrate to this arrangement by moving the git_rebase__$backend\ncall from the end of a scriptlet into \"git rebase\" itself.\n\nWhile at it, remove the large comment block from each scriptlet\nexplaining this historic anomaly since it serves no purpose under\nthe new normalized architecture in which a scriptlet is merely a\nfunction library.\n\nSigned-off-by: Wink Saville \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \n---\n git-rebase--am.sh | 11 -----------\n git-rebase--interactive.sh | 11 -----------\n git-rebase--merge.sh | 11 -----------\n git-rebase.sh | 1 +\n 4 files changed, 1 insertion(+), 33 deletions(-)\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex be3f06892..e5fd6101d 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -4,15 +4,6 @@\n # Copyright (c) 2010 Junio C Hamano.\n #\n \n-# The whole contents of this file is run by dot-sourcing it from\n-# inside a shell function. It used to be that \"return\"s we see\n-# below were not inside any function, and expected to return\n-# to the function that dot-sourced us.\n-#\n-# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a\n-# construct and continue to run the statements that follow such a \"return\".\n-# As a work-around, we introduce an extra layer of a function\n-# here, and immediately call it after defining it.\n git_rebase__am () {\n \n case \"$action\" in\n@@ -105,5 +96,3 @@ fi\n move_to_original_branch\n \n }\n-# ... and then we call the whole thing.\n-git_rebase__am\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 561e2660e..213d75f43 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -740,15 +740,6 @@ get_missing_commit_check_level () {\n \tprintf '%s' \"$check_level\" | tr 'A-Z' 'a-z'\n }\n \n-# The whole contents of this file is run by dot-sourcing it from\n-# inside a shell function. It used to be that \"return\"s we see\n-# below were not inside any function, and expected to return\n-# to the function that dot-sourced us.\n-#\n-# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a\n-# construct and continue to run the statements that follow such a \"return\".\n-# As a work-around, we introduce an extra layer of a function\n-# here, and immediately call it after defining it.\n git_rebase__interactive () {\n \n case \"$action\" in\n@@ -1029,5 +1020,3 @@ fi\n do_rest\n \n }\n-# ... and then we call the whole thing.\n-git_rebase__interactive\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex ceb715453..685f48ca4 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -104,15 +104,6 @@ finish_rb_merge () {\n \tsay All done.\n }\n \n-# The whole contents of this file is run by dot-sourcing it from\n-# inside a shell function. It used to be that \"return\"s we see\n-# below were not inside any function, and expected to return\n-# to the function that dot-sourced us.\n-#\n-# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a\n-# construct and continue to run the statements that follow such a \"return\".\n-# As a work-around, we introduce an extra layer of a function\n-# here, and immediately call it after defining it.\n git_rebase__merge () {\n \n case \"$action\" in\n@@ -171,5 +162,3 @@ done\n finish_rb_merge\n \n }\n-# ... and then we call the whole thing.\n-git_rebase__merge\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex a1f6e5de6..6edf8c5b1 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -197,6 +197,7 @@ run_specific_rebase () {\n \t\tautosquash=\n \tfi\n \t. git-rebase--$type\n+\tgit_rebase__$type\n \tret=$?\n \tif test $ret -eq 0\n \tthen\n-- \n2.16.2\n\n"},{"id":"342688","messageId":"e893a9d550f4d09baf0d21adedca841b96feae0d.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"[RFC PATCH v5 3/8] Indent function git_rebase__interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:24Z","receivedAt":"2018-03-23T21:25:59Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"Signed-off-by: Wink Saville \n---\n git-rebase--interactive.sh | 432 ++++++++++++++++++++++-----------------------\n 1 file changed, 215 insertions(+), 217 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 213d75f43..a79330f45 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -741,27 +741,26 @@ get_missing_commit_check_level () {\n }\n \n git_rebase__interactive () {\n-\n-case \"$action\" in\n-continue)\n-\tif test ! -d \"$rewritten\"\n-\tthen\n-\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n-\t\t\t--continue\n-\tfi\n-\t# do we have anything to commit?\n-\tif git diff-index --cached --quiet HEAD --\n-\tthen\n-\t\t# Nothing to commit -- skip this commit\n-\n-\t\ttest ! -f \"$GIT_DIR\"/CHERRY_PICK_HEAD ||\n-\t\trm \"$GIT_DIR\"/CHERRY_PICK_HEAD ||\n-\t\tdie \"$(gettext \"Could not remove CHERRY_PICK_HEAD\")\"\n-\telse\n-\t\tif ! test -f \"$author_script\"\n+\tcase \"$action\" in\n+\tcontinue)\n+\t\tif test ! -d \"$rewritten\"\n \t\tthen\n-\t\t\tgpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")}\n-\t\t\tdie \"$(eval_gettext \"\\\n+\t\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t\t\t--continue\n+\t\tfi\n+\t\t# do we have anything to commit?\n+\t\tif git diff-index --cached --quiet HEAD --\n+\t\tthen\n+\t\t\t# Nothing to commit -- skip this commit\n+\n+\t\t\ttest ! -f \"$GIT_DIR\"/CHERRY_PICK_HEAD ||\n+\t\t\trm \"$GIT_DIR\"/CHERRY_PICK_HEAD ||\n+\t\t\tdie \"$(gettext \"Could not remove CHERRY_PICK_HEAD\")\"\n+\t\telse\n+\t\t\tif ! test -f \"$author_script\"\n+\t\t\tthen\n+\t\t\t\tgpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")}\n+\t\t\t\tdie \"$(eval_gettext \"\\\n You have staged changes in your working tree.\n If these changes are meant to be\n squashed into the previous commit, run:\n@@ -776,197 +775,197 @@ In both cases, once you're done, continue with:\n \n git rebase --continue\n \")\"\n-\t\tfi\n-\t\t. \"$author_script\" ||\n-\t\t\tdie \"$(gettext \"Error trying to find the author identity to amend commit\")\"\n-\t\tif test -f \"$amend\"\n-\t\tthen\n-\t\t\tcurrent_head=$(git rev-parse --verify HEAD)\n-\t\t\ttest \"$current_head\" = $(cat \"$amend\") ||\n-\t\t\tdie \"$(gettext \"\\\n+\t\t\tfi\n+\t\t\t. \"$author_script\" ||\n+\t\t\t\tdie \"$(gettext \"Error trying to find the author identity to amend commit\")\"\n+\t\t\tif test -f \"$amend\"\n+\t\t\tthen\n+\t\t\t\tcurrent_head=$(git rev-parse --verify HEAD)\n+\t\t\t\ttest \"$current_head\" = $(cat \"$amend\") ||\n+\t\t\t\tdie \"$(gettext \"\\\n You have uncommitted changes in your working tree. Please commit them\n first and then run 'git rebase --continue' again.\")\"\n-\t\t\tdo_with_author git commit --amend --no-verify -F \"$msg\" -e \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n-\t\t\t\tdie \"$(gettext \"Could not commit staged changes.\")\"\n-\t\telse\n-\t\t\tdo_with_author git commit --no-verify -F \"$msg\" -e \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n-\t\t\t\tdie \"$(gettext \"Could not commit staged changes.\")\"\n+\t\t\t\tdo_with_author git commit --amend --no-verify -F \"$msg\" -e \\\n+\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n+\t\t\t\t\tdie \"$(gettext \"Could not commit staged changes.\")\"\n+\t\t\telse\n+\t\t\t\tdo_with_author git commit --no-verify -F \"$msg\" -e \\\n+\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} $allow_empty_message ||\n+\t\t\t\t\tdie \"$(gettext \"Could not commit staged changes.\")\"\n+\t\t\tfi\n \t\tfi\n-\tfi\n \n-\tif test -r \"$state_dir\"/stopped-sha\n-\tthen\n-\t\trecord_in_rewritten \"$(cat \"$state_dir\"/stopped-sha)\"\n-\tfi\n+\t\tif test -r \"$state_dir\"/stopped-sha\n+\t\tthen\n+\t\t\trecord_in_rewritten \"$(cat \"$state_dir\"/stopped-sha)\"\n+\t\tfi\n \n-\trequire_clean_work_tree \"rebase\"\n-\tdo_rest\n-\treturn 0\n-\t;;\n-skip)\n-\tgit rerere clear\n+\t\trequire_clean_work_tree \"rebase\"\n+\t\tdo_rest\n+\t\treturn 0\n+\t\t;;\n+\tskip)\n+\t\tgit rerere clear\n \n-\tif test ! -d \"$rewritten\"\n-\tthen\n-\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n-\t\t\t--continue\n-\tfi\n-\tdo_rest\n-\treturn 0\n-\t;;\n-edit-todo)\n-\tgit stripspace --strip-comments <\"$todo\" >\"$todo\".new\n-\tmv -f \"$todo\".new \"$todo\"\n-\tcollapse_todo_ids\n-\tappend_todo_help\n-\tgettext \"\n+\t\tif test ! -d \"$rewritten\"\n+\t\tthen\n+\t\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t\t\t--continue\n+\t\tfi\n+\t\tdo_rest\n+\t\treturn 0\n+\t\t;;\n+\tedit-todo)\n+\t\tgit stripspace --strip-comments <\"$todo\" >\"$todo\".new\n+\t\tmv -f \"$todo\".new \"$todo\"\n+\t\tcollapse_todo_ids\n+\t\tappend_todo_help\n+\t\tgettext \"\n You are editing the todo file of an ongoing interactive rebase.\n To continue rebase after editing, run:\n git rebase --continue\n \n \" | git stripspace --comment-lines >>\"$todo\"\n \n-\tgit_sequence_editor \"$todo\" ||\n-\t\tdie \"$(gettext \"Could not execute editor\")\"\n-\texpand_todo_ids\n-\n-\texit\n-\t;;\n-show-current-patch)\n-\texec git show REBASE_HEAD --\n-\t;;\n-esac\n-\n-comment_for_reflog start\n+\t\tgit_sequence_editor \"$todo\" ||\n+\t\t\tdie \"$(gettext \"Could not execute editor\")\"\n+\t\texpand_todo_ids\n \n-if test ! -z \"$switch_to\"\n-then\n-\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n-\toutput git checkout \"$switch_to\" -- ||\n-\t\tdie \"$(eval_gettext \"Could not checkout \\$switch_to\")\"\n+\t\texit\n+\t\t;;\n+\tshow-current-patch)\n+\t\texec git show REBASE_HEAD --\n+\t\t;;\n+\tesac\n \n \tcomment_for_reflog start\n-fi\n-\n-orig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n-mkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n-rm -f \"$(git rev-parse --git-path REBASE_HEAD)\"\n \n-: > \"$state_dir\"/interactive || die \"$(gettext \"Could not mark as interactive\")\"\n-write_basic_state\n-if test t = \"$preserve_merges\"\n-then\n-\tif test -z \"$rebase_root\"\n+\tif test ! -z \"$switch_to\"\n \tthen\n-\t\tmkdir \"$rewritten\" &&\n-\t\tfor c in $(git merge-base --all $orig_head $upstream)\n-\t\tdo\n-\t\t\techo $onto > \"$rewritten\"/$c ||\n-\t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n-\t\tdone\n-\telse\n-\t\tmkdir \"$rewritten\" &&\n-\t\techo $onto > \"$rewritten\"/root ||\n-\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n+\t\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n+\t\toutput git checkout \"$switch_to\" -- ||\n+\t\t\tdie \"$(eval_gettext \"Could not checkout \\$switch_to\")\"\n+\n+\t\tcomment_for_reflog start\n \tfi\n-\t# No cherry-pick because our first pass is to determine\n-\t# parents to rewrite and skipping dropped commits would\n-\t# prematurely end our probe\n-\tmerges_option=\n-else\n-\tmerges_option=\"--no-merges --cherry-pick\"\n-fi\n-\n-shorthead=$(git rev-parse --short $orig_head)\n-shortonto=$(git rev-parse --short $onto)\n-if test -z \"$rebase_root\"\n-\t# this is now equivalent to ! -z \"$upstream\"\n-then\n-\tshortupstream=$(git rev-parse --short $upstream)\n-\trevisions=$upstream...$orig_head\n-\tshortrevisions=$shortupstream..$shorthead\n-else\n-\trevisions=$onto...$orig_head\n-\tshortrevisions=$shorthead\n-fi\n-if test t != \"$preserve_merges\"\n-then\n-\tgit rebase--helper --make-script ${keep_empty:+--keep-empty} \\\n-\t\t$revisions ${restrict_revision+^$restrict_revision} >\"$todo\" ||\n-\tdie \"$(gettext \"Could not generate todo list\")\"\n-else\n-\tformat=$(git config --get rebase.instructionFormat)\n-\t# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse\n-\tgit rev-list $merges_option --format=\"%m%H ${format:-%s}\" \\\n-\t\t--reverse --left-right --topo-order \\\n-\t\t$revisions ${restrict_revision+^$restrict_revision} | \\\n-\t\tsed -n \"s/^>//p\" |\n-\twhile read -r sha1 rest\n-\tdo\n \n-\t\tif test -z \"$keep_empty\" && is_empty_commit $sha1 && ! is_merge_commit $sha1\n-\t\tthen\n-\t\t\tcomment_out=\"$comment_char \"\n-\t\telse\n-\t\t\tcomment_out=\n-\t\tfi\n+\torig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n+\tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n+\trm -f \"$(git rev-parse --git-path REBASE_HEAD)\"\n \n+\t: > \"$state_dir\"/interactive || die \"$(gettext \"Could not mark as interactive\")\"\n+\twrite_basic_state\n+\tif test t = \"$preserve_merges\"\n+\tthen\n \t\tif test -z \"$rebase_root\"\n \t\tthen\n-\t\t\tpreserve=t\n-\t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n+\t\t\tmkdir \"$rewritten\" &&\n+\t\t\tfor c in $(git merge-base --all $orig_head $upstream)\n \t\t\tdo\n-\t\t\t\tif test -f \"$rewritten\"/$p\n-\t\t\t\tthen\n-\t\t\t\t\tpreserve=f\n-\t\t\t\tfi\n+\t\t\t\techo $onto > \"$rewritten\"/$c ||\n+\t\t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n \t\t\tdone\n \t\telse\n-\t\t\tpreserve=f\n-\t\tfi\n-\t\tif test f = \"$preserve\"\n-\t\tthen\n-\t\t\ttouch \"$rewritten\"/$sha1\n-\t\t\tprintf '%s\\n' \"${comment_out}pick $sha1 $rest\" >>\"$todo\"\n-\t\tfi\n-\tdone\n-fi\n-\n-# Watch for commits that been dropped by --cherry-pick\n-if test t = \"$preserve_merges\"\n-then\n-\tmkdir \"$dropped\"\n-\t# Save all non-cherry-picked changes\n-\tgit rev-list $revisions --left-right --cherry-pick | \\\n-\t\tsed -n \"s/^>//p\" > \"$state_dir\"/not-cherry-picks\n-\t# Now all commits and note which ones are missing in\n-\t# not-cherry-picks and hence being dropped\n-\tgit rev-list $revisions |\n-\twhile read rev\n-\tdo\n-\t\tif test -f \"$rewritten\"/$rev &&\n-\t\t ! sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks >/dev/null\n-\t\tthen\n-\t\t\t# Use -f2 because if rev-list is telling us this commit is\n-\t\t\t# not worthwhile, we don't want to track its multiple heads,\n-\t\t\t# just the history of its first-parent for others that will\n-\t\t\t# be rebasing on top of it\n-\t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n-\t\t\tsha1=$(git rev-list -1 $rev)\n-\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n-\t\t\trm \"$rewritten\"/$rev\n+\t\t\tmkdir \"$rewritten\" &&\n+\t\t\techo $onto > \"$rewritten\"/root ||\n+\t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n \t\tfi\n-\tdone\n-fi\n+\t\t# No cherry-pick because our first pass is to determine\n+\t\t# parents to rewrite and skipping dropped commits would\n+\t\t# prematurely end our probe\n+\t\tmerges_option=\n+\telse\n+\t\tmerges_option=\"--no-merges --cherry-pick\"\n+\tfi\n+\n+\tshorthead=$(git rev-parse --short $orig_head)\n+\tshortonto=$(git rev-parse --short $onto)\n+\tif test -z \"$rebase_root\"\n+\t\t# this is now equivalent to ! -z \"$upstream\"\n+\tthen\n+\t\tshortupstream=$(git rev-parse --short $upstream)\n+\t\trevisions=$upstream...$orig_head\n+\t\tshortrevisions=$shortupstream..$shorthead\n+\telse\n+\t\trevisions=$onto...$orig_head\n+\t\tshortrevisions=$shorthead\n+\tfi\n+\tif test t != \"$preserve_merges\"\n+\tthen\n+\t\tgit rebase--helper --make-script ${keep_empty:+--keep-empty} \\\n+\t\t\t$revisions ${restrict_revision+^$restrict_revision} >\"$todo\" ||\n+\t\tdie \"$(gettext \"Could not generate todo list\")\"\n+\telse\n+\t\tformat=$(git config --get rebase.instructionFormat)\n+\t\t# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse\n+\t\tgit rev-list $merges_option --format=\"%m%H ${format:-%s}\" \\\n+\t\t\t--reverse --left-right --topo-order \\\n+\t\t\t$revisions ${restrict_revision+^$restrict_revision} | \\\n+\t\t\tsed -n \"s/^>//p\" |\n+\t\twhile read -r sha1 rest\n+\t\tdo\n+\n+\t\t\tif test -z \"$keep_empty\" && is_empty_commit $sha1 && ! is_merge_commit $sha1\n+\t\t\tthen\n+\t\t\t\tcomment_out=\"$comment_char \"\n+\t\t\telse\n+\t\t\t\tcomment_out=\n+\t\t\tfi\n+\n+\t\t\tif test -z \"$rebase_root\"\n+\t\t\tthen\n+\t\t\t\tpreserve=t\n+\t\t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n+\t\t\t\tdo\n+\t\t\t\t\tif test -f \"$rewritten\"/$p\n+\t\t\t\t\tthen\n+\t\t\t\t\t\tpreserve=f\n+\t\t\t\t\tfi\n+\t\t\t\tdone\n+\t\t\telse\n+\t\t\t\tpreserve=f\n+\t\t\tfi\n+\t\t\tif test f = \"$preserve\"\n+\t\t\tthen\n+\t\t\t\ttouch \"$rewritten\"/$sha1\n+\t\t\t\tprintf '%s\\n' \"${comment_out}pick $sha1 $rest\" >>\"$todo\"\n+\t\t\tfi\n+\t\tdone\n+\tfi\n+\n+\t# Watch for commits that been dropped by --cherry-pick\n+\tif test t = \"$preserve_merges\"\n+\tthen\n+\t\tmkdir \"$dropped\"\n+\t\t# Save all non-cherry-picked changes\n+\t\tgit rev-list $revisions --left-right --cherry-pick | \\\n+\t\t\tsed -n \"s/^>//p\" > \"$state_dir\"/not-cherry-picks\n+\t\t# Now all commits and note which ones are missing in\n+\t\t# not-cherry-picks and hence being dropped\n+\t\tgit rev-list $revisions |\n+\t\twhile read rev\n+\t\tdo\n+\t\t\tif test -f \"$rewritten\"/$rev &&\n+\t\t\t ! sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks >/dev/null\n+\t\t\tthen\n+\t\t\t\t# Use -f2 because if rev-list is telling us this commit is\n+\t\t\t\t# not worthwhile, we don't want to track its multiple heads,\n+\t\t\t\t# just the history of its first-parent for others that will\n+\t\t\t\t# be rebasing on top of it\n+\t\t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n+\t\t\t\tsha1=$(git rev-list -1 $rev)\n+\t\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n+\t\t\t\trm \"$rewritten\"/$rev\n+\t\t\tfi\n+\t\tdone\n+\tfi\n \n-test -s \"$todo\" || echo noop >> \"$todo\"\n-test -z \"$autosquash\" || git rebase--helper --rearrange-squash || exit\n-test -n \"$cmd\" && git rebase--helper --add-exec-commands \"$cmd\"\n+\ttest -s \"$todo\" || echo noop >> \"$todo\"\n+\ttest -z \"$autosquash\" || git rebase--helper --rearrange-squash || exit\n+\ttest -n \"$cmd\" && git rebase--helper --add-exec-commands \"$cmd\"\n \n-todocount=$(git stripspace --strip-comments <\"$todo\" | wc -l)\n-todocount=${todocount##* }\n+\ttodocount=$(git stripspace --strip-comments <\"$todo\" | wc -l)\n+\ttodocount=${todocount##* }\n \n cat >>\"$todo\" <>\"$todo\"\n+\tappend_todo_help\n+\tgettext \"\n+\tHowever, if you remove everything, the rebase will be aborted.\n \n-if test -z \"$keep_empty\"\n-then\n-\tprintf '%s\\n' \"$comment_char $(gettext \"Note that empty commits are commented out\")\" >>\"$todo\"\n-fi\n+\t\" | git stripspace --comment-lines >>\"$todo\"\n \n+\tif test -z \"$keep_empty\"\n+\tthen\n+\t\tprintf '%s\\n' \"$comment_char $(gettext \"Note that empty commits are commented out\")\" >>\"$todo\"\n+\tfi\n \n-has_action \"$todo\" ||\n-\treturn 2\n \n-cp \"$todo\" \"$todo\".backup\n-collapse_todo_ids\n-git_sequence_editor \"$todo\" ||\n-\tdie_abort \"$(gettext \"Could not execute editor\")\"\n+\thas_action \"$todo\" ||\n+\t\treturn 2\n \n-has_action \"$todo\" ||\n-\treturn 2\n+\tcp \"$todo\" \"$todo\".backup\n+\tcollapse_todo_ids\n+\tgit_sequence_editor \"$todo\" ||\n+\t\tdie_abort \"$(gettext \"Could not execute editor\")\"\n \n-git rebase--helper --check-todo-list || {\n-\tret=$?\n-\tcheckout_onto\n-\texit $ret\n-}\n+\thas_action \"$todo\" ||\n+\t\treturn 2\n \n-expand_todo_ids\n+\tgit rebase--helper --check-todo-list || {\n+\t\tret=$?\n+\t\tcheckout_onto\n+\t\texit $ret\n+\t}\n \n-test -d \"$rewritten\" || test -n \"$force_rebase\" ||\n-onto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n-die \"Could not skip unnecessary pick commands\"\n+\texpand_todo_ids\n \n-checkout_onto\n-if test -z \"$rebase_root\" && test ! -d \"$rewritten\"\n-then\n-\trequire_clean_work_tree \"rebase\"\n-\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n-\t\t--continue\n-fi\n-do_rest\n+\ttest -d \"$rewritten\" || test -n \"$force_rebase\" ||\n+\tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n+\tdie \"Could not skip unnecessary pick commands\"\n \n+\tcheckout_onto\n+\tif test -z \"$rebase_root\" && test ! -d \"$rewritten\"\n+\tthen\n+\t\trequire_clean_work_tree \"rebase\"\n+\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t\t--continue\n+\tfi\n+\tdo_rest\n }\n-- \n2.16.2\n\n"},{"id":"342689","messageId":"baf0d9bab81bb3a80d0428c4aaa33cf32b3823e4.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"[RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:25Z","receivedAt":"2018-03-23T21:26:02Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"The extracted functions are:\n - initiate_action\n - setup_reflog_action\n - init_basic_state\n - init_revisions_and_shortrevisions\n - complete_action\n\nUsed by git_rebase__interactive\n\nSigned-off-by: Wink Saville \nHelped-by: Junio C Hamano \nHelped-by: Johannes Schindelin \n---\n git-rebase--interactive.sh | 182 +++++++++++++++++++++++++++------------------\n 1 file changed, 111 insertions(+), 71 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex a79330f45..2c10a7f1a 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -740,8 +740,20 @@ get_missing_commit_check_level () {\n \tprintf '%s' \"$check_level\" | tr 'A-Z' 'a-z'\n }\n \n-git_rebase__interactive () {\n-\tcase \"$action\" in\n+# Initiate an action. If the cannot be any\n+# further action it may exec a command\n+# or exit and not return.\n+#\n+# TODO: Consider a cleaner return model so it\n+# never exits and always return 0 if process\n+# is complete.\n+#\n+# Parameter 1 is the action to initiate.\n+#\n+# Returns 0 if the action was able to complete\n+# and if 1 if further processing is required.\n+initiate_action () {\n+\tcase \"$1\" in\n \tcontinue)\n \t\tif test ! -d \"$rewritten\"\n \t\tthen\n@@ -836,8 +848,13 @@ To continue rebase after editing, run:\n \tshow-current-patch)\n \t\texec git show REBASE_HEAD --\n \t\t;;\n+\t*)\n+\t\treturn 1 # continue\n+\t\t;;\n \tesac\n+}\n \n+setup_reflog_action () {\n \tcomment_for_reflog start\n \n \tif test ! -z \"$switch_to\"\n@@ -848,13 +865,102 @@ To continue rebase after editing, run:\n \n \t\tcomment_for_reflog start\n \tfi\n+}\n \n+init_basic_state () {\n \torig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n \trm -f \"$(git rev-parse --git-path REBASE_HEAD)\"\n \n \t: > \"$state_dir\"/interactive || die \"$(gettext \"Could not mark as interactive\")\"\n \twrite_basic_state\n+}\n+\n+init_revisions_and_shortrevisions () {\n+\tshorthead=$(git rev-parse --short $orig_head)\n+\tshortonto=$(git rev-parse --short $onto)\n+\tif test -z \"$rebase_root\"\n+\t\t# this is now equivalent to ! -z \"$upstream\"\n+\tthen\n+\t\tshortupstream=$(git rev-parse --short $upstream)\n+\t\trevisions=$upstream...$orig_head\n+\t\tshortrevisions=$shortupstream..$shorthead\n+\telse\n+\t\trevisions=$onto...$orig_head\n+\t\tshortrevisions=$shorthead\n+\tfi\n+}\n+\n+complete_action() {\n+\ttest -s \"$todo\" || echo noop >> \"$todo\"\n+\ttest -z \"$autosquash\" || git rebase--helper --rearrange-squash || exit\n+\ttest -n \"$cmd\" && git rebase--helper --add-exec-commands \"$cmd\"\n+\n+\ttodocount=$(git stripspace --strip-comments <\"$todo\" | wc -l)\n+\ttodocount=${todocount##* }\n+\n+cat >>\"$todo\" <>\"$todo\"\n+\n+\tif test -z \"$keep_empty\"\n+\tthen\n+\t\tprintf '%s\\n' \"$comment_char $(gettext \"Note that empty commits are commented out\")\" >>\"$todo\"\n+\tfi\n+\n+\n+\thas_action \"$todo\" ||\n+\t\treturn 2\n+\n+\tcp \"$todo\" \"$todo\".backup\n+\tcollapse_todo_ids\n+\tgit_sequence_editor \"$todo\" ||\n+\t\tdie_abort \"$(gettext \"Could not execute editor\")\"\n+\n+\thas_action \"$todo\" ||\n+\t\treturn 2\n+\n+\tgit rebase--helper --check-todo-list || {\n+\t\tret=$?\n+\t\tcheckout_onto\n+\t\texit $ret\n+\t}\n+\n+\texpand_todo_ids\n+\n+\ttest -d \"$rewritten\" || test -n \"$force_rebase\" ||\n+\tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n+\tdie \"Could not skip unnecessary pick commands\"\n+\n+\tcheckout_onto\n+\tif test -z \"$rebase_root\" && test ! -d \"$rewritten\"\n+\tthen\n+\t\trequire_clean_work_tree \"rebase\"\n+\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n+\t\t\t--continue\n+\tfi\n+\tdo_rest\n+}\n+\n+git_rebase__interactive () {\n+\tinitiate_action \"$action\"\n+\tret=$?\n+\tif test $ret = 0; then\n+\t\treturn 0\n+\tfi\n+\n+\tsetup_reflog_action\n+\tinit_basic_state\n+\n \tif test t = \"$preserve_merges\"\n \tthen\n \t\tif test -z \"$rebase_root\"\n@@ -878,18 +984,8 @@ To continue rebase after editing, run:\n \t\tmerges_option=\"--no-merges --cherry-pick\"\n \tfi\n \n-\tshorthead=$(git rev-parse --short $orig_head)\n-\tshortonto=$(git rev-parse --short $onto)\n-\tif test -z \"$rebase_root\"\n-\t\t# this is now equivalent to ! -z \"$upstream\"\n-\tthen\n-\t\tshortupstream=$(git rev-parse --short $upstream)\n-\t\trevisions=$upstream...$orig_head\n-\t\tshortrevisions=$shortupstream..$shorthead\n-\telse\n-\t\trevisions=$onto...$orig_head\n-\t\tshortrevisions=$shorthead\n-\tfi\n+\tinit_revisions_and_shortrevisions\n+\n \tif test t != \"$preserve_merges\"\n \tthen\n \t\tgit rebase--helper --make-script ${keep_empty:+--keep-empty} \\\n@@ -960,61 +1056,5 @@ To continue rebase after editing, run:\n \t\tdone\n \tfi\n \n-\ttest -s \"$todo\" || echo noop >> \"$todo\"\n-\ttest -z \"$autosquash\" || git rebase--helper --rearrange-squash || exit\n-\ttest -n \"$cmd\" && git rebase--helper --add-exec-commands \"$cmd\"\n-\n-\ttodocount=$(git stripspace --strip-comments <\"$todo\" | wc -l)\n-\ttodocount=${todocount##* }\n-\n-cat >>\"$todo\" <>\"$todo\"\n-\n-\tif test -z \"$keep_empty\"\n-\tthen\n-\t\tprintf '%s\\n' \"$comment_char $(gettext \"Note that empty commits are commented out\")\" >>\"$todo\"\n-\tfi\n-\n-\n-\thas_action \"$todo\" ||\n-\t\treturn 2\n-\n-\tcp \"$todo\" \"$todo\".backup\n-\tcollapse_todo_ids\n-\tgit_sequence_editor \"$todo\" ||\n-\t\tdie_abort \"$(gettext \"Could not execute editor\")\"\n-\n-\thas_action \"$todo\" ||\n-\t\treturn 2\n-\n-\tgit rebase--helper --check-todo-list || {\n-\t\tret=$?\n-\t\tcheckout_onto\n-\t\texit $ret\n-\t}\n-\n-\texpand_todo_ids\n-\n-\ttest -d \"$rewritten\" || test -n \"$force_rebase\" ||\n-\tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n-\tdie \"Could not skip unnecessary pick commands\"\n-\n-\tcheckout_onto\n-\tif test -z \"$rebase_root\" && test ! -d \"$rewritten\"\n-\tthen\n-\t\trequire_clean_work_tree \"rebase\"\n-\t\texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n-\t\t\t--continue\n-\tfi\n-\tdo_rest\n+\tcomplete_action\n }\n-- \n2.16.2\n\n"},{"id":"342690","messageId":"784c1d59b7d4831c57e923be3515ddf68a3a8f14.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"[RFC PATCH v5 5/8] Add and use git_rebase__interactive__preserve_merges","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:26Z","receivedAt":"2018-03-23T21:26:04Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"At the moment it's an exact copy of git_rebase__interactive except\nthe name has changed.\n\nSigned-off-by: Wink Saville \n---\n git-rebase--interactive.sh | 108 +++++++++++++++++++++++++++++++++++++++++++++\n git-rebase.sh | 2 +-\n 2 files changed, 109 insertions(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2c10a7f1a..ab5513d80 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -1058,3 +1058,111 @@ git_rebase__interactive () {\n \n \tcomplete_action\n }\n+\n+git_rebase__interactive__preserve_merges () {\n+\tinitiate_action \"$action\"\n+\tret=$?\n+\tif test $ret = 0; then\n+\t\treturn 0\n+\tfi\n+\n+\tsetup_reflog_action\n+\tinit_basic_state\n+\n+\tif test t = \"$preserve_merges\"\n+\tthen\n+\t\tif test -z \"$rebase_root\"\n+\t\tthen\n+\t\t\tmkdir \"$rewritten\" &&\n+\t\t\tfor c in $(git merge-base --all $orig_head $upstream)\n+\t\t\tdo\n+\t\t\t\techo $onto > \"$rewritten\"/$c ||\n+\t\t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n+\t\t\tdone\n+\t\telse\n+\t\t\tmkdir \"$rewritten\" &&\n+\t\t\techo $onto > \"$rewritten\"/root ||\n+\t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n+\t\tfi\n+\t\t# No cherry-pick because our first pass is to determine\n+\t\t# parents to rewrite and skipping dropped commits would\n+\t\t# prematurely end our probe\n+\t\tmerges_option=\n+\telse\n+\t\tmerges_option=\"--no-merges --cherry-pick\"\n+\tfi\n+\n+\tinit_revisions_and_shortrevisions\n+\n+\tif test t != \"$preserve_merges\"\n+\tthen\n+\t\tgit rebase--helper --make-script ${keep_empty:+--keep-empty} \\\n+\t\t\t$revisions ${restrict_revision+^$restrict_revision} >\"$todo\" ||\n+\t\tdie \"$(gettext \"Could not generate todo list\")\"\n+\telse\n+\t\tformat=$(git config --get rebase.instructionFormat)\n+\t\t# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse\n+\t\tgit rev-list $merges_option --format=\"%m%H ${format:-%s}\" \\\n+\t\t\t--reverse --left-right --topo-order \\\n+\t\t\t$revisions ${restrict_revision+^$restrict_revision} | \\\n+\t\t\tsed -n \"s/^>//p\" |\n+\t\twhile read -r sha1 rest\n+\t\tdo\n+\n+\t\t\tif test -z \"$keep_empty\" && is_empty_commit $sha1 && ! is_merge_commit $sha1\n+\t\t\tthen\n+\t\t\t\tcomment_out=\"$comment_char \"\n+\t\t\telse\n+\t\t\t\tcomment_out=\n+\t\t\tfi\n+\n+\t\t\tif test -z \"$rebase_root\"\n+\t\t\tthen\n+\t\t\t\tpreserve=t\n+\t\t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n+\t\t\t\tdo\n+\t\t\t\t\tif test -f \"$rewritten\"/$p\n+\t\t\t\t\tthen\n+\t\t\t\t\t\tpreserve=f\n+\t\t\t\t\tfi\n+\t\t\t\tdone\n+\t\t\telse\n+\t\t\t\tpreserve=f\n+\t\t\tfi\n+\t\t\tif test f = \"$preserve\"\n+\t\t\tthen\n+\t\t\t\ttouch \"$rewritten\"/$sha1\n+\t\t\t\tprintf '%s\\n' \"${comment_out}pick $sha1 $rest\" >>\"$todo\"\n+\t\t\tfi\n+\t\tdone\n+\tfi\n+\n+\t# Watch for commits that been dropped by --cherry-pick\n+\tif test t = \"$preserve_merges\"\n+\tthen\n+\t\tmkdir \"$dropped\"\n+\t\t# Save all non-cherry-picked changes\n+\t\tgit rev-list $revisions --left-right --cherry-pick | \\\n+\t\t\tsed -n \"s/^>//p\" > \"$state_dir\"/not-cherry-picks\n+\t\t# Now all commits and note which ones are missing in\n+\t\t# not-cherry-picks and hence being dropped\n+\t\tgit rev-list $revisions |\n+\t\twhile read rev\n+\t\tdo\n+\t\t\tif test -f \"$rewritten\"/$rev &&\n+\t\t\t ! sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks >/dev/null\n+\t\t\tthen\n+\t\t\t\t# Use -f2 because if rev-list is telling us this commit is\n+\t\t\t\t# not worthwhile, we don't want to track its multiple heads,\n+\t\t\t\t# just the history of its first-parent for others that will\n+\t\t\t\t# be rebasing on top of it\n+\t\t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n+\t\t\t\tsha1=$(git rev-list -1 $rev)\n+\t\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n+\t\t\t\trm \"$rewritten\"/$rev\n+\t\t\tfi\n+\t\tdone\n+\tfi\n+\n+\tcomplete_action\n+}\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 6edf8c5b1..fb64ee1fe 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -197,7 +197,7 @@ run_specific_rebase () {\n \t\tautosquash=\n \tfi\n \t. git-rebase--$type\n-\tgit_rebase__$type\n+\tgit_rebase__$type${preserve_merges:+__preserve_merges}\n \tret=$?\n \tif test $ret -eq 0\n \tthen\n-- \n2.16.2\n\n"},{"id":"342691","messageId":"53cd3da5960a1c99a55959990737ca4d54989659.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"[RFC PATCH v5 6/8] Remove unused code paths from git_rebase__interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:27Z","receivedAt":"2018-03-23T21:26:06Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"Since git_rebase__interactive is now never called with\n$preserve_merges = t we can remove those code paths.\n\nSigned-off-by: Wink Saville \n---\n git-rebase--interactive.sh | 95 ++--------------------------------------------\n 1 file changed, 4 insertions(+), 91 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex ab5513d80..346da0f67 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -961,100 +961,13 @@ git_rebase__interactive () {\n \tsetup_reflog_action\n \tinit_basic_state\n \n-\tif test t = \"$preserve_merges\"\n-\tthen\n-\t\tif test -z \"$rebase_root\"\n-\t\tthen\n-\t\t\tmkdir \"$rewritten\" &&\n-\t\t\tfor c in $(git merge-base --all $orig_head $upstream)\n-\t\t\tdo\n-\t\t\t\techo $onto > \"$rewritten\"/$c ||\n-\t\t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n-\t\t\tdone\n-\t\telse\n-\t\t\tmkdir \"$rewritten\" &&\n-\t\t\techo $onto > \"$rewritten\"/root ||\n-\t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n-\t\tfi\n-\t\t# No cherry-pick because our first pass is to determine\n-\t\t# parents to rewrite and skipping dropped commits would\n-\t\t# prematurely end our probe\n-\t\tmerges_option=\n-\telse\n-\t\tmerges_option=\"--no-merges --cherry-pick\"\n-\tfi\n+\tmerges_option=\"--no-merges --cherry-pick\"\n \n \tinit_revisions_and_shortrevisions\n \n-\tif test t != \"$preserve_merges\"\n-\tthen\n-\t\tgit rebase--helper --make-script ${keep_empty:+--keep-empty} \\\n-\t\t\t$revisions ${restrict_revision+^$restrict_revision} >\"$todo\" ||\n-\t\tdie \"$(gettext \"Could not generate todo list\")\"\n-\telse\n-\t\tformat=$(git config --get rebase.instructionFormat)\n-\t\t# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse\n-\t\tgit rev-list $merges_option --format=\"%m%H ${format:-%s}\" \\\n-\t\t\t--reverse --left-right --topo-order \\\n-\t\t\t$revisions ${restrict_revision+^$restrict_revision} | \\\n-\t\t\tsed -n \"s/^>//p\" |\n-\t\twhile read -r sha1 rest\n-\t\tdo\n-\n-\t\t\tif test -z \"$keep_empty\" && is_empty_commit $sha1 && ! is_merge_commit $sha1\n-\t\t\tthen\n-\t\t\t\tcomment_out=\"$comment_char \"\n-\t\t\telse\n-\t\t\t\tcomment_out=\n-\t\t\tfi\n-\n-\t\t\tif test -z \"$rebase_root\"\n-\t\t\tthen\n-\t\t\t\tpreserve=t\n-\t\t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n-\t\t\t\tdo\n-\t\t\t\t\tif test -f \"$rewritten\"/$p\n-\t\t\t\t\tthen\n-\t\t\t\t\t\tpreserve=f\n-\t\t\t\t\tfi\n-\t\t\t\tdone\n-\t\t\telse\n-\t\t\t\tpreserve=f\n-\t\t\tfi\n-\t\t\tif test f = \"$preserve\"\n-\t\t\tthen\n-\t\t\t\ttouch \"$rewritten\"/$sha1\n-\t\t\t\tprintf '%s\\n' \"${comment_out}pick $sha1 $rest\" >>\"$todo\"\n-\t\t\tfi\n-\t\tdone\n-\tfi\n-\n-\t# Watch for commits that been dropped by --cherry-pick\n-\tif test t = \"$preserve_merges\"\n-\tthen\n-\t\tmkdir \"$dropped\"\n-\t\t# Save all non-cherry-picked changes\n-\t\tgit rev-list $revisions --left-right --cherry-pick | \\\n-\t\t\tsed -n \"s/^>//p\" > \"$state_dir\"/not-cherry-picks\n-\t\t# Now all commits and note which ones are missing in\n-\t\t# not-cherry-picks and hence being dropped\n-\t\tgit rev-list $revisions |\n-\t\twhile read rev\n-\t\tdo\n-\t\t\tif test -f \"$rewritten\"/$rev &&\n-\t\t\t ! sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks >/dev/null\n-\t\t\tthen\n-\t\t\t\t# Use -f2 because if rev-list is telling us this commit is\n-\t\t\t\t# not worthwhile, we don't want to track its multiple heads,\n-\t\t\t\t# just the history of its first-parent for others that will\n-\t\t\t\t# be rebasing on top of it\n-\t\t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n-\t\t\t\tsha1=$(git rev-list -1 $rev)\n-\t\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n-\t\t\t\trm \"$rewritten\"/$rev\n-\t\t\tfi\n-\t\tdone\n-\tfi\n+\tgit rebase--helper --make-script ${keep_empty:+--keep-empty} \\\n+\t\t$revisions ${restrict_revision+^$restrict_revision} >\"$todo\" ||\n+\tdie \"$(gettext \"Could not generate todo list\")\"\n \n \tcomplete_action\n }\n-- \n2.16.2\n\n"},{"id":"342692","messageId":"d1e69af862ff0e7a3f04d2c1a73243f5057e804b.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"[RFC PATCH v5 7/8] Remove unused code paths from git_rebase__interactive__preserve_merges","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:28Z","receivedAt":"2018-03-23T21:26:10Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"Since git_rebase__interactive__preserve_merges is now always called with\n$preserve_merges = t we can remove the unused code paths.\n\nSigned-off-by: Wink Saville \n---\n git-rebase--interactive.sh | 152 ++++++++++++++++++++-------------------------\n 1 file changed, 69 insertions(+), 83 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 346da0f67..ddbd126f2 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -982,100 +982,86 @@ git_rebase__interactive__preserve_merges () {\n \tsetup_reflog_action\n \tinit_basic_state\n \n-\tif test t = \"$preserve_merges\"\n+\tif test -z \"$rebase_root\"\n \tthen\n-\t\tif test -z \"$rebase_root\"\n-\t\tthen\n-\t\t\tmkdir \"$rewritten\" &&\n-\t\t\tfor c in $(git merge-base --all $orig_head $upstream)\n-\t\t\tdo\n-\t\t\t\techo $onto > \"$rewritten\"/$c ||\n-\t\t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n-\t\t\tdone\n-\t\telse\n-\t\t\tmkdir \"$rewritten\" &&\n-\t\t\techo $onto > \"$rewritten\"/root ||\n+\t\tmkdir \"$rewritten\" &&\n+\t\tfor c in $(git merge-base --all $orig_head $upstream)\n+\t\tdo\n+\t\t\techo $onto > \"$rewritten\"/$c ||\n \t\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n-\t\tfi\n-\t\t# No cherry-pick because our first pass is to determine\n-\t\t# parents to rewrite and skipping dropped commits would\n-\t\t# prematurely end our probe\n-\t\tmerges_option=\n+\t\tdone\n \telse\n-\t\tmerges_option=\"--no-merges --cherry-pick\"\n+\t\tmkdir \"$rewritten\" &&\n+\t\techo $onto > \"$rewritten\"/root ||\n+\t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n \tfi\n \n+\t# No cherry-pick because our first pass is to determine\n+\t# parents to rewrite and skipping dropped commits would\n+\t# prematurely end our probe\n+\tmerges_option=\n+\n \tinit_revisions_and_shortrevisions\n \n-\tif test t != \"$preserve_merges\"\n-\tthen\n-\t\tgit rebase--helper --make-script ${keep_empty:+--keep-empty} \\\n-\t\t\t$revisions ${restrict_revision+^$restrict_revision} >\"$todo\" ||\n-\t\tdie \"$(gettext \"Could not generate todo list\")\"\n-\telse\n-\t\tformat=$(git config --get rebase.instructionFormat)\n-\t\t# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse\n-\t\tgit rev-list $merges_option --format=\"%m%H ${format:-%s}\" \\\n-\t\t\t--reverse --left-right --topo-order \\\n-\t\t\t$revisions ${restrict_revision+^$restrict_revision} | \\\n-\t\t\tsed -n \"s/^>//p\" |\n-\t\twhile read -r sha1 rest\n-\t\tdo\n+\tformat=$(git config --get rebase.instructionFormat)\n+\t# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse\n+\tgit rev-list $merges_option --format=\"%m%H ${format:-%s}\" \\\n+\t\t--reverse --left-right --topo-order \\\n+\t\t$revisions ${restrict_revision+^$restrict_revision} | \\\n+\t\tsed -n \"s/^>//p\" |\n+\twhile read -r sha1 rest\n+\tdo\n \n-\t\t\tif test -z \"$keep_empty\" && is_empty_commit $sha1 && ! is_merge_commit $sha1\n-\t\t\tthen\n-\t\t\t\tcomment_out=\"$comment_char \"\n-\t\t\telse\n-\t\t\t\tcomment_out=\n-\t\t\tfi\n+\t\tif test -z \"$keep_empty\" && is_empty_commit $sha1 && ! is_merge_commit $sha1\n+\t\tthen\n+\t\t\tcomment_out=\"$comment_char \"\n+\t\telse\n+\t\t\tcomment_out=\n+\t\tfi\n \n-\t\t\tif test -z \"$rebase_root\"\n-\t\t\tthen\n-\t\t\t\tpreserve=t\n-\t\t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n-\t\t\t\tdo\n-\t\t\t\t\tif test -f \"$rewritten\"/$p\n-\t\t\t\t\tthen\n-\t\t\t\t\t\tpreserve=f\n-\t\t\t\t\tfi\n-\t\t\t\tdone\n-\t\t\telse\n-\t\t\t\tpreserve=f\n-\t\t\tfi\n-\t\t\tif test f = \"$preserve\"\n-\t\t\tthen\n-\t\t\t\ttouch \"$rewritten\"/$sha1\n-\t\t\t\tprintf '%s\\n' \"${comment_out}pick $sha1 $rest\" >>\"$todo\"\n-\t\t\tfi\n-\t\tdone\n-\tfi\n+\t\tif test -z \"$rebase_root\"\n+\t\tthen\n+\t\t\tpreserve=t\n+\t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n+\t\t\tdo\n+\t\t\t\tif test -f \"$rewritten\"/$p\n+\t\t\t\tthen\n+\t\t\t\t\tpreserve=f\n+\t\t\t\tfi\n+\t\t\tdone\n+\t\telse\n+\t\t\tpreserve=f\n+\t\tfi\n+\t\tif test f = \"$preserve\"\n+\t\tthen\n+\t\t\ttouch \"$rewritten\"/$sha1\n+\t\t\tprintf '%s\\n' \"${comment_out}pick $sha1 $rest\" >>\"$todo\"\n+\t\tfi\n+\tdone\n \n \t# Watch for commits that been dropped by --cherry-pick\n-\tif test t = \"$preserve_merges\"\n-\tthen\n-\t\tmkdir \"$dropped\"\n-\t\t# Save all non-cherry-picked changes\n-\t\tgit rev-list $revisions --left-right --cherry-pick | \\\n-\t\t\tsed -n \"s/^>//p\" > \"$state_dir\"/not-cherry-picks\n-\t\t# Now all commits and note which ones are missing in\n-\t\t# not-cherry-picks and hence being dropped\n-\t\tgit rev-list $revisions |\n-\t\twhile read rev\n-\t\tdo\n-\t\t\tif test -f \"$rewritten\"/$rev &&\n-\t\t\t ! sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks >/dev/null\n-\t\t\tthen\n-\t\t\t\t# Use -f2 because if rev-list is telling us this commit is\n-\t\t\t\t# not worthwhile, we don't want to track its multiple heads,\n-\t\t\t\t# just the history of its first-parent for others that will\n-\t\t\t\t# be rebasing on top of it\n-\t\t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n-\t\t\t\tsha1=$(git rev-list -1 $rev)\n-\t\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n-\t\t\t\trm \"$rewritten\"/$rev\n-\t\t\tfi\n-\t\tdone\n-\tfi\n+\tmkdir \"$dropped\"\n+\t# Save all non-cherry-picked changes\n+\tgit rev-list $revisions --left-right --cherry-pick | \\\n+\t\tsed -n \"s/^>//p\" > \"$state_dir\"/not-cherry-picks\n+\t# Now all commits and note which ones are missing in\n+\t# not-cherry-picks and hence being dropped\n+\tgit rev-list $revisions |\n+\twhile read rev\n+\tdo\n+\t\tif test -f \"$rewritten\"/$rev &&\n+\t\t ! sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks >/dev/null\n+\t\tthen\n+\t\t\t# Use -f2 because if rev-list is telling us this commit is\n+\t\t\t# not worthwhile, we don't want to track its multiple heads,\n+\t\t\t# just the history of its first-parent for others that will\n+\t\t\t# be rebasing on top of it\n+\t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n+\t\t\tsha1=$(git rev-list -1 $rev)\n+\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n+\t\t\trm \"$rewritten\"/$rev\n+\t\tfi\n+\tdone\n \n \tcomplete_action\n }\n-- \n2.16.2\n\n"},{"id":"342693","messageId":"56ec4f8edf648917dc5cb11ecdf67b16e78d26ce.1521839546.git.wink@saville.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"[RFC PATCH v5 8/8] Remove merges_option and a blank line","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:25:29Z","receivedAt":"2018-03-23T21:26:12Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"merges_option is unused in git_rebase__interactive and always empty in\ngit_rebase__interactive__preserve_merges so it can be removed.\n\nSigned-off-by: Wink Saville \n---\n git-rebase--interactive.sh | 10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex ddbd126f2..50323fc27 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -961,8 +961,6 @@ git_rebase__interactive () {\n \tsetup_reflog_action\n \tinit_basic_state\n \n-\tmerges_option=\"--no-merges --cherry-pick\"\n-\n \tinit_revisions_and_shortrevisions\n \n \tgit rebase--helper --make-script ${keep_empty:+--keep-empty} \\\n@@ -996,22 +994,16 @@ git_rebase__interactive__preserve_merges () {\n \t\t\tdie \"$(gettext \"Could not init rewritten commits\")\"\n \tfi\n \n-\t# No cherry-pick because our first pass is to determine\n-\t# parents to rewrite and skipping dropped commits would\n-\t# prematurely end our probe\n-\tmerges_option=\n-\n \tinit_revisions_and_shortrevisions\n \n \tformat=$(git config --get rebase.instructionFormat)\n \t# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse\n-\tgit rev-list $merges_option --format=\"%m%H ${format:-%s}\" \\\n+\tgit rev-list --format=\"%m%H ${format:-%s}\" \\\n \t\t--reverse --left-right --topo-order \\\n \t\t$revisions ${restrict_revision+^$restrict_revision} | \\\n \t\tsed -n \"s/^>//p\" |\n \twhile read -r sha1 rest\n \tdo\n-\n \t\tif test -z \"$keep_empty\" && is_empty_commit $sha1 && ! is_merge_commit $sha1\n \t\tthen\n \t\t\tcomment_out=\"$comment_char \"\n-- \n2.16.2\n\n"},{"id":"342695","messageId":"CAKk8isqj3OusAE8OJtcys0a-Yj9fgQNn=DtLe-ZGYNzcKp=-3Q@mail.gmail.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T21:34:20Z","receivedAt":"2018-03-23T21:34:47Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville wrote:\n> Reworked patch 1 so that all of the backend scriptlets\n> used by git-rebase use a normal function style invocation.\n>\n> Merged the previous patch 2 and 3 have been squashed which\n> provides reviewers a little easier time to detect any changes\n> during extraction of the functions.\n>\n> Wink Saville (8):\n> rebase-interactive: simplify pick_on_preserving_merges\n> rebase: update invocation of rebase dot-sourced scripts\n> Indent function git_rebase__interactive\n> Extract functions out of git_rebase__interactive\n> Add and use git_rebase__interactive__preserve_merges\n> Remove unused code paths from git_rebase__interactive\n> Remove unused code paths from git_rebase__interactive__preserve_merges\n> Remove merges_option and a blank line\n>\n> git-rebase--am.sh | 11 --\n> git-rebase--interactive.sh | 407 ++++++++++++++++++++++++---------------------\n> git-rebase--merge.sh | 11 --\n> git-rebase.sh | 1 +\n> 4 files changed, 216 insertions(+), 214 deletions(-)\n>\n> --\n> 2.16.2\n>\n\nArgh, I misspelled Junio's email address, so when you reply-all try\nto remember to remove \"gister@pobox.com\" from the cc: list.\n\nSorry,\nWink\n"},{"id":"342700","messageId":"xmqqpo3uh26k.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"e893a9d550f4d09baf0d21adedca841b96feae0d.1521839546.git.wink@saville.com","subject":"Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T22:12:03Z","receivedAt":"2018-03-23T22:12:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wink Saville writes:\n\n> Signed-off-by: Wink Saville \n> ---\n> git-rebase--interactive.sh | 432 ++++++++++++++++++++++-----------------------\n> 1 file changed, 215 insertions(+), 217 deletions(-)\n\nThanks for separating this step out. \"git show -w --stat -p\" tells\nus that this is a pure re-indent patch pretty easily ;-).\n\nOverlong lines might want to get rewrapped at some point, and it is\nOK to do that either in this step or in a separate step.\n\n"},{"id":"342701","messageId":"xmqqin9mh1pq.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"baf0d9bab81bb3a80d0428c4aaa33cf32b3823e4.1521839546.git.wink@saville.com","subject":"Re: [RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T22:22:09Z","receivedAt":"2018-03-23T22:22:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wink Saville writes:\n\n> The extracted functions are:\n> - initiate_action\n> - setup_reflog_action\n> - init_basic_state\n> - init_revisions_and_shortrevisions\n> - complete_action\n>\n> Used by git_rebase__interactive\n>\n> Signed-off-by: Wink Saville \n> Helped-by: Junio C Hamano \n> Helped-by: Johannes Schindelin \n> ---\n\nI checked the correspondence of lines between the verison before and\nafter the patch, and did not spot anything suspicious. The fact\nthat we do not use \"local\" and stick to POSIX shell helps a bit, as\nwe do not have to worry about \"does this $variable in the split-out\nfunction refer to the same data as the original?\" ;-)\n\nWill queue.\n"},{"id":"342702","messageId":"xmqqefkah1gl.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"cover.1521839546.git.wink@saville.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T22:27:38Z","receivedAt":"2018-03-23T22:27:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wink Saville writes:\n\n> Wink Saville (8):\n> rebase-interactive: simplify pick_on_preserving_merges\n> rebase: update invocation of rebase dot-sourced scripts\n> Indent function git_rebase__interactive\n> Extract functions out of git_rebase__interactive\n> Add and use git_rebase__interactive__preserve_merges\n> Remove unused code paths from git_rebase__interactive\n> Remove unused code paths from git_rebase__interactive__preserve_merges\n> Remove merges_option and a blank line\n\nI felt that the structure of steps 5-7 that adds an identical copy\nfirst and then removes irrelevant parts from both copies was a bit\nunusual, but I do not think of a better structure I would use if I\nwere doing this series myself, and more importantly, the entire\nseries was a pleasant and straight-forward read.\n\nWill queue and wait for input from others.\n\nThanks.\n"},{"id":"342704","messageId":"xmqq7eq2h0wa.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"CAKk8isqj3OusAE8OJtcys0a-Yj9fgQNn=DtLe-ZGYNzcKp=-3Q@mail.gmail.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T22:39:49Z","receivedAt":"2018-03-23T22:39:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wink Saville writes:\n\n> On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville wrote:\n>> Reworked patch 1 so that all of the backend scriptlets\n>> used by git-rebase use a normal function style invocation.\n>>\n>> Merged the previous patch 2 and 3 have been squashed which\n>> provides reviewers a little easier time to detect any changes\n>> during extraction of the functions.\n>>\n>> Wink Saville (8):\n>> rebase-interactive: simplify pick_on_preserving_merges\n>> rebase: update invocation of rebase dot-sourced scripts\n>> Indent function git_rebase__interactive\n>> Extract functions out of git_rebase__interactive\n>> Add and use git_rebase__interactive__preserve_merges\n>> Remove unused code paths from git_rebase__interactive\n>> Remove unused code paths from git_rebase__interactive__preserve_merges\n>> Remove merges_option and a blank line\n>>\n>> git-rebase--am.sh | 11 --\n>> git-rebase--interactive.sh | 407 ++++++++++++++++++++++++---------------------\n>> git-rebase--merge.sh | 11 --\n>> git-rebase.sh | 1 +\n>> 4 files changed, 216 insertions(+), 214 deletions(-)\n>>\n>> --\n>> 2.16.2\n>>\n>\n> Argh, I misspelled Junio's email address, so when you reply-all try\n> to remember to remove \"gister@pobox.com\" from the cc: list.\n\nHeh, too late ;-)\n\nI queued everything (with all patch 3-8/8 retitled to share a\ncommon prefix, so that \"git shortlog\" output would stay sane)\nand I think I resolved the conflicts with Dscho's recreate-merges\ntopic correctly. Please double check what will appear on 'pu' later\ntoday.\n\nThanks.\n\n"},{"id":"342707","messageId":"CAKk8israKrrF4PBH4csLQDyrQXwap0oZ3FkihswR1DUf8nqrxQ@mail.gmail.com","threadId":"48123","inReplyTo":"xmqqpo3uh26k.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T22:52:04Z","receivedAt":"2018-03-23T22:52:31Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"On Fri, Mar 23, 2018 at 3:12 PM, Junio C Hamano wrote:\n> Wink Saville writes:\n>\n>> Signed-off-by: Wink Saville \n>> ---\n>> git-rebase--interactive.sh | 432 ++++++++++++++++++++++-----------------------\n>> 1 file changed, 215 insertions(+), 217 deletions(-)\n>\n> Thanks for separating this step out. \"git show -w --stat -p\" tells\n> us that this is a pure re-indent patch pretty easily ;-).\n>\n> Overlong lines might want to get rewrapped at some point, and it is\n> OK to do that either in this step or in a separate step.\n>\n\nThe longest line in the file before this change was line 532 which is 108\ncharacters, now there are three lines longer because of the indentation.\nLine 762 is 112, line 957 is 110 and 985 110.\n\nMy initial reaction is to leave these long lines as is, but if you want them\nshorter what is the maximum line length? At 80 characters per line\nI count about 25 lines will need to be shortened.\n\nAlso, I assume you want me to only change lines in\ngit_rebase__interactive.\n\n-- Wink\n"},{"id":"342708","messageId":"CAKk8isoJQrikitO7ezRajgphUXYR6207k4UkXP6r57WJEFBaDA@mail.gmail.com","threadId":"48123","inReplyTo":"xmqq7eq2h0wa.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-23T22:54:41Z","receivedAt":"2018-03-23T22:55:08Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"On Fri, Mar 23, 2018 at 3:39 PM, Junio C Hamano wrote:\n> Wink Saville writes:\n>\n>> On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville wrote:\n>>> Reworked patch 1 so that all of the backend scriptlets\n>>> used by git-rebase use a normal function style invocation.\n>>>\n>>> Merged the previous patch 2 and 3 have been squashed which\n>>> provides reviewers a little easier time to detect any changes\n>>> during extraction of the functions.\n>>>\n>>> Wink Saville (8):\n>>> rebase-interactive: simplify pick_on_preserving_merges\n>>> rebase: update invocation of rebase dot-sourced scripts\n>>> Indent function git_rebase__interactive\n>>> Extract functions out of git_rebase__interactive\n>>> Add and use git_rebase__interactive__preserve_merges\n>>> Remove unused code paths from git_rebase__interactive\n>>> Remove unused code paths from git_rebase__interactive__preserve_merges\n>>> Remove merges_option and a blank line\n>>>\n>>> git-rebase--am.sh | 11 --\n>>> git-rebase--interactive.sh | 407 ++++++++++++++++++++++++---------------------\n>>> git-rebase--merge.sh | 11 --\n>>> git-rebase.sh | 1 +\n>>> 4 files changed, 216 insertions(+), 214 deletions(-)\n>>>\n>>> --\n>>> 2.16.2\n>>>\n>>\n>> Argh, I misspelled Junio's email address, so when you reply-all try\n>> to remember to remove \"gister@pobox.com\" from the cc: list.\n>\n> Heh, too late ;-)\n>\n> I queued everything (with all patch 3-8/8 retitled to share a\n> common prefix, so that \"git shortlog\" output would stay sane)\n> and I think I resolved the conflicts with Dscho's recreate-merges\n> topic correctly. Please double check what will appear on 'pu' later\n> today.\n>\n> Thanks.\n>\n\nOK, thank you!\n"},{"id":"342709","messageId":"xmqq370qgzod.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"CAKk8israKrrF4PBH4csLQDyrQXwap0oZ3FkihswR1DUf8nqrxQ@mail.gmail.com","subject":"Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T23:06:10Z","receivedAt":"2018-03-23T23:06:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wink Saville writes:\n\n> Also, I assume you want me to only change lines in\n> git_rebase__interactive.\n\nI actually do not care if line-wrapping is done; it is perfectly\nfine to leave it for future clean-up and leave it outside the scope\nof this series. If you are going to do as a part of the series,\nyes, I do prefer you limit yourself to those lines that are involved\nin the series in some other way.\n\nThanks.\n"},{"id":"342711","messageId":"CAKk8ispE3o6XA=1oZ+CtuLrZPB8cuCpC--ZPM9ifUja=aPJufw@mail.gmail.com","threadId":"48123","inReplyTo":"xmqq370qgzod.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-24T00:01:19Z","receivedAt":"2018-03-24T00:01:48Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"> I actually do not care if line-wrapping is done; it is perfectly\n> fine to leave it for future clean-up and leave it outside the scope\n> of this series. If you are going to do as a part of the series,\n> yes, I do prefer you limit yourself to those lines that are involved\n> in the series in some other way.\n\nThen lets leave the long lines alone for this series.\n"},{"id":"342723","messageId":"CAKk8ispSgNgZxS7KfuOyxfU53tzesvNyLRaNXFZa3K7SCbaRkQ@mail.gmail.com","threadId":"48123","inReplyTo":"CAKk8isoJQrikitO7ezRajgphUXYR6207k4UkXP6r57WJEFBaDA@mail.gmail.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-24T05:36:49Z","receivedAt":"2018-03-24T05:37:16Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":">> I queued everything (with all patch 3-8/8 retitled to share a\n>> common prefix, so that \"git shortlog\" output would stay sane)\n>> and I think I resolved the conflicts with Dscho's recreate-merges\n>> topic correctly. Please double check what will appear on 'pu' later\n>> today.\n>>\n>> Thanks.\n>>\n>\n> OK, thank you!\n\nI looked at 'pu' and it LGTM, so I pushed 'pu' as a branch to my\ngithub account to test with Travis-CI. All the linux builds are\ngreen, but the 2 OSX builds are red[1] and the logs show compile\nerrors:\n\n CC ident.o\n CC json-writer.o\n\njson-writer.c:123:38: error: format specifies type 'uintmax_t' (aka\n'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned\nlong long') [-Werror,-Wformat]\n\n strbuf_addf(&jw->json, \":%\"PRIuMAX, value);\n ~~ ^~~~~\njson-writer.c:228:37: error: format specifies type 'uintmax_t' (aka\n'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned\nlong long') [-Werror,-Wformat] [0m\n\n strbuf_addf(&jw->json, \"%\"PRIuMAX, value);\n ~~ ^~~~~\n2 errors generated.\nmake: *** [json-writer.o] Error 1\nmake: *** Waiting for unfinished jobs....\n\n[RFC Patch 1/1] changes the PRIuMax to PRIu64 to correct the compile error\nand now Travis-CI is green [2].\n\n[1]: https://travis-ci.org/winksaville/git/builds/357660624\n[2]: https://travis-ci.org/winksaville/git/builds/357681929\n\n-- Wink\n"},{"id":"342743","messageId":"CAPig+cQO78r9ym-Tq7Qfheq6OyjDFQB=SngXiGGVmh23piv=7A@mail.gmail.com","threadId":"48123","inReplyTo":"baf0d9bab81bb3a80d0428c4aaa33cf32b3823e4.1521839546.git.wink@saville.com","subject":"Re: [RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-24T07:20:04Z","receivedAt":"2018-03-24T07:20:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 23, 2018 at 5:25 PM, Wink Saville wrote:\n> The extracted functions are:\n> [...]\n> Signed-off-by: Wink Saville \n> ---\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> @@ -740,8 +740,20 @@ get_missing_commit_check_level () {\n> +# Initiate an action. If the cannot be any\n\ns/the/there/\n\n> +# further action it may exec a command\n\ns/\\s+/ /\n\n> +# or exit and not return.\n> +#\n> +# TODO: Consider a cleaner return model so it\n> +# never exits and always return 0 if process\n> +# is complete.\n> +#\n> +# Parameter 1 is the action to initiate.\n> +#\n> +# Returns 0 if the action was able to complete\n> +# and if 1 if further processing is required.\n> +initiate_action () {\n"},{"id":"343015","messageId":"xmqqzi2ude4w.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"CAKk8ispSgNgZxS7KfuOyxfU53tzesvNyLRaNXFZa3K7SCbaRkQ@mail.gmail.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-26T15:56:31Z","receivedAt":"2018-03-26T15:56:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wink Saville writes:\n\n> json-writer.c:123:38: error: format specifies type 'uintmax_t' (aka\n> 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned\n> long long') [-Werror,-Wformat]\n>\n> strbuf_addf(&jw->json, \":%\"PRIuMAX, value);\n> ~~ ^~~~~\n> json-writer.c:228:37: error: format specifies type 'uintmax_t' (aka\n> 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned\n> long long') [-Werror,-Wformat] [0m\n>\n> strbuf_addf(&jw->json, \"%\"PRIuMAX, value);\n> ~~ ^~~~~\n> 2 errors generated.\n> make: *** [json-writer.o] Error 1\n> make: *** Waiting for unfinished jobs....\n\nFor whatever reason, our codebase seems to shy away from PRIu64,\neven though there are liberal uses of PRIu32. Showing the value\ncasted to uintmax_t with PRIuMAX seems to be our preferred way to\nsay \"We cannot say how wide this type is on different platforms, and\nare playing safe by using widest-possible int type\" (e.g. showing a\npid_t value from daemon.c).\n\nIn this codepath, the actual values are specified to be uint64_t, so\nthe use of PRIu64 may be OK, but I have to wonder why the codepath\nis not dealing with uintmax_t in the first place. When even larger\nthan present archs are prevalent in N years and 64-bit starts to\nfeel a tad small (like we feel for 16-bit ints these days), it will\nfeel a bit silly to have a subsystem that is limited to such a\n\"fixed and a tad small these days\" types and pretend it to be be a\ngeneric seriealizer, I suspect.\n"},{"id":"343024","messageId":"9ca76d31-828d-0b6f-5069-375792c1f55d@jeffhostetler.com","threadId":"48123","inReplyTo":"xmqqzi2ude4w.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-03-26T17:01:50Z","receivedAt":"2018-03-26T17:01:57Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/26/2018 11:56 AM, Junio C Hamano wrote:\n> Wink Saville writes:\n> \n>> json-writer.c:123:38: error: format specifies type 'uintmax_t' (aka\n>> 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned\n>> long long') [-Werror,-Wformat]\n>>\n>> strbuf_addf(&jw->json, \":%\"PRIuMAX, value);\n>> ~~ ^~~~~\n>> json-writer.c:228:37: error: format specifies type 'uintmax_t' (aka\n>> 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned\n>> long long') [-Werror,-Wformat] [0m\n>>\n>> strbuf_addf(&jw->json, \"%\"PRIuMAX, value);\n>> ~~ ^~~~~\n>> 2 errors generated.\n>> make: *** [json-writer.o] Error 1\n>> make: *** Waiting for unfinished jobs....\n> \n> For whatever reason, our codebase seems to shy away from PRIu64,\n> even though there are liberal uses of PRIu32. Showing the value\n> casted to uintmax_t with PRIuMAX seems to be our preferred way to\n> say \"We cannot say how wide this type is on different platforms, and\n> are playing safe by using widest-possible int type\" (e.g. showing a\n> pid_t value from daemon.c).\n> \n> In this codepath, the actual values are specified to be uint64_t, so\n> the use of PRIu64 may be OK, but I have to wonder why the codepath\n> is not dealing with uintmax_t in the first place. When even larger\n> than present archs are prevalent in N years and 64-bit starts to\n> feel a tad small (like we feel for 16-bit ints these days), it will\n> feel a bit silly to have a subsystem that is limited to such a\n> \"fixed and a tad small these days\" types and pretend it to be be a\n> generic seriealizer, I suspect.\n> \n\nI defined that routine to take a uint64_t because I wanted to\npass a nanosecond value received from getnanotime() and that's\nwhat it returns.\n\nMy preference would be to change the PRIuMAX to PRIu64, but there\naren't any other references in the code to that symbol and I didn't\nwant to start a new trend here.\n\nI am concerned that the above compiler error message says that uintmax_t\nis defined as an \"unsigned long\" (which is defined as *at least* 32 bits,\nbut not necessarily 64. But a uint64_t is defined as a \"unsigned long long\"\nand guaranteed as a 64 bit value.\n\nSo while I'm not really worried about 128 bit integers right now, I'm\nmore concerned about 32 bit compilers truncating that value without any\nwarnings.\n\nJeff\n\n"},{"id":"343038","messageId":"xmqqh8p2d8jh.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"9ca76d31-828d-0b6f-5069-375792c1f55d@jeffhostetler.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-26T17:57:22Z","receivedAt":"2018-03-26T17:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> I defined that routine to take a uint64_t because I wanted to\n> pass a nanosecond value received from getnanotime() and that's\n> what it returns.\n\nHmph, but the target format does not have different representation\nof inttypes in different sizes, no? \n\nI personally doubt that we would benefit from having a group of\nfunctions (i.e. format_int{8,16,32,64}_to_json()) that callers have\nto choose from, depending on the exact size of the integer they want\nto serialize. The de-serializing side would be the same story.\n\nEven if the variable a potential caller of the formetter is a sized\ntype that is different from uintmax_t, the caller shouldn't have to\nadd an extra cast.\n\nAm I missing some obvious merit for having these separate functions\nfor explicit sizes?\n"},{"id":"343039","messageId":"xmqqd0zqd8dw.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"9ca76d31-828d-0b6f-5069-375792c1f55d@jeffhostetler.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-26T18:00:43Z","receivedAt":"2018-03-26T18:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> I am concerned that the above compiler error message says that uintmax_t\n> is defined as an \"unsigned long\" (which is defined as *at least* 32 bits,\n> but not necessarily 64. But a uint64_t is defined as a \"unsigned long long\"\n> and guaranteed as a 64 bit value.\n\nOn a platform whose uintmax_t is u32, is it realistic to expect that\nwe would be able to use u64, even if we explicitly ask for it, in\nthe first place?\n\nIn other words, on a platform that handles uint64_t, I would expect\nuintmax_t to be wide enough to hold an uint64_t value without\ntruncation.\n"},{"id":"343042","messageId":"d56a60a8-e735-b147-a2e6-4e48461ad701@jeffhostetler.com","threadId":"48123","inReplyTo":"xmqqh8p2d8jh.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-03-26T18:22:39Z","receivedAt":"2018-03-26T18:22:47Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/26/2018 1:57 PM, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n> \n>> I defined that routine to take a uint64_t because I wanted to\n>> pass a nanosecond value received from getnanotime() and that's\n>> what it returns.\n> \n> Hmph, but the target format does not have different representation\n> of inttypes in different sizes, no?\n> \n> I personally doubt that we would benefit from having a group of\n> functions (i.e. format_int{8,16,32,64}_to_json()) that callers have\n> to choose from, depending on the exact size of the integer they want\n> to serialize. The de-serializing side would be the same story.\n> \n> Even if the variable a potential caller of the formetter is a sized\n> type that is different from uintmax_t, the caller shouldn't have to\n> add an extra cast.\n> \n> Am I missing some obvious merit for having these separate functions\n> for explicit sizes?\n> \n\nI did the uint64_t for the unsigned ns times.\n\nI did the other one for the usual signed ints.\n\nI could convert them both to a single signed 64 bit typed function\nif we only want to have one function.\n\nJeff\n\n"},{"id":"343046","messageId":"3d845e99-e392-a62f-b83e-33b58482fc54@jeffhostetler.com","threadId":"48123","inReplyTo":"xmqqd0zqd8dw.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-03-26T18:33:04Z","receivedAt":"2018-03-26T18:33:13Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/26/2018 2:00 PM, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n> \n>> I am concerned that the above compiler error message says that uintmax_t\n>> is defined as an \"unsigned long\" (which is defined as *at least* 32 bits,\n>> but not necessarily 64. But a uint64_t is defined as a \"unsigned long long\"\n>> and guaranteed as a 64 bit value.\n> \n> On a platform whose uintmax_t is u32, is it realistic to expect that\n> we would be able to use u64, even if we explicitly ask for it, in\n> the first place?\n> \n> In other words, on a platform that handles uint64_t, I would expect\n> uintmax_t to be wide enough to hold an uint64_t value without\n> truncation.\n> \n\nI was just going by what the reported compiler error message was.\nIt said that \"unsigned long\" didn't match the uint64_t variable.\nAnd that made me nervous.\n\nIf all of the platforms we build on define uintmax_t >= 64 bits,\nthen it doesn't matter.\n\nIf we do have a platform where uintmax_t is u32, then we'll have a\nlot more breakage than in just the new function I added.\n\nThanks,\nJeff\n"},{"id":"343050","messageId":"CAKk8isp_qx1ajgRryhBw6TYBoaa8fJU6hP3JyUWAx20knQSLXA@mail.gmail.com","threadId":"48123","inReplyTo":"3d845e99-e392-a62f-b83e-33b58482fc54@jeffhostetler.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2018-03-26T18:43:46Z","receivedAt":"2018-03-26T18:44:13Z","isPatch":true,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"> I was just going by what the reported compiler error message was.\n> It said that \"unsigned long\" didn't match the uint64_t variable.\n> And that made me nervous.\n>\n> If all of the platforms we build on define uintmax_t >= 64 bits,\n> then it doesn't matter.\n>\n> If we do have a platform where uintmax_t is u32, then we'll have a\n> lot more breakage than in just the new function I added.\n>\n> Thanks,\n> Jeff\n\nShould we add a \"_Static_assert\" that sizeof(uintmax_t) >= sizeof(uint64_t) ?\n"},{"id":"343057","messageId":"xmqqtvt2bpcb.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"CAKk8isp_qx1ajgRryhBw6TYBoaa8fJU6hP3JyUWAx20knQSLXA@mail.gmail.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-26T19:37:24Z","receivedAt":"2018-03-26T19:37:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wink Saville writes:\n\n> Should we add a \"_Static_assert\" that sizeof(uintmax_t) >= sizeof(uint64_t) ?\n\nIf that expression compiles, then both types are understood by the\nplatform. Because\n\nhttp://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html\n\ntells us:\n\n Greatest-width integer types\n\n The following type designates a signed integer type capable of\n representing any value of any signed integer type: intmax_t\n\n The following type designates an unsigned integer type capable of\n representing any value of any unsigned integer type: uintmax_t\n\n These types are required.\n\nwe know what that expression evaluates to, no?\n\n"},{"id":"343084","messageId":"nycvar.QRO.7.76.6.1803270034040.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48123","inReplyTo":"CAKk8isp_qx1ajgRryhBw6TYBoaa8fJU6hP3JyUWAx20knQSLXA@mail.gmail.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-26T22:35:25Z","receivedAt":"2018-03-26T22:35:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi all,\n\nOn Mon, 26 Mar 2018, Wink Saville wrote:\n\n> > I was just going by what the reported compiler error message was.\n> > It said that \"unsigned long\" didn't match the uint64_t variable.\n> > And that made me nervous.\n> >\n> > If all of the platforms we build on define uintmax_t >= 64 bits,\n> > then it doesn't matter.\n> >\n> > If we do have a platform where uintmax_t is u32, then we'll have a\n> > lot more breakage than in just the new function I added.\n> >\n> > Thanks,\n> > Jeff\n> \n> Should we add a \"_Static_assert\" that sizeof(uintmax_t) >= sizeof(uint64_t) ?\n\nTo come back to the subject that all of the mails in this here mail thread\nof you gentle people bear: Wink's patches look good to me, and I would\nlike to have them be bumped down the next/master waterfall.\n\nCiao,\nDscho\n"},{"id":"343096","messageId":"xmqqy3ie9kdv.fsf@gitster-ct.c.googlers.com","threadId":"48123","inReplyTo":"d56a60a8-e735-b147-a2e6-4e48461ad701@jeffhostetler.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-27T05:07:24Z","receivedAt":"2018-03-27T05:07:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> I did the uint64_t for the unsigned ns times.\n>\n> I did the other one for the usual signed ints.\n>\n> I could convert them both to a single signed 64 bit typed function\n> if we only want to have one function.\n\nI still think having sized version is a horrible idea, and recommend\ninstrad to use \"the widest possible on the platform\" type, for the\nsame reason why you would only have variant for double but not for\nfloat.\n\nintmax and uintmax are by definition wide enough to hold any value\nthat would fit in any integral type the platform supports, so if a\ncaller that wants to handle 64-bit unsigned timestamp for example\nuses uint64_t variable to pass such a timestamp around, and the\nplatform is capable of groking that code, you should be able to\nsafely pass that to json serializer you are writing that takes\nuintmax_t just fine, and (1) your caller that passes around uint64_t\ntimestamps won't compile on a platform that is incapable of doing\n64-bit and you have bigger problem than uintmax_t being narrower\nthan 64-bit on such a platform, and (2) your caller can just pass\nuint64_t value to your JSON formatter that expects uintmax_t without\nexplicit casting, as normal integral type promotion rule would\napply.\n\nSo I would think it is most sensible to have double, uintmax_t and\nintmax_t variants. If you do not care about the extra value range\nthat unsigned integral types afford, a single intmax_t variant would\nalso be fine.\n\n"},{"id":"343117","messageId":"f655f5d3-9a8a-ead7-04b2-eda3c14ed8f5@jeffhostetler.com","threadId":"48123","inReplyTo":"xmqqy3ie9kdv.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5 0/8] rebase-interactive","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-03-27T10:03:28Z","receivedAt":"2018-03-27T10:03:33Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/27/2018 1:07 AM, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n[...]\n> So I would think it is most sensible to have double, uintmax_t and\n> intmax_t variants. If you do not care about the extra value range\n> that unsigned integral types afford, a single intmax_t variant would\n> also be fine.\n\nI'll reroll with just the double and intmax_t variants.\nThanks for the feedback and sorry for all the noise.\n\nJeff\n\n"}]}