Cherry-pick dangles and forgets helpful advice in next

Cherry-pick dangles and forgets helpful advice in next

[Phil Hord]

In git.git 'master' when I cherry-pick a commit which is eventually
empty, git gives me a friendly description of my supposed error, leaves
my cherry-pick "pending" and exits with an error code.


$ git cherry-pick a0aff2d                          
# On branch master
nothing to commit (working directory clean)
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'


In 'next' this is broken.  Now git does not tell me anything and it does
not exit with an error code.  It does still leave the cherry-pick
pending, though.


$ git cherry-pick a0aff2d
$ cat /tmp/repo/.git/CHERRY_PICK_HEAD
a0aff2da3d12a9a097c02e39570611f359433c23



It bisects to this commit:

commit b27cfb0d8d4cbb6d079c70ffeadac9c0dcfff250
Author: Neil Horman <nhorman@tuxdriver.com>
Date:   Fri Apr 20 10:36:15 2012 -0400

    git-cherry-pick: Add keep-redundant-commits option
   
    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.
   
    Note that this patch changes the default behavior of git cherry-pick
slightly.
    Prior to this patch all commits in a cherry-pick sequence were
applied and git
    commit was run.  The implication here was that, if a commit was
redundant, and
    the commit did not trigger the fast forward logic, the git commit
operation, and
    therefore the git cherry-pick operation would fail, displaying the
cherry pick
    advice (i.e. run git commit --allow-empty).  With this patch
however, such
    redundant commits are automatically skipped without stopping, unless
    --keep-redundant-commits is specified, in which case, they are
automatically
    applied as empty commits.
   
    Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>


I don't have time to chase it any further today.  Hopefully someone can
see the flub and straighten it out before I get a chance to look again. 
If not, I'll probably forget anyway.

Phil

Re: Cherry-pick dangles and forgets helpful advice in next

[Junio C Hamano]

Phil Hord <hordp@cisco.com> writes:

> In git.git 'master' when I cherry-pick a commit which is eventually
> empty, git gives me a friendly description of my supposed error, leaves
> my cherry-pick "pending" and exits with an error code.
>
>
> $ git cherry-pick a0aff2d                          
> # On branch master
> nothing to commit (working directory clean)
> The previous cherry-pick is now empty, possibly due to conflict resolution.
> If you wish to commit it anyway, use:
>
>     git commit --allow-empty
>
> Otherwise, please use 'git reset'
>
>
> In 'next' this is broken.  Now git does not tell me anything and it does
> not exit with an error code.  It does still leave the cherry-pick
> pending, though.
>
>
> $ git cherry-pick a0aff2d
> $ cat /tmp/repo/.git/CHERRY_PICK_HEAD
> a0aff2da3d12a9a097c02e39570611f359433c23
>
> It bisects to this commit:
>
> commit b27cfb0d8d4cbb6d079c70ffeadac9c0dcfff250
> Author: Neil Horman <nhorman@tuxdriver.com>
> Date:   Fri Apr 20 10:36:15 2012 -0400
>
>     git-cherry-pick: Add keep-redundant-commits option
> ...
> I don't have time to chase it any further today.  Hopefully someone can
> see the flub and straighten it out before I get a chance to look again. 
> If not, I'll probably forget anyway.

Given that I am currently very deep in today's integration round, I
wouldn't have a chance to look into this.  I am OK with reverting the
whole topic (and I am also very much tempted to do so), as nobody other
than Neil seemed to have wanted this "feature". and even though it is
"optional", I doubt it encourages a good workflow.

Let's see if somebody comes up with a fix quickly enough before the -rc1
is tagged.

Thanks for a report.

Re: Cherry-pick dangles and forgets helpful advice in next

[Junio C Hamano]

Phil Hord <hordp@cisco.com> writes:

> In git.git 'master' when I cherry-pick a commit which is eventually
> empty, git gives me a friendly description of my supposed error, leaves
> my cherry-pick "pending" and exits with an error code.
>
>
> $ git cherry-pick a0aff2d                          
> # On branch master
> nothing to commit (working directory clean)
> The previous cherry-pick is now empty, possibly due to conflict resolution.
> If you wish to commit it anyway, use:
>
>     git commit --allow-empty
>
> Otherwise, please use 'git reset'
>
> In 'next' this is broken.  Now git does not tell me anything and it does
> not exit with an error code.

