{"thread":{"id":"36963","subject":"[RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible","startedAt":"2014-06-19T03:28:26Z","lastAt":"2014-06-23T16:09:56Z","messageCount":16,"participants":["Fabian Ruch","Michael Haggerty","Junio C Hamano","Eric Sunshine","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"244618","messageId":"53A258DA.3030903@gmail.com","threadId":"36963","inReplyTo":"cover.1403146774.git.bafain@gmail.com","subject":"[RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-19T03:28:26Z","receivedAt":"2014-06-19T03:28:26Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"`pick_one` and `pick_one_preserving_merges` are wrappers around\n`cherry-pick` in `rebase --interactive`. They take the hash of a commit\nand build a `cherry-pick` command line that\n\n - respects the user-supplied merge options\n - disables complaints about empty commits\n - tries to fast-forward the rebase head unless rebase is forced\n - suppresses output unless the user requested higher verbosity\n - rewrites merge commits to point to their rebased parents.\n\n`pick_one` is used to implement not only `pick` but also `squash`, which\namends the previous commit rather than creating a new one. When\n`pick_one` is called from `squash`, it receives a second argument `-n`.\nThis tells `pick_one` to apply the changes to the index without\ncommitting them. Since the argument is almost directly passed to\n`cherry-pick`, we might want to do the same with other `cherry-pick`\noptions. Currently, `pick_one` expects `-n` to be the first and only\nargument except for the commit hash.\n\nPrepare `pick_one` for additional `cherry-pick` options by allowing `-n`\nto appear anywhere before the commit hash in the argument list. Loop\nover the argument list and pop each handled item until the commit hash\nis the only parameter left on the list. If an option is not supported,\nignore it and issue a warning on the console. Construct a new arguments\nlist `extra_args` of recognized options that shall be passed to\n`cherry-pick` on the command line.\n\nSigned-off-by: Fabian Ruch \n---\n git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------\n 1 file changed, 45 insertions(+), 16 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f267d8b..ea5514e 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -237,8 +237,26 @@ git_sequence_editor () {\n \n pick_one () {\n \tff=--ff\n+\textra_args=\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-n)\n+\t\t\tff=\n+\t\t\textra_args=\"$extra_args -n\"\n+\t\t\t;;\n+\t\t-*)\n+\t\t\twarn \"pick_one: ignored option -- $1\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\ttest $# -ne 1 && die \"pick_one: wrong number of arguments\"\n+\tsha1=$1\n \n-\tcase \"$1\" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac\n \tcase \"$force_rebase\" in '') ;; ?*) ff= ;; esac\n \toutput git rev-parse --verify $sha1 || die \"Invalid commit name: $sha1\"\n \n@@ -248,24 +266,35 @@ pick_one () {\n \tfi\n \n \ttest -d \"$rewritten\" &&\n-\t\tpick_one_preserving_merges \"$@\" && return\n+\t\tpick_one_preserving_merges $extra_args $sha1 && return\n \toutput eval git cherry-pick \\\n \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n-\t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n+\t\t\t\"$strategy_args\" $empty_args $ff $extra_args $sha1\n }\n \n pick_one_preserving_merges () {\n \tfast_forward=t\n-\tcase \"$1\" in\n-\t-n)\n-\t\tfast_forward=f\n-\t\tsha1=$2\n-\t\t;;\n-\t*)\n-\t\tsha1=$1\n-\t\t;;\n-\tesac\n-\tsha1=$(git rev-parse $sha1)\n+\tno_commit=\n+\textra_args=\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-n)\n+\t\t\tfast_forward=f\n+\t\t\textra_args=\"$extra_args -n\"\n+\t\t\tno_commit=y\n+\t\t\t;;\n+\t\t-*)\n+\t\t\twarn \"pick_one_preserving_merges: ignored option -- $1\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\ttest $# -ne 1 && die \"pick_one_preserving_merges: wrong number of arguments\"\n+\tsha1=$(git rev-parse $1)\n \n \tif test -f \"$state_dir\"/current-commit\n \tthen\n@@ -335,7 +364,7 @@ pick_one_preserving_merges () {\n \tf)\n \t\tfirst_parent=$(expr \"$new_parents\" : ' \\([^ ]*\\)')\n \n-\t\tif [ \"$1\" != \"-n\" ]\n+\t\tif test -z \"$no_commit\"\n \t\tthen\n \t\t\t# detach HEAD to current parent\n \t\t\toutput git checkout $first_parent 2> /dev/null ||\n@@ -344,7 +373,7 @@ pick_one_preserving_merges () {\n \n \t\tcase \"$new_parents\" in\n \t\t' '*' '*)\n-\t\t\ttest \"a$1\" = a-n && die \"Refusing to squash a merge: $sha1\"\n+\t\t\ttest -n \"$no_commit\" && die \"Refusing to squash a merge: $sha1\"\n \n \t\t\t# redo merge\n \t\t\tauthor_script_content=$(get_author_ident_from_commit $sha1)\n@@ -365,7 +394,7 @@ pick_one_preserving_merges () {\n \t\t*)\n \t\t\toutput eval git cherry-pick \\\n \t\t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n-\t\t\t\t\"$strategy_args\" \"$@\" ||\n+\t\t\t\t\"$strategy_args\" $extra_args $sha1 ||\n \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n \t\t\t;;\n \t\tesac\n-- \n2.0.0\n"},{"id":"244619","messageId":"53A258DE.10407@gmail.com","threadId":"36963","inReplyTo":"cover.1403146774.git.bafain@gmail.com","subject":"[RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-19T03:28:30Z","receivedAt":"2014-06-19T03:28:30Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"The to-do list command `reword` replays a commit like `pick` but lets\nthe user also edit the commit's log message. If one thinks of `pick`\nentries as scheduled `cherry-pick` command lines, then `reword` becomes\nan alias for the command line `cherry-pick --edit`. The porcelain\n`rebase--interactive` defines a function `do_pick` for processing the\n`pick` entries on to-do lists. Teach `do_pick` to handle the option\n`--edit` and reimplement `reword` in terms of `do_pick --edit`. Refer to\n`pick_one` for the way options are parsed.\n\n`do_pick` ought to act as a wrapper around `cherry-pick`. Unfortunately,\nit cannot just forward `--edit` to the `cherry-pick` command line. The\nassembled command line is executed within a command substitution for\ncontrolling the verbosity of `rebase--interactive`. Passing `--edit`\nwould either hang the terminal or clutter the substituted command output\nwith control sequences. Execute the `reword` code from `do_next` instead\nif the option `--edit` is specified. Adjust the fragment in two regards.\nFirstly, discard the additional message which is printed if an error\noccurs because\n\n Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.)\n Could not amend commit after successfully picking 1234567... Some change\n\nis more readable than and contains (almost) the same information as\n\n Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.)\n Could not amend commit after successfully picking 1234567... Some change\n This is most likely due to an empty commit message, or the pre-commit hook\n failed. If the pre-commit hook failed, you may need to resolve the issue before\n you are able to reword the commit.\n\n(It is true that a hook might not output any diagnosis but the same\nproblem arises when using git-commit directly. git-rebase at least\nprints a generic message saying that it failed to commit.) Secondly,\nsneak in additional git-commit arguments:\n\n - `--allow-empty` is missing: `rebase--interactive` suddenly fails if\n an empty commit is picked using `reword` instead of `pick`. The\n whether a commit is empty or not is not changed by an altered log\n message, so do as `pick` does. Add test.\n\n - `-n`: Hide the fact that we are committing several times by not\n executing the pre-commit hook. Caveat: The altered log message is not\n verified because `-n` also skips the commit-msg hook. Either the log\n message verification must be included in the post-rewrite hook or\n git-commit must support more fine-grained control over which hooks\n are executed.\n\n - `-q`: Hide the fact that we are committing several times by not\n printing the commit summary.\n\nSigned-off-by: Fabian Ruch \n---\n git-rebase--interactive.sh | 52 ++++++++++++++++++++++++++++++++++++-------\n t/t3404-rebase-interactive.sh | 8 +++++++\n 2 files changed, 52 insertions(+), 8 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex ea5514e..fffdfa5 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -490,7 +490,42 @@ record_in_rewritten() {\n \tesac\n }\n \n+# Apply the changes introduced by the given commit to the current head.\n+#\n+# do_pick [--edit] \n+#\n+# Wrapper around git-cherry-pick.\n+#\n+# <title>\n+# The commit message title of <commit>. Used for information\n+# purposes only.\n+#\n+# <commit>\n+# The commit to cherry-pick.\n+#\n+# -e, --edit\n+# After picking <commit>, open an editor and let the user edit the\n+# commit message. The editor contents becomes the commit message of\n+# the new head.\n do_pick () {\n+\tedit=\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-e|--edit)\n+\t\t\tedit=y\n+\t\t\t;;\n+\t\t-*)\n+\t\t\twarn \"do_pick: ignored option -- $1\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\ttest $# -ne 2 && die \"do_pick: wrong number of arguments\"\n+\n \tif test \"$(git rev-parse HEAD)\" = \"$squash_onto\"\n \tthen\n \t\t# Set the correct commit message and author info on the\n@@ -512,6 +547,14 @@ do_pick () {\n \t\tpick_one $1 ||\n \t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n \tfi\n+\n+\tif test -n \"$edit\"\n+\tthen\n+\t\tgit commit --allow-empty --amend --no-post-rewrite -n -q ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n+\t\t\twarn \"Could not amend commit after successfully picking $1... $2\"\n+\t\t\texit_with_patch $1 1\n+\t\t}\n+\tfi\n }\n \n do_next () {\n@@ -532,14 +575,7 @@ do_next () {\n \t\tcomment_for_reflog reword\n \n \t\tmark_action_done\n-\t\tdo_pick $sha1 \"$rest\"\n-\t\tgit commit --amend --no-post-rewrite ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n-\t\t\twarn \"Could not amend commit after successfully picking $sha1... $rest\"\n-\t\t\twarn \"This is most likely due to an empty commit message, or the pre-commit hook\"\n-\t\t\twarn \"failed. If the pre-commit hook failed, you may need to resolve the issue before\"\n-\t\t\twarn \"you are able to reword the commit.\"\n-\t\t\texit_with_patch $sha1 1\n-\t\t}\n+\t\tdo_pick --edit $sha1 \"$rest\"\n \t\trecord_in_rewritten $sha1\n \t\t;;\n \tedit|e)\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 8197ed2..9931143 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -75,6 +75,14 @@ test_expect_success 'rebase --keep-empty' '\n \ttest_line_count = 6 actual\n '\n \n+test_expect_success 'rebase --keep-empty' '\n+\tgit checkout emptybranch &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 reword 2\" git rebase --keep-empty -i HEAD~2 &&\n+\tgit log --oneline >actual &&\n+\ttest_line_count = 6 actual\n+'\n+\n test_expect_success 'rebase -i with the exec command' '\n \tgit checkout master &&\n \t(\n-- \n2.0.0\n"},{"id":"244620","messageId":"53A258E2.501@gmail.com","threadId":"36963","inReplyTo":"cover.1403146774.git.bafain@gmail.com","subject":"[RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-19T03:28:34Z","receivedAt":"2014-06-19T03:28:34Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"When `rebase` is executed with `--root` but no `--onto` is specified,\n`rebase` creates a sentinel commit which is replaced with the root\ncommit in three steps. This combination of options results never in a\nfast-forward.\n\n 1. The sentinel commit is forced into the authorship of the root\n commit.\n\n 2. The changes introduced by the root commit are applied to the index\n but not committed. If this step fails for whatever reason, all\n commit information will be there and the user can safely run\n `git-commit --amend` after resolving the problems.\n\n 3. The new root commit is created by squashing the changes into the\n sentinel commit which already carries the authorship of the\n cherry-picked root commit.\n\nThe command line used to create the commit in the third step specifies\neffectless and erroneous options. Remove those.\n\n - `--allow-empty-message` is erroneous: If the root's commit message is\n empty, the rebase shall fail like for any other commit that is on the\n to-do list and has an empty commit message.\n\n Fix the bug that git-rebase does not fail when the initial commit has\n an empty log message but is replayed using `--root` is specified.\n Add test.\n\n - `-C` is effectless: The commit being amended, which is the sentinel\n commit, already carries the authorship and log message of the\n cherry-picked root commit. The committer email and commit date fields\n are reset either way.\n\nAfter all, if step two fails, `rebase --continue` won't include these\nflags in the git-commit command line either.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n git-rebase--interactive.sh | 4 ++--\n t/t3412-rebase-root.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex fffdfa5..f09eeae 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -539,8 +539,8 @@ do_pick () {\n \t\tgit commit --allow-empty --allow-empty-message --amend \\\n \t\t\t --no-post-rewrite -n -q -C $1 &&\n \t\t\tpick_one -n $1 &&\n-\t\t\tgit commit --allow-empty --allow-empty-message \\\n-\t\t\t\t --amend --no-post-rewrite -n -q -C $1 \\\n+\t\t\tgit commit --allow-empty --amend \\\n+\t\t\t\t --no-post-rewrite -n -q \\\n \t\t\t\t ${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n \t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n \telse\ndiff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh\nindex 0b52105..3608db4 100755\n--- a/t/t3412-rebase-root.sh\n+++ b/t/t3412-rebase-root.sh\n@@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '\n \ttest_cmp expect-conflict-p out\n '\n \n+test_expect_success 'stop rebase --root on empty root log message' '\n+\t# create a root commit with a non-empty tree so that rebase does\n+\t# not fail because of an empty commit, and an empty log message\n+\techo root-commit >file &&\n+\tgit add file &&\n+\ttree=$(git write-tree) &&\n+\troot=$(git commit-tree $tree </dev/null) &&\n+\tgit checkout -b no-message-root-commit $root &&\n+\t# do not ff because otherwise neither the patch nor the message\n+\t# are looked at and checked for emptiness\n+\ttest_must_fail env EDITOR=true git rebase -i --force-rebase --root &&\n+\techo root-commit >file.expected &&\n+\ttest_cmp file file.expected &&\n+\tgit rebase --abort\n+'\n+\n+test_expect_success 'stop rebase --root on empty child log message' '\n+\t# create a root commit with a non-empty tree and provide a log\n+\t# message so that rebase does not fail until the root commit is\n+\t# successfully replayed\n+\techo root-commit >file &&\n+\tgit add file &&\n+\ttree=$(git write-tree) &&\n+\troot=$(git commit-tree $tree -m root-commit) &&\n+\tgit checkout -b no-message-child-commit $root &&\n+\t# create a child commit with a non-empty patch so that rebase\n+\t# does not fail because of an empty commit, but an empty log\n+\t# message\n+\techo child-commit >file &&\n+\tgit add file &&\n+\tgit commit --allow-empty-message --no-edit &&\n+\t# do not ff because otherwise neither the patch nor the message\n+\t# are looked at and checked for emptiness\n+\ttest_must_fail env EDITOR=true git rebase -i --force-rebase --root &&\n+\techo child-commit >file.expected &&\n+\ttest_cmp file file.expected &&\n+\tgit rebase --abort\n+'\n+\n test_done\n-- \n2.0.0\n"},{"id":"244621","messageId":"53A258E6.5050301@gmail.com","threadId":"36963","inReplyTo":"cover.1403146774.git.bafain@gmail.com","subject":"[RFC PATCH 4/7] rebase -i: Commit only once when rewriting picks","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-19T03:28:38Z","receivedAt":"2014-06-19T03:28:38Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"The options passed to `do_pick` determine whether the picked commit will\nbe rewritten or not. If the commit gets rewritten, because the user\nrequested to edit the commit message for instance, let `pick_one` merely\napply the changes introduced by the commit and do not commit the\nresulting tree yet. If the commit is replayed as is, leave it to\n`pick_one` to recreate the commit (possibly by fast-forwarding the\nhead). This makes it easier to combine git-commit options like `--edit`\nand `--amend` in `do_pick` because git-cherry-pick does not support\n`--amend`.\n\nIn the case of `--edit`, do not `exit_with_patch` but assign `rewrite`\nto pick the changes with `-n`. If the pick conflicts, no commit is\ncreated which we would have to amend when continuing the rebase. To\ncomplete the pick after the conflicts are resolved the user just resumes\nwith `git rebase --continue`.\n\nIf `rebase--interactive` is used to rebase a complete branch onto some\nhead, `rebase` creates a sentinel commit that requires special treatment\nby `do_pick`. Do not finalize the pick here either because its commit\nmessage can be altered as for any other pick. Since the orphaned root\ncommit gets a temporary parent, it is always rewritten. Safely use the\nrewrite infrastructure of `do_pick` to create the final commit.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n git-rebase--interactive.sh | 31 ++++++++++++++++++-------------\n 1 file changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f09eeae..f903599 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -63,7 +63,8 @@ msgnum=\"$state_dir\"/msgnum\n author_script=\"$state_dir\"/author-script\n \n # When an \"edit\" rebase command is being processed, the SHA1 of the\n-# commit to be edited is recorded in this file. When \"git rebase\n+# commit to be edited is recorded in this file. The same happens when\n+# rewriting a commit fails, for instance \"reword\". When \"git rebase\n # --continue\" is executed, if there are any staged changes then they\n # will be amended to the HEAD commit, but only provided the HEAD\n # commit is still the commit to be edited. When any other rebase\n@@ -508,12 +509,15 @@ record_in_rewritten() {\n # commit message. The editor contents becomes the commit message of\n # the new head.\n do_pick () {\n-\tedit=\n+\trewrite=\n+\trewrite_amend=\n+\trewrite_edit=\n \twhile test $# -gt 0\n \tdo\n \t\tcase \"$1\" in\n \t\t-e|--edit)\n-\t\t\tedit=y\n+\t\t\trewrite=y\n+\t\t\trewrite_edit=y\n \t\t\t;;\n \t\t-*)\n \t\t\twarn \"do_pick: ignored option -- $1\"\n@@ -528,6 +532,9 @@ do_pick () {\n \n \tif test \"$(git rev-parse HEAD)\" = \"$squash_onto\"\n \tthen\n+\t\trewrite=y\n+\t\trewrite_amend=y\n+\t\tgit rev-parse --verify HEAD >\"$amend\"\n \t\t# Set the correct commit message and author info on the\n \t\t# sentinel root before cherry-picking the original changes\n \t\t# without committing (-n). Finally, update the sentinel again\n@@ -538,22 +545,20 @@ do_pick () {\n \t\t# rebase --continue.\n \t\tgit commit --allow-empty --allow-empty-message --amend \\\n \t\t\t --no-post-rewrite -n -q -C $1 &&\n-\t\t\tpick_one -n $1 &&\n-\t\t\tgit commit --allow-empty --amend \\\n-\t\t\t\t --no-post-rewrite -n -q \\\n-\t\t\t\t ${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\tpick_one -n $1 ||\n \t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n \telse\n-\t\tpick_one $1 ||\n+\t\tpick_one ${rewrite:+-n} $1 ||\n \t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n \tfi\n \n-\tif test -n \"$edit\"\n+\tif test -n \"$rewrite\"\n \tthen\n-\t\tgit commit --allow-empty --amend --no-post-rewrite -n -q ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n-\t\t\twarn \"Could not amend commit after successfully picking $1... $2\"\n-\t\t\texit_with_patch $1 1\n-\t\t}\n+\t\tgit commit --allow-empty --no-post-rewrite -n -q \\\n+\t\t\t ${rewrite_amend:+--amend} \\\n+\t\t\t ${rewrite_edit:+--edit} \\\n+\t\t\t ${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n+\t\t\tdie_with_patch $1 \"Could not rewrite commit after successfully picking $1... $2\"\n \tfi\n }\n \n-- \n2.0.0\n"},{"id":"244622","messageId":"53A258E9.7030909@gmail.com","threadId":"36963","inReplyTo":"cover.1403146774.git.bafain@gmail.com","subject":"[RFC PATCH 5/7] rebase -i: Do not die in do_pick","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-19T03:28:41Z","receivedAt":"2014-06-19T03:28:41Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Since `do_pick` might be executed in a sub-shell (a modified author\nenvironment for instance), calling `die` in `do_pick` has no effect but\nexiting the sub-shell with a failure exit status. Moreover, if `do_pick`\nis called while a squash or fixup is happening, `die_with_patch` will\ndiscard `$squash_msg` as commit message. Indicate an error in `do_pick`\nusing return statements and properly kill the script at the call sites.\nRemove unused commit message title argument from `do_pick`'s signature.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n git-rebase--interactive.sh | 33 ++++++++++++++++++---------------\n 1 file changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f903599..e4992dc 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -493,14 +493,10 @@ record_in_rewritten() {\n \n # Apply the changes introduced by the given commit to the current head.\n #\n-# do_pick [--edit] <commit> <title>\n+# do_pick [--edit] <commit>\n #\n # Wrapper around git-cherry-pick.\n #\n-# <title>\n-# The commit message title of <commit>. Used for information\n-# purposes only.\n-#\n # <commit>\n # The commit to cherry-pick.\n #\n@@ -508,6 +504,12 @@ record_in_rewritten() {\n # After picking <commit>, open an editor and let the user edit the\n # commit message. The editor contents becomes the commit message of\n # the new head.\n+#\n+# The return value is 1 if applying the changes resulted in a conflict\n+# and 2 if the specified arguments were incorrect. If the changes could\n+# be applied successfully but creating the commit failed, a value\n+# greater than 2 is returned. No commit is created in either case and\n+# the index is left with the (conflicting) changes in place.\n do_pick () {\n \trewrite=\n \trewrite_amend=\n@@ -528,7 +530,11 @@ do_pick () {\n \t\tesac\n \t\tshift\n \tdone\n-\ttest $# -ne 2 && die \"do_pick: wrong number of arguments\"\n+\tif test $# -ne 1\n+\tthen\n+\t\twarn \"do_pick: wrong number of arguments\"\n+\t\treturn 2\n+\tfi\n \n \tif test \"$(git rev-parse HEAD)\" = \"$squash_onto\"\n \tthen\n@@ -545,11 +551,9 @@ do_pick () {\n \t\t# rebase --continue.\n \t\tgit commit --allow-empty --allow-empty-message --amend \\\n \t\t\t --no-post-rewrite -n -q -C $1 &&\n-\t\t\tpick_one -n $1 ||\n-\t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n+\t\t\tpick_one -n $1 || return 1\n \telse\n-\t\tpick_one ${rewrite:+-n} $1 ||\n-\t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n+\t\tpick_one ${rewrite:+-n} $1 || return 1\n \tfi\n \n \tif test -n \"$rewrite\"\n@@ -557,8 +561,7 @@ do_pick () {\n \t\tgit commit --allow-empty --no-post-rewrite -n -q \\\n \t\t\t ${rewrite_amend:+--amend} \\\n \t\t\t ${rewrite_edit:+--edit} \\\n-\t\t\t ${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n-\t\t\tdie_with_patch $1 \"Could not rewrite commit after successfully picking $1... $2\"\n+\t\t\t ${gpg_sign_opt:+\"$gpg_sign_opt\"} || return 3\n \tfi\n }\n \n@@ -573,21 +576,21 @@ do_next () {\n \t\tcomment_for_reflog pick\n \n \t\tmark_action_done\n-\t\tdo_pick $sha1 \"$rest\"\n+\t\tdo_pick $sha1 || die_with_patch $sha1 \"Could not apply $sha1... $rest\"\n \t\trecord_in_rewritten $sha1\n \t\t;;\n \treword|r)\n \t\tcomment_for_reflog reword\n \n \t\tmark_action_done\n-\t\tdo_pick --edit $sha1 \"$rest\"\n+\t\tdo_pick --edit $sha1 || die_with_patch $sha1 \"Could not apply $sha1... $rest\"\n \t\trecord_in_rewritten $sha1\n \t\t;;\n \tedit|e)\n \t\tcomment_for_reflog edit\n \n \t\tmark_action_done\n-\t\tdo_pick $sha1 \"$rest\"\n+\t\tdo_pick $sha1 || die_with_patch $sha1 \"Could not apply $sha1... $rest\"\n \t\twarn \"Stopped at $sha1... $rest\"\n \t\texit_with_patch $sha1 0\n \t\t;;\n-- \n2.0.0\n"},{"id":"244623","messageId":"53A258ED.1020604@gmail.com","threadId":"36963","inReplyTo":"cover.1403146774.git.bafain@gmail.com","subject":"[RFC PATCH 6/7] rebase -i: Prepare for squash in terms of do_pick --amend","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-19T03:28:45Z","receivedAt":"2014-06-19T03:28:45Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Rewrite `squash` and `fixup` handling in `do_next` using the atomic\nsequence\n\n pick_one\n commit\n\nin order to test the correctness of a single `do_squash` or parametrized\n`do_pick` and make the subsequent patch reimplementing `squash` in terms\nof such a single function more readable.\n\nMight be squashed into the subsequent commit.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n git-rebase--interactive.sh | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex e4992dc..ada520d 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -613,15 +613,15 @@ do_next () {\n \t\tauthor_script_content=$(get_author_ident_from_commit HEAD)\n \t\techo \"$author_script_content\" > \"$author_script\"\n \t\teval \"$author_script_content\"\n-\t\tif ! pick_one -n $sha1\n-\t\tthen\n-\t\t\tgit rev-parse --verify HEAD >\"$amend\"\n-\t\t\tdie_failed_squash $sha1 \"$rest\"\n-\t\tfi\n \t\tcase \"$(peek_next_command)\" in\n \t\tsquash|s|fixup|f)\n \t\t\t# This is an intermediate commit; its message will only be\n \t\t\t# used in case of trouble. So use the long version:\n+\t\t\tif ! pick_one -n $sha1\n+\t\t\tthen\n+\t\t\t\tgit rev-parse --verify HEAD >\"$amend\"\n+\t\t\t\tdie_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n+\t\t\tfi\n \t\t\tdo_with_author output git commit --amend --no-verify -F \"$squash_msg\" \\\n \t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n \t\t\t\tdie_failed_squash $sha1 \"$rest\"\n@@ -630,12 +630,22 @@ do_next () {\n \t\t\t# This is the final command of this squash/fixup group\n \t\t\tif test -f \"$fixup_msg\"\n \t\t\tthen\n+\t\t\t\tif ! pick_one -n $sha1\n+\t\t\t\tthen\n+\t\t\t\t\tgit rev-parse --verify HEAD >\"$amend\"\n+\t\t\t\t\tdie_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n+\t\t\t\tfi\n \t\t\t\tdo_with_author git commit --amend --no-verify -F \"$fixup_msg\" \\\n \t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n \t\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n \t\t\telse\n \t\t\t\tcp \"$squash_msg\" \"$GIT_DIR\"/SQUASH_MSG || exit\n \t\t\t\trm -f \"$GIT_DIR\"/MERGE_MSG\n+\t\t\t\tif ! pick_one -n $sha1\n+\t\t\t\tthen\n+\t\t\t\t\tgit rev-parse --verify HEAD >\"$amend\"\n+\t\t\t\t\tdie_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n+\t\t\t\tfi\n \t\t\t\tdo_with_author git commit --amend --no-verify -F \"$GIT_DIR\"/SQUASH_MSG -e \\\n \t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n \t\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n-- \n2.0.0\n"},{"id":"244624","messageId":"53A258F2.8050300@gmail.com","threadId":"36963","inReplyTo":"cover.1403146774.git.bafain@gmail.com","subject":"[RFC PATCH 7/7] rebase -i: Teach do_pick the options --amend and --file","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-19T03:28:50Z","receivedAt":"2014-06-19T03:28:50Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"The to-do list command `squash` and its close relative `fixup` replay\nthe changes of a commit like `pick` but do not recreate the commit.\nInstead they replace the previous commit with a new commit that also\nintroduces the changes of the squashed commit. This is roughly like\ncherry-picking without committing and using git-commit to amend the\nprevious commit.\n\nThe to-do list\n\n pick a Some changes\n squash b Some more changes\n\ngets translated into the sequence of git commands\n\n git cherry-pick a\n git cherry-pick -n b\n git commit --amend\n\nand if `cherry-pick` supported `--amend` this would look even more like\nthe to-do list it is based on\n\n git cherry-pick a\n git cherry-pick --amend b.\n\nSince `do_pick` takes care of `pick` entries and the above suggests\n`squash` as an alias for `pick --amend`, teach `do_pick` to handle the\noption `--amend` and reimplement `squash` in terms of `do_pick --amend`.\nAlso teach it the option `--file` which is used to specify `$squash_msg`\nas commit message.\n\nBoth `--amend` and `--file` are commit rewriting options. If they are\nencountered during options parsing, assign `rewrite` and pass `--amend`\n(`--file` respectively) to the rewrite command. Be careful when\n`--amend` is used to pick a root commit because HEAD might point to the\nsentinel commit but there is still nothing to amend. Be sure to\ninitialize `$amend` so that commits are squashed even when `rebase` is\ninterrupted for resolving conflicts. It is not a mistake to do the\ninitialization regardless of any conflicts because `$amend` is always\ncleared before the next to-do item is processed.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n\nNotes:\n A question about when to enable the post-rewrite hook.\n \n `rebase` collects the hashes of all processed commits using\n `record_in_rewritten` and runs the post-rewrite script after the rebase\n is complete. Two points seem to confuse me.\n \n 1) For a `pick` the hash is `record_in_rewritten` regardless of whether\n the hash changed or not (the commit was recreated or the head was\n fast-forwarded). Ok, the hook can figure that out. Is this behaviour\n intended?\n \n 2) For a `reword` the amend disables the post-rewrite hook but for a\n `squash` (or `fixup`) the hook is executed each time the squash\n commit is amended. Does not this result in the hook being executed\n twice for each scheduled `squash` command? Once for the amend and\n once for the rebase. The hook most likely does not figure that out.\n \n This patch never executes the post-rewrite hook when processing the\n to-do list. The execution after the rebase is finished is still\n conducted. I am uncertain whether this is correct. The tests seem to\n succeed with both implementations.\n\n git-rebase--interactive.sh | 65 ++++++++++++++++++++++++++++------------------\n 1 file changed, 40 insertions(+), 25 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex ada520d..5ddc59d 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -493,7 +493,7 @@ record_in_rewritten() {\n \n # Apply the changes introduced by the given commit to the current head.\n #\n-# do_pick [--edit] <commit>\n+# do_pick [--file <file>] [--amend] [--edit] <commit>\n #\n # Wrapper around git-cherry-pick.\n #\n@@ -505,6 +505,17 @@ record_in_rewritten() {\n # commit message. The editor contents becomes the commit message of\n # the new head.\n #\n+# --amend\n+# After picking <commit>, replace the current head commit with a new\n+# commit that also introduces the changes of <commit>.\n+#\n+# _This is not a git-cherry-pick option._\n+#\n+# -F <file>, --file <file>\n+# Take the commit message from the given file.\n+#\n+# _This is not a git-cherry-pick option._\n+#\n # The return value is 1 if applying the changes resulted in a conflict\n # and 2 if the specified arguments were incorrect. If the changes could\n # be applied successfully but creating the commit failed, a value\n@@ -514,9 +525,30 @@ do_pick () {\n \trewrite=\n \trewrite_amend=\n \trewrite_edit=\n+\trewrite_message=\n \twhile test $# -gt 0\n \tdo\n \t\tcase \"$1\" in\n+\t\t-F|--file)\n+\t\t\tif test $# -eq 0\n+\t\t\tthen\n+\t\t\t\twarn \"do_pick: option --file specified but no <file> given\"\n+\t\t\t\treturn 2\n+\t\t\tfi\n+\t\t\trewrite=y\n+\t\t\trewrite_message=$2\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--amend)\n+\t\t\tif test \"$(git rev-parse HEAD)\" = \"$squash_onto\" || ! git rev-parse --verify HEAD\n+\t\t\tthen\n+\t\t\t\twarn \"do_pick: nothing to amend\"\n+\t\t\t\treturn 2\n+\t\t\tfi\n+\t\t\trewrite=y\n+\t\t\trewrite_amend=y\n+\t\t\tgit rev-parse --verify HEAD >\"$amend\"\n+\t\t\t;;\n \t\t-e|--edit)\n \t\t\trewrite=y\n \t\t\trewrite_edit=y\n@@ -561,6 +593,7 @@ do_pick () {\n \t\tgit commit --allow-empty --no-post-rewrite -n -q \\\n \t\t\t ${rewrite_amend:+--amend} \\\n \t\t\t ${rewrite_edit:+--edit} \\\n+\t\t\t ${rewrite_message:+--file \"$rewrite_message\"} \\\n \t\t\t ${gpg_sign_opt:+\"$gpg_sign_opt\"} || return 3\n \tfi\n }\n@@ -617,38 +650,20 @@ do_next () {\n \t\tsquash|s|fixup|f)\n \t\t\t# This is an intermediate commit; its message will only be\n \t\t\t# used in case of trouble. So use the long version:\n-\t\t\tif ! pick_one -n $sha1\n-\t\t\tthen\n-\t\t\t\tgit rev-parse --verify HEAD >\"$amend\"\n-\t\t\t\tdie_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n-\t\t\tfi\n-\t\t\tdo_with_author output git commit --amend --no-verify -F \"$squash_msg\" \\\n-\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n-\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n+\t\t\tdo_with_author do_pick --amend -F \"$squash_msg\" $sha1 \\\n+\t\t\t\t|| die_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n \t\t\t;;\n \t\t*)\n \t\t\t# This is the final command of this squash/fixup group\n \t\t\tif test -f \"$fixup_msg\"\n \t\t\tthen\n-\t\t\t\tif ! pick_one -n $sha1\n-\t\t\t\tthen\n-\t\t\t\t\tgit rev-parse --verify HEAD >\"$amend\"\n-\t\t\t\t\tdie_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n-\t\t\t\tfi\n-\t\t\t\tdo_with_author git commit --amend --no-verify -F \"$fixup_msg\" \\\n-\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n-\t\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n+\t\t\t\tdo_with_author do_pick --amend -F \"$fixup_msg\" $sha1 \\\n+\t\t\t\t\t|| die_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n \t\t\telse\n \t\t\t\tcp \"$squash_msg\" \"$GIT_DIR\"/SQUASH_MSG || exit\n \t\t\t\trm -f \"$GIT_DIR\"/MERGE_MSG\n-\t\t\t\tif ! pick_one -n $sha1\n-\t\t\t\tthen\n-\t\t\t\t\tgit rev-parse --verify HEAD >\"$amend\"\n-\t\t\t\t\tdie_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n-\t\t\t\tfi\n-\t\t\t\tdo_with_author git commit --amend --no-verify -F \"$GIT_DIR\"/SQUASH_MSG -e \\\n-\t\t\t\t\t${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n-\t\t\t\t\tdie_failed_squash $sha1 \"$rest\"\n+\t\t\t\tdo_with_author do_pick --amend -F \"$GIT_DIR\"/SQUASH_MSG -e $sha1 \\\n+\t\t\t\t\t|| die_failed_squash $sha1 \"Could not apply $sha1... $rest\"\n \t\t\tfi\n \t\t\trm -f \"$squash_msg\" \"$fixup_msg\"\n \t\t\t;;\n-- \n2.0.0\n"},{"id":"244753","messageId":"53A439B2.7000106@alum.mit.edu","threadId":"36963","inReplyTo":"53A258DA.3030903@gmail.com","subject":"Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-06-20T13:40:02Z","receivedAt":"2014-06-20T13:40:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/19/2014 05:28 AM, Fabian Ruch wrote:\n> `pick_one` and `pick_one_preserving_merges` are wrappers around\n> `cherry-pick` in `rebase --interactive`. They take the hash of a commit\n> and build a `cherry-pick` command line that\n> \n> - respects the user-supplied merge options\n> - disables complaints about empty commits\n> - tries to fast-forward the rebase head unless rebase is forced\n> - suppresses output unless the user requested higher verbosity\n> - rewrites merge commits to point to their rebased parents.\n> \n> `pick_one` is used to implement not only `pick` but also `squash`, which\n> amends the previous commit rather than creating a new one. When\n> `pick_one` is called from `squash`, it receives a second argument `-n`.\n> This tells `pick_one` to apply the changes to the index without\n> committing them. Since the argument is almost directly passed to\n> `cherry-pick`, we might want to do the same with other `cherry-pick`\n> options. Currently, `pick_one` expects `-n` to be the first and only\n> argument except for the commit hash.\n> \n> Prepare `pick_one` for additional `cherry-pick` options by allowing `-n`\n> to appear anywhere before the commit hash in the argument list. Loop\n> over the argument list and pop each handled item until the commit hash\n> is the only parameter left on the list. If an option is not supported,\n> ignore it and issue a warning on the console. Construct a new arguments\n> list `extra_args` of recognized options that shall be passed to\n> `cherry-pick` on the command line.\n> \n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n> git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------\n> 1 file changed, 45 insertions(+), 16 deletions(-)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index f267d8b..ea5514e 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -237,8 +237,26 @@ git_sequence_editor () {\n> \n> pick_one () {\n> \tff=--ff\n> +\textra_args=\n> +\twhile test $# -gt 0\n> +\tdo\n> +\t\tcase \"$1\" in\n> +\t\t-n)\n> +\t\t\tff=\n> +\t\t\textra_args=\"$extra_args -n\"\n> +\t\t\t;;\n> +\t\t-*)\n> +\t\t\twarn \"pick_one: ignored option -- $1\"\n> +\t\t\t;;\n\nThis is an internal interface, right? I.e., user input isn't being\nprocessed here? If so, then the presence of an unrecognized option is a\nbug and it is preferable to \"die\" here rather than \"warn\".\n\nThe same below and in at least one later commit.\n\n> +\t\t*)\n> +\t\t\tbreak\n> +\t\t\t;;\n> +\t\tesac\n> +\t\tshift\n> +\tdone\n> +\ttest $# -ne 1 && die \"pick_one: wrong number of arguments\"\n> +\tsha1=$1\n> \n> -\tcase \"$1\" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac\n> \tcase \"$force_rebase\" in '') ;; ?*) ff= ;; esac\n> \toutput git rev-parse --verify $sha1 || die \"Invalid commit name: $sha1\"\n> \n> @@ -248,24 +266,35 @@ pick_one () {\n> \tfi\n> \n> \ttest -d \"$rewritten\" &&\n> -\t\tpick_one_preserving_merges \"$@\" && return\n> +\t\tpick_one_preserving_merges $extra_args $sha1 && return\n> \toutput eval git cherry-pick \\\n> \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n> -\t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n> +\t\t\t\"$strategy_args\" $empty_args $ff $extra_args $sha1\n> }\n\nIt might be confusing that extra_args is used both in pick_one and in\npick_one_preserving_merges. Since these are not local variables, the\ncall to the latter changes the value of the variable in the former. I\ndon't know if that could be a problem now (can\npick_one_preserving_merges return with a nonzero exit code?) but even if\nnot, it is a trap for future developers. I recommend giving the two\nvariables different names.\n\n> pick_one_preserving_merges () {\n> \tfast_forward=t\n> -\tcase \"$1\" in\n> -\t-n)\n> -\t\tfast_forward=f\n> -\t\tsha1=$2\n> -\t\t;;\n> -\t*)\n> -\t\tsha1=$1\n> -\t\t;;\n> -\tesac\n> -\tsha1=$(git rev-parse $sha1)\n> +\tno_commit=\n> +\textra_args=\n> +\twhile test $# -gt 0\n> +\tdo\n> +\t\tcase \"$1\" in\n> +\t\t-n)\n> +\t\t\tfast_forward=f\n> +\t\t\textra_args=\"$extra_args -n\"\n> +\t\t\tno_commit=y\n> +\t\t\t;;\n> +\t\t-*)\n> +\t\t\twarn \"pick_one_preserving_merges: ignored option -- $1\"\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tbreak\n> +\t\t\t;;\n> +\t\tesac\n> +\t\tshift\n> +\tdone\n> +\ttest $# -ne 1 && die \"pick_one_preserving_merges: wrong number of arguments\"\n> +\tsha1=$(git rev-parse $1)\n> \n> \tif test -f \"$state_dir\"/current-commit\n> \tthen\n> @@ -335,7 +364,7 @@ pick_one_preserving_merges () {\n> \tf)\n> \t\tfirst_parent=$(expr \"$new_parents\" : ' \\([^ ]*\\)')\n> \n> -\t\tif [ \"$1\" != \"-n\" ]\n> +\t\tif test -z \"$no_commit\"\n> \t\tthen\n> \t\t\t# detach HEAD to current parent\n> \t\t\toutput git checkout $first_parent 2> /dev/null ||\n> @@ -344,7 +373,7 @@ pick_one_preserving_merges () {\n> \n> \t\tcase \"$new_parents\" in\n> \t\t' '*' '*)\n> -\t\t\ttest \"a$1\" = a-n && die \"Refusing to squash a merge: $sha1\"\n> +\t\t\ttest -n \"$no_commit\" && die \"Refusing to squash a merge: $sha1\"\n> \n> \t\t\t# redo merge\n> \t\t\tauthor_script_content=$(get_author_ident_from_commit $sha1)\n> @@ -365,7 +394,7 @@ pick_one_preserving_merges () {\n> \t\t*)\n> \t\t\toutput eval git cherry-pick \\\n> \t\t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n> -\t\t\t\t\"$strategy_args\" \"$@\" ||\n> +\t\t\t\t\"$strategy_args\" $extra_args $sha1 ||\n> \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n> \t\t\t;;\n> \t\tesac\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"244754","messageId":"53A43A03.5030506@alum.mit.edu","threadId":"36963","inReplyTo":"53A258DE.10407@gmail.com","subject":"Re: [RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-06-20T13:41:23Z","receivedAt":"2014-06-20T13:41:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/19/2014 05:28 AM, Fabian Ruch wrote:\n> The to-do list command `reword` replays a commit like `pick` but lets\n> the user also edit the commit's log message. If one thinks of `pick`\n> entries as scheduled `cherry-pick` command lines, then `reword` becomes\n> an alias for the command line `cherry-pick --edit`. The porcelain\n> `rebase--interactive` defines a function `do_pick` for processing the\n> `pick` entries on to-do lists. Teach `do_pick` to handle the option\n> `--edit` and reimplement `reword` in terms of `do_pick --edit`. Refer to\n> `pick_one` for the way options are parsed.\n> \n> `do_pick` ought to act as a wrapper around `cherry-pick`. Unfortunately,\n> it cannot just forward `--edit` to the `cherry-pick` command line. The\n> assembled command line is executed within a command substitution for\n> controlling the verbosity of `rebase--interactive`. Passing `--edit`\n> would either hang the terminal or clutter the substituted command output\n> with control sequences. Execute the `reword` code from `do_next` instead\n> if the option `--edit` is specified. Adjust the fragment in two regards.\n> Firstly, discard the additional message which is printed if an error\n> occurs because\n> \n> Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.)\n> Could not amend commit after successfully picking 1234567... Some change\n> \n> is more readable than and contains (almost) the same information as\n> \n> Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.)\n> Could not amend commit after successfully picking 1234567... Some change\n> This is most likely due to an empty commit message, or the pre-commit hook\n> failed. If the pre-commit hook failed, you may need to resolve the issue before\n> you are able to reword the commit.\n> \n> (It is true that a hook might not output any diagnosis but the same\n> problem arises when using git-commit directly. git-rebase at least\n> prints a generic message saying that it failed to commit.) Secondly,\n> sneak in additional git-commit arguments:\n> \n> - `--allow-empty` is missing: `rebase--interactive` suddenly fails if\n> an empty commit is picked using `reword` instead of `pick`. The\n> whether a commit is empty or not is not changed by an altered log\n> message, so do as `pick` does. Add test.\n> \n> - `-n`: Hide the fact that we are committing several times by not\n> executing the pre-commit hook. Caveat: The altered log message is not\n> verified because `-n` also skips the commit-msg hook. Either the log\n> message verification must be included in the post-rewrite hook or\n> git-commit must support more fine-grained control over which hooks\n> are executed.\n> \n> - `-q`: Hide the fact that we are committing several times by not\n> printing the commit summary.\n\nIt is preferable that each commit makes one logical change (though it\nmust always be a self-contained change; i.e., the code should never be\nbroken at the end of a commit). It would be clearer if you would split\nthis commit into one refactoring commit (moving the handling of --edit\nto do_pick) plus one commit for each \"git commit\" option change and\nerror message change. That way,\n\n* Each commit (and log message) becomes simpler, making it easier\n to review.\n* The changes can be discussed separately.\n* If there is an error, \"git bisect\" can help determine which of\n the changes is at fault.\n\n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n> git-rebase--interactive.sh | 52 ++++++++++++++++++++++++++++++++++++-------\n> t/t3404-rebase-interactive.sh | 8 +++++++\n> 2 files changed, 52 insertions(+), 8 deletions(-)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index ea5514e..fffdfa5 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -490,7 +490,42 @@ record_in_rewritten() {\n> \tesac\n> }\n> \n> +# Apply the changes introduced by the given commit to the current head.\n> +#\n> +# do_pick [--edit] <commit> <title>\n> +#\n> +# Wrapper around git-cherry-pick.\n> +#\n> +# <title>\n> +# The commit message title of <commit>. Used for information\n> +# purposes only.\n> +#\n> +# <commit>\n> +# The commit to cherry-pick.\n\nUnless there is a reason to do otherwise, please order the documentation\nto match the order that the do_pick arguments appear.\n\n> +#\n> +# -e, --edit\n> +# After picking <commit>, open an editor and let the user edit the\n> +# commit message. The editor contents becomes the commit message of\n> +# the new head.\n> do_pick () {\n> +\tedit=\n> +\twhile test $# -gt 0\n> +\tdo\n> +\t\tcase \"$1\" in\n> +\t\t-e|--edit)\n> +\t\t\tedit=y\n> +\t\t\t;;\n> +\t\t-*)\n> +\t\t\twarn \"do_pick: ignored option -- $1\"\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tbreak\n> +\t\t\t;;\n> +\t\tesac\n> +\t\tshift\n> +\tdone\n> +\ttest $# -ne 2 && die \"do_pick: wrong number of arguments\"\n> +\n> \tif test \"$(git rev-parse HEAD)\" = \"$squash_onto\"\n> \tthen\n> \t\t# Set the correct commit message and author info on the\n> @@ -512,6 +547,14 @@ do_pick () {\n> \t\tpick_one $1 ||\n> \t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n> \tfi\n> +\n> +\tif test -n \"$edit\"\n> +\tthen\n> +\t\tgit commit --allow-empty --amend --no-post-rewrite -n -q ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n> +\t\t\twarn \"Could not amend commit after successfully picking $1... $2\"\n> +\t\t\texit_with_patch $1 1\n> +\t\t}\n> +\tfi\n> }\n> \n> do_next () {\n> @@ -532,14 +575,7 @@ do_next () {\n> \t\tcomment_for_reflog reword\n> \n> \t\tmark_action_done\n> -\t\tdo_pick $sha1 \"$rest\"\n> -\t\tgit commit --amend --no-post-rewrite ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n> -\t\t\twarn \"Could not amend commit after successfully picking $sha1... $rest\"\n> -\t\t\twarn \"This is most likely due to an empty commit message, or the pre-commit hook\"\n> -\t\t\twarn \"failed. If the pre-commit hook failed, you may need to resolve the issue before\"\n> -\t\t\twarn \"you are able to reword the commit.\"\n> -\t\t\texit_with_patch $sha1 1\n> -\t\t}\n> +\t\tdo_pick --edit $sha1 \"$rest\"\n> \t\trecord_in_rewritten $sha1\n> \t\t;;\n> \tedit|e)\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 8197ed2..9931143 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -75,6 +75,14 @@ test_expect_success 'rebase --keep-empty' '\n> \ttest_line_count = 6 actual\n> '\n> \n> +test_expect_success 'rebase --keep-empty' '\n> +\tgit checkout emptybranch &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"1 reword 2\" git rebase --keep-empty -i HEAD~2 &&\n> +\tgit log --oneline >actual &&\n> +\ttest_line_count = 6 actual\n> +'\n> +\n> test_expect_success 'rebase -i with the exec command' '\n> \tgit checkout master &&\n> \t(\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"244805","messageId":"xmqqvbrvcq8p.fsf@gitster.dls.corp.google.com","threadId":"36963","inReplyTo":"53A439B2.7000106@alum.mit.edu","subject":"Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-20T19:53:10Z","receivedAt":"2014-06-20T19:53:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>> pick_one () {\n>> \tff=--ff\n>> +\textra_args=\n>> +\twhile test $# -gt 0\n>> +\tdo\n>> +\t\tcase \"$1\" in\n>> +\t\t-n)\n>> +\t\t\tff=\n>> +\t\t\textra_args=\"$extra_args -n\"\n>> +\t\t\t;;\n>> +\t\t-*)\n>> +\t\t\twarn \"pick_one: ignored option -- $1\"\n>> +\t\t\t;;\n>\n> This is an internal interface, right? I.e., user input isn't being\n> processed here? If so, then the presence of an unrecognized option is a\n> bug and it is preferable to \"die\" here rather than \"warn\".\n>\n> The same below and in at least one later commit.\n\nAnd if this is purely an internal interface, then I really do not\nsee the point of allowing -n to be anywhere other than the front.\nIf we are planning to accept other random options to cherry-pick in\nlater steps, but we are not yet doing so at this step, then I do not\nthin we want to have any loop like this before we actually start\naccepting and passing them to the underlying cherry-pick.\n\nFurthermore, if the \"-n\" is currently used as an internal signal\nfrom the caller to pick_one() that it is executing the end-user\nsupplied \"squash\" in the insn sheet, it may be a good idea to change\nthat \"-n\" to something that is *NOT* a valid option to cherry-pick\nat this step, before we start accepting user-supplied options and\nrelaying them to underlying cherry-pick.\n\nOne way to do so cleanly may be to _always_ add the type of pick as\nthe first parameter to pick_one, i.e. either \"pick\" or \"squash\", and\ndo:\n\n pick_one () {\n ...\n n_arg=\n case \"$1\" in\n pick) ;;\n squash) n_arg=-n ;;\n *)\tdie \"BUG: pick_one $1???\" ;;\n esac\n shift\n sha1=$1\n ...\n output eval git cherry-pick $n_arg \\\n ...\n }\n\nAlso I suspect that you would need to be careful *not* to allow \"-n\"\nto be given as part of the \"random user-specified options\" and pass\nthat to cherry-pick in the later steps of your series [*1*], and for\nthat you may need a loop that inspects the arguments like you had in\nthis patch.\n\n[Footnote]\n\n*1* The existing callers of \"pick_one -n\" very well know and expect\n that the step will only update the working tree and the index\n and it is the callers' responsibility to create a commit out of\n that state (either by amending or committing); similarly the\n existing callers of \"pick_one\" without \"-n\" very well know and\n expect that the step will make a commit unless there is a\n problem. I do not think you would consider it such a \"problem\n to replay the change in the named commit\" for the end user's\n insn sheet to pass a \"-n\".\n"},{"id":"244825","messageId":"CAPig+cS48D1GBX9duHLVWnsW+g5_8rBGOdU=UBRuxrS9JpUh_A@mail.gmail.com","threadId":"36963","inReplyTo":"53A258E2.501@gmail.com","subject":"Re: [RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-21T00:33:00Z","receivedAt":"2014-06-21T00:33:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch <bafain@gmail.com> wrote:\n> When `rebase` is executed with `--root` but no `--onto` is specified,\n> `rebase` creates a sentinel commit which is replaced with the root\n> commit in three steps. This combination of options results never in a\n> fast-forward.\n>\n> 1. The sentinel commit is forced into the authorship of the root\n> commit.\n>\n> 2. The changes introduced by the root commit are applied to the index\n> but not committed. If this step fails for whatever reason, all\n> commit information will be there and the user can safely run\n> `git-commit --amend` after resolving the problems.\n>\n> 3. The new root commit is created by squashing the changes into the\n> sentinel commit which already carries the authorship of the\n> cherry-picked root commit.\n>\n> The command line used to create the commit in the third step specifies\n> effectless and erroneous options. Remove those.\n>\n> - `--allow-empty-message` is erroneous: If the root's commit message is\n> empty, the rebase shall fail like for any other commit that is on the\n> to-do list and has an empty commit message.\n>\n> Fix the bug that git-rebase does not fail when the initial commit has\n> an empty log message but is replayed using `--root` is specified.\n> Add test.\n>\n> - `-C` is effectless: The commit being amended, which is the sentinel\n> commit, already carries the authorship and log message of the\n> cherry-picked root commit. The committer email and commit date fields\n> are reset either way.\n>\n> After all, if step two fails, `rebase --continue` won't include these\n> flags in the git-commit command line either.\n>\n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n> git-rebase--interactive.sh | 4 ++--\n> t/t3412-rebase-root.sh | 39 +++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 41 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index fffdfa5..f09eeae 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -539,8 +539,8 @@ do_pick () {\n> git commit --allow-empty --allow-empty-message --amend \\\n> --no-post-rewrite -n -q -C $1 &&\n> pick_one -n $1 &&\n> - git commit --allow-empty --allow-empty-message \\\n> - --amend --no-post-rewrite -n -q -C $1 \\\n> + git commit --allow-empty --amend \\\n> + --no-post-rewrite -n -q \\\n> ${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n> die_with_patch $1 \"Could not apply $1... $2\"\n> else\n> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh\n> index 0b52105..3608db4 100755\n> --- a/t/t3412-rebase-root.sh\n> +++ b/t/t3412-rebase-root.sh\n> @@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '\n> test_cmp expect-conflict-p out\n> '\n>\n> +test_expect_success 'stop rebase --root on empty root log message' '\n> + # create a root commit with a non-empty tree so that rebase does\n> + # not fail because of an empty commit, and an empty log message\n> + echo root-commit >file &&\n> + git add file &&\n> + tree=$(git write-tree) &&\n> + root=$(git commit-tree $tree </dev/null) &&\n> + git checkout -b no-message-root-commit $root &&\n> + # do not ff because otherwise neither the patch nor the message\n> + # are looked at and checked for emptiness\n> + test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&\n> + echo root-commit >file.expected &&\n> + test_cmp file file.expected &&\n\nIt is customary, and provides nicer diagnostic output upon failure, to\nhave the \"expected\" file mentioned first:\n\n test_cmp file.expected file &&\n\n> + git rebase --abort\n\nDo you want to place this under control of test_when_finished\nsomewhere above the \"git rebase\" invocation to ensure cleanup if\nsomething fails before this point?\n\n> +'\n> +\n> +test_expect_success 'stop rebase --root on empty child log message' '\n> + # create a root commit with a non-empty tree and provide a log\n> + # message so that rebase does not fail until the root commit is\n> + # successfully replayed\n> + echo root-commit >file &&\n> + git add file &&\n> + tree=$(git write-tree) &&\n> + root=$(git commit-tree $tree -m root-commit) &&\n> + git checkout -b no-message-child-commit $root &&\n> + # create a child commit with a non-empty patch so that rebase\n> + # does not fail because of an empty commit, but an empty log\n> + # message\n> + echo child-commit >file &&\n> + git add file &&\n> + git commit --allow-empty-message --no-edit &&\n> + # do not ff because otherwise neither the patch nor the message\n> + # are looked at and checked for emptiness\n> + test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&\n> + echo child-commit >file.expected &&\n> + test_cmp file file.expected &&\n> + git rebase --abort\n\nSame two comments as for previous test.\n\n> +'\n> +\n> test_done\n> --\n> 2.0.0\n"},{"id":"244829","messageId":"53A6137A.7040803@gmail.com","threadId":"36963","inReplyTo":"53A439B2.7000106@alum.mit.edu","subject":"Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-21T23:21:30Z","receivedAt":"2014-06-21T23:21:30Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi Michael,\n\nOn 06/20/2014 03:40 PM, Michael Haggerty wrote:\n> On 06/19/2014 05:28 AM, Fabian Ruch wrote:\n>> `pick_one` and `pick_one_preserving_merges` are wrappers around\n>> `cherry-pick` in `rebase --interactive`. They take the hash of a commit\n>> and build a `cherry-pick` command line that\n>>\n>> - respects the user-supplied merge options\n>> - disables complaints about empty commits\n>> - tries to fast-forward the rebase head unless rebase is forced\n>> - suppresses output unless the user requested higher verbosity\n>> - rewrites merge commits to point to their rebased parents.\n>>\n>> `pick_one` is used to implement not only `pick` but also `squash`, which\n>> amends the previous commit rather than creating a new one. When\n>> `pick_one` is called from `squash`, it receives a second argument `-n`.\n>> This tells `pick_one` to apply the changes to the index without\n>> committing them. Since the argument is almost directly passed to\n>> `cherry-pick`, we might want to do the same with other `cherry-pick`\n>> options. Currently, `pick_one` expects `-n` to be the first and only\n>> argument except for the commit hash.\n>>\n>> Prepare `pick_one` for additional `cherry-pick` options by allowing `-n`\n>> to appear anywhere before the commit hash in the argument list. Loop\n>> over the argument list and pop each handled item until the commit hash\n>> is the only parameter left on the list. If an option is not supported,\n>> ignore it and issue a warning on the console. Construct a new arguments\n>> list `extra_args` of recognized options that shall be passed to\n>> `cherry-pick` on the command line.\n>>\n>> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n>> ---\n>> git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------\n>> 1 file changed, 45 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n>> index f267d8b..ea5514e 100644\n>> --- a/git-rebase--interactive.sh\n>> +++ b/git-rebase--interactive.sh\n>> @@ -237,8 +237,26 @@ git_sequence_editor () {\n>> \n>> pick_one () {\n>> \tff=--ff\n>> +\textra_args=\n>> +\twhile test $# -gt 0\n>> +\tdo\n>> +\t\tcase \"$1\" in\n>> +\t\t-n)\n>> +\t\t\tff=\n>> +\t\t\textra_args=\"$extra_args -n\"\n>> +\t\t\t;;\n>> +\t\t-*)\n>> +\t\t\twarn \"pick_one: ignored option -- $1\"\n>> +\t\t\t;;\n> \n> This is an internal interface, right? I.e., user input isn't being\n> processed here? If so, then the presence of an unrecognized option is a\n> bug and it is preferable to \"die\" here rather than \"warn\".\n> \n> The same below and in at least one later commit.\n\nOk.\n\n>> +\t\t*)\n>> +\t\t\tbreak\n>> +\t\t\t;;\n>> +\t\tesac\n>> +\t\tshift\n>> +\tdone\n>> +\ttest $# -ne 1 && die \"pick_one: wrong number of arguments\"\n>> +\tsha1=$1\n>> \n>> -\tcase \"$1\" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac\n>> \tcase \"$force_rebase\" in '') ;; ?*) ff= ;; esac\n>> \toutput git rev-parse --verify $sha1 || die \"Invalid commit name: $sha1\"\n>> \n>> @@ -248,24 +266,35 @@ pick_one () {\n>> \tfi\n>> \n>> \ttest -d \"$rewritten\" &&\n>> -\t\tpick_one_preserving_merges \"$@\" && return\n>> +\t\tpick_one_preserving_merges $extra_args $sha1 && return\n>> \toutput eval git cherry-pick \\\n>> \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n>> -\t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n>> +\t\t\t\"$strategy_args\" $empty_args $ff $extra_args $sha1\n>> }\n> \n> It might be confusing that extra_args is used both in pick_one and in\n> pick_one_preserving_merges. Since these are not local variables, the\n> call to the latter changes the value of the variable in the former. I\n> don't know if that could be a problem now (can\n> pick_one_preserving_merges return with a nonzero exit code?) but even if\n> not, it is a trap for future developers. I recommend giving the two\n> variables different names.\n\nPlease correct me if I missed something. At the moment (786a89d),\npick_one_preserving_merges will not and cannot return with a non-zero\nexit code because it doesn't explicitly return and all possibly failing\nlast statements are guarded with a ... || die \"...\". Since this can only\nbe established by carefully looking at the code, I will change the reuse\nof extra_args.\n\nNow that we're at it, what I didn't understand when creating this patch\nwas why the code doesn't explicitly say \"one or the other\" in the first\nplace:\n\n> if test -d \"$rewritten\"\n> then\n> \tpick_one_preserving_merges \"$@\"\n> else\n> \toutput eval git cherry-pick \\\n> \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n> \t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n> fi\n\nAt least that is how I interpreted it. After all, if\npick_one_preserving_merges failed to recreate a merge commit, wouldn't\nit be a bug to record the changes in a single-parent commit?\n\nJohannes, I cc'd you this email since you're the author of f09c9b8 -\n\"Teach rebase -i about --preserve-merges\". I hope you don't resent me\ndigging out this seven-year-old patch.\n\n Fabian\n\n>> pick_one_preserving_merges () {\n>> \tfast_forward=t\n>> -\tcase \"$1\" in\n>> -\t-n)\n>> -\t\tfast_forward=f\n>> -\t\tsha1=$2\n>> -\t\t;;\n>> -\t*)\n>> -\t\tsha1=$1\n>> -\t\t;;\n>> -\tesac\n>> -\tsha1=$(git rev-parse $sha1)\n>> +\tno_commit=\n>> +\textra_args=\n>> +\twhile test $# -gt 0\n>> +\tdo\n>> +\t\tcase \"$1\" in\n>> +\t\t-n)\n>> +\t\t\tfast_forward=f\n>> +\t\t\textra_args=\"$extra_args -n\"\n>> +\t\t\tno_commit=y\n>> +\t\t\t;;\n>> +\t\t-*)\n>> +\t\t\twarn \"pick_one_preserving_merges: ignored option -- $1\"\n>> +\t\t\t;;\n>> +\t\t*)\n>> +\t\t\tbreak\n>> +\t\t\t;;\n>> +\t\tesac\n>> +\t\tshift\n>> +\tdone\n>> +\ttest $# -ne 1 && die \"pick_one_preserving_merges: wrong number of arguments\"\n>> +\tsha1=$(git rev-parse $1)\n>> \n>> \tif test -f \"$state_dir\"/current-commit\n>> \tthen\n>> @@ -335,7 +364,7 @@ pick_one_preserving_merges () {\n>> \tf)\n>> \t\tfirst_parent=$(expr \"$new_parents\" : ' \\([^ ]*\\)')\n>> \n>> -\t\tif [ \"$1\" != \"-n\" ]\n>> +\t\tif test -z \"$no_commit\"\n>> \t\tthen\n>> \t\t\t# detach HEAD to current parent\n>> \t\t\toutput git checkout $first_parent 2> /dev/null ||\n>> @@ -344,7 +373,7 @@ pick_one_preserving_merges () {\n>> \n>> \t\tcase \"$new_parents\" in\n>> \t\t' '*' '*)\n>> -\t\t\ttest \"a$1\" = a-n && die \"Refusing to squash a merge: $sha1\"\n>> +\t\t\ttest -n \"$no_commit\" && die \"Refusing to squash a merge: $sha1\"\n>> \n>> \t\t\t# redo merge\n>> \t\t\tauthor_script_content=$(get_author_ident_from_commit $sha1)\n>> @@ -365,7 +394,7 @@ pick_one_preserving_merges () {\n>> \t\t*)\n>> \t\t\toutput eval git cherry-pick \\\n>> \t\t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n>> -\t\t\t\t\"$strategy_args\" \"$@\" ||\n>> +\t\t\t\t\"$strategy_args\" $extra_args $sha1 ||\n>> \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n>> \t\t\t;;\n>> \t\tesac\n>>\n"},{"id":"244830","messageId":"53A61EA2.2060402@gmail.com","threadId":"36963","inReplyTo":"53A43A03.5030506@alum.mit.edu","subject":"Re: [RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-22T00:09:06Z","receivedAt":"2014-06-22T00:09:06Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi Michael,\n\nOn 06/20/2014 03:41 PM, Michael Haggerty wrote:\n> On 06/19/2014 05:28 AM, Fabian Ruch wrote:\n>> The to-do list command `reword` replays a commit like `pick` but lets\n>> the user also edit the commit's log message. If one thinks of `pick`\n>> entries as scheduled `cherry-pick` command lines, then `reword` becomes\n>> an alias for the command line `cherry-pick --edit`. The porcelain\n>> `rebase--interactive` defines a function `do_pick` for processing the\n>> `pick` entries on to-do lists. Teach `do_pick` to handle the option\n>> `--edit` and reimplement `reword` in terms of `do_pick --edit`. Refer to\n>> `pick_one` for the way options are parsed.\n>>\n>> `do_pick` ought to act as a wrapper around `cherry-pick`. Unfortunately,\n>> it cannot just forward `--edit` to the `cherry-pick` command line. The\n>> assembled command line is executed within a command substitution for\n>> controlling the verbosity of `rebase--interactive`. Passing `--edit`\n>> would either hang the terminal or clutter the substituted command output\n>> with control sequences. Execute the `reword` code from `do_next` instead\n>> if the option `--edit` is specified. Adjust the fragment in two regards.\n>> Firstly, discard the additional message which is printed if an error\n>> occurs because\n>>\n>> Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.)\n>> Could not amend commit after successfully picking 1234567... Some change\n>>\n>> is more readable than and contains (almost) the same information as\n>>\n>> Aborting commit due to empty commit message. (Duplicate Signed-off-by lines.)\n>> Could not amend commit after successfully picking 1234567... Some change\n>> This is most likely due to an empty commit message, or the pre-commit hook\n>> failed. If the pre-commit hook failed, you may need to resolve the issue before\n>> you are able to reword the commit.\n>>\n>> (It is true that a hook might not output any diagnosis but the same\n>> problem arises when using git-commit directly. git-rebase at least\n>> prints a generic message saying that it failed to commit.) Secondly,\n>> sneak in additional git-commit arguments:\n>>\n>> - `--allow-empty` is missing: `rebase--interactive` suddenly fails if\n>> an empty commit is picked using `reword` instead of `pick`. The\n>> whether a commit is empty or not is not changed by an altered log\n>> message, so do as `pick` does. Add test.\n>>\n>> - `-n`: Hide the fact that we are committing several times by not\n>> executing the pre-commit hook. Caveat: The altered log message is not\n>> verified because `-n` also skips the commit-msg hook. Either the log\n>> message verification must be included in the post-rewrite hook or\n>> git-commit must support more fine-grained control over which hooks\n>> are executed.\n>>\n>> - `-q`: Hide the fact that we are committing several times by not\n>> printing the commit summary.\n> \n> It is preferable that each commit makes one logical change (though it\n> must always be a self-contained change; i.e., the code should never be\n> broken at the end of a commit). It would be clearer if you would split\n> this commit into one refactoring commit (moving the handling of --edit\n> to do_pick) plus one commit for each \"git commit\" option change and\n> error message change. That way,\n> \n> * Each commit (and log message) becomes simpler, making it easier\n> to review.\n> * The changes can be discussed separately.\n> * If there is an error, \"git bisect\" can help determine which of\n> the changes is at fault.\n\nHmph, I neglected that totally here. I'm sorry. If it's all right, I\nwill reply with the five separate commits (refactoring, error message,\n--allow-empty, -n, -q) to this email. The whole patch series is still\nRFC and the combination of the five will be exactly this one, so that\nshouldn't confuse or burden anyone.\n\n>> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n>> ---\n>> git-rebase--interactive.sh | 52 ++++++++++++++++++++++++++++++++++++-------\n>> t/t3404-rebase-interactive.sh | 8 +++++++\n>> 2 files changed, 52 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n>> index ea5514e..fffdfa5 100644\n>> --- a/git-rebase--interactive.sh\n>> +++ b/git-rebase--interactive.sh\n>> @@ -490,7 +490,42 @@ record_in_rewritten() {\n>> \tesac\n>> }\n>> \n>> +# Apply the changes introduced by the given commit to the current head.\n>> +#\n>> +# do_pick [--edit] <commit> <title>\n>> +#\n>> +# Wrapper around git-cherry-pick.\n>> +#\n>> +# <title>\n>> +# The commit message title of <commit>. Used for information\n>> +# purposes only.\n>> +#\n>> +# <commit>\n>> +# The commit to cherry-pick.\n> \n> Unless there is a reason to do otherwise, please order the documentation\n> to match the order that the do_pick arguments appear.\n\nOk.\n\nThe reason was to document the non-option arguments first and I ended up\ndocumenting the arguments in reverse order to simply not abandon all\norder. Having a look at several man-pages of git commands (cherry-pick,\ncommit, am, rebase), I wasn't able to extract a common pattern of order\nof argument documentation.\n\n Fabian\n\n>> +#\n>> +# -e, --edit\n>> +# After picking <commit>, open an editor and let the user edit the\n>> +# commit message. The editor contents becomes the commit message of\n>> +# the new head.\n>> do_pick () {\n>> +\tedit=\n>> +\twhile test $# -gt 0\n>> +\tdo\n>> +\t\tcase \"$1\" in\n>> +\t\t-e|--edit)\n>> +\t\t\tedit=y\n>> +\t\t\t;;\n>> +\t\t-*)\n>> +\t\t\twarn \"do_pick: ignored option -- $1\"\n>> +\t\t\t;;\n>> +\t\t*)\n>> +\t\t\tbreak\n>> +\t\t\t;;\n>> +\t\tesac\n>> +\t\tshift\n>> +\tdone\n>> +\ttest $# -ne 2 && die \"do_pick: wrong number of arguments\"\n>> +\n>> \tif test \"$(git rev-parse HEAD)\" = \"$squash_onto\"\n>> \tthen\n>> \t\t# Set the correct commit message and author info on the\n>> @@ -512,6 +547,14 @@ do_pick () {\n>> \t\tpick_one $1 ||\n>> \t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n>> \tfi\n>> +\n>> +\tif test -n \"$edit\"\n>> +\tthen\n>> +\t\tgit commit --allow-empty --amend --no-post-rewrite -n -q ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n>> +\t\t\twarn \"Could not amend commit after successfully picking $1... $2\"\n>> +\t\t\texit_with_patch $1 1\n>> +\t\t}\n>> +\tfi\n>> }\n>> \n>> do_next () {\n>> @@ -532,14 +575,7 @@ do_next () {\n>> \t\tcomment_for_reflog reword\n>> \n>> \t\tmark_action_done\n>> -\t\tdo_pick $sha1 \"$rest\"\n>> -\t\tgit commit --amend --no-post-rewrite ${gpg_sign_opt:+\"$gpg_sign_opt\"} || {\n>> -\t\t\twarn \"Could not amend commit after successfully picking $sha1... $rest\"\n>> -\t\t\twarn \"This is most likely due to an empty commit message, or the pre-commit hook\"\n>> -\t\t\twarn \"failed. If the pre-commit hook failed, you may need to resolve the issue before\"\n>> -\t\t\twarn \"you are able to reword the commit.\"\n>> -\t\t\texit_with_patch $sha1 1\n>> -\t\t}\n>> +\t\tdo_pick --edit $sha1 \"$rest\"\n>> \t\trecord_in_rewritten $sha1\n>> \t\t;;\n>> \tedit|e)\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 8197ed2..9931143 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -75,6 +75,14 @@ test_expect_success 'rebase --keep-empty' '\n>> \ttest_line_count = 6 actual\n>> '\n>> \n>> +test_expect_success 'rebase --keep-empty' '\n>> +\tgit checkout emptybranch &&\n>> +\tset_fake_editor &&\n>> +\tFAKE_LINES=\"1 reword 2\" git rebase --keep-empty -i HEAD~2 &&\n>> +\tgit log --oneline >actual &&\n>> +\ttest_line_count = 6 actual\n>> +'\n>> +\n>> test_expect_success 'rebase -i with the exec command' '\n>> \tgit checkout master &&\n>> \t(\n"},{"id":"244831","messageId":"53A6240E.9000201@gmail.com","threadId":"36963","inReplyTo":"CAPig+cS48D1GBX9duHLVWnsW+g5_8rBGOdU=UBRuxrS9JpUh_A@mail.gmail.com","subject":"Re: [RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-22T00:32:14Z","receivedAt":"2014-06-22T00:32:14Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi Eric,\n\nOn 06/21/2014 02:33 AM, Eric Sunshine wrote:\n> On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch <bafain@gmail.com> wrote:\n>> When `rebase` is executed with `--root` but no `--onto` is specified,\n>> `rebase` creates a sentinel commit which is replaced with the root\n>> commit in three steps. This combination of options results never in a\n>> fast-forward.\n>>\n>> 1. The sentinel commit is forced into the authorship of the root\n>> commit.\n>>\n>> 2. The changes introduced by the root commit are applied to the index\n>> but not committed. If this step fails for whatever reason, all\n>> commit information will be there and the user can safely run\n>> `git-commit --amend` after resolving the problems.\n>>\n>> 3. The new root commit is created by squashing the changes into the\n>> sentinel commit which already carries the authorship of the\n>> cherry-picked root commit.\n>>\n>> The command line used to create the commit in the third step specifies\n>> effectless and erroneous options. Remove those.\n>>\n>> - `--allow-empty-message` is erroneous: If the root's commit message is\n>> empty, the rebase shall fail like for any other commit that is on the\n>> to-do list and has an empty commit message.\n>>\n>> Fix the bug that git-rebase does not fail when the initial commit has\n>> an empty log message but is replayed using `--root` is specified.\n>> Add test.\n>>\n>> - `-C` is effectless: The commit being amended, which is the sentinel\n>> commit, already carries the authorship and log message of the\n>> cherry-picked root commit. The committer email and commit date fields\n>> are reset either way.\n>>\n>> After all, if step two fails, `rebase --continue` won't include these\n>> flags in the git-commit command line either.\n>>\n>> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n>> ---\n>> git-rebase--interactive.sh | 4 ++--\n>> t/t3412-rebase-root.sh | 39 +++++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 41 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n>> index fffdfa5..f09eeae 100644\n>> --- a/git-rebase--interactive.sh\n>> +++ b/git-rebase--interactive.sh\n>> @@ -539,8 +539,8 @@ do_pick () {\n>> git commit --allow-empty --allow-empty-message --amend \\\n>> --no-post-rewrite -n -q -C $1 &&\n>> pick_one -n $1 &&\n>> - git commit --allow-empty --allow-empty-message \\\n>> - --amend --no-post-rewrite -n -q -C $1 \\\n>> + git commit --allow-empty --amend \\\n>> + --no-post-rewrite -n -q \\\n>> ${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n>> die_with_patch $1 \"Could not apply $1... $2\"\n>> else\n>> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh\n>> index 0b52105..3608db4 100755\n>> --- a/t/t3412-rebase-root.sh\n>> +++ b/t/t3412-rebase-root.sh\n>> @@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '\n>> test_cmp expect-conflict-p out\n>> '\n>>\n>> +test_expect_success 'stop rebase --root on empty root log message' '\n>> + # create a root commit with a non-empty tree so that rebase does\n>> + # not fail because of an empty commit, and an empty log message\n>> + echo root-commit >file &&\n>> + git add file &&\n>> + tree=$(git write-tree) &&\n>> + root=$(git commit-tree $tree </dev/null) &&\n>> + git checkout -b no-message-root-commit $root &&\n>> + # do not ff because otherwise neither the patch nor the message\n>> + # are looked at and checked for emptiness\n>> + test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&\n>> + echo root-commit >file.expected &&\n>> + test_cmp file file.expected &&\n> \n> It is customary, and provides nicer diagnostic output upon failure, to\n> have the \"expected\" file mentioned first:\n> \n> test_cmp file.expected file &&\n\nOk.\n\n>> + git rebase --abort\n> \n> Do you want to place this under control of test_when_finished\n> somewhere above the \"git rebase\" invocation to ensure cleanup if\n> something fails before this point?\n\nThanks a lot for the hint, I will place the test_when_finished directly\nabove the rebase -i command line because it is not relevant before, the\nexact position shouldn't matter though. I didn't know that test_run_\nskips the cleanup code if -i (for immediate mode) is passed to the test\ndriver and the test case fails.\n\n Fabian\n\n>> +'\n>> +\n>> +test_expect_success 'stop rebase --root on empty child log message' '\n>> + # create a root commit with a non-empty tree and provide a log\n>> + # message so that rebase does not fail until the root commit is\n>> + # successfully replayed\n>> + echo root-commit >file &&\n>> + git add file &&\n>> + tree=$(git write-tree) &&\n>> + root=$(git commit-tree $tree -m root-commit) &&\n>> + git checkout -b no-message-child-commit $root &&\n>> + # create a child commit with a non-empty patch so that rebase\n>> + # does not fail because of an empty commit, but an empty log\n>> + # message\n>> + echo child-commit >file &&\n>> + git add file &&\n>> + git commit --allow-empty-message --no-edit &&\n>> + # do not ff because otherwise neither the patch nor the message\n>> + # are looked at and checked for emptiness\n>> + test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&\n>> + echo child-commit >file.expected &&\n>> + test_cmp file file.expected &&\n>> + git rebase --abort\n> \n> Same two comments as for previous test.\n\nOk.\n\n>> +'\n>> +\n>> test_done\n"},{"id":"244840","messageId":"53A76EFE.3080909@gmail.com","threadId":"36963","inReplyTo":"xmqqvbrvcq8p.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-23T00:04:14Z","receivedAt":"2014-06-23T00:04:14Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi Junio,\n\nOn 06/20/2014 09:53 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>>> pick_one () {\n>>> \tff=--ff\n>>> +\textra_args=\n>>> +\twhile test $# -gt 0\n>>> +\tdo\n>>> +\t\tcase \"$1\" in\n>>> +\t\t-n)\n>>> +\t\t\tff=\n>>> +\t\t\textra_args=\"$extra_args -n\"\n>>> +\t\t\t;;\n>>> +\t\t-*)\n>>> +\t\t\twarn \"pick_one: ignored option -- $1\"\n>>> +\t\t\t;;\n>>\n>> This is an internal interface, right? I.e., user input isn't being\n>> processed here? If so, then the presence of an unrecognized option is a\n>> bug and it is preferable to \"die\" here rather than \"warn\".\n>>\n>> The same below and in at least one later commit.\n> \n> And if this is purely an internal interface, then I really do not\n> see the point of allowing -n to be anywhere other than the front.\n> If we are planning to accept other random options to cherry-pick in\n> later steps, but we are not yet doing so at this step, then I do not\n> thin we want to have any loop like this before we actually start\n> accepting and passing them to the underlying cherry-pick.\n\nOk, until we require pick_one to accept options apart from -n, this\npatch is postponed, for the presence of a single option is checked\neasiest without the loop. It might be the case that rewriting replayed\ncommits in do_pick is the better alternative anyway and that it will\nnever be required to relay user-specified options beyond do_pick.\n\n> Furthermore, if the \"-n\" is currently used as an internal signal\n> from the caller to pick_one() that it is executing the end-user\n> supplied \"squash\" in the insn sheet, it may be a good idea to change\n> that \"-n\" to something that is *NOT* a valid option to cherry-pick\n> at this step, before we start accepting user-supplied options and\n> relaying them to underlying cherry-pick.\n> \n> One way to do so cleanly may be to _always_ add the type of pick as\n> the first parameter to pick_one, i.e. either \"pick\" or \"squash\", and\n> do:\n> \n> pick_one () {\n> ...\n> n_arg=\n> case \"$1\" in\n> pick) ;;\n> squash) n_arg=-n ;;\n> *)\tdie \"BUG: pick_one $1???\" ;;\n> esac\n> shift\n> sha1=$1\n> ...\n> output eval git cherry-pick $n_arg \\\n> ...\n> }\n> \n> Also I suspect that you would need to be careful *not* to allow \"-n\"\n> to be given as part of the \"random user-specified options\" and pass\n> that to cherry-pick in the later steps of your series [*1*], and for\n> that you may need a loop that inspects the arguments like you had in\n> this patch.\n\nI really like the idea of being explicit about how pick_one shall replay\nthe named commit and not using the cherry-pick option name for the\nsquash case. However, pick_one will never receive random user-specified\noptions. do_pick is the interface function which handles the pick\narguments. If any user-specified options are relayed to pick_one and\ncherry-pick, they will be validated by do_pick first (using a loop like\nabove).\n\n Fabian\n\n> [Footnote]\n> \n> *1* The existing callers of \"pick_one -n\" very well know and expect\n> that the step will only update the working tree and the index\n> and it is the callers' responsibility to create a commit out of\n> that state (either by amending or committing); similarly the\n> existing callers of \"pick_one\" without \"-n\" very well know and\n> expect that the step will make a commit unless there is a\n> problem. I do not think you would consider it such a \"problem\n> to replay the change in the named commit\" for the end user's\n> insn sheet to pass a \"-n\".\n"},{"id":"244873","messageId":"alpine.DEB.1.00.1406231801190.14982@s15462909.onlinehome-server.info","threadId":"36963","inReplyTo":"53A6137A.7040803@gmail.com","subject":"Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-06-23T16:09:56Z","receivedAt":"2014-06-23T16:09:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 22 Jun 2014, Fabian Ruch wrote:\n\n> On 06/20/2014 03:40 PM, Michael Haggerty wrote:\n> > On 06/19/2014 05:28 AM, Fabian Ruch wrote:\n> >> `pick_one` and `pick_one_preserving_merges` are wrappers around\n> >> `cherry-pick` in `rebase --interactive`. They take the hash of a commit\n> >> and build a `cherry-pick` command line that\n> >>\n> >> - respects the user-supplied merge options\n> >> - disables complaints about empty commits\n> >> - tries to fast-forward the rebase head unless rebase is forced\n> >> - suppresses output unless the user requested higher verbosity\n> >> - rewrites merge commits to point to their rebased parents.\n> >>\n> >> `pick_one` is used to implement not only `pick` but also `squash`, which\n> >> amends the previous commit rather than creating a new one. When\n> >> `pick_one` is called from `squash`, it receives a second argument `-n`.\n> >> This tells `pick_one` to apply the changes to the index without\n> >> committing them. Since the argument is almost directly passed to\n> >> `cherry-pick`, we might want to do the same with other `cherry-pick`\n> >> options. Currently, `pick_one` expects `-n` to be the first and only\n> >> argument except for the commit hash.\n> >>\n> >> Prepare `pick_one` for additional `cherry-pick` options by allowing `-n`\n> >> to appear anywhere before the commit hash in the argument list. Loop\n> >> over the argument list and pop each handled item until the commit hash\n> >> is the only parameter left on the list. If an option is not supported,\n> >> ignore it and issue a warning on the console. Construct a new arguments\n> >> list `extra_args` of recognized options that shall be passed to\n> >> `cherry-pick` on the command line.\n> >>\n> >> [...]\n> >> +\t\t*)\n> >> +\t\t\tbreak\n> >> +\t\t\t;;\n> >> +\t\tesac\n> >> +\t\tshift\n> >> +\tdone\n> >> +\ttest $# -ne 1 && die \"pick_one: wrong number of arguments\"\n> >> +\tsha1=$1\n> >> \n> >> -\tcase \"$1\" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac\n> >> \tcase \"$force_rebase\" in '') ;; ?*) ff= ;; esac\n> >> \toutput git rev-parse --verify $sha1 || die \"Invalid commit name: $sha1\"\n> >> \n> >> @@ -248,24 +266,35 @@ pick_one () {\n> >> \tfi\n> >> \n> >> \ttest -d \"$rewritten\" &&\n> >> -\t\tpick_one_preserving_merges \"$@\" && return\n> >> +\t\tpick_one_preserving_merges $extra_args $sha1 && return\n> >> \toutput eval git cherry-pick \\\n> >> \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n> >> -\t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n> >> +\t\t\t\"$strategy_args\" $empty_args $ff $extra_args $sha1\n> >> }\n> > \n> > It might be confusing that extra_args is used both in pick_one and in\n> > pick_one_preserving_merges. Since these are not local variables, the\n> > call to the latter changes the value of the variable in the former. I\n> > don't know if that could be a problem now (can\n> > pick_one_preserving_merges return with a nonzero exit code?) but even if\n> > not, it is a trap for future developers. I recommend giving the two\n> > variables different names.\n> \n> Please correct me if I missed something. At the moment (786a89d),\n> pick_one_preserving_merges will not and cannot return with a non-zero\n> exit code because it doesn't explicitly return and all possibly failing\n> last statements are guarded with a ... || die \"...\". Since this can only\n> be established by carefully looking at the code, I will change the reuse\n> of extra_args.\n\nI agree that this is a good change, but I would like to point out that it\nwould be a good change even if it pick_one_preserving_merged could return\nwith a non-zero exit code, even if it could be established easier that it\ncannot return with a non-zero exit code: it is plainly confusing. (My\nfault, of course.)\n\n> Now that we're at it, what I didn't understand when creating this patch\n> was why the code doesn't explicitly say \"one or the other\" in the first\n> place:\n> \n> > if test -d \"$rewritten\"\n> > then\n> > \tpick_one_preserving_merges \"$@\"\n> > else\n> > \toutput eval git cherry-pick \\\n> > \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n> > \t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n> > fi\n> \n> At least that is how I interpreted it. After all, if\n> pick_one_preserving_merges failed to recreate a merge commit, wouldn't\n> it be a bug to record the changes in a single-parent commit?\n> \n> Johannes, I cc'd you this email since you're the author of f09c9b8 -\n> \"Teach rebase -i about --preserve-merges\". I hope you don't resent me\n> digging out this seven-year-old patch.\n\nI do not resent you. Nor do I resent that the seven-year-old change is\ncoming to light again. But I have to admit that my recollection as to the\nrationale for the way the code was written is very faint indeed.\n\nAlso, my commit messages were substantially inferior back then, so I guess\nthat the commit message of f09c9b8 does not quite help to jog my memory.\n\nLooking at that revision of git-rebase--interactive.sh, however, I see\nthree call sites of pick_one, and I would suspect that I wanted to\nmaintain DRY code. Having said that, today I would replace\n\n\t@@ -68,6 +71,8 @@ die_abort () {\n\t pick_one () {\n\t\tcase \"$1\" in -n) sha1=$2 ;; *) sha1=$1 ;; esac\n\t\tgit rev-parse --verify $sha1 || die \"Invalid commit name: $sha1\"\n\t+ test -d \"$REWRITTEN\" &&\n\t+ pick_one_preserving_merges \"$@\" && return\n\t\tparent_sha1=$(git rev-parse --verify $sha1^ 2>/dev/null)\n\t\tcurrent_sha1=$(git rev-parse --verify HEAD)\n\t\tif [ $current_sha1 = $parent_sha1 ]; then\n\nby\n\n\t@@ -68,6 +71,8 @@ die_abort () {\n\t pick_one () {\n\t\tcase \"$1\" in -n) sha1=$2 ;; *) sha1=$1 ;; esac\n\t\tgit rev-parse --verify $sha1 || die \"Invalid commit name: $sha1\"\n\t+ if test -d \"$REWRITTEN\"\n\t+ then\n\t+ pick_one_preserving_merges \"$@\"\n\t+ return\n\t+ fi\n\t\tparent_sha1=$(git rev-parse --verify $sha1^ 2>/dev/null)\n\t\tcurrent_sha1=$(git rev-parse --verify HEAD)\n\t\tif [ $current_sha1 = $parent_sha1 ]; then\n\ninstead.\n\nHope this clarifies things!\nDscho\n"}]}