[PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits

[PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits

[Neil Horman]

git's ability to handle empty commits is somewhat lacking, especially when
preforming a rebase.  Nominally empty commits are undesireable entries, the 
result of commits that are made empty by prior commits covering the same changs.
But occasionally, empty commits are useful to developers (e.g. inserting notes 
into the development history without changing any code along the way).  In these
cases its desireable to easily preserve empty commits during operations like 
rebases.

This patch series enhances git to do just that.  It adds two options to the 
git-cherry-pick command, --allow-empty, which allows git cherry-pick to preserve
an empty commit, even if the fast forward logic isn't applicable during the 
operation, and --keep-redundant-commits, which allows the user to also keep
commits that were made empty via conflict resolution.  It also enhances
git-rebase to add a --keep-empty option which enables rebases to preserve empty
commits. 

I've tested these operations out myself here and they work well for me

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

---
Change notes:

Based on version 1 feedback from this list, the following changes have been made

V2)
	* Changed --keep-empty to --allow-empty in the git cherry-pick command

	* Converted run_git_commit to use argv_array

	* Updated cherry-pick --allow-empty description in man page
	
	* added ignore-if-made-empty option to git-cherry-pick

	* Added test to test suite to validate the new cherry-pick options

	* Updated git-rebase man page to be less verbose and more accurate in the
	description of the keep-empty option

	* squashed the addition of the keep-empty flag in git-rebase down to one
	commit from 3

	* fixed up coding style in git-rebase script

	* Optimized detection of empty commits

	* Only augmented git-rebase-editor message if empty commits are
	possible
	
V3)
	* reversed the --ignore-if-empty-logic to by default only keep initially
	empty commits

	* replaced --ignore-if-empty with --keep-redundant-commits, to allow
	empty commits that are made empty via conflict resolution, in addition
	to commits that were created as empty

	* reworked is_original_commit_empty to be more efficient and portable

	* Misc sylistic and spelling cleanups

V4)
	* Reverted the cherry-pick advice changes in V3 based on in-thread
	discussion

	* Rewrote my changes to is_original_commit_empty and run_git_commit to
	not have to fork, making them more efficient.

[PATCH v4 1/4] git-cherry-pick: add allow-empty option

[Neil Horman]

git cherry-pick fails when picking a non-ff commit that is empty.  The advice
given with the failure is that a git-commit --allow-empty should be issued to
explicitly add the empty commit during the cherry pick.  This option allows a
user to specify before hand that they want to keep the empty commit.  This
eliminates the need to issue both a cherry pick and a commit operation.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
---
 Documentation/git-cherry-pick.txt |    9 +++++++++
 builtin/revert.c                  |    2 ++
 sequencer.c                       |    7 +++++--
 sequencer.h                       |    1 +
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index fed5097..730237a 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -103,6 +103,15 @@ effect to your index in a row.
 	cherry-pick'ed commit, then a fast forward to this commit will
 	be performed.
 