There probably is something else that is broken in _your_ build.  The test
t3505.2 is about failing an attempt to cherry-pick an empty commit:

        test_expect_success 'cherry-pick an empty commit' '
                git checkout master && {
                        git cherry-pick empty-branch^
                        test "$?" = 1
                }
        '

If I insert an "exit" immediately after this test and run the test with
"-i -v" option, it ends like this:

    $ make && cd t && sh t3505-cherry-pick-empty.sh -i -v
    ...
    ok 1 - setup

    expecting success:
            git checkout master && {
                    git cherry-pick empty-branch^
                    test "$?" = 1
            }

    Switched to branch 'master'
    Already up-to-date!
    # On branch master
    nothing to commit (working directory clean)
    The previous cherry-pick is now empty, possibly due to conflict
    resolution.
    If you wish to commit it anyway, use:

        git commit --allow-empty

    Otherwise, please use 'git reset'
    ok 2 - cherry-pick an empty commit

    FATAL: Unexpected exit with code 0

It does fail with non-zero exit code (it might be better to test with
test_expect_failure, but that is a minor point here), and we see the error
message.

Re: Cherry-pick dangles and forgets helpful advice in next

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Phil Hord <hordp@cisco.com> writes:
>
>> In git.git 'master' when I cherry-pick a commit which is eventually
>> empty, git gives me a friendly description of my supposed error, leaves
>> my cherry-pick "pending" and exits with an error code.
>>
>>
>> $ git cherry-pick a0aff2d                          
>> # On branch master
>> nothing to commit (working directory clean)
>> The previous cherry-pick is now empty, possibly due to conflict resolution.
>> If you wish to commit it anyway, use:
>>
>>     git commit --allow-empty
>>
>> Otherwise, please use 'git reset'
>>
>> In 'next' this is broken.  Now git does not tell me anything and it does
>> not exit with an error code.
>
> ...  The test
> t3505.2 is about failing an attempt to cherry-pick an empty commit:

Ahh, disregard that one.  It is not testing the case where a cherry-pick
results in empty.

Re: Cherry-pick dangles and forgets helpful advice in next

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Phil Hord <hordp@cisco.com> writes:
>>
>>> In git.git 'master' when I cherry-pick a commit which is eventually
>>> empty, git gives me a friendly description of my supposed error, leaves
>>> my cherry-pick "pending" and exits with an error code.
>>> ...
>>> In 'next' this is broken.

The above was written back when nh/empty-rebase topic was not in 'master'
yet.

The attached will help reproducing it.

 t/t3505-cherry-pick-empty.sh | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index 92f00cd..bd06981 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -23,8 +23,19 @@ test_expect_success setup '
 	git checkout master &&
 	git checkout -b empty-branch2 &&
 	test_tick &&
-	git commit --allow-empty -m "empty"
+	git commit --allow-empty -m "empty" &&
+
+	git checkout master &&
+	echo second >>file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit -m "second" &&
 
+	git checkout -b merge-to-empty master^ &&
+	echo second >>file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit -m "will merge to empty"
 '
 
 test_expect_success 'cherry-pick an empty commit' '
@@ -40,7 +51,13 @@ test_expect_success 'index lockfile was removed' '
 
 '
 
+test_expect_failure 'cherry-pick to result in empty' '
+	git checkout master^0 &&
+	test_must_fail git cherry-pick merge-to-empty
+'
+
 test_expect_success 'cherry-pick a commit with an empty message' '
