{"thread":{"id":"30217","subject":"[PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits","startedAt":"2012-04-11T20:21:52Z","lastAt":"2012-04-12T15:58:49Z","messageCount":16,"participants":["Neil Horman","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"189025","messageId":"1334175716-11391-1-git-send-email-nhorman@tuxdriver.com","threadId":"30217","inReplyTo":null,"subject":"[PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-11T20:21:52Z","receivedAt":"2012-04-11T20:21:52Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"git's ability to handle empty commits is somewhat lacking, especially when\npreforming a rebase.  Nominally empty commits are undesireable entries, the \nresult of commits that are made empty by prior commits covering the same changs.\nBut occasionally, empty commits are useful to developers (e.g. inserting notes \ninto the development history without changing any code along the way).  In these\ncases its desireable to easily preserve empty commits during operations like \nrebases.\n\nThis patch series enhances git to do just that.  It adds two options to the \ngit-cherry-pick command, --allow-empty, which allows git cherry-pick to preserve\nan empty commit, even if the fast forward logic isn't applicable during the \noperation, and --keep-redundant-commits, which allows the user to also keep\ncommits that were made empty via conflict resolution.  It also enhances\ngit-rebase to add a --keep-empty option which enables rebases to preserve empty\ncommits. \n\nI've tested these operations out myself here and they work well for me\n\nSigned-off-by: Neil Horman <nhorman@tuxdriver.com>\n\n---\nChange notes:\n\nBased on version 1 feedback from this list, the following changes have been made\n\nV2)\n\t* Changed --keep-empty to --allow-empty in the git cherry-pick command\n\n\t* Converted run_git_commit to use argv_array\n\n\t* Updated cherry-pick --allow-empty description in man page\n\t\n\t* added ignore-if-made-empty option to git-cherry-pick\n\n\t* Added test to test suite to validate the new cherry-pick options\n\n\t* Updated git-rebase man page to be less verbose and more accurate in the\n\tdescription of the keep-empty option\n\n\t* squashed the addition of the keep-empty flag in git-rebase down to one\n\tcommit from 3\n\n\t* fixed up coding style in git-rebase script\n\n\t* Optimized detection of empty commits\n\n\t* Only augmented git-rebase-editor message if empty commits are\n\tpossible\n\t\nV3)\n\t* reversed the --ignore-if-empty-logic to by default only keep initially\n\tempty commits\n\n\t* replaced --ignore-if-empty with --keep-redundant-commits, to allow\n\tempty commits that are made empty via conflict resolution, in addition\n\tto commits that were created as empty\n\n\t* reworked is_original_commit_empty to be more efficient and portable\n\n\t* Misc sylistic and spelling cleanups\n\nV4)\n\t* Reverted the cherry-pick advice changes in V3 based on in-thread\n\tdiscussion\n\n\t* Rewrote my changes to is_original_commit_empty and run_git_commit to\n\tnot have to fork, making them more efficient.\n\n"},{"id":"189026","messageId":"1334175716-11391-2-git-send-email-nhorman@tuxdriver.com","threadId":"30217","inReplyTo":"1334175716-11391-1-git-send-email-nhorman@tuxdriver.com","subject":"[PATCH v4 1/4] git-cherry-pick: add allow-empty option","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-11T20:21:53Z","receivedAt":"2012-04-11T20:21:53Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"git cherry-pick fails when picking a non-ff commit that is empty.  The advice\ngiven with the failure is that a git-commit --allow-empty should be issued to\nexplicitly add the empty commit during the cherry pick.  This option allows a\nuser to specify before hand that they want to keep the empty commit.  This\neliminates the need to issue both a cherry pick and a commit operation.\n\nSigned-off-by: Neil Horman <nhorman@tuxdriver.com>\n---\n Documentation/git-cherry-pick.txt |    9 +++++++++\n builtin/revert.c                  |    2 ++\n sequencer.c                       |    7 +++++--\n sequencer.h                       |    1 +\n 4 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex fed5097..730237a 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -103,6 +103,15 @@ effect to your index in a row.\n \tcherry-pick'ed commit, then a fast forward to this commit will\n \tbe performed.\n \n+--allow-empty::\n+\tBy default, cherry-picking an empty commit will fail,\n+\tindicating that an explicit invocation of `git commit\n+\t--allow-empty` is required. This option overrides that\n+\tbehavior, allowing empty commits to be preserved automatically\n+\tin a cherry-pick. Note that when \"--ff\" is in effect, empty\n+\tcommits that meet the \"fast-forward\" requirement will be kept\n+\teven without this option.\n+\n --strategy=<strategy>::\n \tUse the given merge strategy.  Should only be used once.\n \tSee the MERGE STRATEGIES section in linkgit:git-merge[1]\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e6840f2..06b00e6 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -114,12 +114,14 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\n+\t\tOPT_END(),\n \t};\n \n \tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n+\t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, \"preserve empty commits\"),\n \t\t\tOPT_END(),\n \t\t};\n \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\ndiff --git a/sequencer.c b/sequencer.c\nindex a37846a..71929ba 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -260,8 +260,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n  */\n static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n {\n-\t/* 6 is max possible length of our args array including NULL */\n-\tconst char *args[6];\n+\t/* 7 is max possible length of our args array including NULL */\n+\tconst char *args[7];\n \tint i = 0;\n \n \targs[i++] = \"commit\";\n@@ -272,6 +272,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n \t\targs[i++] = \"-F\";\n \t\targs[i++] = defmsg;\n \t}\n+\tif (opts->allow_empty)\n+\t\targs[i++] = \"--allow-empty\";\n+\n \targs[i] = NULL;\n \n \treturn run_command_v_opt(args, RUN_GIT_CMD);\ndiff --git a/sequencer.h b/sequencer.h\nindex bb4b138..e2cd725 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -29,6 +29,7 @@ struct replay_opts {\n \tint signoff;\n \tint allow_ff;\n \tint allow_rerere_auto;\n+\tint allow_empty;\n \n \tint mainline;\n \n-- \n1.7.7.6\n"},{"id":"189027","messageId":"1334175716-11391-3-git-send-email-nhorman@tuxdriver.com","threadId":"30217","inReplyTo":"1334175716-11391-1-git-send-email-nhorman@tuxdriver.com","subject":"[PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-11T20:21:54Z","receivedAt":"2012-04-11T20:21:54Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"The git-cherry-pick --allow-empty command by default only preserves empty\ncommits that were originally empty, i.e only those commits for which\n<commit>^{tree} and <commit>^^{tree} are equal.  By default commits which are\nnon-empty, but were made empty by the inclusion of a prior commit on the current\nhistory are filtered out.  This option allows us to override that behavior and\ninclude redundant commits as empty commits in the change history.\n\nSigned-off-by: Neil Horman <nhorman@tuxdriver.com>\n---\n Documentation/git-cherry-pick.txt |   12 +++++-\n builtin/revert.c                  |    8 +++-\n sequencer.c                       |   91 +++++++++++++++++++++++++++++++-----\n sequencer.h                       |    1 +\n 4 files changed, 97 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex 730237a..f96b8c5 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -110,7 +110,17 @@ effect to your index in a row.\n \tbehavior, allowing empty commits to be preserved automatically\n \tin a cherry-pick. Note that when \"--ff\" is in effect, empty\n \tcommits that meet the \"fast-forward\" requirement will be kept\n-\teven without this option.\n+\teven without this option.  Note also, that use of this option only\n+\tkeeps commits that were initially empty (i.e. where for commit C\n+\tC^{tree} and C^^{tree} are equal).  Commits which are made empty due to\n+\ta previous commit are ignored.  To force the inclusion of those commits\n+\tuse `--keep-redundant-commits`\n+\n+--keep-redundant-commits::\n+\tIf a commit being cherry picked duplicates a commit already in the\n+\tcurrent history, it will result in an empty changeset.  By default these\n+\tredundant commits are ignored.  This option overrides that behavior and\n+\tcreates an empty commit object.  Implies `--allow-empty`\n \n --strategy=<strategy>::\n \tUse the given merge strategy.  Should only be used once.\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 06b00e6..4f0d979 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\n+\t\tOPT_END(),\n \t};\n \n \tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n-\t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, \"preserve empty commits\"),\n+\t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, \"preserve initially empty commits\"),\n+\t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_if_made_empty, \"keep redundant, empty commits\"),\n \t\t\tOPT_END(),\n \t\t};\n \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\n@@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--abort\", rollback,\n \t\t\t\tNULL);\n \n+\t/* keep_if_made_empty implies allow_empty */\n+\tif (opts->keep_if_made_empty)\n+\t\topts->allow_empty = 1;\n+\n \t/* Set the subcommand */\n \tif (remove_state)\n \t\topts->subcommand = REPLAY_REMOVE_STATE;\ndiff --git a/sequencer.c b/sequencer.c\nindex 71929ba..442f364 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -13,6 +13,7 @@\n #include \"rerere.h\"\n #include \"merge-recursive.h\"\n #include \"refs.h\"\n+#include \"argv-array.h\"\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n  * If we are revert, or if our cherry-pick results in a hand merge,\n  * we had better say that the current user is responsible for that.\n  */\n-static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n+int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)\n {\n-\t/* 7 is max possible length of our args array including NULL */\n-\tconst char *args[7];\n-\tint i = 0;\n+\tstruct argv_array array;\n+\tint rc;\n+\n+\tif (!empty && !opts->keep_if_made_empty) {\n+\t\tunsigned char head_sha1[20];\n+\t\tstruct commit *head_commit;\n+\t\tint need_free = 0;\n+\n+\t\tresolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n+\t\thead_commit = lookup_commit(head_sha1);\n+\t\tif (parse_commit(head_commit) < 0)\n+\t\t\treturn error(_(\"could not parse commit %s\\n\"),\n+\t\t\t\t     sha1_to_hex(head_commit->object.sha1));\n+\n+\t\tif (!active_cache_tree) {\n+\t\t\tactive_cache_tree = cache_tree();\n+\t\t\tneed_free = 1;\n+\t\t}\n+\n+\t\tif (!cache_tree_fully_valid(active_cache_tree))\n+\t\t\tcache_tree_update(active_cache_tree, active_cache,\n+\t\t\t\t\t  active_nr, 0);\n+\n+\t\trc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);\n+\n+\t\tif (need_free)\n+\t\t\tcache_tree_free(&active_cache_tree);\n+\n+\t\tif (rc)\n+\t\t\t/*\n+ \t\t\t * The head tree and the parent tree match\n+ \t\t\t * meaning the commit is empty.  Since it wasn't created\n+ \t\t\t * empty (based on the previous test), we can conclude\n+ \t\t\t * the commit has been made redundant.  Since we don't\n+ \t\t\t * want to keep redundant commits, just skip this one\n+ \t\t\t */\n+\t\t\treturn 0;\n+\t}\n+\n+\targv_array_init(&array);\n+\targv_array_push(&array, \"commit\");\n+\targv_array_push(&array, \"-n\");\n \n-\targs[i++] = \"commit\";\n-\targs[i++] = \"-n\";\n \tif (opts->signoff)\n-\t\targs[i++] = \"-s\";\n+\t\targv_array_push(&array, \"-s\");\n \tif (!opts->edit) {\n-\t\targs[i++] = \"-F\";\n-\t\targs[i++] = defmsg;\n+\t\targv_array_push(&array, \"-F\");\n+\t\targv_array_push(&array, defmsg);\n \t}\n+\t\n \tif (opts->allow_empty)\n-\t\targs[i++] = \"--allow-empty\";\n+\t\targv_array_push(&array, \"--allow-empty\");\n+\n+\n+\trc = run_command_v_opt(array.argv, RUN_GIT_CMD);\n+\targv_array_clear(&array);\n+\treturn rc;\n+}\n \n-\targs[i] = NULL;\n+static int is_original_commit_empty(struct commit *commit)\n+{\n+\tconst unsigned char *ptree_sha1;\n+\t\n+\tif (parse_commit(commit))\n+\t\treturn error(_(\"Could not parse commit %s\\n\"),\n+\t\t\t     sha1_to_hex(commit->object.sha1));\n+\tif (commit->parents) {\n+\t\tstruct commit *parent = commit->parents->item;\n+\t\tif (parse_commit(parent))\n+\t\t\treturn error(_(\"Could not parse parent commit %s\\n\"),\n+\t\t\t\tsha1_to_hex(parent->object.sha1));\n+\t\tptree_sha1 = parent->tree->object.sha1;\n+\t} else {\n+\t\tptree_sha1 = EMPTY_TREE_SHA1_BIN; /* commit is root */\n+\t}\n \n-\treturn run_command_v_opt(args, RUN_GIT_CMD);\n+\treturn !hashcmp(ptree_sha1, commit->tree->object.sha1);\n }\n \n static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n@@ -289,6 +349,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res;\n+\tint empty_commit;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -414,6 +475,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tfree_commit_list(remotes);\n \t}\n \n+\tempty_commit = is_original_commit_empty(commit);\n+\tif (empty_commit < 0)\n+\t\treturn empty_commit;\n+\n \t/*\n \t * If the merge was clean or if it failed due to conflict, we write\n \t * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.\n@@ -435,7 +500,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\trerere(opts->allow_rerere_auto);\n \t} else {\n \t\tif (!opts->no_commit)\n-\t\t\tres = run_git_commit(defmsg, opts);\n+\t\t\tres = run_git_commit(defmsg, opts, empty_commit);\n \t}\n \n \tfree_message(&msg);\ndiff --git a/sequencer.h b/sequencer.h\nindex e2cd725..862a79a 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -30,6 +30,7 @@ struct replay_opts {\n \tint allow_ff;\n \tint allow_rerere_auto;\n \tint allow_empty;\n+\tint keep_if_made_empty;\n \n \tint mainline;\n \n-- \n1.7.7.6\n"},{"id":"189029","messageId":"1334175716-11391-4-git-send-email-nhorman@tuxdriver.com","threadId":"30217","inReplyTo":"1334175716-11391-1-git-send-email-nhorman@tuxdriver.com","subject":"[PATCH v4 3/4] git-cherry-pick: Add test to validate new options","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-11T20:21:55Z","receivedAt":"2012-04-11T20:21:55Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"Since we've added the --allow-empty and --keep-redundant-commits\noptions to git cherry-pick we should also add a test to ensure that its working\nproperly\n\nSigned-off-by: Neil Horman <nhorman@tuxdriver.com>\n---\n t/t3505-cherry-pick-empty.sh |   31 ++++++++++++++++++++++++++++++-\n 1 files changed, 30 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\nindex c10b28c..9d419ae 100755\n--- a/t/t3505-cherry-pick-empty.sh\n+++ b/t/t3505-cherry-pick-empty.sh\n@@ -18,7 +18,12 @@ test_expect_success setup '\n \techo third >> file1 &&\n \tgit add file1 &&\n \ttest_tick &&\n-\tgit commit --allow-empty-message -m \"\"\n+\tgit commit --allow-empty-message -m \"\" &&\n+\n+\tgit checkout master &&\n+\tgit checkout -b empty-branch2 &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"empty\"\n \n '\n \n@@ -48,4 +53,28 @@ test_expect_success 'index lockfile was removed' '\n \n '\n \n+test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '\n+\tgit checkout master &&\n+\techo fourth >> file2 &&\n+\tgit add file2 &&\n+\tgit commit -m \"fourth\" && {\n+\t\tgit cherry-pick empty-branch2\n+\t\ttest \"$?\" = 1 \n+\t}\n+'\n+\n+test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '\n+\tgit checkout master && {\n+\t\tgit cherry-pick --allow-empty empty-branch2\n+\t\ttest \"$?\" = 0\n+\t}\n+'\n+\n+test_expect_success 'cherry pick with --keep-redundant-commits' '\n+\tgit checkout master && {\n+\t\tgit cherry-pick --keep-redundant-commits HEAD^\n+\t\ttest \"$?\" = 0\n+\t}\n+'\n+\n test_done\n-- \n1.7.7.6\n"},{"id":"189028","messageId":"1334175716-11391-5-git-send-email-nhorman@tuxdriver.com","threadId":"30217","inReplyTo":"1334175716-11391-1-git-send-email-nhorman@tuxdriver.com","subject":"[PATCH v4 4/4] git-rebase: add keep_empty flag","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-11T20:21:56Z","receivedAt":"2012-04-11T20:21:56Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"Add a command line switch to git-rebase to allow a user the ability to specify\nthat they want to keep any commits in a series that are empty.\n\nWhen git-rebase's type is am, then this option will automatically keep any\ncommit that has a tree object identical to its parent.\n\nThis patch changes the default behavior of interactive rebases as well.  With\nthis patch, git-rebase -i will produce a revision set passed to\ngit-revision-editor, in which empty commits are commented out.  Empty commits\nmay be kept manually by uncommenting them.  If the new --keep-empty option is\nused in an interactive rebase the empty commits will automatically all be\nuncommented in the editor.\n\nSigned-off-by: Neil Horman <nhorman@tuxdriver.com>\n---\n Documentation/git-rebase.txt |    4 ++++\n git-rebase--am.sh            |   19 ++++++++++++++-----\n git-rebase--interactive.sh   |   35 ++++++++++++++++++++++++++++++++---\n git-rebase.sh                |    5 +++++\n 4 files changed, 55 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 504945c..131c35d 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -238,6 +238,10 @@ leave out at most one of A and B, in which case it defaults to HEAD.\n \twill be reset to where it was when the rebase operation was\n \tstarted.\n \n+--keep-empty::\n+\tKeep the commits that do not change anything from its\n+\tparents in the result.\n+\n --skip::\n \tRestart the rebasing process by skipping the current patch.\n \ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex c815a24..040289c 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -20,11 +20,20 @@ esac\n \n test -n \"$rebase_root\" && root_flag=--root\n \n-git format-patch -k --stdout --full-index --ignore-if-in-upstream \\\n-\t--src-prefix=a/ --dst-prefix=b/ \\\n-\t--no-renames $root_flag \"$revisions\" |\n-git am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\" &&\n-move_to_original_branch\n+if test -n \"$keep_empty\" \n+then\n+\t# we have to do this the hard way.  git format-patch completely squashes\n+\t# empty commits and even if it didn't the format doesn't really lend\n+\t# itself well to recording empty patches.  fortunately, cherry-pick\n+\t# makes this easy\n+\tgit cherry-pick --allow-empty \"$revisions\"\n+else\n+\tgit format-patch -k --stdout --full-index --ignore-if-in-upstream \\\n+\t\t--src-prefix=a/ --dst-prefix=b/ \\\n+\t\t--no-renames $root_flag \"$revisions\" |\n+\tgit am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\"\n+fi && move_to_original_branch\n+\n ret=$?\n test 0 != $ret -a -d \"$state_dir\" && write_basic_state\n exit $ret\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 5812222..597d60a 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -167,6 +167,12 @@ has_action () {\n \tsane_grep '^[^#]' \"$1\" >/dev/null\n }\n \n+is_empty_commit() {\n+\tptree=$(git rev-parse \"$1\"^{tree})\n+\tpptree=$(git rev-parse \"$1\"^^{tree})\n+\treturn $(test \"$ptree\" = \"$pptree\")\n+}\n+\n # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n # GIT_AUTHOR_DATE exported from the current environment.\n do_with_author () {\n@@ -191,12 +197,18 @@ git_sequence_editor () {\n \n pick_one () {\n \tff=--ff\n+\n+\tif is_empty_commit $@ \n+\tthen\n+\t\tempty_args=\"--allow-empty\"\n+\tfi\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 \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput git cherry-pick $ff \"$@\"\n+\toutput git cherry-pick $empty_args $ff \"$@\"\n }\n \n pick_one_preserving_merges () {\n@@ -780,9 +792,17 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \\\n \tsed -n \"s/^>//p\" |\n while read -r shortsha1 rest\n do\n+\n+\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1\n+\tthen\n+\t\tcomment_out=\"# pick\"\n+\telse\n+\t\tcomment_out=\"pick\"\n+\tfi\n+\n \tif test t != \"$preserve_merges\"\n \tthen\n-\t\tprintf '%s\\n' \"pick $shortsha1 $rest\" >> \"$todo\"\n+\t\tprintf '%s\\n' \"$comment_out $shortsha1 $rest\" >> \"$todo\"\n \telse\n \t\tsha1=$(git rev-parse $shortsha1)\n \t\tif test -z \"$rebase_root\"\n@@ -801,7 +821,7 @@ do\n \t\tif test f = \"$preserve\"\n \t\tthen\n \t\t\ttouch \"$rewritten\"/$sha1\n-\t\t\tprintf '%s\\n' \"pick $shortsha1 $rest\" >> \"$todo\"\n+\t\t\tprintf '%s\\n' \"$comment_out $shortsha1 $rest\" >> \"$todo\"\n \t\tfi\n \tfi\n done\n@@ -851,6 +871,15 @@ cat >> \"$todo\" << EOF\n #\n EOF\n \n+if test -z \"$keep_empty\"\n+then\n+\tcat >> \"$todo\" << EOF\n+\t# Note that commits which are empty at the time of rebasing are \n+\t# commented out. \n+EOF\n+fi\n+\n+\n has_action \"$todo\" ||\n \tdie_abort \"Nothing to do\"\n \ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 69c1374..24a2840 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -43,6 +43,7 @@ s,strategy=!       use the given merge strategy\n no-ff!             cherry-pick all commits, even if unchanged\n m,merge!           use merging strategies to rebase\n i,interactive!     let the user edit the list of commits to rebase\n+k,keep-empty\t   preserve empty commits during rebase\n f,force-rebase!    force rebase even if branch is up to date\n X,strategy-option=! pass the argument through to the merge strategy\n stat!              display a diffstat of what changed upstream\n@@ -97,6 +98,7 @@ state_dir=\n action=\n preserve_merges=\n autosquash=\n+keep_empty=\n test \"$(git config --bool rebase.autosquash)\" = \"true\" && autosquash=t\n \n read_basic_state () {\n@@ -220,6 +222,9 @@ do\n \t-i)\n \t\tinteractive_rebase=explicit\n \t\t;;\n+\t-k)\n+\t\tkeep_empty=yes\n+\t\t;;\n \t-p)\n \t\tpreserve_merges=t\n \t\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n-- \n1.7.7.6\n"},{"id":"189030","messageId":"7v62d6j8qz.fsf@alter.siamese.dyndns.org","threadId":"30217","inReplyTo":"1334175716-11391-3-git-send-email-nhorman@tuxdriver.com","subject":"Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T20:47:48Z","receivedAt":"2012-04-11T20:47:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"You inherited the following from your previous round.\n\nApplying: git-cherry-pick: Add keep-redundant-commits option\n/srv/project/git/git.git/.git/rebase-apply/patch:115: space before tab in indent.\n \t\t\t * The head tree and the parent tree match\n/srv/project/git/git.git/.git/rebase-apply/patch:116: space before tab in indent.\n \t\t\t * meaning the commit is empty.  Since it wasn't created\n/srv/project/git/git.git/.git/rebase-apply/patch:117: space before tab in indent.\n \t\t\t * empty (based on the previous test), we can conclude\n/srv/project/git/git.git/.git/rebase-apply/patch:118: space before tab in indent.\n \t\t\t * the commit has been made redundant.  Since we don't\n/srv/project/git/git.git/.git/rebase-apply/patch:119: space before tab in indent.\n \t\t\t * want to keep redundant commits, just skip this one\nwarning: squelched 3 whitespace errors\nfatal: 8 lines add whitespace errors.\nRepository lacks necessary blobs to fall back on 3-way merge.\nCannot fall back to three-way merge.\nPatch failed at 0001 git-cherry-pick: Add keep-redundant-commits option\n"},{"id":"189031","messageId":"7v1unuj8az.fsf@alter.siamese.dyndns.org","threadId":"30217","inReplyTo":"1334175716-11391-3-git-send-email-nhorman@tuxdriver.com","subject":"Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T20:57:24Z","receivedAt":"2012-04-11T20:57:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neil Horman <nhorman@tuxdriver.com> writes:\n\n> The git-cherry-pick --allow-empty command by default only preserves empty\n> commits that were originally empty, i.e only those commits for which\n> <commit>^{tree} and <commit>^^{tree} are equal.  By default commits which are\n> non-empty, but were made empty by the inclusion of a prior commit on the current\n> history are filtered out.  This option allows us to override that behavior and\n> include redundant commits as empty commits in the change history.\n>\n> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>\n> ---\n>  Documentation/git-cherry-pick.txt |   12 +++++-\n>  builtin/revert.c                  |    8 +++-\n>  sequencer.c                       |   91 +++++++++++++++++++++++++++++++-----\n>  sequencer.h                       |    1 +\n>  4 files changed, 97 insertions(+), 15 deletions(-)\n>\n> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n> index 730237a..f96b8c5 100644\n> --- a/Documentation/git-cherry-pick.txt\n> +++ b/Documentation/git-cherry-pick.txt\n> @@ -110,7 +110,17 @@ effect to your index in a row.\n>  \tbehavior, allowing empty commits to be preserved automatically\n>  \tin a cherry-pick. Note that when \"--ff\" is in effect, empty\n>  \tcommits that meet the \"fast-forward\" requirement will be kept\n> -\teven without this option.\n> +\teven without this option.  Note also, that use of this option only\n> +\tkeeps commits that were initially empty (i.e. where for commit C\n> +\tC^{tree} and C^^{tree} are equal).  Commits which are made empty due to\n\nIt is OK to be technical in the log message, but I think\n\n\tcommits that were initially empty (i.e. the commit recorded the\n\tsame tree as its parent)\n\nwould be far easier to read in the documentation meant for the end users.\n\n> +\ta previous commit are ignored.  To force the inclusion of those commits\n> +\tuse `--keep-redundant-commits`\n\nAnd end the sentence with a full-stop.\n\n> +--keep-redundant-commits::\n> +\tIf a commit being cherry picked duplicates a commit already in the\n> +\tcurrent history, it will result in an empty changeset.  By default these\n> +\tredundant commits are ignored.  This option overrides that behavior and\n> +\tcreates an empty commit object.  Implies `--allow-empty`\n\nLikewise.\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 06b00e6..4f0d979 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>  \t\tOPT_END(),\n>  \t\tOPT_END(),\n>  \t\tOPT_END(),\n> +\t\tOPT_END(),\n>  \t};\n>  \n>  \tif (opts->action == REPLAY_PICK) {\n>  \t\tstruct option cp_extra[] = {\n>  \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n>  \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n> -\t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, \"preserve empty commits\"),\n> +\t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, \"preserve initially empty commits\"),\n> +\t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_if_made_empty, \"keep redundant, empty commits\"),\n>  \t\t\tOPT_END(),\n>  \t\t};\n>  \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\n> @@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n>  \t\t\t\t\"--abort\", rollback,\n>  \t\t\t\tNULL);\n>  \n> +\t/* keep_if_made_empty implies allow_empty */\n> +\tif (opts->keep_if_made_empty)\n> +\t\topts->allow_empty = 1;\n> +\n>  \t/* Set the subcommand */\n>  \tif (remove_state)\n>  \t\topts->subcommand = REPLAY_REMOVE_STATE;\n> diff --git a/sequencer.c b/sequencer.c\n> index 71929ba..442f364 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -13,6 +13,7 @@\n>  #include \"rerere.h\"\n>  #include \"merge-recursive.h\"\n>  #include \"refs.h\"\n> +#include \"argv-array.h\"\n>  \n>  #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n>  \n> @@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n>   * If we are revert, or if our cherry-pick results in a hand merge,\n>   * we had better say that the current user is responsible for that.\n>   */\n> -static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n> +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)\n>  {\n> -\t/* 7 is max possible length of our args array including NULL */\n> -\tconst char *args[7];\n> -\tint i = 0;\n> +\tstruct argv_array array;\n> +\tint rc;\n> +\n> +\tif (!empty && !opts->keep_if_made_empty) {\n> +\t\tunsigned char head_sha1[20];\n> +\t\tstruct commit *head_commit;\n> +\t\tint need_free = 0;\n> +\n> +\t\tresolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n> +\t\thead_commit = lookup_commit(head_sha1);\n\nNo error checking whatsoever?  HEAD might be pointing at a branch that\nhasn't been born, for example.\n\n> +\t\tif (parse_commit(head_commit) < 0)\n> +\t\t\treturn error(_(\"could not parse commit %s\\n\"),\n> +\t\t\t\t     sha1_to_hex(head_commit->object.sha1));\n> +\n> +\t\tif (!active_cache_tree) {\n> +\t\t\tactive_cache_tree = cache_tree();\n> +\t\t\tneed_free = 1;\n> +\t\t}\n\nI think this is wrong.  If for whatever reason the process does not have\nthe index it is about to tell \"git commit\" to use to make a commit in-core,\nit should first be doing read_cache().\n> +\n> +\t\tif (!cache_tree_fully_valid(active_cache_tree))\n> +\t\t\tcache_tree_update(active_cache_tree, active_cache,\n> +\t\t\t\t\t  active_nr, 0);\n\nI do not recall offhand if cache_tree_update() can give you an error, but\nif it does, it should be checked here.\n\n> +\t\trc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);\n> +\n> +\t\tif (need_free)\n> +\t\t\tcache_tree_free(&active_cache_tree);\n\nand I don't see a need for nuking the cache tree here, unless you are\ndiscarding the_index as well.\n\n> +\t\tif (rc)\n> +\t\t\t/*\n> + \t\t\t * The head tree and the parent tree match\n> + \t\t\t * meaning the commit is empty.  Since it wasn't created\n> + \t\t\t * empty (based on the previous test), we can conclude\n> + \t\t\t * the commit has been made redundant.  Since we don't\n> + \t\t\t * want to keep redundant commits, just skip this one\n> + \t\t\t */\n> +\t\t\treturn 0;\n> +\t}\n> +\n> +\targv_array_init(&array);\n> +\targv_array_push(&array, \"commit\");\n> +\targv_array_push(&array, \"-n\");\n>  \n> -\targs[i++] = \"commit\";\n> -\targs[i++] = \"-n\";\n>  \tif (opts->signoff)\n> -\t\targs[i++] = \"-s\";\n> +\t\targv_array_push(&array, \"-s\");\n>  \tif (!opts->edit) {\n> -\t\targs[i++] = \"-F\";\n> -\t\targs[i++] = defmsg;\n> +\t\targv_array_push(&array, \"-F\");\n> +\t\targv_array_push(&array, defmsg);\n>  \t}\n> +\t\n>  \tif (opts->allow_empty)\n> -\t\targs[i++] = \"--allow-empty\";\n> +\t\targv_array_push(&array, \"--allow-empty\");\n\nIf --keep-if-made-empty is not given but --allow-empty was, it is fine to\ngive --allow-empty here, but otherwise, it logically is iffy and is likely\nto become a cause of future bugs to pass --allow-empty to \"git commit\",\neven though the earlier check _ought_ to catch problematic cases, no?\n"},{"id":"189032","messageId":"7vwr5mhtb9.fsf@alter.siamese.dyndns.org","threadId":"30217","inReplyTo":"1334175716-11391-4-git-send-email-nhorman@tuxdriver.com","subject":"Re: [PATCH v4 3/4] git-cherry-pick: Add test to validate new options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T21:06:34Z","receivedAt":"2012-04-11T21:06:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neil Horman <nhorman@tuxdriver.com> writes:\n\n> Since we've added the --allow-empty and --keep-redundant-commits\n> options to git cherry-pick we should also add a test to ensure that its working\n> properly\n\nMissing full-stop at the end of the sentence\n\n> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>\n\n> +test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '\n> +\tgit checkout master &&\n> +\techo fourth >> file2 &&\n> +\tgit add file2 &&\n> +\tgit commit -m \"fourth\" && {\n> +\t\tgit cherry-pick empty-branch2\n> +\t\ttest \"$?\" = 1 \n> +\t}\n> +'\n\nDo we _deeply_ care that it fails with status 1, or should this be test_must_fail?\n\n> +test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '\n> +\tgit checkout master && {\n> +\t\tgit cherry-pick --allow-empty empty-branch2\n> +\t\ttest \"$?\" = 0\n> +\t}\n> +'\n\nLose the 'test \"$?\" = 0' here.\n\n> +test_expect_success 'cherry pick with --keep-redundant-commits' '\n> +\tgit checkout master && {\n> +\t\tgit cherry-pick --keep-redundant-commits HEAD^\n> +\t\ttest \"$?\" = 0\n\nLikewise.\n\n> +\t}\n> +'\n> +\n>  test_done\n\nThanks.\n"},{"id":"189033","messageId":"7vsjgaht7d.fsf@alter.siamese.dyndns.org","threadId":"30217","inReplyTo":"1334175716-11391-5-git-send-email-nhorman@tuxdriver.com","subject":"Re: [PATCH v4 4/4] git-rebase: add keep_empty flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T21:08:54Z","receivedAt":"2012-04-11T21:08:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This looks quite nice, but it seems that the change breaks at least t3416\nand t3404.  Has this series been tested?\n"},{"id":"189059","messageId":"20120411233558.GA16937@hmsreliant.think-freely.org","threadId":"30217","inReplyTo":"7v62d6j8qz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-11T23:35:58Z","receivedAt":"2012-04-11T23:35:58Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Wed, Apr 11, 2012 at 01:47:48PM -0700, Junio C Hamano wrote:\n> You inherited the following from your previous round.\n> \n> Applying: git-cherry-pick: Add keep-redundant-commits option\n> /srv/project/git/git.git/.git/rebase-apply/patch:115: space before tab in indent.\n>  \t\t\t * The head tree and the parent tree match\n> /srv/project/git/git.git/.git/rebase-apply/patch:116: space before tab in indent.\n>  \t\t\t * meaning the commit is empty.  Since it wasn't created\n> /srv/project/git/git.git/.git/rebase-apply/patch:117: space before tab in indent.\n>  \t\t\t * empty (based on the previous test), we can conclude\n> /srv/project/git/git.git/.git/rebase-apply/patch:118: space before tab in indent.\n>  \t\t\t * the commit has been made redundant.  Since we don't\n> /srv/project/git/git.git/.git/rebase-apply/patch:119: space before tab in indent.\n>  \t\t\t * want to keep redundant commits, just skip this one\n> warning: squelched 3 whitespace errors\n> fatal: 8 lines add whitespace errors.\n> Repository lacks necessary blobs to fall back on 3-way merge.\n> Cannot fall back to three-way merge.\n> Patch failed at 0001 git-cherry-pick: Add keep-redundant-commits option\n> \n\nGrr, thanks, forgot to run checkpatch\nNeil\n"},{"id":"189064","messageId":"20120411235509.GB16937@hmsreliant.think-freely.org","threadId":"30217","inReplyTo":"7v1unuj8az.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-11T23:55:09Z","receivedAt":"2012-04-11T23:55:09Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Wed, Apr 11, 2012 at 01:57:24PM -0700, Junio C Hamano wrote:\n> Neil Horman <nhorman@tuxdriver.com> writes:\n> \n> > The git-cherry-pick --allow-empty command by default only preserves empty\n> > commits that were originally empty, i.e only those commits for which\n> > <commit>^{tree} and <commit>^^{tree} are equal.  By default commits which are\n> > non-empty, but were made empty by the inclusion of a prior commit on the current\n> > history are filtered out.  This option allows us to override that behavior and\n> > include redundant commits as empty commits in the change history.\n> >\n> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>\n> > ---\n> >  Documentation/git-cherry-pick.txt |   12 +++++-\n> >  builtin/revert.c                  |    8 +++-\n> >  sequencer.c                       |   91 +++++++++++++++++++++++++++++++-----\n> >  sequencer.h                       |    1 +\n> >  4 files changed, 97 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n> > index 730237a..f96b8c5 100644\n> > --- a/Documentation/git-cherry-pick.txt\n> > +++ b/Documentation/git-cherry-pick.txt\n> > @@ -110,7 +110,17 @@ effect to your index in a row.\n> >  \tbehavior, allowing empty commits to be preserved automatically\n> >  \tin a cherry-pick. Note that when \"--ff\" is in effect, empty\n> >  \tcommits that meet the \"fast-forward\" requirement will be kept\n> > -\teven without this option.\n> > +\teven without this option.  Note also, that use of this option only\n> > +\tkeeps commits that were initially empty (i.e. where for commit C\n> > +\tC^{tree} and C^^{tree} are equal).  Commits which are made empty due to\n> \n> It is OK to be technical in the log message, but I think\n> \n> \tcommits that were initially empty (i.e. the commit recorded the\n> \tsame tree as its parent)\n> \n> would be far easier to read in the documentation meant for the end users.\n> \n> > +\ta previous commit are ignored.  To force the inclusion of those commits\n> > +\tuse `--keep-redundant-commits`\n> \n> And end the sentence with a full-stop.\n> \nCopy that.\n\n> > +--keep-redundant-commits::\n> > +\tIf a commit being cherry picked duplicates a commit already in the\n> > +\tcurrent history, it will result in an empty changeset.  By default these\n> > +\tredundant commits are ignored.  This option overrides that behavior and\n> > +\tcreates an empty commit object.  Implies `--allow-empty`\n> \n> Likewise.\n> \nACK\n\n> > diff --git a/builtin/revert.c b/builtin/revert.c\n> > index 06b00e6..4f0d979 100644\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> >  \t\tOPT_END(),\n> >  \t\tOPT_END(),\n> >  \t\tOPT_END(),\n> > +\t\tOPT_END(),\n> >  \t};\n> >  \n> >  \tif (opts->action == REPLAY_PICK) {\n> >  \t\tstruct option cp_extra[] = {\n> >  \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n> >  \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n> > -\t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, \"preserve empty commits\"),\n> > +\t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, \"preserve initially empty commits\"),\n> > +\t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_if_made_empty, \"keep redundant, empty commits\"),\n> >  \t\t\tOPT_END(),\n> >  \t\t};\n> >  \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\n> > @@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n> >  \t\t\t\t\"--abort\", rollback,\n> >  \t\t\t\tNULL);\n> >  \n> > +\t/* keep_if_made_empty implies allow_empty */\n> > +\tif (opts->keep_if_made_empty)\n> > +\t\topts->allow_empty = 1;\n> > +\n> >  \t/* Set the subcommand */\n> >  \tif (remove_state)\n> >  \t\topts->subcommand = REPLAY_REMOVE_STATE;\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 71929ba..442f364 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -13,6 +13,7 @@\n> >  #include \"rerere.h\"\n> >  #include \"merge-recursive.h\"\n> >  #include \"refs.h\"\n> > +#include \"argv-array.h\"\n> >  \n> >  #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n> >  \n> > @@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> >   * If we are revert, or if our cherry-pick results in a hand merge,\n> >   * we had better say that the current user is responsible for that.\n> >   */\n> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)\n> >  {\n> > -\t/* 7 is max possible length of our args array including NULL */\n> > -\tconst char *args[7];\n> > -\tint i = 0;\n> > +\tstruct argv_array array;\n> > +\tint rc;\n> > +\n> > +\tif (!empty && !opts->keep_if_made_empty) {\n> > +\t\tunsigned char head_sha1[20];\n> > +\t\tstruct commit *head_commit;\n> > +\t\tint need_free = 0;\n> > +\n> > +\t\tresolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n> > +\t\thead_commit = lookup_commit(head_sha1);\n> \n> No error checking whatsoever?  HEAD might be pointing at a branch that\n> hasn't been born, for example.\n> \nSorry, I had presumed that HEAD could be guaranteed to be good here, given that\nwe validate it in rollback_single_pick via read_ref_full.  But I can add\nadditional checking here.\n\n> > +\t\tif (parse_commit(head_commit) < 0)\n> > +\t\t\treturn error(_(\"could not parse commit %s\\n\"),\n> > +\t\t\t\t     sha1_to_hex(head_commit->object.sha1));\n> > +\n> > +\t\tif (!active_cache_tree) {\n> > +\t\t\tactive_cache_tree = cache_tree();\n> > +\t\t\tneed_free = 1;\n> > +\t\t}\n> \n> I think this is wrong.  If for whatever reason the process does not have\n> the index it is about to tell \"git commit\" to use to make a commit in-core,\n> it should first be doing read_cache().\nI would have thought so too, and perhaps it should, but it doesn't seem to be\nthe case.  Initially I hadn't added this code, but during testing I received a\nsegfault indicating that the_index.cache_tree was NULL.  I thought it should\nhave been filled in via the calls to read_cache in do_recursive_merge or\nfast_forward_to, but it didn't seem to be the case.  I added a read_cache call\nright before my call to cache_tree_update, but still the_index.cache_tree\nremained NULL.  Looking at the read_cache code, it appears that cache_tree is\nonly allocated if a CACHE_EXT_TREE is found when parsing the index.  Not sure\nwhy I wouldn't have one, but certainly it didn't seem I did.  The above code\nfixed it, which I lifted directly from write_tree_from_memory()\n\n\n> > +\n> > +\t\tif (!cache_tree_fully_valid(active_cache_tree))\n> > +\t\t\tcache_tree_update(active_cache_tree, active_cache,\n> > +\t\t\t\t\t  active_nr, 0);\n> \n> I do not recall offhand if cache_tree_update() can give you an error, but\n> if it does, it should be checked here.\n> \nIt can, and I'll add checking there, but I hit the segfault above prior to\ngetting any error checknig done.\n\n> > +\t\trc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);\n> > +\n> > +\t\tif (need_free)\n> > +\t\t\tcache_tree_free(&active_cache_tree);\n> \n> and I don't see a need for nuking the cache tree here, unless you are\n> discarding the_index as well.\n> \nJust balancing the allocation above with a free.  I didn't see a need for it\nany longer, given that we were shortly going to call git commit and then exit\nthis process.  Is it safe to leave hanging around?\n\n> > +\t\tif (rc)\n> > +\t\t\t/*\n> > + \t\t\t * The head tree and the parent tree match\n> > + \t\t\t * meaning the commit is empty.  Since it wasn't created\n> > + \t\t\t * empty (based on the previous test), we can conclude\n> > + \t\t\t * the commit has been made redundant.  Since we don't\n> > + \t\t\t * want to keep redundant commits, just skip this one\n> > + \t\t\t */\n> > +\t\t\treturn 0;\n> > +\t}\n> > +\n> > +\targv_array_init(&array);\n> > +\targv_array_push(&array, \"commit\");\n> > +\targv_array_push(&array, \"-n\");\n> >  \n> > -\targs[i++] = \"commit\";\n> > -\targs[i++] = \"-n\";\n> >  \tif (opts->signoff)\n> > -\t\targs[i++] = \"-s\";\n> > +\t\targv_array_push(&array, \"-s\");\n> >  \tif (!opts->edit) {\n> > -\t\targs[i++] = \"-F\";\n> > -\t\targs[i++] = defmsg;\n> > +\t\targv_array_push(&array, \"-F\");\n> > +\t\targv_array_push(&array, defmsg);\n> >  \t}\n> > +\t\n> >  \tif (opts->allow_empty)\n> > -\t\targs[i++] = \"--allow-empty\";\n> > +\t\targv_array_push(&array, \"--allow-empty\");\n> \n> If --keep-if-made-empty is not given but --allow-empty was, it is fine to\n> give --allow-empty here, but otherwise, it logically is iffy and is likely\n> to become a cause of future bugs to pass --allow-empty to \"git commit\",\n> even though the earlier check _ought_ to catch problematic cases, no?\n> \nNot sure I follow your reasoning here.  We need to pass allow-empty to git\ncommit here if git cherry-pick set either allow-empty or keep-redundant-commits\n(the latter setting opts->empty), Can you give an example of what might be\nproblematic in the future?\n\nNeil\n"},{"id":"189065","messageId":"20120411235909.GC16937@hmsreliant.think-freely.org","threadId":"30217","inReplyTo":"7vsjgaht7d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 4/4] git-rebase: add keep_empty flag","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-11T23:59:09Z","receivedAt":"2012-04-11T23:59:09Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Wed, Apr 11, 2012 at 02:08:54PM -0700, Junio C Hamano wrote:\n> This looks quite nice, but it seems that the change breaks at least t3416\n> and t3404.  Has this series been tested?\n> \nI did not, mostly because those tests didn't seem to work with empty commits at\nall.  And rebases (interactive and automatic) that I've done manually in testing\nworked fine.  I'll check on them in the AM though.\n\nNeil\n"},{"id":"189066","messageId":"7vty0phc8s.fsf@alter.siamese.dyndns.org","threadId":"30217","inReplyTo":"20120411235509.GB16937@hmsreliant.think-freely.org","subject":"Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-12T03:15:15Z","receivedAt":"2012-04-12T03:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neil Horman <nhorman@tuxdriver.com> writes:\n\n>> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n>> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)\n>> >  {\n>> > -\t/* 7 is max possible length of our args array including NULL */\n>> > -\tconst char *args[7];\n>> > -\tint i = 0;\n>> > +\tstruct argv_array array;\n>> > +\tint rc;\n>> > +\n>> > +\tif (!empty && !opts->keep_if_made_empty) {\n>> > +\t\tunsigned char head_sha1[20];\n>> > +\t\tstruct commit *head_commit;\n>> > +\t\tint need_free = 0;\n>> > +\n>> > +\t\tresolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n>> > +\t\thead_commit = lookup_commit(head_sha1);\n>> \n>> No error checking whatsoever?  HEAD might be pointing at a branch that\n>> hasn't been born, for example.\n>> \n> Sorry, I had presumed that HEAD could be guaranteed to be good here, given that\n> we validate it in rollback_single_pick via read_ref_full.  But I can add\n> additional checking here.\n\nAs long as it is clear that we always have \"HEAD\" here, it is not strictly\nnecessary, but we'd probably feel better, especially now that the function\nis (for some reason) made into a global one and may gain other new callers\noutside this file scope.\n\n>> >  \tif (opts->allow_empty)\n>> > -\t\targs[i++] = \"--allow-empty\";\n>> > +\t\targv_array_push(&array, \"--allow-empty\");\n>> \n>> If --keep-if-made-empty is not given but --allow-empty was, it is fine to\n>> give --allow-empty here, but otherwise, it logically is iffy and is likely\n>> to become a cause of future bugs to pass --allow-empty to \"git commit\",\n>> even though the earlier check _ought_ to catch problematic cases, no?\n>> \n> Not sure I follow your reasoning here.  We need to pass allow-empty to git\n> commit here if git cherry-pick set either allow-empty or keep-redundant-commits\n> (the latter setting opts->empty), Can you give an example of what might be\n> problematic in the future?\n\nThinking about it again, I think this part of your patch is correct.\n\nWe may pass an index that yields the same tree as \"HEAD\" if the original\nwas empty, or the original was not empty but the merge found the commit\nunneeded.  In either case, if \"--keep-unnecessary\" or \"--allow-empty\" was\ngiven, we want to record the empty commit, and opts->allow_empty is set\nwhen either option was given from the command line.\n\nThanks.\n"},{"id":"189067","messageId":"7vpqbdhc74.fsf@alter.siamese.dyndns.org","threadId":"30217","inReplyTo":"20120411235909.GC16937@hmsreliant.think-freely.org","subject":"Re: [PATCH v4 4/4] git-rebase: add keep_empty flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-12T03:16:15Z","receivedAt":"2012-04-12T03:16:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neil Horman <nhorman@tuxdriver.com> writes:\n\n> I did not, mostly because those tests didn't seem to work with empty commits at\n> all.\n\nYeah, existing tests won't even try to rebase an empty or unnecessary\ncommit, so if the change breaks them, you found regressions.\n"},{"id":"189098","messageId":"20120412104916.GB32669@hmsreliant.think-freely.org","threadId":"30217","inReplyTo":"7vty0phc8s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-12T10:49:16Z","receivedAt":"2012-04-12T10:49:16Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Wed, Apr 11, 2012 at 08:15:15PM -0700, Junio C Hamano wrote:\n> Neil Horman <nhorman@tuxdriver.com> writes:\n> \n> >> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n> >> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)\n> >> >  {\n> >> > -\t/* 7 is max possible length of our args array including NULL */\n> >> > -\tconst char *args[7];\n> >> > -\tint i = 0;\n> >> > +\tstruct argv_array array;\n> >> > +\tint rc;\n> >> > +\n> >> > +\tif (!empty && !opts->keep_if_made_empty) {\n> >> > +\t\tunsigned char head_sha1[20];\n> >> > +\t\tstruct commit *head_commit;\n> >> > +\t\tint need_free = 0;\n> >> > +\n> >> > +\t\tresolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n> >> > +\t\thead_commit = lookup_commit(head_sha1);\n> >> \n> >> No error checking whatsoever?  HEAD might be pointing at a branch that\n> >> hasn't been born, for example.\n> >> \n> > Sorry, I had presumed that HEAD could be guaranteed to be good here, given that\n> > we validate it in rollback_single_pick via read_ref_full.  But I can add\n> > additional checking here.\n> \n> As long as it is clear that we always have \"HEAD\" here, it is not strictly\n> necessary, but we'd probably feel better, especially now that the function\n> is (for some reason) made into a global one and may gain other new callers\n> outside this file scope.\n> \nGah!  Thanks for that, its not meant to be static.  I made that global so that\ngdb could see it and set a breakpoint on it, while I was figuring out the\nsegfault I was getting when active_cache_tree was NULL.  I'll fix that.\n\n> >> >  \tif (opts->allow_empty)\n> >> > -\t\targs[i++] = \"--allow-empty\";\n> >> > +\t\targv_array_push(&array, \"--allow-empty\");\n> >> \n> >> If --keep-if-made-empty is not given but --allow-empty was, it is fine to\n> >> give --allow-empty here, but otherwise, it logically is iffy and is likely\n> >> to become a cause of future bugs to pass --allow-empty to \"git commit\",\n> >> even though the earlier check _ought_ to catch problematic cases, no?\n> >> \n> > Not sure I follow your reasoning here.  We need to pass allow-empty to git\n> > commit here if git cherry-pick set either allow-empty or keep-redundant-commits\n> > (the latter setting opts->empty), Can you give an example of what might be\n> > problematic in the future?\n> \n> Thinking about it again, I think this part of your patch is correct.\n> \n> We may pass an index that yields the same tree as \"HEAD\" if the original\n> was empty, or the original was not empty but the merge found the commit\n> unneeded.  In either case, if \"--keep-unnecessary\" or \"--allow-empty\" was\n> given, we want to record the empty commit, and opts->allow_empty is set\n> when either option was given from the command line.\n> \nYes, exactly.\nThanks.\n\n> Thanks.\n> \n"},{"id":"189114","messageId":"20120412155849.GA2749@neilslaptop.think-freely.org","threadId":"30217","inReplyTo":"7vsjgaht7d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 4/4] git-rebase: add keep_empty flag","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-04-12T15:58:49Z","receivedAt":"2012-04-12T15:58:49Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Wed, Apr 11, 2012 at 02:08:54PM -0700, Junio C Hamano wrote:\n> This looks quite nice, but it seems that the change breaks at least t3416\n> and t3404.  Has this series been tested?\n> \nI've looked at these, and I think it appears that this is the result of the\nchanges I made to interactive rebasing in which empty comments were commented\nout, and additional notes were added to the commit editor template.  It seems to\nbe messing with what the fake editor for these tests is expecting.  I've got\nsome things I need to do tonight and tomorrow, but I'll fix this up over the\nweekend.\n\nThanks!\nNeil\n"}]}