+--allow-empty::
+	By default, cherry-picking an empty commit will fail,
+	indicating that an explicit invocation of `git commit
+	--allow-empty` is required. This option overrides that
+	behavior, allowing empty commits to be preserved automatically
+	in a cherry-pick. Note that when "--ff" is in effect, empty
+	commits that meet the "fast-forward" requirement will be kept
+	even without this option.
+
 --strategy=<strategy>::
 	Use the given merge strategy.  Should only be used once.
 	See the MERGE STRATEGIES section in linkgit:git-merge[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index e6840f2..06b00e6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -114,12 +114,14 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
+			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve empty commits"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
diff --git a/sequencer.c b/sequencer.c
index a37846a..71929ba 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -260,8 +260,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 {
-	/* 6 is max possible length of our args array including NULL */
-	const char *args[6];
+	/* 7 is max possible length of our args array including NULL */
+	const char *args[7];
 	int i = 0;
 
 	args[i++] = "commit";
@@ -272,6 +272,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 		args[i++] = "-F";
 		args[i++] = defmsg;
 	}
+	if (opts->allow_empty)
+		args[i++] = "--allow-empty";
+
 	args[i] = NULL;
 
 	return run_command_v_opt(args, RUN_GIT_CMD);
diff --git a/sequencer.h b/sequencer.h
index bb4b138..e2cd725 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -29,6 +29,7 @@ struct replay_opts {
 	int signoff;
 	int allow_ff;
 	int allow_rerere_auto;
+	int allow_empty;
 
 	int mainline;
 
-- 
1.7.7.6

[PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option

[Neil Horman]

The git-cherry-pick --allow-empty command by default only preserves empty
commits that were originally empty, i.e only those commits for which
<commit>^{tree} and <commit>^^{tree} are equal.  By default commits which are
non-empty, but were made empty by the inclusion of a prior commit on the current
history are filtered out.  This option allows us to override that behavior and
include redundant commits as empty commits in the change history.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
---
 Documentation/git-cherry-pick.txt |   12 +++++-
 builtin/revert.c                  |    8 +++-
 sequencer.c                       |   91 +++++++++++++++++++++++++++++++-----
 sequencer.h                       |    1 +
 4 files changed, 97 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 730237a..f96b8c5 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -110,7 +110,17 @@ effect to your index in a row.
 	behavior, allowing empty commits to be preserved automatically
 	in a cherry-pick. Note that when "--ff" is in effect, empty
 	commits that meet the "fast-forward" requirement will be kept
-	even without this option.
+	even without this option.  Note also, that use of this option only
+	keeps commits that were initially empty (i.e. where for commit C
+	C^{tree} and C^^{tree} are equal).  Commits which are made empty due to
+	a previous commit are ignored.  To force the inclusion of those commits
+	use `--keep-redundant-commits`
+
+--keep-redundant-commits::
+	If a commit being cherry picked duplicates a commit already in the
+	current history, it will result in an empty changeset.  By default these
+	redundant commits are ignored.  This option overrides that behavior and
+	creates an empty commit object.  Implies `--allow-empty`
 
 --strategy=<strategy>::
 	Use the given merge strategy.  Should only be used once.
diff --git a/builtin/revert.c b/builtin/revert.c
index 06b00e6..4f0d979 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
-			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve empty commits"),
+			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve initially empty commits"),
+			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
@@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"--abort", rollback,
 				NULL);
 
+	/* keep_if_made_empty implies allow_empty */
+	if (opts->keep_if_made_empty)
+		opts->allow_empty = 1;
+
 	/* Set the subcommand */
 	if (remove_state)
 		opts->subcommand = REPLAY_REMOVE_STATE;
diff --git a/sequencer.c b/sequencer.c
index 71929ba..442f364 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -13,6 +13,7 @@
 #include "rerere.h"
 #include "merge-recursive.h"
 #include "refs.h"
+#include "argv-array.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
  * If we are revert, or if our cherry-pick results in a hand merge,
  * we had better say that the current user is responsible for that.
  */
-static int run_git_commit(const char *defmsg, struct replay_opts *opts)
+int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
 {
-	/* 7 is max possible length of our args array including NULL */
-	const char *args[7];
-	int i = 0;
+	struct argv_array array;
+	int rc;
+
+	if (!empty && !opts->keep_if_made_empty) {
+		unsigned char head_sha1[20];
+		struct commit *head_commit;
+		int need_free = 0;
+
+		resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
+		head_commit = lookup_commit(head_sha1);
+		if (parse_commit(head_commit) < 0)
+			return error(_("could not parse commit %s\n"),
+				     sha1_to_hex(head_commit->object.sha1));
+
+		if (!active_cache_tree) {
+			active_cache_tree = cache_tree();
+			need_free = 1;
+		}
+
+		if (!cache_tree_fully_valid(active_cache_tree))
+			cache_tree_update(active_cache_tree, active_cache,
+					  active_nr, 0);
+
+		rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
+
+		if (need_free)
+			cache_tree_free(&active_cache_tree);
+
+		if (rc)
+			/*
+ 			 * The head tree and the parent tree match
+ 			 * meaning the commit is empty.  Since it wasn't created
+ 			 * empty (based on the previous test), we can conclude
+ 			 * the commit has been made redundant.  Since we don't
+ 			 * want to keep redundant commits, just skip this one
+ 			 */
+			return 0;
+	}
+
+	argv_array_init(&array);
+	argv_array_push(&array, "commit");
+	argv_array_push(&array, "-n");
 
-	args[i++] = "commit";
-	args[i++] = "-n";
 	if (opts->signoff)
-		args[i++] = "-s";
+		argv_array_push(&array, "-s");
 	if (!opts->edit) {
-		args[i++] = "-F";
-		args[i++] = defmsg;
+		argv_array_push(&array, "-F");
+		argv_array_push(&array, defmsg);
 	}
+	
 	if (opts->allow_empty)
-		args[i++] = "--allow-empty";
+		argv_array_push(&array, "--allow-empty");
+
+
+	rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
+	argv_array_clear(&array);
+	return rc;
+}
 
-	args[i] = NULL;
+static int is_original_commit_empty(struct commit *commit)
+{
+	const unsigned char *ptree_sha1;
+	
+	if (parse_commit(commit))
+		return error(_("Could not parse commit %s\n"),
+			     sha1_to_hex(commit->object.sha1));
+	if (commit->parents) {
+		struct commit *parent = commit->parents->item;
+		if (parse_commit(parent))
+			return error(_("Could not parse parent commit %s\n"),
+				sha1_to_hex(parent->object.sha1));
+		ptree_sha1 = parent->tree->object.sha1;
+	} else {
+		ptree_sha1 = EMPTY_TREE_SHA1_BIN; /* commit is root */
+	}
 
-	return run_command_v_opt(args, RUN_GIT_CMD);
+	return !hashcmp(ptree_sha1, commit->tree->object.sha1);
 }
 
 static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
@@ -289,6 +349,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
+	int empty_commit;
 
 	if (opts->no_commit) {
 		/*
@@ -414,6 +475,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		free_commit_list(remotes);
 	}
 
+	empty_commit = is_original_commit_empty(commit);
+	if (empty_commit < 0)
+		return empty_commit;
+
 	/*
 	 * If the merge was clean or if it failed due to conflict, we write
 	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
@@ -435,7 +500,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		rerere(opts->allow_rerere_auto);
 	} else {
 		if (!opts->no_commit)
-			res = run_git_commit(defmsg, opts);
+			res = run_git_commit(defmsg, opts, empty_commit);
 	}
 
 	free_message(&msg);
diff --git a/sequencer.h b/sequencer.h
index e2cd725..862a79a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -30,6 +30,7 @@ struct replay_opts {
 	int allow_ff;
 	int allow_rerere_auto;
 	int allow_empty;
+	int keep_if_made_empty;
 
 	int mainline;
 
-- 
1.7.7.6

Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option

[Junio C Hamano]

You inherited the following from your previous round.

Applying: git-cherry-pick: Add keep-redundant-commits option
/srv/project/git/git.git/.git/rebase-apply/patch:115: space before tab in indent.
 			 * The head tree and the parent tree match
/srv/project/git/git.git/.git/rebase-apply/patch:116: space before tab in indent.
 			 * meaning the commit is empty.  Since it wasn't created
/srv/project/git/git.git/.git/rebase-apply/patch:117: space before tab in indent.
 			 * empty (based on the previous test), we can conclude
/srv/project/git/git.git/.git/rebase-apply/patch:118: space before tab in indent.
 			 * the commit has been made redundant.  Since we don't
/srv/project/git/git.git/.git/rebase-apply/patch:119: space before tab in indent.
 			 * want to keep redundant commits, just skip this one
warning: squelched 3 whitespace errors
fatal: 8 lines add whitespace errors.
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 git-cherry-pick: Add keep-redundant-commits option

Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option

[Neil Horman]

On Wed, Apr 11, 2012 at 01:47:48PM -0700, Junio C Hamano wrote:
> You inherited the following from your previous round.
> 
> Applying: git-cherry-pick: Add keep-redundant-commits option
> /srv/project/git/git.git/.git/rebase-apply/patch:115: space before tab in indent.
>  			 * The head tree and the parent tree match
> /srv/project/git/git.git/.git/rebase-apply/patch:116: space before tab in indent.
>  			 * meaning the commit is empty.  Since it wasn't created
> /srv/project/git/git.git/.git/rebase-apply/patch:117: space before tab in indent.
>  			 * empty (based on the previous test), we can conclude
> /srv/project/git/git.git/.git/rebase-apply/patch:118: space before tab in indent.
>  			 * the commit has been made redundant.  Since we don't
> /srv/project/git/git.git/.git/rebase-apply/patch:119: space before tab in indent.
>  			 * want to keep redundant commits, just skip this one
> warning: squelched 3 whitespace errors
> fatal: 8 lines add whitespace errors.
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 git-cherry-pick: Add keep-redundant-commits option
> 

Grr, thanks, forgot to run checkpatch
Neil

Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option

[Junio C Hamano]

Neil Horman <nhorman@tuxdriver.com> writes:

> The git-cherry-pick --allow-empty command by default only preserves empty
> commits that were originally empty, i.e only those commits for which
> <commit>^{tree} and <commit>^^{tree} are equal.  By default commits which are
> non-empty, but were made empty by the inclusion of a prior commit on the current
> history are filtered out.  This option allows us to override that behavior and
> include redundant commits as empty commits in the change history.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> ---
>  Documentation/git-cherry-pick.txt |   12 +++++-
>  builtin/revert.c                  |    8 +++-
>  sequencer.c                       |   91 +++++++++++++++++++++++++++++++-----
>  sequencer.h                       |    1 +
>  4 files changed, 97 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index 730237a..f96b8c5 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -110,7 +110,17 @@ effect to your index in a row.
>  	behavior, allowing empty commits to be preserved automatically
>  	in a cherry-pick. Note that when "--ff" is in effect, empty
>  	commits that meet the "fast-forward" requirement will be kept
> -	even without this option.
> +	even without this option.  Note also, that use of this option only
> +	keeps commits that were initially empty (i.e. where for commit C
> +	C^{tree} and C^^{tree} are equal).  Commits which are made empty due to

It is OK to be technical in the log message, but I think

	commits that were initially empty (i.e. the commit recorded the
	same tree as its parent)

would be far easier to read in the documentation meant for the end users.

> +	a previous commit are ignored.  To force the inclusion of those commits
> +	use `--keep-redundant-commits`

And end the sentence with a full-stop.

> +--keep-redundant-commits::
> +	If a commit being cherry picked duplicates a commit already in the
> +	current history, it will result in an empty changeset.  By default these
> +	redundant commits are ignored.  This option overrides that behavior and
> +	creates an empty commit object.  Implies `--allow-empty`

Likewise.

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 06b00e6..4f0d979 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  		OPT_END(),
>  		OPT_END(),
>  		OPT_END(),
> +		OPT_END(),
>  	};
>  
>  	if (opts->action == REPLAY_PICK) {
>  		struct option cp_extra[] = {
>  			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
>  			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
> -			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve empty commits"),
> +			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve initially empty commits"),
> +			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"),
>  			OPT_END(),
>  		};
>  		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
> @@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  				"--abort", rollback,
>  				NULL);
>  
> +	/* keep_if_made_empty implies allow_empty */
> +	if (opts->keep_if_made_empty)
> +		opts->allow_empty = 1;
> +
>  	/* Set the subcommand */
>  	if (remove_state)
>  		opts->subcommand = REPLAY_REMOVE_STATE;
> diff --git a/sequencer.c b/sequencer.c
> index 71929ba..442f364 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -13,6 +13,7 @@
>  #include "rerere.h"
>  #include "merge-recursive.h"
>  #include "refs.h"
> +#include "argv-array.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>   * If we are revert, or if our cherry-pick results in a hand merge,
>   * we had better say that the current user is responsible for that.
>   */
> -static int run_git_commit(const char *defmsg, struct replay_opts *opts)
> +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
>  {
> -	/* 7 is max possible length of our args array including NULL */
> -	const char *args[7];
> -	int i = 0;
> +	struct argv_array array;
> +	int rc;
> +
> +	if (!empty && !opts->keep_if_made_empty) {
> +		unsigned char head_sha1[20];
> +		struct commit *head_commit;
> +		int need_free = 0;
> +
> +		resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
> +		head_commit = lookup_commit(head_sha1);

No error checking whatsoever?  HEAD might be pointing at a branch that
hasn't been born, for example.

> +		if (parse_commit(head_commit) < 0)
> +			return error(_("could not parse commit %s\n"),
> +				     sha1_to_hex(head_commit->object.sha1));
> +
> +		if (!active_cache_tree) {
> +			active_cache_tree = cache_tree();
> +			need_free = 1;
> +		}

I think this is wrong.  If for whatever reason the process does not have
the index it is about to tell "git commit" to use to make a commit in-core,
it should first be doing read_cache().
> +
> +		if (!cache_tree_fully_valid(active_cache_tree))
> +			cache_tree_update(active_cache_tree, active_cache,
> +					  active_nr, 0);

I do not recall offhand if cache_tree_update() can give you an error, but
if it does, it should be checked here.

> +		rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
> +
> +		if (need_free)
> +			cache_tree_free(&active_cache_tree);

and I don't see a need for nuking the cache tree here, unless you are
discarding the_index as well.

> +		if (rc)
> +			/*
> + 			 * The head tree and the parent tree match
> + 			 * meaning the commit is empty.  Since it wasn't created
> + 			 * empty (based on the previous test), we can conclude
> + 			 * the commit has been made redundant.  Since we don't
> + 			 * want to keep redundant commits, just skip this one
> + 			 */
> +			return 0;
> +	}
> +
> +	argv_array_init(&array);
> +	argv_array_push(&array, "commit");
> +	argv_array_push(&array, "-n");
>  
> -	args[i++] = "commit";
> -	args[i++] = "-n";
>  	if (opts->signoff)
> -		args[i++] = "-s";
> +		argv_array_push(&array, "-s");
>  	if (!opts->edit) {
> -		args[i++] = "-F";
> -		args[i++] = defmsg;
> +		argv_array_push(&array, "-F");
> +		argv_array_push(&array, defmsg);
>  	}
> +	
>  	if (opts->allow_empty)
> -		args[i++] = "--allow-empty";
> +		argv_array_push(&array, "--allow-empty");

If --keep-if-made-empty is not given but --allow-empty was, it is fine to
give --allow-empty here, but otherwise, it logically is iffy and is likely
to become a cause of future bugs to pass --allow-empty to "git commit",
even though the earlier check _ought_ to catch problematic cases, no?

Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option

[Neil Horman]

On Wed, Apr 11, 2012 at 01:57:24PM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > The git-cherry-pick --allow-empty command by default only preserves empty
> > commits that were originally empty, i.e only those commits for which
> > <commit>^{tree} and <commit>^^{tree} are equal.  By default commits which are
> > non-empty, but were made empty by the inclusion of a prior commit on the current
> > history are filtered out.  This option allows us to override that behavior and
> > include redundant commits as empty commits in the change history.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > ---
> >  Documentation/git-cherry-pick.txt |   12 +++++-
> >  builtin/revert.c                  |    8 +++-
> >  sequencer.c                       |   91 +++++++++++++++++++++++++++++++-----
> >  sequencer.h                       |    1 +
> >  4 files changed, 97 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> > index 730237a..f96b8c5 100644
> > --- a/Documentation/git-cherry-pick.txt
> > +++ b/Documentation/git-cherry-pick.txt
> > @@ -110,7 +110,17 @@ effect to your index in a row.
> >  	behavior, allowing empty commits to be preserved automatically
> >  	in a cherry-pick. Note that when "--ff" is in effect, empty
> >  	commits that meet the "fast-forward" requirement will be kept
> > -	even without this option.
> > +	even without this option.  Note also, that use of this option only
> > +	keeps commits that were initially empty (i.e. where for commit C
> > +	C^{tree} and C^^{tree} are equal).  Commits which are made empty due to
> 
> It is OK to be technical in the log message, but I think
> 
> 	commits that were initially empty (i.e. the commit recorded the
> 	same tree as its parent)
> 
> would be far easier to read in the documentation meant for the end users.
> 
> > +	a previous commit are ignored.  To force the inclusion of those commits
> > +	use `--keep-redundant-commits`
> 
> And end the sentence with a full-stop.
> 
Copy that.

> > +--keep-redundant-commits::
> > +	If a commit being cherry picked duplicates a commit already in the
> > +	current history, it will result in an empty changeset.  By default these
> > +	redundant commits are ignored.  This option overrides that behavior and
> > +	creates an empty commit object.  Implies `--allow-empty`
> 
> Likewise.
> 
ACK

> > diff --git a/builtin/revert.c b/builtin/revert.c
> > index 06b00e6..4f0d979 100644
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> >  		OPT_END(),
> >  		OPT_END(),
> >  		OPT_END(),
> > +		OPT_END(),
> >  	};
> >  
> >  	if (opts->action == REPLAY_PICK) {
> >  		struct option cp_extra[] = {
> >  			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
> >  			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
> > -			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve empty commits"),
> > +			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve initially empty commits"),
> > +			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"),
> >  			OPT_END(),
> >  		};
> >  		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
> > @@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> >  				"--abort", rollback,
> >  				NULL);
> >  
> > +	/* keep_if_made_empty implies allow_empty */
> > +	if (opts->keep_if_made_empty)
> > +		opts->allow_empty = 1;
> > +
> >  	/* Set the subcommand */
> >  	if (remove_state)
> >  		opts->subcommand = REPLAY_REMOVE_STATE;
> > diff --git a/sequencer.c b/sequencer.c
> > index 71929ba..442f364 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -13,6 +13,7 @@
> >  #include "rerere.h"
> >  #include "merge-recursive.h"
> >  #include "refs.h"
> > +#include "argv-array.h"
> >  
> >  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
> >  
> > @@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> >   * If we are revert, or if our cherry-pick results in a hand merge,
> >   * we had better say that the current user is responsible for that.
> >   */
> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts)
> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
> >  {
> > -	/* 7 is max possible length of our args array including NULL */
> > -	const char *args[7];
> > -	int i = 0;
> > +	struct argv_array array;
> > +	int rc;
> > +
> > +	if (!empty && !opts->keep_if_made_empty) {
> > +		unsigned char head_sha1[20];
> > +		struct commit *head_commit;
> > +		int need_free = 0;
> > +
> > +		resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
> > +		head_commit = lookup_commit(head_sha1);
> 
> No error checking whatsoever?  HEAD might be pointing at a branch that
> hasn't been born, for example.
> 
Sorry, I had presumed that HEAD could be guaranteed to be good here, given that
we validate it in rollback_single_pick via read_ref_full.  But I can add
additional checking here.

> > +		if (parse_commit(head_commit) < 0)
> > +			return error(_("could not parse commit %s\n"),
> > +				     sha1_to_hex(head_commit->object.sha1));
> > +
> > +		if (!active_cache_tree) {
> > +			active_cache_tree = cache_tree();
> > +			need_free = 1;
> > +		}
> 
> I think this is wrong.  If for whatever reason the process does not have
> the index it is about to tell "git commit" to use to make a commit in-core,
> it should first be doing read_cache().
I would have thought so too, and perhaps it should, but it doesn't seem to be
the case.  Initially I hadn't added this code, but during testing I received a
segfault indicating that the_index.cache_tree was NULL.  I thought it should
have been filled in via the calls to read_cache in do_recursive_merge or
fast_forward_to, but it didn't seem to be the case.  I added a read_cache call
right before my call to cache_tree_update, but still the_index.cache_tree
remained NULL.  Looking at the read_cache code, it appears that cache_tree is
only allocated if a CACHE_EXT_TREE is found when parsing the index.  Not sure
why I wouldn't have one, but certainly it didn't seem I did.  The above code
fixed it, which I lifted directly from write_tree_from_memory()


> > +
> > +		if (!cache_tree_fully_valid(active_cache_tree))
> > +			cache_tree_update(active_cache_tree, active_cache,
> > +					  active_nr, 0);
> 
> I do not recall offhand if cache_tree_update() can give you an error, but
> if it does, it should be checked here.
> 
It can, and I'll add checking there, but I hit the segfault above prior to
getting any error checknig done.

> > +		rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
> > +
> > +		if (need_free)
> > +			cache_tree_free(&active_cache_tree);
> 
> and I don't see a need for nuking the cache tree here, unless you are
> discarding the_index as well.
> 
Just balancing the allocation above with a free.  I didn't see a need for it
any longer, given that we were shortly going to call git commit and then exit
this process.  Is it safe to leave hanging around?

> > +		if (rc)
> > +			/*
> > + 			 * The head tree and the parent tree match
> > + 			 * meaning the commit is empty.  Since it wasn't created
> > + 			 * empty (based on the previous test), we can conclude
> > + 			 * the commit has been made redundant.  Since we don't
> > + 			 * want to keep redundant commits, just skip this one
> > + 			 */
> > +			return 0;
> > +	}
> > +
> > +	argv_array_init(&array);
> > +	argv_array_push(&array, "commit");
> > +	argv_array_push(&array, "-n");
> >  
> > -	args[i++] = "commit";
> > -	args[i++] = "-n";
> >  	if (opts->signoff)
> > -		args[i++] = "-s";
> > +		argv_array_push(&array, "-s");
> >  	if (!opts->edit) {
> > -		args[i++] = "-F";
> > -		args[i++] = defmsg;
> > +		argv_array_push(&array, "-F");
> > +		argv_array_push(&array, defmsg);
> >  	}
> > +	
> >  	if (opts->allow_empty)
> > -		args[i++] = "--allow-empty";
> > +		argv_array_push(&array, "--allow-empty");
> 
> If --keep-if-made-empty is not given but --allow-empty was, it is fine to
> give --allow-empty here, but otherwise, it logically is iffy and is likely
> to become a cause of future bugs to pass --allow-empty to "git commit",
> even though the earlier check _ought_ to catch problematic cases, no?
> 
Not sure I follow your reasoning here.  We need to pass allow-empty to git
commit here if git cherry-pick set either allow-empty or keep-redundant-commits
(the latter setting opts->empty), Can you give an example of what might be
problematic in the future?

Neil

Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option

[Junio C Hamano]

Neil Horman <nhorman@tuxdriver.com> writes:

>> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts)
>> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
>> >  {
>> > -	/* 7 is max possible length of our args array including NULL */
>> > -	const char *args[7];
>> > -	int i = 0;
>> > +	struct argv_array array;
>> > +	int rc;
>> > +
>> > +	if (!empty && !opts->keep_if_made_empty) {
>> > +		unsigned char head_sha1[20];
>> > +		struct commit *head_commit;
>> > +		int need_free = 0;
>> > +
>> > +		resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
>> > +		head_commit = lookup_commit(head_sha1);
>> 
>> No error checking whatsoever?  HEAD might be pointing at a branch that
>> hasn't been born, for example.
>> 
> Sorry, I had presumed that HEAD could be guaranteed to be good here, given that
> we validate it in rollback_single_pick via read_ref_full.  But I can add
> additional checking here.

As long as it is clear that we always have "HEAD" here, it is not strictly
necessary, but we'd probably feel better, especially now that the function
is (for some reason) made into a global one and may gain other new callers
outside this file scope.

>> >  	if (opts->allow_empty)
>> > -		args[i++] = "--allow-empty";
>> > +		argv_array_push(&array, "--allow-empty");
>> 
>> If --keep-if-made-empty is not given but --allow-empty was, it is fine to
>> give --allow-empty here, but otherwise, it logically is iffy and is likely
>> to become a cause of future bugs to pass --allow-empty to "git commit",
>> even though the earlier check _ought_ to catch problematic cases, no?
>> 
> Not sure I follow your reasoning here.  We need to pass allow-empty to git
> commit here if git cherry-pick set either allow-empty or keep-redundant-commits
> (the latter setting opts->empty), Can you give an example of what might be
> problematic in the future?

Thinking about it again, I think this part of your patch is correct.

We may pass an index that yields the same tree as "HEAD" if the original
was empty, or the original was not empty but the merge found the commit
unneeded.  In either case, if "--keep-unnecessary" or "--allow-empty" was
given, we want to record the empty commit, and opts->allow_empty is set
when either option was given from the command line.

Thanks.

Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option

[Neil Horman]

On Wed, Apr 11, 2012 at 08:15:15PM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> >> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts)
> >> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
> >> >  {
> >> > -	/* 7 is max possible length of our args array including NULL */
> >> > -	const char *args[7];
> >> > -	int i = 0;
> >> > +	struct argv_array array;
> >> > +	int rc;
> >> > +
> >> > +	if (!empty && !opts->keep_if_made_empty) {
> >> > +		unsigned char head_sha1[20];
> >> > +		struct commit *head_commit;
> >> > +		int need_free = 0;
> >> > +
> >> > +		resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
> >> > +		head_commit = lookup_commit(head_sha1);
> >> 
> >> No error checking whatsoever?  HEAD might be pointing at a branch that
> >> hasn't been born, for example.
> >> 
> > Sorry, I had presumed that HEAD could be guaranteed to be good here, given that
> > we validate it in rollback_single_pick via read_ref_full.  But I can add
> > additional checking here.
> 
> As long as it is clear that we always have "HEAD" here, it is not strictly
> necessary, but we'd probably feel better, especially now that the function
> is (for some reason) made into a global one and may gain other new callers
> outside this file scope.
> 
Gah!  Thanks for that, its not meant to be static.  I made that global so that
gdb could see it and set a breakpoint on it, while I was figuring out the
segfault I was getting when active_cache_tree was NULL.  I'll fix that.

> >> >  	if (opts->allow_empty)
> >> > -		args[i++] = "--allow-empty";
> >> > +		argv_array_push(&array, "--allow-empty");
> >> 
> >> If --keep-if-made-empty is not given but --allow-empty was, it is fine to
> >> give --allow-empty here, but otherwise, it logically is iffy and is likely
> >> to become a cause of future bugs to pass --allow-empty to "git commit",
> >> even though the earlier check _ought_ to catch problematic cases, no?
> >> 
> > Not sure I follow your reasoning here.  We need to pass allow-empty to git
> > commit here if git cherry-pick set either allow-empty or keep-redundant-commits
> > (the latter setting opts->empty), Can you give an example of what might be
> > problematic in the future?
> 
> Thinking about it again, I think this part of your patch is correct.
> 
> We may pass an index that yields the same tree as "HEAD" if the original
> was empty, or the original was not empty but the merge found the commit
> unneeded.  In either case, if "--keep-unnecessary" or "--allow-empty" was
> given, we want to record the empty commit, and opts->allow_empty is set
> when either option was given from the command line.
> 
Yes, exactly.
Thanks.

> Thanks.
> 

[PATCH v4 3/4] git-cherry-pick: Add test to validate new options

[Neil Horman]

Since we've added the --allow-empty and --keep-redundant-commits
options to git cherry-pick we should also add a test to ensure that its working
properly

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
---
 t/t3505-cherry-pick-empty.sh |   31 ++++++++++++++++++++++++++++++-
 1 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index c10b28c..9d419ae 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -18,7 +18,12 @@ test_expect_success setup '
 	echo third >> file1 &&
 	git add file1 &&
 	test_tick &&
-	git commit --allow-empty-message -m ""
+	git commit --allow-empty-message -m "" &&
+
+	git checkout master &&
+	git checkout -b empty-branch2 &&
+	test_tick &&
+	git commit --allow-empty -m "empty"
 
 '
 
@@ -48,4 +53,28 @@ test_expect_success 'index lockfile was removed' '
 
 '
 
+test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '
+	git checkout master &&
+	echo fourth >> file2 &&
+	git add file2 &&
+	git commit -m "fourth" && {
+		git cherry-pick empty-branch2
+		test "$?" = 1 
+	}
+'
+
+test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '
+	git checkout master && {
+		git cherry-pick --allow-empty empty-branch2
+		test "$?" = 0
+	}
+'
+
+test_expect_success 'cherry pick with --keep-redundant-commits' '
+	git checkout master && {
+		git cherry-pick --keep-redundant-commits HEAD^
+		test "$?" = 0
+	}
+'
+
 test_done
-- 
1.7.7.6

Re: [PATCH v4 3/4] git-cherry-pick: Add test to validate new options

[Junio C Hamano]

Neil Horman <nhorman@tuxdriver.com> writes:

> Since we've added the --allow-empty and --keep-redundant-commits
> options to git cherry-pick we should also add a test to ensure that its working
> properly

Missing full-stop at the end of the sentence

> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

> +test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '
> +	git checkout master &&
> +	echo fourth >> file2 &&
> +	git add file2 &&
> +	git commit -m "fourth" && {
> +		git cherry-pick empty-branch2
> +		test "$?" = 1 
> +	}
> +'

Do we _deeply_ care that it fails with status 1, or should this be test_must_fail?

> +test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '
> +	git checkout master && {
> +		git cherry-pick --allow-empty empty-branch2
> +		test "$?" = 0
> +	}
> +'

Lose the 'test "$?" = 0' here.

> +test_expect_success 'cherry pick with --keep-redundant-commits' '
> +	git checkout master && {
> +		git cherry-pick --keep-redundant-commits HEAD^
> +		test "$?" = 0

Likewise.

> +	}
> +'
> +
>  test_done

Thanks.

[PATCH v4 4/4] git-rebase: add keep_empty flag

[Neil Horman]

Add a command line switch to git-rebase to allow a user the ability to specify
that they want to keep any commits in a series that are empty.

When git-rebase's type is am, then this option will automatically keep any
commit that has a tree object identical to its parent.

This patch changes the default behavior of interactive rebases as well.  With
this patch, git-rebase -i will produce a revision set passed to
git-revision-editor, in which empty commits are commented out.  Empty commits
may be kept manually by uncommenting them.  If the new --keep-empty option is
used in an interactive rebase the empty commits will automatically all be
uncommented in the editor.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
---
 Documentation/git-rebase.txt |    4 ++++
 git-rebase--am.sh            |   19 ++++++++++++++-----
 git-rebase--interactive.sh   |   35 ++++++++++++++++++++++++++++++++---
 git-rebase.sh                |    5 +++++
 4 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 504945c..131c35d 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -238,6 +238,10 @@ leave out at most one of A and B, in which case it defaults to HEAD.
 	will be reset to where it was when the rebase operation was
 	started.
 
+--keep-empty::
+	Keep the commits that do not change anything from its
+	parents in the result.
+
 --skip::
 	Restart the rebasing process by skipping the current patch.
 
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index c815a24..040289c 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -20,11 +20,20 @@ esac
 
 test -n "$rebase_root" && root_flag=--root
 
-git format-patch -k --stdout --full-index --ignore-if-in-upstream \
-	--src-prefix=a/ --dst-prefix=b/ \
-	--no-renames $root_flag "$revisions" |
-git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" &&
-move_to_original_branch
+if test -n "$keep_empty" 
+then
+	# we have to do this the hard way.  git format-patch completely squashes
+	# empty commits and even if it didn't the format doesn't really lend
+	# itself well to recording empty patches.  fortunately, cherry-pick
+	# makes this easy
+	git cherry-pick --allow-empty "$revisions"
+else
+	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
+		--src-prefix=a/ --dst-prefix=b/ \
+		--no-renames $root_flag "$revisions" |
+	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg"
+fi && move_to_original_branch
+
 ret=$?
 test 0 != $ret -a -d "$state_dir" && write_basic_state
 exit $ret
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5812222..597d60a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -167,6 +167,12 @@ has_action () {
 	sane_grep '^[^#]' "$1" >/dev/null
 }
 
+is_empty_commit() {
+	ptree=$(git rev-parse "$1"^{tree})
+	pptree=$(git rev-parse "$1"^^{tree})
+	return $(test "$ptree" = "$pptree")
+}
+
 # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
 # GIT_AUTHOR_DATE exported from the current environment.
 do_with_author () {
@@ -191,12 +197,18 @@ git_sequence_editor () {
 
 pick_one () {
 	ff=--ff
+
+	if is_empty_commit $@ 
+	then
+		empty_args="--allow-empty"
+	fi
+
 	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
 	case "$force_rebase" in '') ;; ?*) ff= ;; esac
 	output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output git cherry-pick $ff "$@"
+	output git cherry-pick $empty_args $ff "$@"
 }
 
 pick_one_preserving_merges () {
@@ -780,9 +792,17 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
 	sed -n "s/^>//p" |
 while read -r shortsha1 rest
 do
+
+	if test -z "$keep_empty" && is_empty_commit $shortsha1
+	then
+		comment_out="# pick"
+	else
+		comment_out="pick"
+	fi
+
 	if test t != "$preserve_merges"
 	then
-		printf '%s\n' "pick $shortsha1 $rest" >> "$todo"
+		printf '%s\n' "$comment_out $shortsha1 $rest" >> "$todo"
 	else
 		sha1=$(git rev-parse $shortsha1)
 		if test -z "$rebase_root"
@@ -801,7 +821,7 @@ do
 		if test f = "$preserve"
 		then
 			touch "$rewritten"/$sha1
-			printf '%s\n' "pick $shortsha1 $rest" >> "$todo"
+			printf '%s\n' "$comment_out $shortsha1 $rest" >> "$todo"
 		fi
 	fi
 done
@@ -851,6 +871,15 @@ cat >> "$todo" << EOF
 #
 EOF
 
+if test -z "$keep_empty"
+then
+	cat >> "$todo" << EOF
+	# Note that commits which are empty at the time of rebasing are 
+	# commented out. 
+EOF
+fi
+
+
 has_action "$todo" ||
 	die_abort "Nothing to do"
 
diff --git a/git-rebase.sh b/git-rebase.sh
index 69c1374..24a2840 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -43,6 +43,7 @@ s,strategy=!       use the given merge strategy
 no-ff!             cherry-pick all commits, even if unchanged
 m,merge!           use merging strategies to rebase
 i,interactive!     let the user edit the list of commits to rebase
+k,keep-empty	   preserve empty commits during rebase
 f,force-rebase!    force rebase even if branch is up to date
 X,strategy-option=! pass the argument through to the merge strategy
 stat!              display a diffstat of what changed upstream
@@ -97,6 +98,7 @@ state_dir=
 action=
 preserve_merges=
 autosquash=
+keep_empty=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 
 read_basic_state () {
@@ -220,6 +222,9 @@ do
 	-i)
 		interactive_rebase=explicit
 		;;
+	-k)
+		keep_empty=yes
+		;;
 	-p)
 		preserve_merges=t
 		test -z "$interactive_rebase" && interactive_rebase=implied
-- 
1.7.7.6

Re: [PATCH v4 4/4] git-rebase: add keep_empty flag

[Junio C Hamano]

This looks quite nice, but it seems that the change breaks at least t3416
and t3404.  Has this series been tested?

Re: [PATCH v4 4/4] git-rebase: add keep_empty flag

[Neil Horman]

On Wed, Apr 11, 2012 at 02:08:54PM -0700, Junio C Hamano wrote:
> This looks quite nice, but it seems that the change breaks at least t3416
> and t3404.  Has this series been tested?
> 
I did not, mostly because those tests didn't seem to work with empty commits at
all.  And rebases (interactive and automatic) that I've done manually in testing
worked fine.  I'll check on them in the AM though.

Neil

Re: [PATCH v4 4/4] git-rebase: add keep_empty flag

[Junio C Hamano]

Neil Horman <nhorman@tuxdriver.com> writes:

> I did not, mostly because those tests didn't seem to work with empty commits at
> all.

Yeah, existing tests won't even try to rebase an empty or unnecessary
commit, so if the change breaks them, you found regressions.

Re: [PATCH v4 4/4] git-rebase: add keep_empty flag

[Neil Horman]

On Wed, Apr 11, 2012 at 02:08:54PM -0700, Junio C Hamano wrote:
> This looks quite nice, but it seems that the change breaks at least t3416
> and t3404.  Has this series been tested?
> 
I've looked at these, and I think it appears that this is the result of the
changes I made to interactive rebasing in which empty comments were commented
out, and additional notes were added to the commit editor template.  It seems to
be messing with what the fake editor for these tests is expecting.  I've got
some things I need to do tonight and tomorrow, but I'll fix this up over the
weekend.

Thanks!
Neil