+	git reset --hard &&
 	git checkout master && {
 		git cherry-pick empty-branch
 		test "$?" = 1

Re: Cherry-pick dangles and forgets helpful advice in next

[Phil Hord]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Phil Hord <hordp@cisco.com> writes:
>>
>>> In git.git 'master' when I cherry-pick a commit which is eventually
>>> empty, git gives me a friendly description of my supposed error, leaves
>>> my cherry-pick "pending" and exits with an error code.
>>>
>>>
>>> $ git cherry-pick a0aff2d                          
>>> # On branch master
>>> nothing to commit (working directory clean)
>>> The previous cherry-pick is now empty, possibly due to conflict resolution.
>>> If you wish to commit it anyway, use:
>>>
>>>     git commit --allow-empty
>>>
>>> Otherwise, please use 'git reset'
>>>
>>> In 'next' this is broken.  Now git does not tell me anything and it does
>>> not exit with an error code.
>> ...  The test
>> t3505.2 is about failing an attempt to cherry-pick an empty commit:
> Ahh, disregard that one.  It is not testing the case where a cherry-pick
> results in empty.
>

Yes, but the last one checks for something similar.  However, I tried
this and it does not trigger the same failure.

$ git cherry-pick HEAD^                              
# On branch master
nothing to commit (working directory clean)
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'



So I think I need to be more specific about the error condition I am seeing.

mkdir repo && cd repo && git init &&
touch foo && git add foo && git commit -mfoo &&
echo foo > foo && git add foo && git commit -mnewfoo && git branch newfoo &&
git commit --amend -m"new foo" &&
git cherry-pick newfoo

This sequence fails to report an error as of this commit:

commit b27cfb0d8d4cbb6d079c70ffeadac9c0dcfff250
Author: Neil Horman <nhorman@tuxdriver.com>
Date:   Fri Apr 20 10:36:15 2012 -0400

    git-cherry-pick: Add keep-redundant-commits option

But it does report the problem and 'exit 1' prior to that.

Phil

Re: Cherry-pick dangles and forgets helpful advice in next

[Junio C Hamano]

Phil Hord <hordp@cisco.com> writes:

> This sequence fails to report an error as of this commit:
>
> commit b27cfb0d8d4cbb6d079c70ffeadac9c0dcfff250
> Author: Neil Horman <nhorman@tuxdriver.com>
> Date:   Fri Apr 20 10:36:15 2012 -0400
>
>     git-cherry-pick: Add keep-redundant-commits option

Yes, because it deliberately breaks sequencer.c::do_pick_commit() in this
particular case:

	if (!empty_commit && !opts->keep_redundant_commits && index_unchanged)
		/*
		 * The head tree and the index 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, we can just return
		 * here, skipping this commit
		 */
		return 0;

which is not quite right.  We do not want to keep redundant commits, so
this needs to error out as before.

I will queue the following in the meantime.

-- >8 --
Subject: [PATCH] Revert nh/empty-rebase topic

The topic breaks "git cherry-pick $commit" that results in no change
in the tree by not erroring out, which is a grave regression.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-cherry-pick.txt |  19 -------
 Documentation/git-rebase.txt      |   4 --
 builtin/revert.c                  |   8 ---
 git-rebase--am.sh                 |  19 ++-----
 git-rebase--interactive.sh        |  35 ++-----------
 git-rebase.sh                     |   5 --
 sequencer.c                       | 103 ++++----------------------------------
 sequencer.h                       |   2 -
 t/t3505-cherry-pick-empty.sh      |  25 +--------
 9 files changed, 19 insertions(+), 201 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 9f3dae6..06a0bfd 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -103,25 +103,6 @@ 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.  Note also, that use of this option only
-	keeps commits that were initially empty (i.e. the commit recorded the
-	same tree as its parent).  Commits which are made empty due to a
-	previous commit are dropped.  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 become empty.  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.
 	See the MERGE STRATEGIES section in linkgit:git-merge[1]
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 147fa1a..b0e13e5 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -238,10 +238,6 @@ 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/builtin/revert.c b/builtin/revert.c
index 82d1bf8..5462e67 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -115,16 +115,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		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 initially empty commits"),
-			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_redundant_commits, "keep redundant, empty commits"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
@@ -142,10 +138,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"--abort", rollback,
 				NULL);
 
-	/* implies allow_empty */
-	if (opts->keep_redundant_commits)
-		opts->allow_empty = 1;
-
 	/* Set the subcommand */
 	if (remove_state)
 		opts->subcommand = REPLAY_REMOVE_STATE;
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 04d8941..c815a24 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -20,20 +20,11 @@ esac
 
 test -n "$rebase_root" && root_flag=--root
 
-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
-
+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
 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 0c19b7c..2e13258 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -167,14 +167,6 @@ has_action () {
 	sane_grep '^[^#]' "$1" >/dev/null
 }
 
