{"thread":{"id":"46181","subject":"[PATCH v5 00/10] The final building block for a faster rebase -i","startedAt":"2017-06-14T13:07:17Z","lastAt":"2017-06-20T02:35:19Z","messageCount":20,"participants":["Johannes Schindelin","Junio C Hamano","Liam Beguin","Jeff King"],"isPatch":true,"patchVersion":5,"patchTotal":10},"messages":[{"id":"322248","messageId":"cover.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":null,"subject":"[PATCH v5 00/10] The final building block for a faster rebase -i","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:06:58Z","receivedAt":"2017-06-14T13:07:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series reimplements the expensive pre- and post-processing of\nthe todo script in C.\n\nAnd it concludes the work I did to accelerate rebase -i so far.\n\nI am still unwilling to replace a compile-time safe way to pass the\noptions to the revision machinery by the alternative (which I am still\nflabbergasted about) proposed by Junio. This will not change.\n\nChanges since v4:\n\n- replaced the \"sha1s\" part of the names by \"ids\", to reflect the\n current effort to move away from the cryptographically unsafe SHA-1\n\n- replaced the confusing term \"instruction sheet\" in an error message by\n the more commonly used \"todo list\"\n\n\nJohannes Schindelin (10):\n t3415: verify that an empty instructionFormat is handled as before\n rebase -i: generate the script via rebase--helper\n rebase -i: remove useless indentation\n rebase -i: do not invent onelines when expanding/collapsing SHA-1s\n rebase -i: also expand/collapse the SHA-1s via the rebase--helper\n t3404: relax rebase.missingCommitsCheck tests\n rebase -i: check for missing commits in the rebase--helper\n rebase -i: skip unnecessary picks using the rebase--helper\n t3415: test fixup with wrapped oneline\n rebase -i: rearrange fixup/squash lines using the rebase--helper\n\n Documentation/git-rebase.txt | 16 +-\n builtin/rebase--helper.c | 29 ++-\n git-rebase--interactive.sh | 373 ++++-------------------------\n sequencer.c | 530 ++++++++++++++++++++++++++++++++++++++++++\n sequencer.h | 8 +\n t/t3404-rebase-interactive.sh | 22 +-\n t/t3415-rebase-autosquash.sh | 28 ++-\n 7 files changed, 646 insertions(+), 360 deletions(-)\n\n\nbase-commit: 02a2850ad58eff6de70eb2dc5f96345c463857ac\nBased-On: rebase--helper at https://github.com/dscho/git\nFetch-Base-Via: git fetch https://github.com/dscho/git rebase--helper\nPublished-As: https://github.com/dscho/git/releases/tag/rebase-i-extra-v5\nFetch-It-Via: git fetch https://github.com/dscho/git rebase-i-extra-v5\n\nInterdiff vs v4:\n diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\n index e6591f01112..64b36d429fa 100644\n --- a/builtin/rebase--helper.c\n +++ b/builtin/rebase--helper.c\n @@ -25,9 +25,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\t\tABORT),\n \t\tOPT_CMDMODE(0, \"make-script\", &command,\n \t\t\tN_(\"make rebase script\"), MAKE_SCRIPT),\n -\t\tOPT_CMDMODE(0, \"shorten-sha1s\", &command,\n +\t\tOPT_CMDMODE(0, \"shorten-ids\", &command,\n \t\t\tN_(\"shorten SHA-1s in the todo list\"), SHORTEN_SHA1S),\n -\t\tOPT_CMDMODE(0, \"expand-sha1s\", &command,\n +\t\tOPT_CMDMODE(0, \"expand-ids\", &command,\n \t\t\tN_(\"expand SHA-1s in the todo list\"), EXPAND_SHA1S),\n \t\tOPT_CMDMODE(0, \"check-todo-list\", &command,\n \t\t\tN_(\"check the todo list\"), CHECK_TODO_LIST),\n diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n index 71d190766cf..3b0340e7cc9 100644\n --- a/git-rebase--interactive.sh\n +++ b/git-rebase--interactive.sh\n @@ -714,11 +714,11 @@ do_rest () {\n }\n \n expand_todo_ids() {\n -\tgit rebase--helper --expand-sha1s\n +\tgit rebase--helper --expand-ids\n }\n \n collapse_todo_ids() {\n -\tgit rebase--helper --shorten-sha1s\n +\tgit rebase--helper --shorten-ids\n }\n \n # Add commands after a pick or after a squash/fixup serie\n diff --git a/sequencer.c b/sequencer.c\n index 6373f20a019..06c97e12267 100644\n --- a/sequencer.c\n +++ b/sequencer.c\n @@ -2464,7 +2464,7 @@ int sequencer_make_script(int keep_empty, FILE *out,\n }\n \n \n -int transform_todo_ids(int shorten_sha1s)\n +int transform_todo_ids(int shorten_ids)\n {\n \tconst char *todo_file = rebase_path_todo();\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n @@ -2484,7 +2484,7 @@ int transform_todo_ids(int shorten_sha1s)\n \tres = parse_insn_buffer(todo_list.buf.buf, &todo_list);\n \tif (res) {\n \t\ttodo_list_release(&todo_list);\n -\t\treturn error(_(\"unusable instruction sheet: '%s'\"), todo_file);\n +\t\treturn error(_(\"unusable todo list: '%s'\"), todo_file);\n \t}\n \n \tout = fopen(todo_file, \"w\");\n @@ -2503,7 +2503,7 @@ int transform_todo_ids(int shorten_sha1s)\n \t\tif (item->command >= TODO_EXEC && item->command != TODO_DROP)\n \t\t\tfwrite(p, eol - bol, 1, out);\n \t\telse {\n -\t\t\tconst char *sha1 = shorten_sha1s ?\n +\t\t\tconst char *id = shorten_ids ?\n \t\t\t\tshort_commit_name(item->commit) :\n \t\t\t\toid_to_hex(&item->commit->object.oid);\n \t\t\tint len;\n @@ -2512,7 +2512,7 @@ int transform_todo_ids(int shorten_sha1s)\n \t\t\tlen = strcspn(p, \" \\t\"); /* length of command */\n \n \t\t\tfprintf(out, \"%.*s %s %.*s\\n\",\n -\t\t\t\tlen, p, sha1, item->arg_len, item->arg);\n +\t\t\t\tlen, p, id, item->arg_len, item->arg);\n \t\t}\n \t}\n \tfclose(out);\n @@ -2762,9 +2762,9 @@ static int subject2item_cmp(const struct subject2item_entry *a,\n }\n \n /*\n - * Rearrange the todo list that has both \"pick sha1 msg\" and \"pick sha1\n - * fixup!/squash! msg\" in it so that the latter is put immediately after the\n - * former, and change \"pick\" to \"fixup\"/\"squash\".\n + * Rearrange the todo list that has both \"pick commit-id msg\" and \"pick\n + * commit-id fixup!/squash! msg\" in it so that the latter is put immediately\n + * after the former, and change \"pick\" to \"fixup\"/\"squash\".\n *\n * Note that if the config has specified a custom instruction format, each log\n * message will have to be retrieved from the commit (as the oneline in the\n diff --git a/sequencer.h b/sequencer.h\n index 1c94bec7622..6f3d3df82c0 100644\n --- a/sequencer.h\n +++ b/sequencer.h\n @@ -48,7 +48,7 @@ int sequencer_remove_state(struct replay_opts *opts);\n int sequencer_make_script(int keep_empty, FILE *out,\n \t\tint argc, const char **argv);\n \n -int transform_todo_ids(int shorten_sha1s);\n +int transform_todo_ids(int shorten_ids);\n int check_todo_list(void);\n int skip_unnecessary_picks(void);\n int rearrange_squash(void);\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n"},{"id":"322249","messageId":"7cdf532fe2d1798a5371dcbb07ee13857f161c3a.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 02/10] rebase -i: generate the script via rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:07:08Z","receivedAt":"2017-06-14T13:07:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The first step of an interactive rebase is to generate the so-called \"todo\nscript\", to be stored in the state directory as \"git-rebase-todo\" and to\nbe edited by the user.\n\nOriginally, we adjusted the output of `git log ` using a simple\nsed script. Over the course of the years, the code became more\ncomplicated. We now use shell scripting to edit the output of `git log`\nconditionally, depending whether to keep \"empty\" commits (i.e. commits\nthat do not change any files).\n\nOn platforms where shell scripting is not native, this can be a serious\ndrag. And it opens the door for incompatibilities between platforms when\nit comes to shell scripting or to Unix-y commands.\n\nLet's just re-implement the todo script generation in plain C, using the\nrevision machinery directly.\n\nThis is substantially faster, improving the speed relative to the\nshell script version of the interactive rebase from 2x to 3x on Windows.\n\nNote that the rearrange_squash() function in git-rebase--interactive\nrelied on the fact that we set the \"format\" variable to the config setting\nrebase.instructionFormat. Relying on a side effect like this is no good,\nhence we explicitly perform that assignment (possibly again) in\nrearrange_squash().\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase--helper.c | 8 +++++++-\n git-rebase--interactive.sh | 44 +++++++++++++++++++++--------------------\n sequencer.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 82 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex ca1ebb2fa18..821058d452d 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -11,15 +11,19 @@ static const char * const builtin_rebase_helper_usage[] = {\n int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n+\tint keep_empty = 0;\n \tenum {\n-\t\tCONTINUE = 1, ABORT\n+\t\tCONTINUE = 1, ABORT, MAKE_SCRIPT\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n+\t\tOPT_BOOL(0, \"keep-empty\", &keep_empty, N_(\"keep empty commits\")),\n \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n \t\t\t\tCONTINUE),\n \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\n \t\t\t\tABORT),\n+\t\tOPT_CMDMODE(0, \"make-script\", &command,\n+\t\t\tN_(\"make rebase script\"), MAKE_SCRIPT),\n \t\tOPT_END()\n \t};\n \n@@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!sequencer_continue(&opts);\n \tif (command == ABORT && argc == 1)\n \t\treturn !!sequencer_remove_state(&opts);\n+\tif (command == MAKE_SCRIPT && argc > 1)\n+\t\treturn !!sequencer_make_script(keep_empty, stdout, argc, argv);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 90b1fbe9cf6..05766835de1 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -785,6 +785,7 @@ collapse_todo_ids() {\n # each log message will be re-retrieved in order to normalize the\n # autosquash arrangement\n rearrange_squash () {\n+\tformat=$(git config --get rebase.instructionFormat)\n \t# extract fixup!/squash! lines and resolve any referenced sha1's\n \twhile read -r pick sha1 message\n \tdo\n@@ -1210,26 +1211,27 @@ else\n \trevisions=$onto...$orig_head\n \tshortrevisions=$shorthead\n fi\n-format=$(git config --get rebase.instructionFormat)\n-# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse\n-git rev-list $merges_option --format=\"%m%H ${format:-%s}\" \\\n-\t--reverse --left-right --topo-order \\\n-\t$revisions ${restrict_revision+^$restrict_revision} | \\\n-\tsed -n \"s/^>//p\" |\n-while read -r sha1 rest\n-do\n-\n-\tif test -z \"$keep_empty\" && is_empty_commit $sha1 && ! is_merge_commit $sha1\n-\tthen\n-\t\tcomment_out=\"$comment_char \"\n-\telse\n-\t\tcomment_out=\n-\tfi\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+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 \n-\tif test t != \"$preserve_merges\"\n-\tthen\n-\t\tprintf '%s\\n' \"${comment_out}pick $sha1 $rest\" >>\"$todo\"\n-\telse\n \t\tif test -z \"$rebase_root\"\n \t\tthen\n \t\t\tpreserve=t\n@@ -1248,8 +1250,8 @@ do\n \t\t\ttouch \"$rewritten\"/$sha1\n \t\t\tprintf '%s\\n' \"${comment_out}pick $sha1 $rest\" >>\"$todo\"\n \t\tfi\n-\tfi\n-done\n+\tdone\n+fi\n \n # Watch for commits that been dropped by --cherry-pick\n if test t = \"$preserve_merges\"\ndiff --git a/sequencer.c b/sequencer.c\nindex 5282fb849c5..133d9aa7c74 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2412,3 +2412,52 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n \n \tstrbuf_release(&sob);\n }\n+\n+int sequencer_make_script(int keep_empty, FILE *out,\n+\t\tint argc, const char **argv)\n+{\n+\tchar *format = NULL;\n+\tstruct pretty_print_context pp = {0};\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct rev_info revs;\n+\tstruct commit *commit;\n+\n+\tinit_revisions(&revs, NULL);\n+\trevs.verbose_header = 1;\n+\trevs.max_parents = 1;\n+\trevs.cherry_pick = 1;\n+\trevs.limited = 1;\n+\trevs.reverse = 1;\n+\trevs.right_only = 1;\n+\trevs.sort_order = REV_SORT_IN_GRAPH_ORDER;\n+\trevs.topo_order = 1;\n+\n+\trevs.pretty_given = 1;\n+\tgit_config_get_string(\"rebase.instructionFormat\", &format);\n+\tif (!format || !*format) {\n+\t\tfree(format);\n+\t\tformat = xstrdup(\"%s\");\n+\t}\n+\tget_commit_format(format, &revs);\n+\tfree(format);\n+\tpp.fmt = revs.commit_format;\n+\tpp.output_encoding = get_log_output_encoding();\n+\n+\tif (setup_revisions(argc, argv, &revs, NULL) > 1)\n+\t\treturn error(_(\"make_script: unhandled options\"));\n+\n+\tif (prepare_revision_walk(&revs) < 0)\n+\t\treturn error(_(\"make_script: error preparing revisions\"));\n+\n+\twhile ((commit = get_revision(&revs))) {\n+\t\tstrbuf_reset(&buf);\n+\t\tif (!keep_empty && is_original_commit_empty(commit))\n+\t\t\tstrbuf_addf(&buf, \"%c \", comment_line_char);\n+\t\tstrbuf_addf(&buf, \"pick %s \", oid_to_hex(&commit->object.oid));\n+\t\tpretty_print_commit(&pp, commit, &buf);\n+\t\tstrbuf_addch(&buf, '\\n');\n+\t\tfputs(buf.buf, out);\n+\t}\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex f885b68395f..83f2943b7a9 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -45,6 +45,9 @@ int sequencer_continue(struct replay_opts *opts);\n int sequencer_rollback(struct replay_opts *opts);\n int sequencer_remove_state(struct replay_opts *opts);\n \n+int sequencer_make_script(int keep_empty, FILE *out,\n+\t\tint argc, const char **argv);\n+\n extern const char sign_off_header[];\n \n void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322250","messageId":"5ab90ad1de6ece32c959115b046ab303503cd1ae.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 01/10] t3415: verify that an empty instructionFormat is handled as before","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:07:04Z","receivedAt":"2017-06-14T13:07:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"An upcoming patch will move the todo list generation into the\nrebase--helper. An early version of that patch regressed on an empty\nrebase.instructionFormat value (the shell version could not discern\nbetween an empty one and a non-existing one, but the C version used the\nempty one as if that was intended to skip the oneline from the `pick\n` lines).\n\nLet's verify that this still works as before.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3415-rebase-autosquash.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 5848949ec37..6d99f624b62 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -271,6 +271,18 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '\n \ttest 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)\n '\n \n+test_expect_success 'autosquash with empty custom instructionFormat' '\n+\tgit reset --hard base &&\n+\ttest_commit empty-instructionFormat-test &&\n+\t(\n+\t\tset_cat_todo_editor &&\n+\t\ttest_must_fail git -c rebase.instructionFormat= \\\n+\t\t\trebase --autosquash --force -i HEAD^ >actual &&\n+\t\tgit log -1 --format=\"pick %h %s\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n set_backup_editor () {\n \twrite_script backup-editor.sh <<-\\EOF\n \tcp \"$1\" .git/backup-\"$(basename \"$1\")\"\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322251","messageId":"c7af03867e96e6ce6bf0a7bb858f6018513a4341.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 03/10] rebase -i: remove useless indentation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:07:12Z","receivedAt":"2017-06-14T13:07:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The commands used to be indented, and it is nice to look at, but when we\ntransform the SHA-1s, the indentation is removed. So let's do away with it.\n\nFor the moment, at least: when we will use the upcoming rebase--helper\nto transform the SHA-1s, we *will* keep the indentation and can\nreintroduce it. Yet, to be able to validate the rebase--helper against\nthe output of the current shell script version, we need to remove the\nextra indentation.\n\nSigned-off-by: Johannes Schindelin \n---\n git-rebase--interactive.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 05766835de1..93372c62b2e 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -155,13 +155,13 @@ reschedule_last_action () {\n append_todo_help () {\n \tgettext \"\n Commands:\n- p, pick = use commit\n- r, reword = use commit, but edit the commit message\n- e, edit = use commit, but stop for amending\n- s, squash = use commit, but meld into previous commit\n- f, fixup = like \\\"squash\\\", but discard this commit's log message\n- x, exec = run command (the rest of the line) using shell\n- d, drop = remove commit\n+p, pick = use commit\n+r, reword = use commit, but edit the commit message\n+e, edit = use commit, but stop for amending\n+s, squash = use commit, but meld into previous commit\n+f, fixup = like \\\"squash\\\", but discard this commit's log message\n+x, exec = run command (the rest of the line) using shell\n+d, drop = remove commit\n \n These lines can be re-ordered; they are executed from top to bottom.\n \" | git stripspace --comment-lines >>\"$todo\"\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322252","messageId":"4eb4ea601185225c05f391c0855d80711797a994.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 04/10] rebase -i: do not invent onelines when expanding/collapsing SHA-1s","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:07:39Z","receivedAt":"2017-06-14T13:07:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"To avoid problems with short SHA-1s that become non-unique during the\nrebase, we rewrite the todo script with short/long SHA-1s before and\nafter letting the user edit the script. Since SHA-1s are not intuitive\nfor humans, rebase -i also provides the onelines (commit message\nsubjects) in the script, purely for the user's convenience.\n\nIt is very possible to generate a todo script via different means than\nrebase -i and then to let rebase -i run with it; In this case, these\nonelines are not required.\n\nAnd this is where the expand/collapse machinery has a bug: it *expects*\nthat oneline, and failing to find one reuses the previous SHA-1 as\n\"oneline\".\n\nIt was most likely an oversight, and made implementation in the (quite\nlimiting) shell script language less convoluted. However, we are about\nto reimplement performance-critical parts in C (and due to spawning a\ngit.exe process for every single line of the todo script, the\nexpansion/collapsing of the SHA-1s *is* performance-hampering on\nWindows), therefore let's fix this bug to make cross-validation with the\nC version of that functionality possible.\n\nSigned-off-by: Johannes Schindelin \n---\n git-rebase--interactive.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 93372c62b2e..9d65212b7f1 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -760,7 +760,12 @@ transform_todo_ids () {\n \t\t\t;;\n \t\t*)\n \t\t\tsha1=$(git rev-parse --verify --quiet \"$@\" ${rest%%[\t ]*}) &&\n-\t\t\trest=\"$sha1 ${rest#*[\t ]}\"\n+\t\t\tif test \"a$rest\" = \"a${rest#*[\t ]}\"\n+\t\t\tthen\n+\t\t\t\trest=$sha1\n+\t\t\telse\n+\t\t\t\trest=\"$sha1 ${rest#*[\t ]}\"\n+\t\t\tfi\n \t\t\t;;\n \t\tesac\n \t\tprintf '%s\\n' \"$command${rest:+ }$rest\"\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322253","messageId":"f2af44f76483fd083c4db57ba5f245c61455c517.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 05/10] rebase -i: also expand/collapse the SHA-1s via the rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:07:41Z","receivedAt":"2017-06-14T13:08:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is crucial to improve performance on Windows, as the speed is now\nmostly dominated by the SHA-1 transformation (because it spawns a new\nrev-parse process for *every* line, and spawning processes is pretty\nslow from Git for Windows' MSYS2 Bash).\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase--helper.c | 10 +++++++-\n git-rebase--interactive.sh | 27 ++--------------------\n sequencer.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++\n sequencer.h | 2 ++\n 4 files changed, 70 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex 821058d452d..b6ad4008398 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -13,7 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n \tint keep_empty = 0;\n \tenum {\n-\t\tCONTINUE = 1, ABORT, MAKE_SCRIPT\n+\t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\t\tABORT),\n \t\tOPT_CMDMODE(0, \"make-script\", &command,\n \t\t\tN_(\"make rebase script\"), MAKE_SCRIPT),\n+\t\tOPT_CMDMODE(0, \"shorten-ids\", &command,\n+\t\t\tN_(\"shorten SHA-1s in the todo list\"), SHORTEN_SHA1S),\n+\t\tOPT_CMDMODE(0, \"expand-ids\", &command,\n+\t\t\tN_(\"expand SHA-1s in the todo list\"), EXPAND_SHA1S),\n \t\tOPT_END()\n \t};\n \n@@ -42,5 +46,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!sequencer_remove_state(&opts);\n \tif (command == MAKE_SCRIPT && argc > 1)\n \t\treturn !!sequencer_make_script(keep_empty, stdout, argc, argv);\n+\tif (command == SHORTEN_SHA1S && argc == 1)\n+\t\treturn !!transform_todo_ids(1);\n+\tif (command == EXPAND_SHA1S && argc == 1)\n+\t\treturn !!transform_todo_ids(0);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 9d65212b7f1..d5df02435ae 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -750,35 +750,12 @@ skip_unnecessary_picks () {\n \t\tdie \"$(gettext \"Could not skip unnecessary pick commands\")\"\n }\n \n-transform_todo_ids () {\n-\twhile read -r command rest\n-\tdo\n-\t\tcase \"$command\" in\n-\t\t\"$comment_char\"* | exec)\n-\t\t\t# Be careful for oddball commands like 'exec'\n-\t\t\t# that do not have a SHA-1 at the beginning of $rest.\n-\t\t\t;;\n-\t\t*)\n-\t\t\tsha1=$(git rev-parse --verify --quiet \"$@\" ${rest%%[\t ]*}) &&\n-\t\t\tif test \"a$rest\" = \"a${rest#*[\t ]}\"\n-\t\t\tthen\n-\t\t\t\trest=$sha1\n-\t\t\telse\n-\t\t\t\trest=\"$sha1 ${rest#*[\t ]}\"\n-\t\t\tfi\n-\t\t\t;;\n-\t\tesac\n-\t\tprintf '%s\\n' \"$command${rest:+ }$rest\"\n-\tdone <\"$todo\" >\"$todo.new\" &&\n-\tmv -f \"$todo.new\" \"$todo\"\n-}\n-\n expand_todo_ids() {\n-\ttransform_todo_ids\n+\tgit rebase--helper --expand-ids\n }\n \n collapse_todo_ids() {\n-\ttransform_todo_ids --short\n+\tgit rebase--helper --shorten-ids\n }\n \n # Rearrange the todo list that has both \"pick sha1 msg\" and\ndiff --git a/sequencer.c b/sequencer.c\nindex 133d9aa7c74..5b893ca3878 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2461,3 +2461,60 @@ int sequencer_make_script(int keep_empty, FILE *out,\n \tstrbuf_release(&buf);\n \treturn 0;\n }\n+\n+\n+int transform_todo_ids(int shorten_ids)\n+{\n+\tconst char *todo_file = rebase_path_todo();\n+\tstruct todo_list todo_list = TODO_LIST_INIT;\n+\tint fd, res, i;\n+\tFILE *out;\n+\n+\tstrbuf_reset(&todo_list.buf);\n+\tfd = open(todo_file, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n+\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n+\t\tclose(fd);\n+\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n+\t}\n+\tclose(fd);\n+\n+\tres = parse_insn_buffer(todo_list.buf.buf, &todo_list);\n+\tif (res) {\n+\t\ttodo_list_release(&todo_list);\n+\t\treturn error(_(\"unusable todo list: '%s'\"), todo_file);\n+\t}\n+\n+\tout = fopen(todo_file, \"w\");\n+\tif (!out) {\n+\t\ttodo_list_release(&todo_list);\n+\t\treturn error(_(\"unable to open '%s' for writing\"), todo_file);\n+\t}\n+\tfor (i = 0; i < todo_list.nr; i++) {\n+\t\tstruct todo_item *item = todo_list.items + i;\n+\t\tint bol = item->offset_in_buf;\n+\t\tconst char *p = todo_list.buf.buf + bol;\n+\t\tint eol = i + 1 < todo_list.nr ?\n+\t\t\ttodo_list.items[i + 1].offset_in_buf :\n+\t\t\ttodo_list.buf.len;\n+\n+\t\tif (item->command >= TODO_EXEC && item->command != TODO_DROP)\n+\t\t\tfwrite(p, eol - bol, 1, out);\n+\t\telse {\n+\t\t\tconst char *id = shorten_ids ?\n+\t\t\t\tshort_commit_name(item->commit) :\n+\t\t\t\toid_to_hex(&item->commit->object.oid);\n+\t\t\tint len;\n+\n+\t\t\tp += strspn(p, \" \\t\"); /* left-trim command */\n+\t\t\tlen = strcspn(p, \" \\t\"); /* length of command */\n+\n+\t\t\tfprintf(out, \"%.*s %s %.*s\\n\",\n+\t\t\t\tlen, p, id, item->arg_len, item->arg);\n+\t\t}\n+\t}\n+\tfclose(out);\n+\ttodo_list_release(&todo_list);\n+\treturn 0;\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex 83f2943b7a9..71d25374afe 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -48,6 +48,8 @@ int sequencer_remove_state(struct replay_opts *opts);\n int sequencer_make_script(int keep_empty, FILE *out,\n \t\tint argc, const char **argv);\n \n+int transform_todo_ids(int shorten_ids);\n+\n extern const char sign_off_header[];\n \n void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322254","messageId":"bc86ff44d5fa0ee2bf69ab6a7e06d52d4b9df3d8.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 06/10] t3404: relax rebase.missingCommitsCheck tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:07:46Z","receivedAt":"2017-06-14T13:08:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"These tests were a bit anal about the *exact* warning/error message\nprinted by git rebase. But those messages are intended for the *end\nuser*, therefore it does not make sense to test so rigidly for the\n*exact* wording.\n\nIn the following, we will reimplement the missing commits check in\nthe sequencer, with slightly different words.\n\nSo let's just test for the parts in the warning/error message that\nwe *really* care about, nothing more, nothing less.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3404-rebase-interactive.sh | 22 ++++------------------\n 1 file changed, 4 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 5bd0275930b..3b411ea8f1b 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1242,20 +1242,13 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n-cat >expect <actual &&\n-\ttest_i18ncmp expect actual &&\n+\ttest_i18ngrep \"badcmd $(git rev-list --oneline -1 master~1)\" actual &&\n+\ttest_i18ngrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n \tFAKE_LINES=\"1 2 3 drop 4 5\" git rebase --edit-todo &&\n \tgit rebase --continue &&\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n@@ -1277,20 +1270,13 @@ test_expect_success 'tabs and spaces are accepted in the todolist' '\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n-cat >expect <actual &&\n-\ttest_i18ncmp expect actual &&\n+\ttest_i18ngrep \"edit XXXXXXX False commit\" actual &&\n+\ttest_i18ngrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n \tFAKE_LINES=\"1 2 4 5 6\" git rebase --edit-todo &&\n \tgit rebase --continue &&\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322255","messageId":"fe4dd5e016efb032e41c0db5c94025e1235ea0d9.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 07/10] rebase -i: check for missing commits in the rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:07:49Z","receivedAt":"2017-06-14T13:08:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In particular on Windows, where shell scripts are even more expensive\nthan on MacOSX or Linux, it makes sense to move a loop that forks\nGit at least once for every line in the todo list into a builtin.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase--helper.c | 7 +-\n git-rebase--interactive.sh | 164 ++-------------------------------------------\n sequencer.c | 122 +++++++++++++++++++++++++++++++++\n sequencer.h | 1 +\n 4 files changed, 134 insertions(+), 160 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex b6ad4008398..0deba9d1945 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -13,7 +13,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n \tint keep_empty = 0;\n \tenum {\n-\t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S\n+\t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,\n+\t\tCHECK_TODO_LIST\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -28,6 +29,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"shorten SHA-1s in the todo list\"), SHORTEN_SHA1S),\n \t\tOPT_CMDMODE(0, \"expand-ids\", &command,\n \t\t\tN_(\"expand SHA-1s in the todo list\"), EXPAND_SHA1S),\n+\t\tOPT_CMDMODE(0, \"check-todo-list\", &command,\n+\t\t\tN_(\"check the todo list\"), CHECK_TODO_LIST),\n \t\tOPT_END()\n \t};\n \n@@ -50,5 +53,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!transform_todo_ids(1);\n \tif (command == EXPAND_SHA1S && argc == 1)\n \t\treturn !!transform_todo_ids(0);\n+\tif (command == CHECK_TODO_LIST && argc == 1)\n+\t\treturn !!check_todo_list();\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex d5df02435ae..c8cad318fa4 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -867,96 +867,6 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n-# Check if the SHA-1 passed as an argument is a\n-# correct one, if not then print $2 in \"$todo\".badsha\n-# $1: the SHA-1 to test\n-# $2: the line number of the input\n-# $3: the input filename\n-check_commit_sha () {\n-\tbadsha=0\n-\tif test -z \"$1\"\n-\tthen\n-\t\tbadsha=1\n-\telse\n-\t\tsha1_verif=\"$(git rev-parse --verify --quiet $1^{commit})\"\n-\t\tif test -z \"$sha1_verif\"\n-\t\tthen\n-\t\t\tbadsha=1\n-\t\tfi\n-\tfi\n-\n-\tif test $badsha -ne 0\n-\tthen\n-\t\tline=\"$(sed -n -e \"${2}p\" \"$3\")\"\n-\t\twarn \"$(eval_gettext \"\\\n-Warning: the SHA-1 is missing or isn't a commit in the following line:\n- - \\$line\")\"\n-\t\twarn\n-\tfi\n-\n-\treturn $badsha\n-}\n-\n-# prints the bad commits and bad commands\n-# from the todolist in stdin\n-check_bad_cmd_and_sha () {\n-\tretval=0\n-\tlineno=0\n-\twhile read -r command rest\n-\tdo\n-\t\tlineno=$(( $lineno + 1 ))\n-\t\tcase $command in\n-\t\t\"$comment_char\"*|''|noop|x|exec)\n-\t\t\t# Doesn't expect a SHA-1\n-\t\t\t;;\n-\t\t\"$cr\")\n-\t\t\t# Work around CR left by \"read\" (e.g. with Git for\n-\t\t\t# Windows' Bash).\n-\t\t\t;;\n-\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n-\t\t\tif ! check_commit_sha \"${rest%%[ \t]*}\" \"$lineno\" \"$1\"\n-\t\t\tthen\n-\t\t\t\tretval=1\n-\t\t\tfi\n-\t\t\t;;\n-\t\t*)\n-\t\t\tline=\"$(sed -n -e \"${lineno}p\" \"$1\")\"\n-\t\t\twarn \"$(eval_gettext \"\\\n-Warning: the command isn't recognized in the following line:\n- - \\$line\")\"\n-\t\t\twarn\n-\t\t\tretval=1\n-\t\t\t;;\n-\t\tesac\n-\tdone <\"$1\"\n-\treturn $retval\n-}\n-\n-# Print the list of the SHA-1 of the commits\n-# from stdin to stdout\n-todo_list_to_sha_list () {\n-\tgit stripspace --strip-comments |\n-\twhile read -r command sha1 rest\n-\tdo\n-\t\tcase $command in\n-\t\t\"$comment_char\"*|''|noop|x|\"exec\")\n-\t\t\t;;\n-\t\t*)\n-\t\t\tlong_sha=$(git rev-list --no-walk \"$sha1\" 2>/dev/null)\n-\t\t\tprintf \"%s\\n\" \"$long_sha\"\n-\t\t\t;;\n-\t\tesac\n-\tdone\n-}\n-\n-# Use warn for each line in stdin\n-warn_lines () {\n-\twhile read -r line\n-\tdo\n-\t\twarn \" - $line\"\n-\tdone\n-}\n-\n # Switch to the branch in $into and notify it in the reflog\n checkout_onto () {\n \tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n@@ -971,74 +881,6 @@ get_missing_commit_check_level () {\n \tprintf '%s' \"$check_level\" | tr 'A-Z' 'a-z'\n }\n \n-# Check if the user dropped some commits by mistake\n-# Behaviour determined by rebase.missingCommitsCheck.\n-# Check if there is an unrecognized command or a\n-# bad SHA-1 in a command.\n-check_todo_list () {\n-\traise_error=f\n-\n-\tcheck_level=$(get_missing_commit_check_level)\n-\n-\tcase \"$check_level\" in\n-\twarn|error)\n-\t\t# Get the SHA-1 of the commits\n-\t\ttodo_list_to_sha_list <\"$todo\".backup >\"$todo\".oldsha1\n-\t\ttodo_list_to_sha_list <\"$todo\" >\"$todo\".newsha1\n-\n-\t\t# Sort the SHA-1 and compare them\n-\t\tsort -u \"$todo\".oldsha1 >\"$todo\".oldsha1+\n-\t\tmv \"$todo\".oldsha1+ \"$todo\".oldsha1\n-\t\tsort -u \"$todo\".newsha1 >\"$todo\".newsha1+\n-\t\tmv \"$todo\".newsha1+ \"$todo\".newsha1\n-\t\tcomm -2 -3 \"$todo\".oldsha1 \"$todo\".newsha1 >\"$todo\".miss\n-\n-\t\t# Warn about missing commits\n-\t\tif test -s \"$todo\".miss\n-\t\tthen\n-\t\t\ttest \"$check_level\" = error && raise_error=t\n-\n-\t\t\twarn \"$(gettext \"\\\n-Warning: some commits may have been dropped accidentally.\n-Dropped commits (newer to older):\")\"\n-\n-\t\t\t# Make the list user-friendly and display\n-\t\t\topt=\"--no-walk=sorted --format=oneline --abbrev-commit --stdin\"\n-\t\t\tgit rev-list $opt <\"$todo\".miss | warn_lines\n-\n-\t\t\twarn \"$(gettext \"\\\n-To avoid this message, use \\\"drop\\\" to explicitly remove a commit.\n-\n-Use 'git config rebase.missingCommitsCheck' to change the level of warnings.\n-The possible behaviours are: ignore, warn, error.\")\"\n-\t\t\twarn\n-\t\tfi\n-\t\t;;\n-\tignore)\n-\t\t;;\n-\t*)\n-\t\twarn \"$(eval_gettext \"Unrecognized setting \\$check_level for option rebase.missingCommitsCheck. Ignoring.\")\"\n-\t\t;;\n-\tesac\n-\n-\tif ! check_bad_cmd_and_sha \"$todo\"\n-\tthen\n-\t\traise_error=t\n-\tfi\n-\n-\tif test $raise_error = t\n-\tthen\n-\t\t# Checkout before the first commit of the\n-\t\t# rebase: this way git rebase --continue\n-\t\t# will work correctly as it expects HEAD to be\n-\t\t# placed before the commit of the next action\n-\t\tcheckout_onto\n-\n-\t\twarn \"$(gettext \"You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\")\"\n-\t\tdie \"$(gettext \"Or you can abort the rebase with 'git rebase --abort'.\")\"\n-\tfi\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@@ -1299,7 +1141,11 @@ git_sequence_editor \"$todo\" ||\n has_action \"$todo\" ||\n \treturn 2\n \n-check_todo_list\n+git rebase--helper --check-todo-list || {\n+\tret=$?\n+\tcheckout_onto\n+\texit $ret\n+}\n \n expand_todo_ids\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 5b893ca3878..a697906d463 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2518,3 +2518,125 @@ int transform_todo_ids(int shorten_ids)\n \ttodo_list_release(&todo_list);\n \treturn 0;\n }\n+\n+enum check_level {\n+\tCHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR\n+};\n+\n+static enum check_level get_missing_commit_check_level(void)\n+{\n+\tconst char *value;\n+\n+\tif (git_config_get_value(\"rebase.missingcommitscheck\", &value) ||\n+\t\t\t!strcasecmp(\"ignore\", value))\n+\t\treturn CHECK_IGNORE;\n+\tif (!strcasecmp(\"warn\", value))\n+\t\treturn CHECK_WARN;\n+\tif (!strcasecmp(\"error\", value))\n+\t\treturn CHECK_ERROR;\n+\twarning(_(\"unrecognized setting %s for option\"\n+\t\t \"rebase.missingCommitsCheck. Ignoring.\"), value);\n+\treturn CHECK_IGNORE;\n+}\n+\n+/*\n+ * Check if the user dropped some commits by mistake\n+ * Behaviour determined by rebase.missingCommitsCheck.\n+ * Check if there is an unrecognized command or a\n+ * bad SHA-1 in a command.\n+ */\n+int check_todo_list(void)\n+{\n+\tenum check_level check_level = get_missing_commit_check_level();\n+\tstruct strbuf todo_file = STRBUF_INIT;\n+\tstruct todo_list todo_list = TODO_LIST_INIT;\n+\tstruct strbuf missing = STRBUF_INIT;\n+\tint advise_to_edit_todo = 0, res = 0, fd, i;\n+\n+\tstrbuf_addstr(&todo_file, rebase_path_todo());\n+\tfd = open(todo_file.buf, O_RDONLY);\n+\tif (fd < 0) {\n+\t\tres = error_errno(_(\"could not open '%s'\"), todo_file.buf);\n+\t\tgoto leave_check;\n+\t}\n+\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n+\t\tclose(fd);\n+\t\tres = error(_(\"could not read '%s'.\"), todo_file.buf);\n+\t\tgoto leave_check;\n+\t}\n+\tclose(fd);\n+\tadvise_to_edit_todo = res =\n+\t\tparse_insn_buffer(todo_list.buf.buf, &todo_list);\n+\n+\tif (res || check_level == CHECK_IGNORE)\n+\t\tgoto leave_check;\n+\n+\t/* Mark the commits in git-rebase-todo as seen */\n+\tfor (i = 0; i < todo_list.nr; i++) {\n+\t\tstruct commit *commit = todo_list.items[i].commit;\n+\t\tif (commit)\n+\t\t\tcommit->util = (void *)1;\n+\t}\n+\n+\ttodo_list_release(&todo_list);\n+\tstrbuf_addstr(&todo_file, \".backup\");\n+\tfd = open(todo_file.buf, O_RDONLY);\n+\tif (fd < 0) {\n+\t\tres = error_errno(_(\"could not open '%s'\"), todo_file.buf);\n+\t\tgoto leave_check;\n+\t}\n+\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n+\t\tclose(fd);\n+\t\tres = error(_(\"could not read '%s'.\"), todo_file.buf);\n+\t\tgoto leave_check;\n+\t}\n+\tclose(fd);\n+\tstrbuf_release(&todo_file);\n+\tres = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);\n+\n+\t/* Find commits in git-rebase-todo.backup yet unseen */\n+\tfor (i = todo_list.nr - 1; i >= 0; i--) {\n+\t\tstruct todo_item *item = todo_list.items + i;\n+\t\tstruct commit *commit = item->commit;\n+\t\tif (commit && !commit->util) {\n+\t\t\tstrbuf_addf(&missing, \" - %s %.*s\\n\",\n+\t\t\t\t short_commit_name(commit),\n+\t\t\t\t item->arg_len, item->arg);\n+\t\t\tcommit->util = (void *)1;\n+\t\t}\n+\t}\n+\n+\t/* Warn about missing commits */\n+\tif (!missing.len)\n+\t\tgoto leave_check;\n+\n+\tif (check_level == CHECK_ERROR)\n+\t\tadvise_to_edit_todo = res = 1;\n+\n+\tfprintf(stderr,\n+\t\t_(\"Warning: some commits may have been dropped accidentally.\\n\"\n+\t\t\"Dropped commits (newer to older):\\n\"));\n+\n+\t/* Make the list user-friendly and display */\n+\tfputs(missing.buf, stderr);\n+\tstrbuf_release(&missing);\n+\n+\tfprintf(stderr, _(\"To avoid this message, use \\\"drop\\\" to \"\n+\t\t\"explicitly remove a commit.\\n\\n\"\n+\t\t\"Use 'git config rebase.missingCommitsCheck' to change \"\n+\t\t\"the level of warnings.\\n\"\n+\t\t\"The possible behaviours are: ignore, warn, error.\\n\\n\"));\n+\n+leave_check:\n+\tstrbuf_release(&todo_file);\n+\ttodo_list_release(&todo_list);\n+\n+\tif (advise_to_edit_todo)\n+\t\tfprintf(stderr,\n+\t\t\t_(\"You can fix this with 'git rebase --edit-todo' \"\n+\t\t\t \"and then run 'git rebase --continue'.\\n\"\n+\t\t\t \"Or you can abort the rebase with 'git rebase\"\n+\t\t\t \" --abort'.\\n\"));\n+\n+\treturn res;\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex 71d25374afe..878dd296f8c 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -49,6 +49,7 @@ int sequencer_make_script(int keep_empty, FILE *out,\n \t\tint argc, const char **argv);\n \n int transform_todo_ids(int shorten_ids);\n+int check_todo_list(void);\n \n extern const char sign_off_header[];\n \n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322256","messageId":"72bbfcae2abcb14f6b1288051a244faadbee29e0.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:08:07Z","receivedAt":"2017-06-14T13:08:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In particular on Windows, where shell scripts are even more expensive\nthan on MacOSX or Linux, it makes sense to move a loop that forks\nGit at least once for every line in the todo list into a builtin.\n\nNote: The original code did not try to skip unnecessary picks of root\ncommits but punts instead (probably --root was not considered common\nenough of a use case to bother optimizing). We do the same, for now.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase--helper.c | 6 ++-\n git-rebase--interactive.sh | 41 ++---------------\n sequencer.c | 107 +++++++++++++++++++++++++++++++++++++++++++++\n sequencer.h | 1 +\n 4 files changed, 116 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex 0deba9d1945..fdc933dc64a 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tint keep_empty = 0;\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,\n-\t\tCHECK_TODO_LIST\n+\t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -31,6 +31,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"expand SHA-1s in the todo list\"), EXPAND_SHA1S),\n \t\tOPT_CMDMODE(0, \"check-todo-list\", &command,\n \t\t\tN_(\"check the todo list\"), CHECK_TODO_LIST),\n+\t\tOPT_CMDMODE(0, \"skip-unnecessary-picks\", &command,\n+\t\t\tN_(\"skip unnecessary picks\"), SKIP_UNNECESSARY_PICKS),\n \t\tOPT_END()\n \t};\n \n@@ -55,5 +57,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!transform_todo_ids(0);\n \tif (command == CHECK_TODO_LIST && argc == 1)\n \t\treturn !!check_todo_list();\n+\tif (command == SKIP_UNNECESSARY_PICKS && argc == 1)\n+\t\treturn !!skip_unnecessary_picks();\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c8cad318fa4..af8d7bd77fb 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -713,43 +713,6 @@ do_rest () {\n \tdone\n }\n \n-# skip picking commits whose parents are unchanged\n-skip_unnecessary_picks () {\n-\tfd=3\n-\twhile read -r command rest\n-\tdo\n-\t\t# fd=3 means we skip the command\n-\t\tcase \"$fd,$command\" in\n-\t\t3,pick|3,p)\n-\t\t\t# pick a commit whose parent is current $onto -> skip\n-\t\t\tsha1=${rest%% *}\n-\t\t\tcase \"$(git rev-parse --verify --quiet \"$sha1\"^)\" in\n-\t\t\t\"$onto\"*)\n-\t\t\t\tonto=$sha1\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\tfd=1\n-\t\t\t\t;;\n-\t\t\tesac\n-\t\t\t;;\n-\t\t3,\"$comment_char\"*|3,)\n-\t\t\t# copy comments\n-\t\t\t;;\n-\t\t*)\n-\t\t\tfd=1\n-\t\t\t;;\n-\t\tesac\n-\t\tprintf '%s\\n' \"$command${rest:+ }$rest\" >&$fd\n-\tdone <\"$todo\" >\"$todo.new\" 3>>\"$done\" &&\n-\tmv -f \"$todo\".new \"$todo\" &&\n-\tcase \"$(peek_next_command)\" in\n-\tsquash|s|fixup|f)\n-\t\trecord_in_rewritten \"$onto\"\n-\t\t;;\n-\tesac ||\n-\t\tdie \"$(gettext \"Could not skip unnecessary pick commands\")\"\n-}\n-\n expand_todo_ids() {\n \tgit rebase--helper --expand-ids\n }\n@@ -1149,7 +1112,9 @@ git rebase--helper --check-todo-list || {\n \n expand_todo_ids\n \n-test -d \"$rewritten\" || test -n \"$force_rebase\" || skip_unnecessary_picks\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 \n checkout_onto\n if test -z \"$rebase_root\" && test ! -d \"$rewritten\"\ndiff --git a/sequencer.c b/sequencer.c\nindex a697906d463..a0e020dab09 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2640,3 +2640,110 @@ int check_todo_list(void)\n \n \treturn res;\n }\n+\n+/* skip picking commits whose parents are unchanged */\n+int skip_unnecessary_picks(void)\n+{\n+\tconst char *todo_file = rebase_path_todo();\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct todo_list todo_list = TODO_LIST_INIT;\n+\tstruct object_id onto_oid, *oid = &onto_oid, *parent_oid;\n+\tint fd, i;\n+\n+\tif (!read_oneliner(&buf, rebase_path_onto(), 0))\n+\t\treturn error(_(\"could not read 'onto'\"));\n+\tif (get_sha1(buf.buf, onto_oid.hash)) {\n+\t\tstrbuf_release(&buf);\n+\t\treturn error(_(\"need a HEAD to fixup\"));\n+\t}\n+\tstrbuf_release(&buf);\n+\n+\tfd = open(todo_file, O_RDONLY);\n+\tif (fd < 0) {\n+\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n+\t}\n+\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n+\t\tclose(fd);\n+\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n+\t}\n+\tclose(fd);\n+\tif (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {\n+\t\ttodo_list_release(&todo_list);\n+\t\treturn -1;\n+\t}\n+\n+\tfor (i = 0; i < todo_list.nr; i++) {\n+\t\tstruct todo_item *item = todo_list.items + i;\n+\n+\t\tif (item->command >= TODO_NOOP)\n+\t\t\tcontinue;\n+\t\tif (item->command != TODO_PICK)\n+\t\t\tbreak;\n+\t\tif (parse_commit(item->commit)) {\n+\t\t\ttodo_list_release(&todo_list);\n+\t\t\treturn error(_(\"could not parse commit '%s'\"),\n+\t\t\t\toid_to_hex(&item->commit->object.oid));\n+\t\t}\n+\t\tif (!item->commit->parents)\n+\t\t\tbreak; /* root commit */\n+\t\tif (item->commit->parents->next)\n+\t\t\tbreak; /* merge commit */\n+\t\tparent_oid = &item->commit->parents->item->object.oid;\n+\t\tif (hashcmp(parent_oid->hash, oid->hash))\n+\t\t\tbreak;\n+\t\toid = &item->commit->object.oid;\n+\t}\n+\tif (i > 0) {\n+\t\tint offset = i < todo_list.nr ?\n+\t\t\ttodo_list.items[i].offset_in_buf : todo_list.buf.len;\n+\t\tconst char *done_path = rebase_path_done();\n+\n+\t\tfd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);\n+\t\tif (fd < 0) {\n+\t\t\terror_errno(_(\"could not open '%s' for writing\"),\n+\t\t\t\t done_path);\n+\t\t\ttodo_list_release(&todo_list);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (write_in_full(fd, todo_list.buf.buf, offset) < 0) {\n+\t\t\terror_errno(_(\"could not write to '%s'\"), done_path);\n+\t\t\ttodo_list_release(&todo_list);\n+\t\t\tclose(fd);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tclose(fd);\n+\n+\t\tfd = open(rebase_path_todo(), O_WRONLY, 0666);\n+\t\tif (fd < 0) {\n+\t\t\terror_errno(_(\"could not open '%s' for writing\"),\n+\t\t\t\t rebase_path_todo());\n+\t\t\ttodo_list_release(&todo_list);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (write_in_full(fd, todo_list.buf.buf + offset,\n+\t\t\t\ttodo_list.buf.len - offset) < 0) {\n+\t\t\terror_errno(_(\"could not write to '%s'\"),\n+\t\t\t\t rebase_path_todo());\n+\t\t\tclose(fd);\n+\t\t\ttodo_list_release(&todo_list);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (ftruncate(fd, todo_list.buf.len - offset) < 0) {\n+\t\t\terror_errno(_(\"could not truncate '%s'\"),\n+\t\t\t\t rebase_path_todo());\n+\t\t\ttodo_list_release(&todo_list);\n+\t\t\tclose(fd);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tclose(fd);\n+\n+\t\ttodo_list.current = i;\n+\t\tif (is_fixup(peek_command(&todo_list, 0)))\n+\t\t\trecord_in_rewritten(oid, peek_command(&todo_list, 0));\n+\t}\n+\n+\ttodo_list_release(&todo_list);\n+\tprintf(\"%s\\n\", oid_to_hex(oid));\n+\n+\treturn 0;\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex 878dd296f8c..04a57e09a1d 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -50,6 +50,7 @@ int sequencer_make_script(int keep_empty, FILE *out,\n \n int transform_todo_ids(int shorten_ids);\n int check_todo_list(void);\n+int skip_unnecessary_picks(void);\n \n extern const char sign_off_header[];\n \n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322257","messageId":"169d8d252b1d46b3c6ad6c06000b2b9e8f054f24.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 09/10] t3415: test fixup with wrapped oneline","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:08:11Z","receivedAt":"2017-06-14T13:08:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The `git commit --fixup` command unwraps wrapped onelines when\nconstructing the commit message, without wrapping the result.\n\nWe need to make sure that `git rebase --autosquash` keeps handling such\ncases correctly, in particular since we are about to move the autosquash\nhandling into the rebase--helper.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3415-rebase-autosquash.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 6d99f624b62..62cb977e4ec 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -316,4 +316,18 @@ test_expect_success 'extra spaces after fixup!' '\n \ttest $base = $parent\n '\n \n+test_expect_success 'wrapped original subject' '\n+\tif test -d .git/rebase-merge; then git rebase --abort; fi &&\n+\tbase=$(git rev-parse HEAD) &&\n+\techo \"wrapped subject\" >wrapped &&\n+\tgit add wrapped &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"$(printf \"To\\nfixup\")\" &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"fixup! To fixup\" &&\n+\tgit rebase -i --autosquash --keep-empty HEAD~2 &&\n+\tparent=$(git rev-parse HEAD^) &&\n+\ttest $base = $parent\n+'\n+\n test_done\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n\n\n"},{"id":"322258","messageId":"2737d42651af164d81ba4eea2c5e2cc14ff4182c.1497444257.git.johannes.schindelin@gmx.de","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 10/10] rebase -i: rearrange fixup/squash lines using the rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-14T13:08:14Z","receivedAt":"2017-06-14T13:08:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This operation has quadratic complexity, which is especially painful\non Windows, where shell scripts are *already* slow (mainly due to the\noverhead of the POSIX emulation layer).\n\nLet's reimplement this with linear complexity (using a hash map to\nmatch the commits' subject lines) for the common case; Sadly, the\nfixup/squash feature's design neglected performance considerations,\nallowing arbitrary prefixes (read: `fixup! hell` will match the\ncommit subject `hello world`), which means that we are stuck with\nquadratic performance in the worst case.\n\nThe reimplemented logic also happens to fix a bug where commented-out\nlines (representing empty patches) were dropped by the previous code.\n\nWhile at it, clarify how the fixup/squash feature works in `git rebase\n-i`'s man page.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/git-rebase.txt | 16 ++--\n builtin/rebase--helper.c | 6 +-\n git-rebase--interactive.sh | 90 +-------------------\n sequencer.c | 195 +++++++++++++++++++++++++++++++++++++++++++\n sequencer.h | 1 +\n t/t3415-rebase-autosquash.sh | 2 +-\n 6 files changed, 212 insertions(+), 98 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 53f4e144444..c5464aa5365 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -430,13 +430,15 @@ without an explicit `--interactive`.\n --autosquash::\n --no-autosquash::\n \tWhen the commit log message begins with \"squash! ...\" (or\n-\t\"fixup! ...\"), and there is a commit whose title begins with\n-\tthe same ..., automatically modify the todo list of rebase -i\n-\tso that the commit marked for squashing comes right after the\n-\tcommit to be modified, and change the action of the moved\n-\tcommit from `pick` to `squash` (or `fixup`). Ignores subsequent\n-\t\"fixup! \" or \"squash! \" after the first, in case you referred to an\n-\tearlier fixup/squash with `git commit --fixup/--squash`.\n+\t\"fixup! ...\"), and there is already a commit in the todo list that\n+\tmatches the same `...`, automatically modify the todo list of rebase\n+\t-i so that the commit marked for squashing comes right after the\n+\tcommit to be modified, and change the action of the moved commit\n+\tfrom `pick` to `squash` (or `fixup`). A commit matches the `...` if\n+\tthe commit subject matches, or if the `...` refers to the commit's\n+\thash. As a fall-back, partial matches of the commit subject work,\n+\ttoo. The recommended way to create fixup/squash commits is by using\n+\tthe `--fixup`/`--squash` options of linkgit:git-commit[1].\n +\n This option is only valid when the `--interactive` option is used.\n +\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex fdc933dc64a..64b36d429fa 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tint keep_empty = 0;\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,\n-\t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS\n+\t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -33,6 +33,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"check the todo list\"), CHECK_TODO_LIST),\n \t\tOPT_CMDMODE(0, \"skip-unnecessary-picks\", &command,\n \t\t\tN_(\"skip unnecessary picks\"), SKIP_UNNECESSARY_PICKS),\n+\t\tOPT_CMDMODE(0, \"rearrange-squash\", &command,\n+\t\t\tN_(\"rearrange fixup/squash lines\"), REARRANGE_SQUASH),\n \t\tOPT_END()\n \t};\n \n@@ -59,5 +61,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!check_todo_list();\n \tif (command == SKIP_UNNECESSARY_PICKS && argc == 1)\n \t\treturn !!skip_unnecessary_picks();\n+\tif (command == REARRANGE_SQUASH && argc == 1)\n+\t\treturn !!rearrange_squash();\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex af8d7bd77fb..3b0340e7cc9 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -721,94 +721,6 @@ collapse_todo_ids() {\n \tgit rebase--helper --shorten-ids\n }\n \n-# Rearrange the todo list that has both \"pick sha1 msg\" and\n-# \"pick sha1 fixup!/squash! msg\" appears in it so that the latter\n-# comes immediately after the former, and change \"pick\" to\n-# \"fixup\"/\"squash\".\n-#\n-# Note that if the config has specified a custom instruction format\n-# each log message will be re-retrieved in order to normalize the\n-# autosquash arrangement\n-rearrange_squash () {\n-\tformat=$(git config --get rebase.instructionFormat)\n-\t# extract fixup!/squash! lines and resolve any referenced sha1's\n-\twhile read -r pick sha1 message\n-\tdo\n-\t\ttest -z \"${format}\" || message=$(git log -n 1 --format=\"%s\" ${sha1})\n-\t\tcase \"$message\" in\n-\t\t\"squash! \"*|\"fixup! \"*)\n-\t\t\taction=\"${message%%!*}\"\n-\t\t\trest=$message\n-\t\t\tprefix=\n-\t\t\t# skip all squash! or fixup! (but save for later)\n-\t\t\twhile :\n-\t\t\tdo\n-\t\t\t\tcase \"$rest\" in\n-\t\t\t\t\"squash! \"*|\"fixup! \"*)\n-\t\t\t\t\tprefix=\"$prefix${rest%%!*},\"\n-\t\t\t\t\trest=\"${rest#*! }\"\n-\t\t\t\t\t;;\n-\t\t\t\t*)\n-\t\t\t\t\tbreak\n-\t\t\t\t\t;;\n-\t\t\t\tesac\n-\t\t\tdone\n-\t\t\tprintf '%s %s %s %s\\n' \"$sha1\" \"$action\" \"$prefix\" \"$rest\"\n-\t\t\t# if it's a single word, try to resolve to a full sha1 and\n-\t\t\t# emit a second copy. This allows us to match on both message\n-\t\t\t# and on sha1 prefix\n-\t\t\tif test \"${rest#* }\" = \"$rest\"; then\n-\t\t\t\tfullsha=\"$(git rev-parse -q --verify \"$rest\" 2>/dev/null)\"\n-\t\t\t\tif test -n \"$fullsha\"; then\n-\t\t\t\t\t# prefix the action to uniquely identify this line as\n-\t\t\t\t\t# intended for full sha1 match\n-\t\t\t\t\techo \"$sha1 +$action $prefix $fullsha\"\n-\t\t\t\tfi\n-\t\t\tfi\n-\t\tesac\n-\tdone >\"$1.sq\" <\"$1\"\n-\ttest -s \"$1.sq\" || return\n-\n-\tused=\n-\twhile read -r pick sha1 message\n-\tdo\n-\t\tcase \" $used\" in\n-\t\t*\" $sha1 \"*) continue ;;\n-\t\tesac\n-\t\tprintf '%s\\n' \"$pick $sha1 $message\"\n-\t\ttest -z \"${format}\" || message=$(git log -n 1 --format=\"%s\" ${sha1})\n-\t\tused=\"$used$sha1 \"\n-\t\twhile read -r squash action msg_prefix msg_content\n-\t\tdo\n-\t\t\tcase \" $used\" in\n-\t\t\t*\" $squash \"*) continue ;;\n-\t\t\tesac\n-\t\t\temit=0\n-\t\t\tcase \"$action\" in\n-\t\t\t+*)\n-\t\t\t\taction=\"${action#+}\"\n-\t\t\t\t# full sha1 prefix test\n-\t\t\t\tcase \"$msg_content\" in \"$sha1\"*) emit=1;; esac ;;\n-\t\t\t*)\n-\t\t\t\t# message prefix test\n-\t\t\t\tcase \"$message\" in \"$msg_content\"*) emit=1;; esac ;;\n-\t\t\tesac\n-\t\t\tif test $emit = 1; then\n-\t\t\t\tif test -n \"${format}\"\n-\t\t\t\tthen\n-\t\t\t\t\tmsg_content=$(git log -n 1 --format=\"${format}\" ${squash})\n-\t\t\t\telse\n-\t\t\t\t\tmsg_content=\"$(echo \"$msg_prefix\" | sed \"s/,/! /g\")$msg_content\"\n-\t\t\t\tfi\n-\t\t\t\tprintf '%s\\n' \"$action $squash $msg_content\"\n-\t\t\t\tused=\"$used$squash \"\n-\t\t\tfi\n-\t\tdone <\"$1.sq\"\n-\tdone >\"$1.rearranged\" <\"$1\"\n-\tcat \"$1.rearranged\" >\"$1\"\n-\trm -f \"$1.sq\" \"$1.rearranged\"\n-}\n-\n # Add commands after a pick or after a squash/fixup serie\n # in the todo list.\n add_exec_commands () {\n@@ -1068,7 +980,7 @@ then\n fi\n \n test -s \"$todo\" || echo noop >> \"$todo\"\n-test -n \"$autosquash\" && rearrange_squash \"$todo\"\n+test -z \"$autosquash\" || git rebase--helper --rearrange-squash || exit\n test -n \"$cmd\" && add_exec_commands \"$todo\"\n \n todocount=$(git stripspace --strip-comments <\"$todo\" | wc -l)\ndiff --git a/sequencer.c b/sequencer.c\nindex a0e020dab09..06c97e12267 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -19,6 +19,7 @@\n #include \"trailer.h\"\n #include \"log-tree.h\"\n #include \"wt-status.h\"\n+#include \"hashmap.h\"\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -2747,3 +2748,197 @@ int skip_unnecessary_picks(void)\n \n \treturn 0;\n }\n+\n+struct subject2item_entry {\n+\tstruct hashmap_entry entry;\n+\tint i;\n+\tchar subject[FLEX_ARRAY];\n+};\n+\n+static int subject2item_cmp(const struct subject2item_entry *a,\n+\tconst struct subject2item_entry *b, const void *key)\n+{\n+\treturn key ? strcmp(a->subject, key) : strcmp(a->subject, b->subject);\n+}\n+\n+/*\n+ * Rearrange the todo list that has both \"pick commit-id msg\" and \"pick\n+ * commit-id fixup!/squash! msg\" in it so that the latter is put immediately\n+ * after the former, and change \"pick\" to \"fixup\"/\"squash\".\n+ *\n+ * Note that if the config has specified a custom instruction format, each log\n+ * message will have to be retrieved from the commit (as the oneline in the\n+ * script cannot be trusted) in order to normalize the autosquash arrangement.\n+ */\n+int rearrange_squash(void)\n+{\n+\tconst char *todo_file = rebase_path_todo();\n+\tstruct todo_list todo_list = TODO_LIST_INIT;\n+\tstruct hashmap subject2item;\n+\tint res = 0, rearranged = 0, *next, *tail, fd, i;\n+\tchar **subjects;\n+\n+\tfd = open(todo_file, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n+\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n+\t\tclose(fd);\n+\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n+\t}\n+\tclose(fd);\n+\tif (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {\n+\t\ttodo_list_release(&todo_list);\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * The hashmap maps onelines to the respective todo list index.\n+\t *\n+\t * If any items need to be rearranged, the next[i] value will indicate\n+\t * which item was moved directly after the i'th.\n+\t *\n+\t * In that case, last[i] will indicate the index of the latest item to\n+\t * be moved to appear after the i'th.\n+\t */\n+\thashmap_init(&subject2item, (hashmap_cmp_fn) subject2item_cmp,\n+\t\t todo_list.nr);\n+\tALLOC_ARRAY(next, todo_list.nr);\n+\tALLOC_ARRAY(tail, todo_list.nr);\n+\tALLOC_ARRAY(subjects, todo_list.nr);\n+\tfor (i = 0; i < todo_list.nr; i++) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstruct todo_item *item = todo_list.items + i;\n+\t\tconst char *commit_buffer, *subject, *p;\n+\t\tsize_t subject_len;\n+\t\tint i2 = -1;\n+\t\tstruct subject2item_entry *entry;\n+\n+\t\tnext[i] = tail[i] = -1;\n+\t\tif (item->command >= TODO_EXEC) {\n+\t\t\tsubjects[i] = NULL;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (is_fixup(item->command)) {\n+\t\t\ttodo_list_release(&todo_list);\n+\t\t\treturn error(_(\"the script was already rearranged.\"));\n+\t\t}\n+\n+\t\titem->commit->util = item;\n+\n+\t\tparse_commit(item->commit);\n+\t\tcommit_buffer = get_commit_buffer(item->commit, NULL);\n+\t\tfind_commit_subject(commit_buffer, &subject);\n+\t\tformat_subject(&buf, subject, \" \");\n+\t\tsubject = subjects[i] = strbuf_detach(&buf, &subject_len);\n+\t\tunuse_commit_buffer(item->commit, commit_buffer);\n+\t\tif ((skip_prefix(subject, \"fixup! \", &p) ||\n+\t\t skip_prefix(subject, \"squash! \", &p))) {\n+\t\t\tstruct commit *commit2;\n+\n+\t\t\tfor (;;) {\n+\t\t\t\twhile (isspace(*p))\n+\t\t\t\t\tp++;\n+\t\t\t\tif (!skip_prefix(p, \"fixup! \", &p) &&\n+\t\t\t\t !skip_prefix(p, \"squash! \", &p))\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tif ((entry = hashmap_get_from_hash(&subject2item,\n+\t\t\t\t\t\t\t strhash(p), p)))\n+\t\t\t\t/* found by title */\n+\t\t\t\ti2 = entry->i;\n+\t\t\telse if (!strchr(p, ' ') &&\n+\t\t\t\t (commit2 =\n+\t\t\t\t lookup_commit_reference_by_name(p)) &&\n+\t\t\t\t commit2->util)\n+\t\t\t\t/* found by commit name */\n+\t\t\t\ti2 = (struct todo_item *)commit2->util\n+\t\t\t\t\t- todo_list.items;\n+\t\t\telse {\n+\t\t\t\t/* copy can be a prefix of the commit subject */\n+\t\t\t\tfor (i2 = 0; i2 < i; i2++)\n+\t\t\t\t\tif (subjects[i2] &&\n+\t\t\t\t\t starts_with(subjects[i2], p))\n+\t\t\t\t\t\tbreak;\n+\t\t\t\tif (i2 == i)\n+\t\t\t\t\ti2 = -1;\n+\t\t\t}\n+\t\t}\n+\t\tif (i2 >= 0) {\n+\t\t\trearranged = 1;\n+\t\t\ttodo_list.items[i].command =\n+\t\t\t\tstarts_with(subject, \"fixup!\") ?\n+\t\t\t\tTODO_FIXUP : TODO_SQUASH;\n+\t\t\tif (next[i2] < 0)\n+\t\t\t\tnext[i2] = i;\n+\t\t\telse\n+\t\t\t\tnext[tail[i2]] = i;\n+\t\t\ttail[i2] = i;\n+\t\t} else if (!hashmap_get_from_hash(&subject2item,\n+\t\t\t\t\t\tstrhash(subject), subject)) {\n+\t\t\tFLEX_ALLOC_MEM(entry, subject, subject, subject_len);\n+\t\t\tentry->i = i;\n+\t\t\thashmap_entry_init(entry, strhash(entry->subject));\n+\t\t\thashmap_put(&subject2item, entry);\n+\t\t}\n+\t}\n+\n+\tif (rearranged) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tfor (i = 0; i < todo_list.nr; i++) {\n+\t\t\tenum todo_command command = todo_list.items[i].command;\n+\t\t\tint cur = i;\n+\n+\t\t\t/*\n+\t\t\t * Initially, all commands are 'pick's. If it is a\n+\t\t\t * fixup or a squash now, we have rearranged it.\n+\t\t\t */\n+\t\t\tif (is_fixup(command))\n+\t\t\t\tcontinue;\n+\n+\t\t\twhile (cur >= 0) {\n+\t\t\t\tint offset = todo_list.items[cur].offset_in_buf;\n+\t\t\t\tint end_offset = cur + 1 < todo_list.nr ?\n+\t\t\t\t\ttodo_list.items[cur + 1].offset_in_buf :\n+\t\t\t\t\ttodo_list.buf.len;\n+\t\t\t\tchar *bol = todo_list.buf.buf + offset;\n+\t\t\t\tchar *eol = todo_list.buf.buf + end_offset;\n+\n+\t\t\t\t/* replace 'pick', by 'fixup' or 'squash' */\n+\t\t\t\tcommand = todo_list.items[cur].command;\n+\t\t\t\tif (is_fixup(command)) {\n+\t\t\t\t\tstrbuf_addstr(&buf,\n+\t\t\t\t\t\ttodo_command_info[command].str);\n+\t\t\t\t\tbol += strcspn(bol, \" \\t\");\n+\t\t\t\t}\n+\n+\t\t\t\tstrbuf_add(&buf, bol, eol - bol);\n+\n+\t\t\t\tcur = next[cur];\n+\t\t\t}\n+\t\t}\n+\n+\t\tfd = open(todo_file, O_WRONLY);\n+\t\tif (fd < 0)\n+\t\t\tres = error_errno(_(\"could not open '%s'\"), todo_file);\n+\t\telse if (write(fd, buf.buf, buf.len) < 0)\n+\t\t\tres = error_errno(_(\"could not read '%s'.\"), todo_file);\n+\t\telse if (ftruncate(fd, buf.len) < 0)\n+\t\t\tres = error_errno(_(\"could not finish '%s'\"),\n+\t\t\t\t\t todo_file);\n+\t\tclose(fd);\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n+\tfree(next);\n+\tfree(tail);\n+\tfor (i = 0; i < todo_list.nr; i++)\n+\t\tfree(subjects[i]);\n+\tfree(subjects);\n+\thashmap_free(&subject2item, 1);\n+\ttodo_list_release(&todo_list);\n+\n+\treturn res;\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex 04a57e09a1d..6f3d3df82c0 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -51,6 +51,7 @@ int sequencer_make_script(int keep_empty, FILE *out,\n int transform_todo_ids(int shorten_ids);\n int check_todo_list(void);\n int skip_unnecessary_picks(void);\n+int rearrange_squash(void);\n \n extern const char sign_off_header[];\n \ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 62cb977e4ec..e364c12622f 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -290,7 +290,7 @@ set_backup_editor () {\n \ttest_set_editor \"$PWD/backup-editor.sh\"\n }\n \n-test_expect_failure 'autosquash with multiple empty patches' '\n+test_expect_success 'autosquash with multiple empty patches' '\n \ttest_tick &&\n \tgit commit --allow-empty -m \"empty\" &&\n \ttest_tick &&\n-- \n2.13.1.windows.1.1.ga36e14b3aaa\n"},{"id":"322415","messageId":"xmqqo9toyhd9.fsf@gitster.mtv.corp.google.com","threadId":"46181","inReplyTo":"cover.1497444257.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v5 00/10] The final building block for a faster rebase -i","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-15T23:13:22Z","receivedAt":"2017-06-15T23:18:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Changes since v4:\n>\n> - replaced the \"sha1s\" part of the names by \"ids\", to reflect the\n> current effort to move away from the cryptographically unsafe SHA-1\n>\n> - replaced the confusing term \"instruction sheet\" in an error message by\n> the more commonly used \"todo list\"\n\nBoth are good changes. Once this follows the API properly, it would\nbe perfect.\n\nWill replace.\n"},{"id":"322430","messageId":"a12767b9-a947-e0a2-fc82-fc25992fd0d1@gmail.com","threadId":"46181","inReplyTo":"72bbfcae2abcb14f6b1288051a244faadbee29e0.1497444257.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper","fromName":"Liam Beguin","fromEmail":"liambeguin@gmail.com","sentAt":"2017-06-16T02:39:29Z","receivedAt":"2017-06-16T02:39:40Z","isPatch":true,"sender":{"key":"liambeguin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3811160?v=4"},"body":"Hi, \n\nOn 14/06/17 09:08 AM, Johannes Schindelin wrote:\n> diff --git a/sequencer.c b/sequencer.c\n> index a697906d463..a0e020dab09 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2640,3 +2640,110 @@ int check_todo_list(void)\n> \n> \treturn res;\n> }\n> +\n> +/* skip picking commits whose parents are unchanged */\n> +int skip_unnecessary_picks(void)\n> +{\n> +\tconst char *todo_file = rebase_path_todo();\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct todo_list todo_list = TODO_LIST_INIT;\n> +\tstruct object_id onto_oid, *oid = &onto_oid, *parent_oid;\n> +\tint fd, i;\n> +\n> +\tif (!read_oneliner(&buf, rebase_path_onto(), 0))\n> +\t\treturn error(_(\"could not read 'onto'\"));\n> +\tif (get_sha1(buf.buf, onto_oid.hash)) {\n\nI missed this last time but we could also replace `get_sha1` with `get_oid`\n\n> +\t\tstrbuf_release(&buf);\n> +\t\treturn error(_(\"need a HEAD to fixup\"));\n> +\t}\n> +\tstrbuf_release(&buf);\n> +\n> +\tfd = open(todo_file, O_RDONLY);\n> +\tif (fd < 0) {\n> +\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n> +\t}\n> +\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n> +\t\tclose(fd);\n> +\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n> +\t}\n> +\tclose(fd);\n> +\tif (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {\n> +\t\ttodo_list_release(&todo_list);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tfor (i = 0; i < todo_list.nr; i++) {\n> +\t\tstruct todo_item *item = todo_list.items + i;\n> +\n> +\t\tif (item->command >= TODO_NOOP)\n> +\t\t\tcontinue;\n> +\t\tif (item->command != TODO_PICK)\n> +\t\t\tbreak;\n> +\t\tif (parse_commit(item->commit)) {\n> +\t\t\ttodo_list_release(&todo_list);\n> +\t\t\treturn error(_(\"could not parse commit '%s'\"),\n> +\t\t\t\toid_to_hex(&item->commit->object.oid));\n> +\t\t}\n> +\t\tif (!item->commit->parents)\n> +\t\t\tbreak; /* root commit */\n> +\t\tif (item->commit->parents->next)\n> +\t\t\tbreak; /* merge commit */\n> +\t\tparent_oid = &item->commit->parents->item->object.oid;\n> +\t\tif (hashcmp(parent_oid->hash, oid->hash))\n> +\t\t\tbreak;\n> +\t\toid = &item->commit->object.oid;\n> +\t}\n> +\tif (i > 0) {\n> +\t\tint offset = i < todo_list.nr ?\n> +\t\t\ttodo_list.items[i].offset_in_buf : todo_list.buf.len;\n> +\t\tconst char *done_path = rebase_path_done();\n> +\n> +\t\tfd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);\n> +\t\tif (fd < 0) {\n> +\t\t\terror_errno(_(\"could not open '%s' for writing\"),\n> +\t\t\t\t done_path);\n> +\t\t\ttodo_list_release(&todo_list);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tif (write_in_full(fd, todo_list.buf.buf, offset) < 0) {\n> +\t\t\terror_errno(_(\"could not write to '%s'\"), done_path);\n> +\t\t\ttodo_list_release(&todo_list);\n> +\t\t\tclose(fd);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tclose(fd);\n> +\n> +\t\tfd = open(rebase_path_todo(), O_WRONLY, 0666);\n> +\t\tif (fd < 0) {\n> +\t\t\terror_errno(_(\"could not open '%s' for writing\"),\n> +\t\t\t\t rebase_path_todo());\n> +\t\t\ttodo_list_release(&todo_list);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tif (write_in_full(fd, todo_list.buf.buf + offset,\n> +\t\t\t\ttodo_list.buf.len - offset) < 0) {\n> +\t\t\terror_errno(_(\"could not write to '%s'\"),\n> +\t\t\t\t rebase_path_todo());\n> +\t\t\tclose(fd);\n> +\t\t\ttodo_list_release(&todo_list);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tif (ftruncate(fd, todo_list.buf.len - offset) < 0) {\n> +\t\t\terror_errno(_(\"could not truncate '%s'\"),\n> +\t\t\t\t rebase_path_todo());\n> +\t\t\ttodo_list_release(&todo_list);\n> +\t\t\tclose(fd);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tclose(fd);\n> +\n> +\t\ttodo_list.current = i;\n> +\t\tif (is_fixup(peek_command(&todo_list, 0)))\n> +\t\t\trecord_in_rewritten(oid, peek_command(&todo_list, 0));\n> +\t}\n> +\n> +\ttodo_list_release(&todo_list);\n> +\tprintf(\"%s\\n\", oid_to_hex(oid));\n> +\n> +\treturn 0;\n> +}\n> diff --git a/sequencer.h b/sequencer.h\n> index 878dd296f8c..04a57e09a1d 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -50,6 +50,7 @@ int sequencer_make_script(int keep_empty, FILE *out,\n> \n> int transform_todo_ids(int shorten_ids);\n> int check_todo_list(void);\n> +int skip_unnecessary_picks(void);\n> \n> extern const char sign_off_header[];\n> \n> \n\n - Liam \n"},{"id":"322460","messageId":"alpine.DEB.2.21.1.1706161550110.4200@virtualbox","threadId":"46181","inReplyTo":"xmqqo9toyhd9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 00/10] The final building block for a faster rebase -i","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-16T13:50:43Z","receivedAt":"2017-06-16T13:51:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 15 Jun 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Changes since v4:\n> >\n> > - replaced the \"sha1s\" part of the names by \"ids\", to reflect the\n> > current effort to move away from the cryptographically unsafe SHA-1\n> >\n> > - replaced the confusing term \"instruction sheet\" in an error message by\n> > the more commonly used \"todo list\"\n> \n> Both are good changes. Once this follows the API properly, it would\n> be perfect.\n\nOkay, then drop it. What you call \"properly\", I call bad programming, and\nI won't do it.\n\nCiao,\nDscho\n"},{"id":"322461","messageId":"alpine.DEB.2.21.1.1706161551030.4200@virtualbox","threadId":"46181","inReplyTo":"a12767b9-a947-e0a2-fc82-fc25992fd0d1@gmail.com","subject":"Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-16T13:56:31Z","receivedAt":"2017-06-16T13:56:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Liam,\n\nOn Thu, 15 Jun 2017, Liam Beguin wrote:\n\n> On 14/06/17 09:08 AM, Johannes Schindelin wrote:\n> > diff --git a/sequencer.c b/sequencer.c\n> > index a697906d463..a0e020dab09 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -2640,3 +2640,110 @@ int check_todo_list(void)\n> > \n> > \treturn res;\n> > }\n> > +\n> > +/* skip picking commits whose parents are unchanged */\n> > +int skip_unnecessary_picks(void)\n> > +{\n> > +\tconst char *todo_file = rebase_path_todo();\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tstruct todo_list todo_list = TODO_LIST_INIT;\n> > +\tstruct object_id onto_oid, *oid = &onto_oid, *parent_oid;\n> > +\tint fd, i;\n> > +\n> > +\tif (!read_oneliner(&buf, rebase_path_onto(), 0))\n> > +\t\treturn error(_(\"could not read 'onto'\"));\n> > +\tif (get_sha1(buf.buf, onto_oid.hash)) {\n> \n> I missed this last time but we could also replace `get_sha1` with `get_oid`\n\nGood point!\n\nI replaced this locally and force-pushed, but there is actually little\nchance of this patch series being integrated in a form with which I would\nbe comfortable.\n\nCiao,\nDscho\n"},{"id":"322513","messageId":"529f6cc7-ab2b-72bb-bc51-f4cca4e98652@gmail.com","threadId":"46181","inReplyTo":"alpine.DEB.2.21.1.1706161551030.4200@virtualbox","subject":"Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper","fromName":"Liam Beguin","fromEmail":"liambeguin@gmail.com","sentAt":"2017-06-17T22:48:47Z","receivedAt":"2017-06-17T22:48:56Z","isPatch":true,"sender":{"key":"liambeguin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3811160?v=4"},"body":"Hi Johannes, \n\nOn 16/06/17 09:56 AM, Johannes Schindelin wrote:\n> Hi Liam,\n> \n> On Thu, 15 Jun 2017, Liam Beguin wrote:\n> \n>> On 14/06/17 09:08 AM, Johannes Schindelin wrote:\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index a697906d463..a0e020dab09 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -2640,3 +2640,110 @@ int check_todo_list(void)\n>>> \n>>> \treturn res;\n>>> }\n>>> +\n>>> +/* skip picking commits whose parents are unchanged */\n>>> +int skip_unnecessary_picks(void)\n>>> +{\n>>> +\tconst char *todo_file = rebase_path_todo();\n>>> +\tstruct strbuf buf = STRBUF_INIT;\n>>> +\tstruct todo_list todo_list = TODO_LIST_INIT;\n>>> +\tstruct object_id onto_oid, *oid = &onto_oid, *parent_oid;\n>>> +\tint fd, i;\n>>> +\n>>> +\tif (!read_oneliner(&buf, rebase_path_onto(), 0))\n>>> +\t\treturn error(_(\"could not read 'onto'\"));\n>>> +\tif (get_sha1(buf.buf, onto_oid.hash)) {\n>>\n>> I missed this last time but we could also replace `get_sha1` with `get_oid`\n> \n> Good point!\n> \n> I replaced this locally and force-pushed, but there is actually little\n> chance of this patch series being integrated in a form with which I would\n> be comfortable.\n\nIs there any chance of this moving forward? I was hoping to resend my path to\nabbreviate command names on top of this.\n\n> \n> Ciao,\n> Dscho\n> \nThanks,\n\n - Liam\n"},{"id":"322555","messageId":"alpine.DEB.2.21.1.1706191127450.57822@virtualbox","threadId":"46181","inReplyTo":"529f6cc7-ab2b-72bb-bc51-f4cca4e98652@gmail.com","subject":"Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-19T09:45:50Z","receivedAt":"2017-06-19T09:46:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Liam,\n\nOn Sat, 17 Jun 2017, Liam Beguin wrote:\n\n> On 16/06/17 09:56 AM, Johannes Schindelin wrote:\n> \n> > On Thu, 15 Jun 2017, Liam Beguin wrote:\n> > \n> >> On 14/06/17 09:08 AM, Johannes Schindelin wrote:\n> >>> diff --git a/sequencer.c b/sequencer.c\n> >>> index a697906d463..a0e020dab09 100644\n> >>> --- a/sequencer.c\n> >>> +++ b/sequencer.c\n> >>> @@ -2640,3 +2640,110 @@ int check_todo_list(void)\n> >>> \n> >>> \treturn res;\n> >>> }\n> >>> +\n> >>> +/* skip picking commits whose parents are unchanged */\n> >>> +int skip_unnecessary_picks(void)\n> >>> +{\n> >>> +\tconst char *todo_file = rebase_path_todo();\n> >>> +\tstruct strbuf buf = STRBUF_INIT;\n> >>> +\tstruct todo_list todo_list = TODO_LIST_INIT;\n> >>> +\tstruct object_id onto_oid, *oid = &onto_oid, *parent_oid;\n> >>> +\tint fd, i;\n> >>> +\n> >>> +\tif (!read_oneliner(&buf, rebase_path_onto(), 0))\n> >>> +\t\treturn error(_(\"could not read 'onto'\"));\n> >>> +\tif (get_sha1(buf.buf, onto_oid.hash)) {\n> >>\n> >> I missed this last time but we could also replace `get_sha1` with\n> >> `get_oid`\n> > \n> > Good point!\n> > \n> > I replaced this locally and force-pushed, but there is actually little\n> > chance of this patch series being integrated in a form with which I\n> > would be comfortable.\n> \n> Is there any chance of this moving forward? I was hoping to resend my\n> path to abbreviate command names on top of this.\n\nWe are at an impasse right now.\n\nJunio wants me to change this code:\n\n revs.pretty_given = 1;\n git_config_get_string(\"rebase.instructionFormat\", &format);\n if (!format || !*format) {\n free(format);\n format = xstrdup(\"%s\");\n }\n get_commit_format(format, &revs);\n free(format);\n pp.fmt = revs.commit_format;\n pp.output_encoding = get_log_output_encoding();\n\n if (setup_revisions(argc, argv, &revs, NULL) > 1)\n\t...\n\nwhich is reasonably compile-time safe, to something like this:\n\n\tstruct strbuf userformat = STRBUF_INIT;\n\tstruct argv_array args = ARGV_ARRAY_INIT;\n\t...\n\tfor (i = 0; i < argc; i++)\n\t\targv_array_push(&args, argv[i]);\n git_config_get_string(\"rebase.instructionFormat\", &format);\n if (!format || !*format)\n\t\targv_array_push(&args, \"--format=%s\");\n\telse {\n\t\tstrbuf_addf(&userformat, \"--format=%s\", format);\n argv_array_push(&args, userformat.buf);\n }\n\n if (setup_revisions(args.argc, args.argv, &revs, NULL) > 1)\n\t...\n\targv_array_clear(&args);\n\tstrbuf_release(&userformat);\n\nwhich is not compile-time safe, harder to read, sloppy coding in my book.\n\nThe reason for this suggestion is that one of the revision machinery's\nimplementation details is an ugly little semi-secret: the pretty-printing\nmachinery uses a global state, and that is why we need the \"pretty_given\"\nflag in the first place.\n\nJunio thinks that it would be better to not use the pretty_given flag in\nthis patch series. I disagree: It would be better to use the pretty_given\nflag, also as a good motivator to work on removing the global state in the\npretty-printing machinery.\n\nJunio wants to strong-arm me into accepting authorship for this sloppy\ncoding, and I simply won't do it.\n\nThat's why we are at an impasse right now.\n\nI am really, really sorry that this affects your patch series, as I had\nnot foreseen Junio's insistence on the sloppy coding when I suggested that\nyou rebase your work on top of my patch series. I just was really\nunprepared for this contention, and I am still surprised/shocked that this\nis even an issue up for discussion.\n\nBe that as it may, I see that this is a very bad experience for a\nmotivated contributor such as yourself. I am very sorry about that!\n\nSo it may actually be better for you to go forward with your original\npatch series targeting git-rebase--interactive.sh.\n\nMy apologies,\nDscho\n"},{"id":"322571","messageId":"20170619161051.silyrlwrnjjspoxe@sigill.intra.peff.net","threadId":"46181","inReplyTo":"alpine.DEB.2.21.1.1706191127450.57822@virtualbox","subject":"Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-19T16:10:51Z","receivedAt":"2017-06-19T16:11:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 19, 2017 at 11:45:50AM +0200, Johannes Schindelin wrote:\n\n> The reason for this suggestion is that one of the revision machinery's\n> implementation details is an ugly little semi-secret: the pretty-printing\n> machinery uses a global state, and that is why we need the \"pretty_given\"\n> flag in the first place.\n\nI think that's mis-stating Junio's complaint. The point is not the\npretty_given flag itself, which we know about and can work around. The\npoint is that we don't know what other similar problems we have or will\nhave due to future changes in the revision code.\n\nIn other words, there are two APIs: the one where C code manipulates\nrev_info directly, and the one where revision.c responds to string\narguments. From a maintenance perspective, it is easy for somebody make\na change that works for the latter but not the former.\n\nI do agree that the lack of compile-time safety for obvious mistakes\nlike \"--pertty\" is a downside, though. On the other hand, there are\nstrong run-time checks there, so the tests would catch it.\n\nI do not have a strong opinion myself in either direction.\n\n-Peff\n"},{"id":"322579","messageId":"xmqqinjrub9x.fsf@gitster.mtv.corp.google.com","threadId":"46181","inReplyTo":"20170619161051.silyrlwrnjjspoxe@sigill.intra.peff.net","subject":"Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-19T17:39:54Z","receivedAt":"2017-06-19T17:40:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jun 19, 2017 at 11:45:50AM +0200, Johannes Schindelin wrote:\n>\n>> The reason for this suggestion is that one of the revision machinery's\n>> implementation details is an ugly little semi-secret: the pretty-printing\n>> machinery uses a global state, and that is why we need the \"pretty_given\"\n>> flag in the first place.\n>\n> I think that's mis-stating Junio's complaint. The point is not the\n> pretty_given flag itself, which we know about and can work around. The\n> point is that we don't know what other similar problems we have or will\n> have due to future changes in the revision code.\n>\n> In other words, there are two APIs: the one where C code manipulates\n> rev_info directly, and the one where revision.c responds to string\n> arguments. From a maintenance perspective, it is easy for somebody make\n> a change that works for the latter but not the former.\n\nThere are (1) the API for users of revision traversal machinery and\n(2) the implementation detail of the API. Of course, the code that\nactually parses the plumbing and end-user supplied set of options\nneeds to manipulate rev_info directly, but we should avoid direct\nmanipulation when we can to future-proof the codebase.\n\nMy main complaint is that Johannes's \"compiler can catch mistakes\"\nis true only for the most trivial kind of mistakes.\n\nThe current implementation detail for reacting to --format=\"\"\ngiven by the revision traversal machinery happens to be to set three\nthings in rev_info structure: set verbose_header and pretty_given to\n1 and then call get_commit_format() on the format string. Dscho is\ncorrect to say that the compiler can catch a mistake that misspells,\nsay verbose_header as verbos_header. The compiler would not catch\nan equivalent mistake to misspell the option as --formta=\"\",\nso from that point of view, his argument may seem to make sense.\n\nBut the compiler would not help catching a copy-and-paste mistake to\ndo only two out of the three things needed (e.g. forgetting to set\npretty_given). If the code relies on setup_revisions() to react to\noptions just the way \"rev-list\" plumbing does, such a mistake cannot\nhappen in the first place---there is no need to worry about \"catching\".\n\nAs you clarified correctly, the writer of the code that _uses_ the\nmachinery, like the one in sequencer_make_script(), cannot\nanticipate how the internal implementation of reacting to the\nfeatures will change, and more importantly, it should not have to\nknow how it will change. By using the setup_revisions() API that\nuses the exactly the same parser as plumbing command \"git rev-list\"\ndoes, the forward compatibility is guaranteed. Copying and pasting\nthe current internal implementation detail will break that.\n\n> I do agree that the lack of compile-time safety for obvious mistakes\n> like \"--pertty\" is a downside, though. On the other hand, there are\n> strong run-time checks there, so the tests would catch it.\n\nTrue.\n\nAfter setup_revisions() returns, if there are unrecognized options\n(e.g. misspelt \"--pertty\"), that can be used as the indication of a\nprogramming error, as the new code is not even parsing arbitrary set\nof options given by the end-user.\n"},{"id":"322641","messageId":"7e5cd36f-772e-b73e-12cd-d52878bdf52c@gmail.com","threadId":"46181","inReplyTo":"alpine.DEB.2.21.1.1706191127450.57822@virtualbox","subject":"Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper","fromName":"Liam Beguin","fromEmail":"liambeguin@gmail.com","sentAt":"2017-06-20T02:35:01Z","receivedAt":"2017-06-20T02:35:19Z","isPatch":true,"sender":{"key":"liambeguin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3811160?v=4"},"body":"\n\nOn 19/06/17 05:45 AM, Johannes Schindelin wrote:\n> Hi Liam,\n> \n> On Sat, 17 Jun 2017, Liam Beguin wrote:\n> \n>> On 16/06/17 09:56 AM, Johannes Schindelin wrote:\n>>\n>>> On Thu, 15 Jun 2017, Liam Beguin wrote:\n>>>\n>>>> On 14/06/17 09:08 AM, Johannes Schindelin wrote:\n>>>>> diff --git a/sequencer.c b/sequencer.c\n>>>>> index a697906d463..a0e020dab09 100644\n>>>>> --- a/sequencer.c\n>>>>> +++ b/sequencer.c\n>>>>> @@ -2640,3 +2640,110 @@ int check_todo_list(void)\n>>>>> \n>>>>> \treturn res;\n>>>>> }\n>>>>> +\n>>>>> +/* skip picking commits whose parents are unchanged */\n>>>>> +int skip_unnecessary_picks(void)\n>>>>> +{\n>>>>> +\tconst char *todo_file = rebase_path_todo();\n>>>>> +\tstruct strbuf buf = STRBUF_INIT;\n>>>>> +\tstruct todo_list todo_list = TODO_LIST_INIT;\n>>>>> +\tstruct object_id onto_oid, *oid = &onto_oid, *parent_oid;\n>>>>> +\tint fd, i;\n>>>>> +\n>>>>> +\tif (!read_oneliner(&buf, rebase_path_onto(), 0))\n>>>>> +\t\treturn error(_(\"could not read 'onto'\"));\n>>>>> +\tif (get_sha1(buf.buf, onto_oid.hash)) {\n>>>>\n>>>> I missed this last time but we could also replace `get_sha1` with\n>>>> `get_oid`\n>>>\n>>> Good point!\n>>>\n>>> I replaced this locally and force-pushed, but there is actually little\n>>> chance of this patch series being integrated in a form with which I\n>>> would be comfortable.\n>>\n>> Is there any chance of this moving forward? I was hoping to resend my\n>> path to abbreviate command names on top of this.\n> \n> We are at an impasse right now.\n> \n> Junio wants me to change this code:\n> \n> revs.pretty_given = 1;\n> git_config_get_string(\"rebase.instructionFormat\", &format);\n> if (!format || !*format) {\n> free(format);\n> format = xstrdup(\"%s\");\n> }\n> get_commit_format(format, &revs);\n> free(format);\n> pp.fmt = revs.commit_format;\n> pp.output_encoding = get_log_output_encoding();\n> \n> if (setup_revisions(argc, argv, &revs, NULL) > 1)\n> \t...\n> \n> which is reasonably compile-time safe, to something like this:\n> \n> \tstruct strbuf userformat = STRBUF_INIT;\n> \tstruct argv_array args = ARGV_ARRAY_INIT;\n> \t...\n> \tfor (i = 0; i < argc; i++)\n> \t\targv_array_push(&args, argv[i]);\n> git_config_get_string(\"rebase.instructionFormat\", &format);\n> if (!format || !*format)\n> \t\targv_array_push(&args, \"--format=%s\");\n> \telse {\n> \t\tstrbuf_addf(&userformat, \"--format=%s\", format);\n> argv_array_push(&args, userformat.buf);\n> }\n> \n> if (setup_revisions(args.argc, args.argv, &revs, NULL) > 1)\n> \t...\n> \targv_array_clear(&args);\n> \tstrbuf_release(&userformat);\n> \n> which is not compile-time safe, harder to read, sloppy coding in my book.\n> \n> The reason for this suggestion is that one of the revision machinery's\n> implementation details is an ugly little semi-secret: the pretty-printing\n> machinery uses a global state, and that is why we need the \"pretty_given\"\n> flag in the first place.\n> \n> Junio thinks that it would be better to not use the pretty_given flag in\n> this patch series. I disagree: It would be better to use the pretty_given\n> flag, also as a good motivator to work on removing the global state in the\n> pretty-printing machinery.\n> \n> Junio wants to strong-arm me into accepting authorship for this sloppy\n> coding, and I simply won't do it.\n> \n> That's why we are at an impasse right now.\n> \n> I am really, really sorry that this affects your patch series, as I had\n> not foreseen Junio's insistence on the sloppy coding when I suggested that\n> you rebase your work on top of my patch series. I just was really\n> unprepared for this contention, and I am still surprised/shocked that this\n> is even an issue up for discussion.\n> \n> Be that as it may, I see that this is a very bad experience for a\n> motivated contributor such as yourself. I am very sorry about that!\n\nIt's not such a bad experience, I've found the people on the list to \nbe quite welcoming and supportive.\n\n> \n> So it may actually be better for you to go forward with your original\n> patch series targeting git-rebase--interactive.sh.\n\nI'll wait a bit longer to see how this goes and if it doesn't move, I'll\ntry re-sending an update to that series.\n\n> \n> My apologies,\n> Dscho\n> \n\nThanks,\nLiam \n"}]}