-is_empty_commit() {
-	tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
-		die "$1: not a commit that can be picked")
-	ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
-		ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
-	test "$tree" = "$ptree"
-}
-
 # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
 # GIT_AUTHOR_DATE exported from the current environment.
 do_with_author () {
@@ -199,19 +191,12 @@ git_sequence_editor () {
 
 pick_one () {
 	ff=--ff
-
 	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"
-
-	if is_empty_commit "$sha1"
-	then
-		empty_args="--allow-empty"
-	fi
-
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output git cherry-pick $empty_args $ff "$@"
+	output git cherry-pick $ff "$@"
 }
 
 pick_one_preserving_merges () {
@@ -795,17 +780,9 @@ 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="# "
-	else
-		comment_out=
-	fi
-
 	if test t != "$preserve_merges"
 	then
-		printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo"
+		printf '%s\n' "pick $shortsha1 $rest" >> "$todo"
 	else
 		sha1=$(git rev-parse $shortsha1)
 		if test -z "$rebase_root"
@@ -824,7 +801,7 @@ do
 		if test f = "$preserve"
 		then
 			touch "$rewritten"/$sha1
-			printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo"
+			printf '%s\n' "pick $shortsha1 $rest" >> "$todo"
 		fi
 	fi
 done
@@ -876,12 +853,6 @@ cat >> "$todo" << EOF
 #
 EOF
 
-if test -z "$keep_empty"
-then
-	echo "# Note that empty commits are commented out" >>"$todo"
-fi
-
-
 has_action "$todo" ||
 	die_abort "Nothing to do"
 
diff --git a/git-rebase.sh b/git-rebase.sh
index 24a2840..69c1374 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -43,7 +43,6 @@ 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
@@ -98,7 +97,6 @@ state_dir=
 action=
 preserve_merges=
 autosquash=
-keep_empty=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 
 read_basic_state () {
@@ -222,9 +220,6 @@ do
 	-i)
 		interactive_rebase=explicit
 		;;
-	-k)
-		keep_empty=yes
-		;;
 	-p)
 		preserve_merges=t
 		test -z "$interactive_rebase" && interactive_rebase=implied
diff --git a/sequencer.c b/sequencer.c
index 3c384b9..81d8ace 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -13,7 +13,6 @@
 #include "rerere.h"
 #include "merge-recursive.h"
 #include "refs.h"
-#include "argv-array.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -252,38 +251,6 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	return !clean;
 }
 
-static int is_index_unchanged(void)
-{
-	unsigned char head_sha1[20];
-	struct commit *head_commit;
-
-	if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
-		return error(_("Could not resolve HEAD commit\n"));
-
-	head_commit = lookup_commit(head_sha1);
-
-	/*
-	 * If head_commit is NULL, check_commit, called from
-	 * lookup_commit, would have indicated that head_commit is not
-	 * a commit object already.  parse_commit() will return failure
-	 * without further complaints in such a case.  Otherwise, if
-	 * the commit is invalid, parse_commit() will complain.  So
-	 * there is nothing for us to say here.  Just return failure.
-	 */
-	if (parse_commit(head_commit))
-		return -1;
-
-	if (!active_cache_tree)
-		active_cache_tree = cache_tree();
-
-	if (!cache_tree_fully_valid(active_cache_tree))
-		if (cache_tree_update(active_cache_tree, active_cache,
-				  active_nr, 0))
-			return error(_("Unable to update cache tree\n"));
-
-	return !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
-}
-
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -293,46 +260,21 @@ static int is_index_unchanged(void)
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 {
-	struct argv_array array;
-	int rc;
-
-	argv_array_init(&array);
-	argv_array_push(&array, "commit");
-	argv_array_push(&array, "-n");
+	/* 6 is max possible length of our args array including NULL */
+	const char *args[6];
+	int i = 0;
 
+	args[i++] = "commit";
+	args[i++] = "-n";
 	if (opts->signoff)
-		argv_array_push(&array, "-s");
+		args[i++] = "-s";
 	if (!opts->edit) {
-		argv_array_push(&array, "-F");
-		argv_array_push(&array, defmsg);
+		args[i++] = "-F";
+		args[i++] = defmsg;
 	}
+	args[i] = NULL;
 
-	if (opts->allow_empty)
-		argv_array_push(&array, "--allow-empty");
-
-	rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
-	argv_array_clear(&array);
-	return rc;
-}
-
-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 !hashcmp(ptree_sha1, commit->tree->object.sha1);
+	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
 static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
@@ -344,8 +286,6 @@ 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;
-	int index_unchanged;
 
 	if (opts->no_commit) {
 		/*
@@ -471,10 +411,6 @@ 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.
@@ -495,25 +431,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		print_advice(res == 1, opts);
 		rerere(opts->allow_rerere_auto);
 	} else {
-		index_unchanged = is_index_unchanged();
-		/*
-		 * If index_unchanged is less than 0, that indicates we either
-		 * couldn't parse HEAD or the index, so error out here.
-		 */
-		if (index_unchanged < 0)
-			return index_unchanged;
-
-		if (!empty_commit && !opts->keep_redundant_commits && index_unchanged)
-			/*
-			 * The head tree and the index 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, we can just return
-			 * here, skipping this commit
-			 */
-			return 0;
-
 		if (!opts->no_commit)
 			res = run_git_commit(defmsg, opts);
 	}
diff --git a/sequencer.h b/sequencer.h
index aa5f17c..bb4b138 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -29,8 +29,6 @@ struct replay_opts {
 	int signoff;
 	int allow_ff;
 	int allow_rerere_auto;
-	int allow_empty;
-	int keep_redundant_commits;
 
 	int mainline;
 
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index 92f00cd..c10b28c 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -18,12 +18,7 @@ test_expect_success setup '
 	echo third >> file1 &&
 	git add file1 &&
 	test_tick &&
-	git commit --allow-empty-message -m "" &&
-
-	git checkout master &&
-	git checkout -b empty-branch2 &&
-	test_tick &&
-	git commit --allow-empty -m "empty"
+	git commit --allow-empty-message -m ""
 
 '
 
@@ -53,22 +48,4 @@ 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" &&
-	test_must_fail git cherry-pick empty-branch2
-'
-
-test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '
-	git checkout master &&
-	git cherry-pick --allow-empty empty-branch2
-'
-
-test_expect_success 'cherry pick with --keep-redundant-commits' '
-	git checkout master &&
-	git cherry-pick --keep-redundant-commits HEAD^
-'
-
 test_done
-- 
1.7.10.2.627.g7c93d77

Re: Cherry-pick dangles and forgets helpful advice in next

[Junio C Hamano]

Instead of reverting the entire thing, perhaps we can fix the
regression like this.

With this, we no longer unconditionally give "--allow-empty" when we
run "git commit", when --allow-empty (which is only about commits
that are originally empty) is given to cherry-pick; specifically,
when the user did not ask for --keep-redundant-commit, we do not
give "--allow-empty" if the original commit is not.

Thinking about it again, I _think_ we do not even have to check if
the result is an empty commit ourselves ("git commit" will do that
for us anyway), so we might want to rip "is_empty_commit()" out of
the problematic patch and keep only "is_index_unmodified()" bit, but
for now I think this may be good enough.

Phil, does it fix your issue?

Neil?

-- >8 --

Subject: [PATCH] cherry-pick: regression fix for empty commits

The earlier "--keep-redundant-commit" series broke "cherry-pick"
that is given a commit whose change is already in the current
history. Such a cherry-pick would result in an empty change, and
should stop with an error, telling the user that conflict resolution
may have made the result empty (which is exactly what is happening),
but we silently dropped the change on the floor without any message
nor non-zero exit code.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c                  | 73 +++++++++++++++++++++++++++-----------------
 t/t3505-cherry-pick-empty.sh | 30 ++++++++++++++++++
 2 files changed, 75 insertions(+), 28 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 72cb4ff..1b2168c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -291,7 +291,8 @@ static int is_index_unchanged(void)
  * 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)
+static int run_git_commit(const char *defmsg, struct replay_opts *opts,
+			  int allow_empty)
 {
 	struct argv_array array;
 	int rc;
@@ -307,7 +308,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 		argv_array_push(&array, defmsg);
 	}
 
-	if (opts->allow_empty)
+	if (allow_empty)
 		argv_array_push(&array, "--allow-empty");
 
 	rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
@@ -335,6 +336,44 @@ static int is_original_commit_empty(struct commit *commit)
 	return !hashcmp(ptree_sha1, commit->tree->object.sha1);
 }
 
+/*
+ * Do we run "git commit" with "--allow-empty"?
+ */
+static int allow_empty(struct replay_opts *opts, struct commit *commit)
+{
+	int index_unchanged, empty_commit;
+
+	/*
+	 * Three cases:
+	 *
+	 * (1) we do not allow empty at all and error out.
+	 *
+	 * (2) we allow ones that were initially empty, but
+	 * forbid the ones that become empty;
+	 *
+	 * (3) we allow both.
+	 */
+	if (!opts->allow_empty)
+		return 0; /* let "git commit" barf as necessary */
+
+	index_unchanged = is_index_unchanged();
+	if (index_unchanged < 0)
+		return index_unchanged;
+	if (!index_unchanged)
+		return 0; /* we do not have to say --allow-empty */
+
+	if (opts->keep_redundant_commits)
+		return 1;
+
+	empty_commit = is_original_commit_empty(commit);
+	if (empty_commit < 0)
+		return empty_commit;
+	if (!empty_commit)
+		return 0;
+	else
+		return 1;
+}
+
 static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 {
 	unsigned char head[20];
@@ -344,8 +383,6 @@ 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;
-	int index_unchanged;
 
 	if (opts->no_commit) {
 		/*
@@ -471,10 +508,6 @@ 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.
@@ -495,27 +528,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		print_advice(res == 1, opts);
 		rerere(opts->allow_rerere_auto);
 	} else {
-		index_unchanged = is_index_unchanged();
-		/*
-		 * If index_unchanged is less than 0, that indicates we either
-		 * couldn't parse HEAD or the index, so error out here.
-		 */
-		if (index_unchanged < 0)
-			return index_unchanged;
-
-		if (!empty_commit && !opts->keep_redundant_commits && index_unchanged)
-			/*
-			 * The head tree and the index 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, we can just return
-			 * here, skipping this commit
-			 */
-			return 0;
-
+		int allow = allow_empty(opts, commit);
+		if (allow < 0)
+			return allow;
 		if (!opts->no_commit)
-			res = run_git_commit(defmsg, opts);
+			res = run_git_commit(defmsg, opts, allow);
 	}
 
 	free_message(&msg);
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index 92f00cd..5a1340c 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -71,4 +71,34 @@ test_expect_success 'cherry pick with --keep-redundant-commits' '
 	git cherry-pick --keep-redundant-commits HEAD^
 '
 
+test_expect_success 'cherry-pick a commit that becomes no-op (prep)' '
+	git checkout master &&
+	git branch fork &&
+	echo foo >file2 &&
+	git add file2 &&
+	test_tick &&
+	git commit -m "add file2 on master" &&
+
+	git checkout fork &&
+	echo foo >file2 &&
+	git add file2 &&
+	test_tick &&
+	git commit -m "add file2 on the side"
+'
+
+test_expect_success 'cherry-pick a no-op without --keep-redundant' '
+	git reset --hard &&
+	git checkout fork^0 &&
+	test_must_fail git cherry-pick master
+'
+
+test_expect_success 'cherry-pick a no-op with --keep-redundant' '
+	git reset --hard &&
+	git checkout fork^0 &&
+	git cherry-pick --keep-redundant-commits master &&
+	git show -s --format='%s' >actual &&
+	echo "add file2 on master" >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.11.rc0.54.g0680f74

Re: Cherry-pick dangles and forgets helpful advice in next

[Phil Hord]

Junio C Hamano wrote:

> Instead of reverting the entire thing, perhaps we can fix the
> regression like this.
>
> With this, we no longer unconditionally give "--allow-empty" when we
> run "git commit", when --allow-empty (which is only about commits
> that are originally empty) is given to cherry-pick; specifically,
> when the user did not ask for --keep-redundant-commit, we do not
> give "--allow-empty" if the original commit is not.
>
> Thinking about it again, I _think_ we do not even have to check if
> the result is an empty commit ourselves ("git commit" will do that
> for us anyway), so we might want to rip "is_empty_commit()" out of
> the problematic patch and keep only "is_index_unmodified()" bit, but
> for now I think this may be good enough.
>
> Phil, does it fix your issue?
Yes, it appears to fix my issue.  I don't have the original condition in
play anymore, but it fixes the test case I cooked up earlier.

Thanks,
Phil

Re: Cherry-pick dangles and forgets helpful advice in next

[Junio C Hamano]

Phil Hord <hordp@cisco.com> writes:

> Junio C Hamano wrote:
>
>> Instead of reverting the entire thing, perhaps we can fix the
>> regression like this.
>>
>> With this, we no longer unconditionally give "--allow-empty" when we
>> run "git commit", when --allow-empty (which is only about commits
>> that are originally empty) is given to cherry-pick; specifically,
>> when the user did not ask for --keep-redundant-commit, we do not
>> give "--allow-empty" if the original commit is not.
>>
>> Thinking about it again, I _think_ we do not even have to check if
>> the result is an empty commit ourselves ("git commit" will do that
>> for us anyway), so we might want to rip "is_empty_commit()" out of
>> the problematic patch and keep only "is_index_unmodified()" bit, but
>> for now I think this may be good enough.
>>
>> Phil, does it fix your issue?
>
> Yes, it appears to fix my issue.  I don't have the original condition in
> play anymore, but it fixes the test case I cooked up earlier.

OK, I'm planning to merge the fix down before 1.7.11 final. It may
have broken the use case Neil wanted to support as a side effect (I
tried to be careful but I did not do anything beyond the test cases
as I do not deliberately add empty commits to my history); Neil may
want to double check the result.

Thanks.