patch, 4 partsEnhance git-rebases flexibiilty in handling empty commits

> +--keep-empty:
> +	If a commit is not a fast forward, or if fast forwarding is not allowed,
> +	cherry-picking an empty commit will fail, indicating that an explicit
> +	invokation of git commit --allow-empty is required.  This option
> +	overrides that behavior, allowing empty commits to be preserved
> +	automatically in a cherry-pick

> 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;

> 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 operaion.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jeff King <peff@peff.net>
> CC: Phil Hord <phil.hord@gmail.com>
> CC: Junio C Hamano <gitster@pobox.com>

> 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.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jeff King <peff@peff.net>
> CC: Phil Hord <phil.hord@gmail.com>
> CC: Junio C Hamano <gitster@pobox.com>

>  Documentation/git-rebase.txt |    6 ++++++
>  git-rebase.sh                |    5 +++++
>  2 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 504945c..9717d3e 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -238,6 +238,12 @@ 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::
> +	Informs git-rebase that comits which are empty should not be
> +	automatically removed.  This is at times useful when empty commits
> +	are used to hold developer information and notes, but contain no real
> +	code changes
> +

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > 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.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Jeff King <peff@peff.net>
> > CC: Phil Hord <phil.hord@gmail.com>
> > CC: Junio C Hamano <gitster@pobox.com>
> 
> The same comments on Cc: apply to all of your patches.
> 

> >  Documentation/git-rebase.txt |    6 ++++++
> >  git-rebase.sh                |    5 +++++
> >  2 files changed, 11 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> > index 504945c..9717d3e 100644
> > --- a/Documentation/git-rebase.txt
> > +++ b/Documentation/git-rebase.txt
> > @@ -238,6 +238,12 @@ 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::
> > +	Informs git-rebase that comits which are empty should not be
> > +	automatically removed.  This is at times useful when empty commits
> > +	are used to hold developer information and notes, but contain no real
> > +	code changes
> > +
> 
> Unlike "cherry-pick", I think "--keep-empty" is a better name for the
> option than "--allow-empty" in this context.  The difference is that from
> the end-user's point of view, cherry-pick _replays_ commits that exist
> elsewhere, and you are allowing the command to replay empty ones as well,
> while rebase _rebuilds_ commits on the same branch, and you are telling
> the command to keep empty ones.
> 
> "... which are empty should not be removed" is a bit of double-negation,
> though.  Perhaps
> 
> 	--keep-empty::
> 		Keep the commits that do not change anything from its
> 		parents in the result.  This is at times useful when empty
> 		commits are used to hold developer information and notes
> 		without having any real changes.
> 
> But as I rephrased the first part, the last line may have become redundant
> and could safely be removed.
> 

> apply empty commits to the new branch we are rebasing to
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jeff King <peff@peff.net>
> CC: Phil Hord <phil.hord@gmail.com>
> CC: Junio C Hamano <gitster@pobox.com>
> ---
>  git-rebase--am.sh |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index c815a24..c1d1b60 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -20,11 +20,21 @@ 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 [ -n "$keep_empty" ]
> +then
> +	# we have to do this the hard way.  git format-patch completly 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 --keep-empty "$revisions" && move_to_original_branch
> +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" &&
> +	move_to_original_branch
> +fi

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > Using the keep_empy environment variable, this change allows git-commit-am to
> 
> Is it an environment variable?  I thought not.
> 

> > +	# makes this easy
> > +	git cherry-pick --keep-empty "$revisions" && move_to_original_branch
> > +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" &&
> > +	move_to_original_branch
> > +fi
> 
> Factor out the "&& move_to_original_branch" at the end of then/else, like
> this:
> 
> 	if ...
>         then
> 		...
>         else
> 		...
>         fi && move_to_original_branch
> 

> Using the keep_empy environment variable, this change allows git-commit-am to
> apply empty commits to the new branch we are rebasing to
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jeff King <peff@peff.net>
> CC: Phil Hord <phil.hord@gmail.com>
> CC: Junio C Hamano <gitster@pobox.com>
> ---
>  git-rebase--am.sh |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index c815a24..c1d1b60 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -20,11 +20,21 @@ 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 [ -n "$keep_empty" ]
> +then
> +	# we have to do this the hard way.  git format-patch completly 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 --keep-empty "$revisions" && move_to_original_branch

> +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" &&
> +	move_to_original_branch
> +fi
> +
>  ret=$?
>  test 0 != $ret -a -d "$state_dir" && write_basic_state
>  exit $ret

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > Using the keep_empy environment variable, this change allows git-commit-am to
> > apply empty commits to the new branch we are rebasing to
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Jeff King <peff@peff.net>
> > CC: Phil Hord <phil.hord@gmail.com>
> > CC: Junio C Hamano <gitster@pobox.com>
> > ---
> >  git-rebase--am.sh |   20 +++++++++++++++-----
> >  1 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> > index c815a24..c1d1b60 100644
> > --- a/git-rebase--am.sh
> > +++ b/git-rebase--am.sh
> > @@ -20,11 +20,21 @@ 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 [ -n "$keep_empty" ]
> > +then
> > +	# we have to do this the hard way.  git format-patch completly 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 --keep-empty "$revisions" && move_to_original_branch
> 
> Does cherry-pick know the "--ignore-if-in-upstream" trick?  Otherwise I
> suspect that this will introduce a severe regression to the command, as
> the commits that are already in the new base you are rebasing to will all
> be kept as empty commits, no?
> 

> This updates git-commit-interactive to recognize and make use of the keep_empty
> flag.  When not set, git-rebase -i will now comment out commits that are empty,
> and informs the user that commits which they wish to explicitly keep that are
> empty should be uncommented, or --keep-empty should be specified.  if keep_empty
> is specified, all commits, regardless of their empty status are included.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jeff King <peff@peff.net>
> CC: Phil Hord <phil.hord@gmail.com>
> CC: Junio C Hamano <gitster@pobox.com>
> ---
>  git-rebase--interactive.sh |   38 +++++++++++++++++++++++++++++++++++---
>  1 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 5812222..97eeb21 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -191,12 +191,24 @@ git_sequence_editor () {
>  
>  pick_one () {
>  	ff=--ff
> +	is_empty=$(git show --pretty=format:%b "$@" | wc -l)

> +	then
> +		empty_args=--keep-empty
> +	fi
> +
> +	if [ -n "$keep_empty" ]
> +	then
> +		empty_args=--keep_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,24 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
>  	sed -n "s/^>//p" |
>  while read -r shortsha1 rest
>  do
> +	local comment_out

> +		else
> +			comment_out="pick"
> +		fi
> +	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"

> @@ -849,6 +876,11 @@ cat >> "$todo" << EOF
>  # If you remove a line here THAT COMMIT WILL BE LOST.
>  # However, if you remove everything, the rebase will be aborted.
>  #
> +# Note that commits which are empty at the time of rebasing are 
> +# commented out.  If you wish to keep empty commits, either 
> +# specify the --keep-empty option to the rebase command, or 
> +# uncomment the commits you wish to keep
> +#

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > This updates git-commit-interactive to recognize and make use of the keep_empty
> > flag.  When not set, git-rebase -i will now comment out commits that are empty,
> > and informs the user that commits which they wish to explicitly keep that are
> > empty should be uncommented, or --keep-empty should be specified.  if keep_empty
> > is specified, all commits, regardless of their empty status are included.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Jeff King <peff@peff.net>
> > CC: Phil Hord <phil.hord@gmail.com>
> > CC: Junio C Hamano <gitster@pobox.com>
> > ---
> >  git-rebase--interactive.sh |   38 +++++++++++++++++++++++++++++++++++---
> >  1 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 5812222..97eeb21 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -191,12 +191,24 @@ git_sequence_editor () {
> >  
> >  pick_one () {
> >  	ff=--ff
> > +	is_empty=$(git show --pretty=format:%b "$@" | wc -l)
> 
> That is a very expensive way to see if the commit is empty, no?
> 
> If and only if commit C is empty, "git rev-parse" on C^{tree} and
> C^^{tree}" will yield the same tree object name.
> 

> > +	if [ $is_empty -eq 0 ]
> 
> Also this test (which by the way is against our coding style guideline)
> shows that the variable is misnamed.
> 

> > +	then
> > +		empty_args=--keep-empty
> > +	fi
> > +
> > +	if [ -n "$keep_empty" ]
> > +	then
> > +		empty_args=--keep_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,24 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
> >  	sed -n "s/^>//p" |
> >  while read -r shortsha1 rest
> >  do
> > +	local comment_out
> 
> bashism.
> 

> > +
> > +	if [ -z "$keep_empty" ]
> > +	then
> > +		comment_out=$(git show --pretty=format:%b $shortsha1 | wc -l)
> 
> Ditto.
> 
> > +		if [ $comment_out -eq 0 ]
> > +		then
> > +			comment_out="#pick"
> 
> Perhaps it is easier to read if you say "# pick"?
> 

> > +		else
> > +			comment_out="pick"
> > +		fi
> > +	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"
> 
> The variable comment_out is grossly misnamed.  Why not do it this way?
> 

> 	comment_out=
> 	if test -z "$keep_empty" && is_empty_commit $shortsha1
>         then
>         	comment_out="# "
> 	fi
> 
>         if ...
>         then
> 		printf "%s\n" "${leader}pick $shortsha1 $rest" >>"$todo"
> 
> > @@ -849,6 +876,11 @@ cat >> "$todo" << EOF
> >  # If you remove a line here THAT COMMIT WILL BE LOST.
> >  # However, if you remove everything, the rebase will be aborted.
> >  #
> > +# Note that commits which are empty at the time of rebasing are 
> > +# commented out.  If you wish to keep empty commits, either 
> > +# specify the --keep-empty option to the rebase command, or 
> > +# uncomment the commits you wish to keep
> > +#
> 
> Good.
> 
> I do not think " either specify...rebase command, or" is necessary here,
> though.  This message is meant to be a quick reminder, not a tutorial.
> Keep it short and sweet.
> 

> for interactive rebases, I changed the default selection editor text somewhat.
> By default, empty commits are allowed in this list.  With patch 4 here, empty
> commits are commented out automatically, unless --keep-empty is selected (in
> which case all commits are pick-ed).  The user sees additional text indicating
> that empty commits are commented and if they wish to be kept, then they must be
> uncommented.  The pick_one function then intellegently passes the --keep-empty
> option allong to cherry-pick as needed.

> As a convienience, it would be nice if we could pop entries off the argv_array
> structs so that if they had multiple uses in a function, we wouldn't have to
> clear them and repopulate common entries.  This patch adds the argv_array_pop
> function to do just that.  Common entries can be added to an argv_array first,
> then useage specific ones can be added on the end and removed later on.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

> diff --git a/argv-array.c b/argv-array.c
> index a4e0420..ce24a48 100644
> --- a/argv-array.c
> +++ b/argv-array.c
> @@ -39,6 +39,18 @@ void argv_array_pushf(struct argv_array *array, const char *fmt, ...)
>  	argv_array_push_nodup(array, strbuf_detach(&v, NULL));
>  }
>  
> +int argv_array_pop(struct argv_array *array, unsigned int num)
> +{
> +	if (num > array->argc)
> +		return -1;

> +		free((char **)array->argv[num]);
> +		array->argv[num] = NULL;
> +	}
> +	return 0;
> +}
> +
>  void argv_array_clear(struct argv_array *array)
>  {
>  	if (array->argv != empty_argv) {
> diff --git a/argv-array.h b/argv-array.h
> index 74dd2b1..8233243 100644
> --- a/argv-array.h
> +++ b/argv-array.h
> @@ -15,6 +15,7 @@ void argv_array_init(struct argv_array *);
>  void argv_array_push(struct argv_array *, const char *);
>  __attribute__((format (printf,2,3)))
>  void argv_array_pushf(struct argv_array *, const char *fmt, ...);
> +int argv_array_pop(struct argv_array *, unsigned int num);
>  void argv_array_clear(struct argv_array *);
>  
>  #endif /* ARGV_ARRAY_H */

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > As a convienience, it would be nice if we could pop entries off the argv_array
> > structs so that if they had multiple uses in a function, we wouldn't have to
> > clear them and repopulate common entries.  This patch adds the argv_array_pop
> > function to do just that.  Common entries can be added to an argv_array first,
> > then useage specific ones can be added on the end and removed later on.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
> > CC: Jeff King <peff@peff.net>
> > CC: Phil Hord <phil.hord@gmail.com>
> > CC: Junio C Hamano <gitster@pobox.com>
> > ---
> 
> Please don't do "Cc:" here; they belong to your e-mail header.
> 

> > diff --git a/argv-array.c b/argv-array.c
> > index a4e0420..ce24a48 100644
> > --- a/argv-array.c
> > +++ b/argv-array.c
> > @@ -39,6 +39,18 @@ void argv_array_pushf(struct argv_array *array, const char *fmt, ...)
> >  	argv_array_push_nodup(array, strbuf_detach(&v, NULL));
> >  }
> >  
> > +int argv_array_pop(struct argv_array *array, unsigned int num)
> > +{
> > +	if (num > array->argc)
> > +		return -1;
> 
> If your use case is "After using an argv_array for the first invocation,
> truncate it while keeping the common ones that appear early, so that ones
> that are specific to the second invocation can be pushed", it strikes me
> somewhat odd why you would want to specify "how many to pop".
> 

> 
> > +	for(num--; num>0; num--) {
> 
> Gaah.
> 

> On Thu, Apr 05, 2012 at 01:12:51PM -0700, Junio C Hamano wrote:
> > Neil Horman <nhorman@tuxdriver.com> writes:
> > 
> > > As a convienience, it would be nice if we could pop entries off the argv_array
> > > structs so that if they had multiple uses in a function, we wouldn't have to
> > > clear them and repopulate common entries.  This patch adds the argv_array_pop
> > > function to do just that.  Common entries can be added to an argv_array first,
> > > then useage specific ones can be added on the end and removed later on.
> > >
> > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> > 
> > > CC: Jeff King <peff@peff.net>
> > > CC: Phil Hord <phil.hord@gmail.com>
> > > CC: Junio C Hamano <gitster@pobox.com>
> > > ---
> > 
> > Please don't do "Cc:" here; they belong to your e-mail header.
> > 
> You mean place them below the snip line?  I can do that.
> 

> > > CC: Jeff King <peff@peff.net>
> > > CC: Phil Hord <phil.hord@gmail.com>
> > > CC: Junio C Hamano <gitster@pobox.com>
> > > ---
> > 
> > Please don't do "Cc:" here; they belong to your e-mail header.
> > 
> You mean place them below the snip line?  I can do that.

> > > +int argv_array_pop(struct argv_array *array, unsigned int num)
> > > +{
> > > +	if (num > array->argc)
> > > +		return -1;
> > 
> > If your use case is "After using an argv_array for the first invocation,
> > truncate it while keeping the common ones that appear early, so that ones
> > that are specific to the second invocation can be pushed", it strikes me
> > somewhat odd why you would want to specify "how many to pop".
> > 
> Why?  It seems perfectly logical to me to be able to, as a convienience, specify
> how many items to pop, and the api call seems pleasantly symmetric to the
> push[f] calls.

> > > +	for(num--; num>0; num--) {
> > 
> > Gaah.
> > 
> Eeek :).  If you want something else equally....equal here, please ask for it.
> I prefer for loops, but if you would rather have a while loop here, I'm fine
> with that.

> On Thu, Apr 05, 2012 at 07:24:29PM -0400, Neil Horman wrote:
> 
> > > > CC: Jeff King <peff@peff.net>
> > > > CC: Phil Hord <phil.hord@gmail.com>
> > > > CC: Junio C Hamano <gitster@pobox.com>
> > > > ---
> > > 
> > > Please don't do "Cc:" here; they belong to your e-mail header.
> > > 
> > You mean place them below the snip line?  I can do that.
> 
> No, I think he means to drop them entirely; that information is already
> in the list of people you have cc'd in the email.
> 

> > > > +int argv_array_pop(struct argv_array *array, unsigned int num)
> > > > +{
> > > > +	if (num > array->argc)
> > > > +		return -1;
> > > 
> > > If your use case is "After using an argv_array for the first invocation,
> > > truncate it while keeping the common ones that appear early, so that ones
> > > that are specific to the second invocation can be pushed", it strikes me
> > > somewhat odd why you would want to specify "how many to pop".
> > > 
> > Why?  It seems perfectly logical to me to be able to, as a convienience, specify
> > how many items to pop, and the api call seems pleasantly symmetric to the
> > push[f] calls.
> 
> I don't mind a "pop" call if there is a good use, but personally I find
> your use case to be hard to read. Your patch 3/5 does this:
> 
>   /* make a partial argv */
>   argv_array_init(&array);
>   argv_array_push(&array, "commit");
>   argv_array_push(&array, "-n");
> 
>   /* now do some speculative command */
>   if (some_logic) {
>           argv_array_push(&array, "--porcelain");
>           if (run_command(array.argv)) {
>               argv_array_clear(&array);
>               return 0;
>           }
>           argv_array_pop(&array, 1);
>   }
> 
>   /* and then possibly proceed to reuse part of the array */
>   argv_array_push(&array, ...);
>   argv_array_push(&array, ...);
>   run_command(array.argv);
> 
> It saves you having to repeat "commit -n", but at the expense of making
> the logic much harder to read. I think this is much easier to read:
> 
>   if (some_logic) {
>           const char *argv[] = { "commit", "-n", "--porcelain", NULL };
>           if (run_command(argv))
>                   return 0;
>   }
> 
>   argv_array_init(&array);
>   argv_array_push(&array, "commit");
>   argv_array_push(&array, "-n");
>   argv_array_push(&array, /* other options */);
>   run_command(array.argv);
> 
> You repeat "-n", but it is very clear what goes into the speculative
> command and what goes into the final command (and there is no chance of
> the "1" in your pop becoming stale and sending cruft to the real command).
> 

> > > > +	for(num--; num>0; num--) {
> > > 
> > > Gaah.
> > > 
> > Eeek :).  If you want something else equally....equal here, please ask for it.
> > I prefer for loops, but if you would rather have a while loop here, I'm fine
> > with that.
> 
> I think he may have been responding to the style (lack of whitespace). I

> I think a more traditional way of writing this would be:
> 
>   while (num--) {
>           free((char **)array->argv[num);
>           array->argv[num] = NULL;
>   }
> 

> On Thu, Apr 05, 2012 at 01:12:51PM -0700, Junio C Hamano wrote:
>> Neil Horman <nhorman@tuxdriver.com> writes:
>> 
>> > As a convienience, it would be nice if we could pop entries off the argv_array
>> > structs so that if they had multiple uses in a function, we wouldn't have to
>> > clear them and repopulate common entries.  This patch adds the argv_array_pop
>> > function to do just that.  Common entries can be added to an argv_array first,
>> > then useage specific ones can be added on the end and removed later on.
>> >
>> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>> 
>> 
>> > CC: Jeff King <peff@peff.net>
>> > CC: Phil Hord <phil.hord@gmail.com>
>> > CC: Junio C Hamano <gitster@pobox.com>
>> > ---
>> 
>> Please don't do "Cc:" here; they belong to your e-mail header.
>> 
> You mean place them below the snip line?  I can do that.

>> If your use case is "After using an argv_array for the first invocation,
>> truncate it while keeping the common ones that appear early, so that ones
>> that are specific to the second invocation can be pushed", it strikes me
>> somewhat odd why you would want to specify "how many to pop".
>> 
> Why?

>> > +	for(num--; num>0; num--) {
>> 
>> Gaah.
>> 
> Eeek :).  If you want something else equally....equal here, please ask for it.
> I prefer for loops, but if you would rather have a while loop here, I'm fine
> with that.

> >> > CC: Jeff King <peff@peff.net>
> >> > CC: Phil Hord <phil.hord@gmail.com>
> >> > CC: Junio C Hamano <gitster@pobox.com>
> >> > ---
> >> 
> >> Please don't do "Cc:" here; they belong to your e-mail header.
> >> 
> > You mean place them below the snip line?  I can do that.
> 
> No.  When you review and fix typo in format-patch output, you can add
> these to the e-mail header part and git-send-email will pick them up just
> fine.

> So from the maintainer's and the project's perspective, those cc lines
> are useless noise.
> ...
> So it is a piece of information that is useful to the submitter, but not
> to the maintainer.

> I agree that submitters would benefit from an automated way to propagate
> these addresses from reroll to reroll, and in a larger workflow "tweak and
> record while amending a commit for reroll" would be a natural place to do
> so, so I can see why it is tempting to abuse Cc: in the body of the
> message, but not knowing other ways is not a good excuse for such an
> abuse.  A workable solution is already available [*1*]: commit with a
> "---" line followed by Cc: and whatever extra junk while amending.

> On Thu, Apr 05, 2012 at 11:55:18PM -0700, Junio C Hamano wrote:
> ...
> I've played with that workflow before. While it's a neat trick, note
> that something like "format-patch -s" will put the signoff at the end,
> like:
>
>   commit subject
>
>   commit body
>   ---
>   Cc: whomever
>   Signed-off-by: you
>
> which is not what you want.

> About a year ago I had an RFC series to let "git commit" parse off the
> "---" bit and turn it into a git-note ...
> ... Ultimately I didn't follow up because
> I've found that I just don't end up keeping a lot of notes. I tend to do
> the re-roll and then send it out pretty soon afterward, so I just write
> any notes in the emails as they go out.
>
> For complex "cc" lists and the like, I have a (fairly hacky) script that
> takes an existing message as input and generates a format-patch series
> with the to, cc, and in-reply-to fields filled in (and then I ship the
> result out via my regular MUA after proof-reading and tweaking).
> Potentially git-send-email could do the same thing.

> On Thu, Apr 05, 2012 at 06:20:53PM -0700, Junio C Hamano wrote:
> 
> > >> > CC: Jeff King <peff@peff.net>
> > >> > CC: Phil Hord <phil.hord@gmail.com>
> > >> > CC: Junio C Hamano <gitster@pobox.com>
> > >> > ---
> > >> 
> > >> Please don't do "Cc:" here; they belong to your e-mail header.
> > >> 
> > > You mean place them below the snip line?  I can do that.
> > 
> > No.  When you review and fix typo in format-patch output, you can add
> > these to the e-mail header part and git-send-email will pick them up just
> > fine.
> 
> I think there is a legitimate conflict of interest here.
> 
> It's not clear exactly what "cc" tags in a commit message mean, because
> it is really a per-project thing. I don't work on the kernel, but I
> always took their cc tag to mean "these are people interested in this
> topic area". Send-email helpfully picks up that hint and cc's them on
> the emailed patch. And when the patch is applied, those cc lines remain,
> because people reading "git log" much later may find a bug in the patch,
> and it is helpful to tell them the people interested in the area.
> 

> Since we'll be using git-cherry-pick to enhance git-rebase's ability to preserve
> empty commits, we open ourselves to the possibility of preserving commits that
> are made empty by a previous merge as well, which is almost certainly not what
> we want (most of the time).  To handle this, we can add the ignore-if-made-empty
> option.  If enabled, it will look at cherry-picked commits, and if the origional
> sha1 has the same tree as its parent, then the cherry-pick is comitted as an
> empty commit, otherwise the commit is skipped (because it previously made
> changes to the tree, but no longer does).
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index c283d8c..bb7eb4a 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -103,7 +103,7 @@ effect to your index in a row.
>  	cherry-pick'ed commit, then a fast forward to this commit will
>  	be performed.
>  
> ---allow-empty:
> +--allow-empty:::

> @@ -112,6 +112,14 @@ effect to your index in a row.
>  	commits that meet the "fast-forward" requirement will be kept
>  	even without this option.
>  
> +--ignore-if-made-empty::
> +	If the --allow-empty option is used, all empty commits are kept,

> +static int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
> +{
> +	struct argv_array array;
> +	int rc;
> +
> +	argv_array_init(&array);
> +	argv_array_push(&array, "commit");
> +	argv_array_push(&array, "-n");
> +
> +	if ((!empty) && (opts->ignore_if_made_empty)) {

> +			argv_array_clear(&array);
> +			return 0;
> +		}
> +		argv_array_pop(&array, 1);
> +	}
> +
>  
>  	if (opts->signoff)
> +		argv_array_push(&array, "-s");
>  	if (!opts->edit) {
> +		argv_array_push(&array, "-F");
> +		argv_array_push(&array, defmsg);
>  	}
> +	
>  	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_origional_commit_empty(struct commit *commit)

> +{
> +	struct argv_array argv_array;
> +	struct child_process cp;
> +	char ptree[41], pptree[41];
> +	int pipefd[2];
> +	FILE *output;
> +	int ret = 0;
> +
> +	if (pipe2(pipefd, 0) < 0)
> +		return 0;
> +
> +	output = xfdopen(pipefd[0], "r");
> +
> +	argv_array_init(&argv_array);
> +	memset(&cp, 0, sizeof(struct child_process));
>  
> -	args[i] = NULL;
> +	argv_array_push(&argv_array, "rev-parse");
> +	argv_array_pushf(&argv_array, "%s^{tree}", sha1_to_hex(commit->object.sha1));
>  
> -	return run_command_v_opt(args, RUN_GIT_CMD);
> +	cp.git_cmd = 1;
> +	cp.no_stdin = 1;
> +	cp.no_stderr = 1;
> +	cp.out = pipefd[1];
> +	cp.argv = argv_array.argv;
> +
> +	if (start_command(&cp) < 0)
> +		goto out;
> +
> +	if (fscanf(output, "%s\n", ptree) < 1)
> +		goto out;
> +
> +	finish_command(&cp);
> +
> +	fclose(output);
> +	close(pipefd[0]);
> +	argv_array_clear(&argv_array);
> +
> +	if (pipe2(pipefd, 0) < 0)

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > Subject: Re: [PATCH 3/5] git-cherry-pick: Add ignore-if-made-empty option [v2]
> 
> Please don't do this.  The tools do not strip the garbage at the end.
> 
> Instead, do it like either one of these:
> 
> 	Subject: [PATCH 3/5 (v2)] mumble mumble...
> 	Subject: [PATCH v2 3/5] mumble mumble...
> 
> The latter is more appropriate when resending everything, including the
> ones that did not change from the earlier round.
> 

> > Since we'll be using git-cherry-pick to enhance git-rebase's ability to preserve
> > empty commits, we open ourselves to the possibility of preserving commits that
> > are made empty by a previous merge as well, which is almost certainly not what
> > we want (most of the time).  To handle this, we can add the ignore-if-made-empty
> > option.  If enabled, it will look at cherry-picked commits, and if the origional
> > sha1 has the same tree as its parent, then the cherry-pick is comitted as an
> > empty commit, otherwise the commit is skipped (because it previously made
> > changes to the tree, but no longer does).
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> > index c283d8c..bb7eb4a 100644
> > --- a/Documentation/git-cherry-pick.txt
> > +++ b/Documentation/git-cherry-pick.txt
> > @@ -103,7 +103,7 @@ effect to your index in a row.
> >  	cherry-pick'ed commit, then a fast forward to this commit will
> >  	be performed.
> >  
> > ---allow-empty:
> > +--allow-empty:::
> 
> A single colon is already buggy, but three colons is equally bad.  Please
> fix that in the original point this bug was introduced.  I guess it was
> 2/5 of this series?
> 

> > @@ -112,6 +112,14 @@ effect to your index in a row.
> >  	commits that meet the "fast-forward" requirement will be kept
> >  	even without this option.
> >  
> > +--ignore-if-made-empty::
> > +	If the --allow-empty option is used, all empty commits are kept,
> 
> Literal strings the user may type are typically set in tt font
> (i.e. `--allow-empty`).
> 

> > +	including those which were made empty due to a previous change.
> > +	While this may be desireable, likely it is not.
> 
> Mental note.  Up to this point, the reader is told that "--allow-empty"
> alone is likely to do a wrong thing.
> 
> > +	This option
> > +	restricts the scope of --allow-empty to only those commits which
> > +	were created as empty commits ...
> 
> And the user is asked to give "--ignore-if-made-empty" in addition to
> "--allow-empty" to get a saner and more likely to be useful behaviour.
> Isn't that backwards?  This "the other option is insane, and please make
> it saner" option needs a lot more typing than the more insane option.
> 
> I would expect that "--allow-empty" would by default filter ones that are
> originally non-empty but are made unnecessary (we are allowing empty
> commits in the original history to be cherry-picked, but the general
> principle that unnecessary commits must not be picked still is in effect).
> If you want to give a user the other more insane mode of operation, it is
> OK to let the user give a different option *instead* *of* the saner
> "--allow-empty".
> 
> Perhaps name that "--keep-unnecessary-commit" (it is no longer about
> allowing empty commits in the original history to be picked; it is about
> keeping unnecessary and irrelevant commits in the resulting history).
> 
> And error out if both options are given.
> 

> > +static int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
> > +{
> > +	struct argv_array array;
> > +	int rc;
> > +
> > +	argv_array_init(&array);
> > +	argv_array_push(&array, "commit");
> > +	argv_array_push(&array, "-n");
> > +
> > +	if ((!empty) && (opts->ignore_if_made_empty)) {
> 
> Style: lose the extra and unnecessary parentheses, especially when the
> expression inside them are so trivial, i.e.
> 

> 	if (!empty && opts->ignore_if_made_empty) {
> 
> although I suspect that redesign of the command line interface might make
> it a moot point to polish this part of the code without major rethinking.
> 
> > +		/* Note: This implies --dry-run */
> > +		argv_array_push(&array, "--porcelain");
> > +		if (run_command_v_opt(array.argv, RUN_GIT_CMD) == 1) {
> 
> The only thing you want to check at this point is if the contents of the
> index being committed is the same as the tree of HEAD.  Why do we even
> want to incur this much overhead?
> 
> Isn't running "diff-index --cached HEAD" sufficient?

> 
> > +			/* The dry run exit code of 1 tells us this is
> > + 			 * an empty commit, just skip it.
> > + 			 */
> 
> 	/*
>          * Our multi-line comments are
>          * formatted this way.
>          */
> 

> > +			argv_array_clear(&array);
> > +			return 0;
> > +		}
> > +		argv_array_pop(&array, 1);
> > +	}
> > +
> >  
> >  	if (opts->signoff)
> > +		argv_array_push(&array, "-s");
> >  	if (!opts->edit) {
> > +		argv_array_push(&array, "-F");
> > +		argv_array_push(&array, defmsg);
> >  	}
> > +	
> >  	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_origional_commit_empty(struct commit *commit)
> 
> What is origional?
> 

> > +{
> > +	struct argv_array argv_array;
> > +	struct child_process cp;
> > +	char ptree[41], pptree[41];
> > +	int pipefd[2];
> > +	FILE *output;
> > +	int ret = 0;
> > +
> > +	if (pipe2(pipefd, 0) < 0)
> > +		return 0;
> > +
> > +	output = xfdopen(pipefd[0], "r");
> > +
> > +	argv_array_init(&argv_array);
> > +	memset(&cp, 0, sizeof(struct child_process));
> >  
> > -	args[i] = NULL;
> > +	argv_array_push(&argv_array, "rev-parse");
> > +	argv_array_pushf(&argv_array, "%s^{tree}", sha1_to_hex(commit->object.sha1));
> >  
> > -	return run_command_v_opt(args, RUN_GIT_CMD);
> > +	cp.git_cmd = 1;
> > +	cp.no_stdin = 1;
> > +	cp.no_stderr = 1;
> > +	cp.out = pipefd[1];
> > +	cp.argv = argv_array.argv;
> > +
> > +	if (start_command(&cp) < 0)
> > +		goto out;
> > +
> > +	if (fscanf(output, "%s\n", ptree) < 1)
> > +		goto out;
> > +
> > +	finish_command(&cp);
> > +
> > +	fclose(output);
> > +	close(pipefd[0]);
> > +	argv_array_clear(&argv_array);
> > +
> > +	if (pipe2(pipefd, 0) < 0)
> 
> Huh?  "man pipe2" and see if it is portable.
> 

> On Thu, Apr 05, 2012 at 02:01:43PM -0700, Junio C Hamano wrote:
>
>> I am starting to wonder if it is worth spending time on careful reviewing,
>> or it would be sufficient to give a cursory review quickly to give you
>> more time to polish your re-roll.
>> 
> I'm not sure what you think is so egregious about this changeset, but if
> you have a specific problem, please let me know.

> > +	including those which were made empty due to a previous change.
> > +	While this may be desireable, likely it is not.
> 
> Mental note.  Up to this point, the reader is told that "--allow-empty"
> alone is likely to do a wrong thing.
> 
> > +	This option
> > +	restricts the scope of --allow-empty to only those commits which
> > +	were created as empty commits ...
> 
> And the user is asked to give "--ignore-if-made-empty" in addition to
> "--allow-empty" to get a saner and more likely to be useful behaviour.
> Isn't that backwards?  This "the other option is insane, and please make
> it saner" option needs a lot more typing than the more insane option.
> 
> I would expect that "--allow-empty" would by default filter ones that are
> originally non-empty but are made unnecessary (we are allowing empty
> commits in the original history to be cherry-picked, but the general
> principle that unnecessary commits must not be picked still is in effect).
> If you want to give a user the other more insane mode of operation, it is
> OK to let the user give a different option *instead* *of* the saner
> "--allow-empty".
> 
> Perhaps name that "--keep-unnecessary-commit" (it is no longer about
> allowing empty commits in the original history to be picked; it is about
> keeping unnecessary and irrelevant commits in the resulting history).
> 
> And error out if both options are given.
> 

> +	if (pipe2(pipefd, 0) < 0)
> +		return 0;
> +	output = xfdopen(pipefd[0], "r");
> +	memset(&cp, 0, sizeof(struct child_process));
> +	... setup cp ...
> +	if (start_command(&cp) < 0)
> +		goto out;
> +	if (fscanf(output, "%s\n", ptree)<  1)
> +		goto out;
> +	finish_command(&cp);
> +	fclose(output);
> +	close(pipefd[0]);

> 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/commit.c                  |    6 +++---
>  builtin/revert.c                  |    2 ++
>  sequencer.c                       |    7 +++++--
>  sequencer.h                       |    1 +
>  5 files changed, 20 insertions(+), 5 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/commit.c b/builtin/commit.c
> index 3714582..0cd10ab 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -56,10 +56,10 @@ N_("You asked to amend the most recent commit, but doing so would make\n"
>  "remove the commit entirely with \"git reset HEAD^\".\n");
>  
>  static const char empty_cherry_pick_advice[] =
> -N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\n"
> -"If you wish to commit it anyway, use:\n"
> +N_("The previous cherry-pick is empty.\n"
> +"If the commit was created empty, please use:\n"

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > 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/commit.c                  |    6 +++---
> >  builtin/revert.c                  |    2 ++
> >  sequencer.c                       |    7 +++++--
> >  sequencer.h                       |    1 +
> >  5 files changed, 20 insertions(+), 5 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/commit.c b/builtin/commit.c
> > index 3714582..0cd10ab 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -56,10 +56,10 @@ N_("You asked to amend the most recent commit, but doing so would make\n"
> >  "remove the commit entirely with \"git reset HEAD^\".\n");
> >  
> >  static const char empty_cherry_pick_advice[] =
> > -N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\n"
> > -"If you wish to commit it anyway, use:\n"
> > +N_("The previous cherry-pick is empty.\n"
> > +"If the commit was created empty, please use:\n"
> 
> After reading this three times, I have to say that the updated wording do
> not look like an improvement for two reasons.
> 
>  (1) After a failed cherry-pick, the index can match the current HEAD for
>      two reasons.  Either the original cherry-pick was attempting to pick
>      an empty commit (which is likely to be a mistake unless you are doing
>      something unusual like creating an empty commit in the first place),
>      or the change in the original commit was already found in the current
>      version (may be result of a conflict resolution).  The message before
>      your change used "possibly" to hint this, and if the reader gets it,
>      it is understandable why the reader is seeing this advise.  Updated
>      message loses this information by simply saying "is empty".
> 

>      "the commit" refers to the original commit the user tried to
>      cherry-pick before running this command while reviewing this patch, I
>      suspect that the reader who sees this message may not be able to tell
>      if the "git commit" command created a possibly empty commit and then
>      telling the user to do something further on _that_ commit, or if it
>      is referring to the commit the user tried to pick with the previous
>      "git cherry-pick" command.
> 

> That is, unless you are making "git cherry-pick --allow-empty" not to stop
> and leave it to "git commit" to clean it up.  If that were the case (which
> is not, after applying this patch alone), then this message will be issued
> only when a conflict resolution resulted in an empty commit, so "If the
> commit you were trying to cherry-pick was empty to begin with" would not
> apply, either.

> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

> On Tue, Apr 10, 2012 at 09:45:46AM -0700, Junio C Hamano wrote:
> ...
>>  (2) The message is given by the "git commit" command.  "If the commit was
>>      created empty" looks confusing.  Even though I can understand that
> Its coded within the git commit command code, but is only ever displayed if
> whence is GIT_CHERRY_PICK, so as far as I can see, from a users perspective,
> this will only be seen if they type git cherry-pick on the command line.

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > On Tue, Apr 10, 2012 at 09:45:46AM -0700, Junio C Hamano wrote:
> > ...
> >>  (2) The message is given by the "git commit" command.  "If the commit was
> >>      created empty" looks confusing.  Even though I can understand that
> > Its coded within the git commit command code, but is only ever displayed if
> > whence is GIT_CHERRY_PICK, so as far as I can see, from a users perspective,
> > this will only be seen if they type git cherry-pick on the command line.
> 
> Here is what I tried, and I think you are wrong.
> 
> 	$ git cherry-pick $some_commit
>         ... conflicts ...
>         $ edit so that the working tree matches HEAD
>         $ git commit -a
>         ... message from status ...
>         THE ADVICE IN QUEWSTION COMES HERE!!!
> 
> 

> On Tue, Apr 10, 2012 at 12:32:18PM -0700, Junio C Hamano wrote:
>> Neil Horman <nhorman@tuxdriver.com> writes:
>> 
>> > On Tue, Apr 10, 2012 at 09:45:46AM -0700, Junio C Hamano wrote:
>> > ...
>> >>  (2) The message is given by the "git commit" command.  "If the commit was
>> >>      created empty" looks confusing.  Even though I can understand that
>> > Its coded within the git commit command code, but is only ever displayed if
>> > whence is GIT_CHERRY_PICK, so as far as I can see, from a users perspective,
>> > this will only be seen if they type git cherry-pick on the command line.
>> 
>> Here is what I tried, and I think you are wrong.
>> 
>> 	$ git cherry-pick $some_commit
>>         ... conflicts ...
>>         $ edit so that the working tree matches HEAD
>>         $ git commit -a
>>         ... message from status ...
>>         THE ADVICE IN QUEWSTION COMES HERE!!!
>> 
>> 
> Ok, I admit I didn't really think of that case, but that seems to me to be the
> trivial case, which is unlikely to be encountered.  If you do a git cherry-pick and
> have conflicts, you by definition don't have a commit that is resolved to empty...

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > On Tue, Apr 10, 2012 at 12:32:18PM -0700, Junio C Hamano wrote:
> >> Neil Horman <nhorman@tuxdriver.com> writes:
> >> 
> >> > On Tue, Apr 10, 2012 at 09:45:46AM -0700, Junio C Hamano wrote:
> >> > ...
> >> >>  (2) The message is given by the "git commit" command.  "If the commit was
> >> >>      created empty" looks confusing.  Even though I can understand that
> >> > Its coded within the git commit command code, but is only ever displayed if
> >> > whence is GIT_CHERRY_PICK, so as far as I can see, from a users perspective,
> >> > this will only be seen if they type git cherry-pick on the command line.
> >> 
> >> Here is what I tried, and I think you are wrong.
> >> 
> >> 	$ git cherry-pick $some_commit
> >>         ... conflicts ...
> >>         $ edit so that the working tree matches HEAD
> >>         $ git commit -a
> >>         ... message from status ...
> >>         THE ADVICE IN QUEWSTION COMES HERE!!!
> >> 
> >> 
> > Ok, I admit I didn't really think of that case, but that seems to me to be the
> > trivial case, which is unlikely to be encountered.  If you do a git cherry-pick and
> > have conflicts, you by definition don't have a commit that is resolved to empty...
> 
> Not at all unlikely, especially in a distributed world.
> 
> You may have seen two patches but they make sense as one so that you apply
> to one branch as one change, while the branch you are cherry-picking from
> may have these two changes as individual commits.  Neither of them will
> apply cleanly to your tree, and your resolution will be "keep mine---I
> already have this".
> 

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > ...  What do you say to my proposal regarding the splitting of the device
> > dependent on how we were executed?  It seems we can't use a single advice string
> > in this case, as no matter which we choose there is a use case in which it fails
> > to make sense.
> 
> When your cherry-pick got --allow-empty, it should pass --allow-empty to
> its inner invocation of commit if it is picking an originally empty
> commit, and this advice will not trigger because commit will happily
> commit the no-change change.
> 
> When your cherry-pick got --allow-empty but not --keep-unnecessary-commit,
> its inner invocation of commit must not pass --allow-empty if it is _not_
> picking an originally empty commit.  Then the inner commit will fail if it
> auto resolves to no change, and the user sees the advice.  The current
> advice text is appropriate for this case.
> 
> When your cherry-pick did not get either of these flags, its inner
> invocation of commit must not pass --allow-empty.  The user sees the
> advice when the auto resolved result matches HEAD from the commit invoked
> by cherry-pick.  The current advice text is fine for this case, as we say
> "possibly", not "we definitely know it was due to conflict resolution".
> 
> Or your cherry-pick may have failed due to a conflict, regardless of the
> options like --allow-empty or --keep-unnecessary-commit given to it, and
> the user may have run commit after resolving the conflict.  The current
> advice text is fine for this case, too, as we say "possibly", and it
> indeed is what just happened.
> 
> So I do not think you need to change anything with respect to the advice
> message.
> 
> Am I missing some other cases?
> 

> No, you covered all the cases, but I disagree with your assertion that the advice
> is correct (or at least optimal) in any of these cases. If a cherry-pick without
> any options is preformed and the commit is empty (regardless of the reason), the
> advice given is that git commit --allow-empty should be used.  With the addition
> of these new options, thats not true any longer.  Instead of using git commit
> --allow-empty, you can use git cherry-pick --allow-empty.

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > No, you covered all the cases, but I disagree with your assertion that the advice
> > is correct (or at least optimal) in any of these cases. If a cherry-pick without
> > any options is preformed and the commit is empty (regardless of the reason), the
> > advice given is that git commit --allow-empty should be used.  With the addition
> > of these new options, thats not true any longer.  Instead of using git commit
> > --allow-empty, you can use git cherry-pick --allow-empty.
> 
> Sorry, I am confused.  Do you mean that the sequence goes like this (with
> concrete examples of command line args)?
> 
> 	$ git cherry-pick nh/empty-rebase
>         ... stops because "git show nh/empty-rebase" is empty
>         $ git cherry-pick --allow-empty
> 

> But that cannot be correct, without --continue [*1*], i.e.
> 
> 	$ git cherry-pick --allow-empty --continue
> 
> no?  I didn't check, but if the command without --continue in the above
> sequence does not error out, I think it is a bug.
> 

> I am actually OK with suggesting "git cherry-pick --continue", but then
> "cherry-pick --allow-empty" (or "--keep-unnecessary-commits") that punts
> and gives the control back to the user should leave enough clue for a
> later invocation of itself so that it can realize that the original
> invocation was made with "--allow-empty".  In other words, I am OK if the
> interaction goes like this:
> 
> 	$ git cherry-pick --keep-unnecessary-commits nh/empty-rebase
>         ... stops due to a conflict
>         $ edit builtin/revert.c
>         ... the result ends up being empty
>         $ git add -u ;# resolved
>         $ git cherry-pick --continue
> 

> 
> [Side note]
> 
> *1* It was an original UI mistake to make the users conclude a "git merge"
> that asked the user to help resolving the conflict with "git commit",
> which was inherited by "git cherry-pick" and "git revert", especially when
> these three commands are merely a special "possibly zero or one stoppage"
> case of more general sequencing commands like "am" and "rebase" that can
> stop zero or more times to ask the user for help and the way to resume
> them is to re-run the same command with "--continue" option (and without
> any other arguments), e.g.
> 
> 	$ git am -3 ./+nh.mbox
>         ... stops due to conflict and asks to resolve them
>         $ edit builtin/revert.c
>         $ git add builtin/revert.c
>         $ git am --continue
> 
> and also discussed that in the longer-term it would be nice to teach the
> oddball commands to honor "--continue".  "am" originally took "--resolved"
> (and it still does, and it will do so in the future) for the same purpose,
> and we taught it and "cherry-pick" and "revert" to honor "--continue".
> Probably we should start teaching "merge" to honor it as well to complete
> the vision.
> 

>> But that cannot be correct, without --continue [*1*], i.e.
>> 
>> 	$ git cherry-pick --allow-empty --continue
>> 
>> no?  I didn't check, but if the command without --continue in the above
>> sequence does not error out, I think it is a bug.
>> 
> No, it errors out.  I'm sorry to have confused you.  The only point that I was
> trying to make here is that, when running git cherry-pick, its seems awkward to
> a user to get advice indicating that git commit --allow-empty should be run.

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> >> But that cannot be correct, without --continue [*1*], i.e.
> >> 
> >> 	$ git cherry-pick --allow-empty --continue
> >> 
> >> no?  I didn't check, but if the command without --continue in the above
> >> sequence does not error out, I think it is a bug.
> >> 
> > No, it errors out.  I'm sorry to have confused you.  The only point that I was
> > trying to make here is that, when running git cherry-pick, its seems awkward to
> > a user to get advice indicating that git commit --allow-empty should be run.
> 
> I was only saying that "git cherry-pick --allow-empty" is a *bad*
> suggestion because it does *not* work and errors out, and you seem to
> agree with me on that point.  I also said I am OK if the suggestion for
> this case were to run "git cherry-pick --continue".
> 
> But you sound like you are disagreeing with me; I am not sure where you
> found what I said not agreeable.  So I am not sure what to say at this
> point.
> 

> diff --git a/sequencer.c b/sequencer.c
> index 71929ba..5d033db 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,102 @@ 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)
> +static 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) {
> +		const char *argv[] = { "diff-index", "--quiet", "--exit-code",
> +					"--cached", "HEAD", NULL };
> +
> +		/*
> + 		 * If we run git diff-index with the above option and it returns
> + 		 * zero, then there have been no changes made to the index by
> + 		 * this patch, i.e. its empty.  Since our previous empty test
> + 		 * indicated that this patch was not created empty, its been made
> + 		 * redundant.  Since keep_if_made_empty is not set, we just skip
> + 		 * it
> + 		 */
> +		if (run_command_v_opt(argv, RUN_GIT_CMD) == 0)
> +			return 0;

> +static int is_original_commit_empty(struct commit *commit)
> +{
> +	struct argv_array argv_array;
> +	struct child_process cp;
> +	char ptree[40], pptree[40];
> +	int ret = 0;
> +
> +	argv_array_init(&argv_array);
> +	memset(&cp, 0, sizeof(struct child_process));
> +
> +	argv_array_push(&argv_array, "rev-parse");
> +	argv_array_pushf(&argv_array, "%s^{tree}", sha1_to_hex(commit->object.sha1));

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > +	/* keep_if_made_empty implies allow_empty */
> > +	if (opts->keep_if_made_empty)
> > +		opts->allow_empty = 1;
> > +
>  
> OK.
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 71929ba..5d033db 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,102 @@ 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)
> > +static 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) {
> > +		const char *argv[] = { "diff-index", "--quiet", "--exit-code",
> > +					"--cached", "HEAD", NULL };
> > +
> > +		/*
> > + 		 * If we run git diff-index with the above option and it returns
> > + 		 * zero, then there have been no changes made to the index by
> > + 		 * this patch, i.e. its empty.  Since our previous empty test
> > + 		 * indicated that this patch was not created empty, its been made
> > + 		 * redundant.  Since keep_if_made_empty is not set, we just skip
> > + 		 * it
> > + 		 */
> > +		if (run_command_v_opt(argv, RUN_GIT_CMD) == 0)
> > +			return 0;
> 
> Wouldn't it be far simpler to do this without forking diff-index?  I
> haven't followed the codepath leading to this, but it should be very easy
> to find a commit object that corresponds to the current HEAD and peek the
> tree object in it to find out the current tree, and because the original
> function is going to run an as-is "git commit", the tree you are going to
> commit should be available by calling cache_tree_update() like write-tree
> does.  If they match, you are trying to create an empty commit.  E.g. (error
> checking elided):
> 
> 	unsigned char head_sha1[20];
> 	struct commit *head_commit;
> 
> 	resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
>         head_commit = lookup_commit(head_sha1);
>         parse_commit(head_commit);
> 
>         if (!cache_tree_fully_valid(active_cache_tree))
> 		cache_tree_update(active_cache_tree, active_cache,
> 				active_nr, 0);
> 	return hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
> 
> > +static int is_original_commit_empty(struct commit *commit)
> > +{
> > +	struct argv_array argv_array;
> > +	struct child_process cp;
> > +	char ptree[40], pptree[40];
> > +	int ret = 0;
> > +
> > +	argv_array_init(&argv_array);
> > +	memset(&cp, 0, sizeof(struct child_process));
> > +
> > +	argv_array_push(&argv_array, "rev-parse");
> > +	argv_array_pushf(&argv_array, "%s^{tree}", sha1_to_hex(commit->object.sha1));
> 
> Likewise.  You have the commit object, so just make sure it was parsed,
> and peek its tree object.  Also do the same for its parent commit.  Now
> you have two trees, so you can compare their object names.  E.g. (error
> checking elided):
> 

> +			 * 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");
> +
> +

> On Fri, Apr 13, 2012 at 02:45:05PM -0400, Neil Horman wrote:
> >
> > +                     OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"),
> 
> For consistency, I'd prefer if the variable name were the same as the
> option. Then I wouldn't have to keep the option<->variable translation
> in mind.
> 

> >
> > +     if (!empty && !opts->keep_if_made_empty) {
> [...]
> > +                     return 0;
> [...]
> >       if (opts->allow_empty)
> > +             argv_array_push(&array, "--allow-empty");
> > +
> > +     rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
> 
> I find it a bit strange, that if we cherry-pick a commit that was
> already empty, we _do_ call git commit (and error out), but if we find a
> commit that is made empty, we do _not_ call git commit and quietly
> succeed (in not doing anything). But I suppose that is the legacy
> behavior?
> 

> > +
> > +		rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
> > +
> > +		if (rc)
> 
> rc is short for run_command, for which it stores the return value, no?
> Let's not abuse the variable like this and instead use the result
> directly:
> 

>  if (!hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1))
> 
> If you make the entire paragraph leading up to this a separate function,
> say index_is_empty(), then the above would read more naturally like
> this:
> 
>  if (!opts->keep_if_made_empty && !empty && index_is_empty())
> 
> > +			/*
> > +			 * The head tree and the parent tree match
> > +			 * meaning the commit is empty.  Since it wasn't created
> 
> Don't you mean the head tree and the index?
> 

> > +			 * 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");
> > +
> > +
> 
> Why two newlines?
> 
> > +	rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
> > +	argv_array_clear(&array);
> > +	return rc;
> > +}
> 
> Looks good to me otherwise.
> 

> > 
> > I find it a bit strange, that if we cherry-pick a commit that was
> > already empty, we _do_ call git commit (and error out), but if we find a
> > commit that is made empty, we do _not_ call git commit and quietly
> > succeed (in not doing anything). But I suppose that is the legacy
> > behavior?
>
> Correct, more or less.  The legacy behavior is to call git commit unilaterally.
> [...] The only change is that if we do not specify
> keep-redundant-commits, we check to see if the commit is made empty in
> git-cherry-pick and skip it if it is.  We could, instead of returning prior to
> calling git-commit, use that test to override the keep_empty option below, so
> that we don't pass --allow-empty to git-commit instead.  That would preserve the
> prior code path, but for no real advatage, as the outcome is the same, and this
> way saves us having to fork the git-commit command, which I think is
> adventageous.

> On Mon, Apr 16, 2012 at 11:38:27AM -0400, Neil Horman wrote:
> > > 
> > > I find it a bit strange, that if we cherry-pick a commit that was
> > > already empty, we _do_ call git commit (and error out), but if we find a
> > > commit that is made empty, we do _not_ call git commit and quietly
> > > succeed (in not doing anything). But I suppose that is the legacy
> > > behavior?
> >
> > Correct, more or less.  The legacy behavior is to call git commit unilaterally.
> > [...] The only change is that if we do not specify
> > keep-redundant-commits, we check to see if the commit is made empty in
> > git-cherry-pick and skip it if it is.  We could, instead of returning prior to
> > calling git-commit, use that test to override the keep_empty option below, so
> > that we don't pass --allow-empty to git-commit instead.  That would preserve the
> > prior code path, but for no real advatage, as the outcome is the same, and this
> > way saves us having to fork the git-commit command, which I think is
> > adventageous.
> 
> Except that the outcome is not the same. With and without your changes,
> git cherry-pick <empty-commit> fails. But with your changes, git
> cherry-pick <commit-will-become-empty> will succeed and do nothing,
> while before it would have failed exactly like git cherry-pick
> <empty-commit>.
> 
> So I am not arguing whether failing or skipping is the better default
> behavior. But the legacy behavior is consistent between the empty-commit
> and commit-will-become-empty cases.  And if we change the behavior for
> one, why not also for the other?
> 

> On Mon, Apr 16, 2012 at 11:38:27AM -0400, Neil Horman wrote:
> ...
> Except that the outcome is not the same. With and without your changes,
> git cherry-pick <empty-commit> fails. But with your changes, git
> cherry-pick <commit-will-become-empty> will succeed and do nothing,
> while before it would have failed exactly like git cherry-pick
> <empty-commit>.
>
> So I am not arguing whether failing or skipping is the better default
> behavior. But the legacy behavior is consistent between the empty-commit
> and commit-will-become-empty cases.

> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > On Mon, Apr 16, 2012 at 11:38:27AM -0400, Neil Horman wrote:
> > ...
> > Except that the outcome is not the same. With and without your changes,
> > git cherry-pick <empty-commit> fails. But with your changes, git
> > cherry-pick <commit-will-become-empty> will succeed and do nothing,
> > while before it would have failed exactly like git cherry-pick
> > <empty-commit>.
> >
> > So I am not arguing whether failing or skipping is the better default
> > behavior. But the legacy behavior is consistent between the empty-commit
> > and commit-will-become-empty cases.
> 
> Is that particular "consistency" a good one, though?  If you had an empty
> commit in the original range, it is a lot more likely that it was an error
> that you would want to know about.  You may be the kind of person who
> value an empty commit in your history, using it as some kind of a mark in
> the history, and in that case you would want to know that it is being
> discarded.  On the other hand, if a commit that did something in the
> original context turns out to be unnecessary in the replayed context, that
> is not something you would ever want to keep in the replayed context, and
> erroring out and forcing you to say "yeah, I admit I do not want it" would
> just be annoying.

> On Tue, Apr 17, 2012 at 08:42:52AM -0700, Junio C Hamano wrote:
> > Clemens Buchacher <drizzd@aon.at> writes:
> > 
> > > On Mon, Apr 16, 2012 at 11:38:27AM -0400, Neil Horman wrote:
> > > ...
> > > Except that the outcome is not the same. With and without your changes,
> > > git cherry-pick <empty-commit> fails. But with your changes, git
> > > cherry-pick <commit-will-become-empty> will succeed and do nothing,
> > > while before it would have failed exactly like git cherry-pick
> > > <empty-commit>.
> > >
> > > So I am not arguing whether failing or skipping is the better default
> > > behavior. But the legacy behavior is consistent between the empty-commit
> > > and commit-will-become-empty cases.
> > 
> > Is that particular "consistency" a good one, though?  If you had an empty
> > commit in the original range, it is a lot more likely that it was an error
> > that you would want to know about.  You may be the kind of person who
> > value an empty commit in your history, using it as some kind of a mark in
> > the history, and in that case you would want to know that it is being
> > discarded.  On the other hand, if a commit that did something in the
> > original context turns out to be unnecessary in the replayed context, that
> > is not something you would ever want to keep in the replayed context, and
> > erroring out and forcing you to say "yeah, I admit I do not want it" would
> > just be annoying.
> 
> Yeah, that makes sense.
> 
> > So "consistency" between the two would actually be a mistake that we may
> > want to "break", I would think.
> 
> Agreed. But we should document the change in the commit message and
> maybe add a comment, because it is really strange to read
> 
>  if (!empty && index_empty)
>  	return "it's all good"
> 
>  if (empty)
>  	return "oh noes!"
> 
> without an explanation as to why empty and index_empty are so different.
> Neil, what do you think?
> 

>  
> +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^
> +	}
> +'

> On Fri, Apr 13, 2012 at 02:45:06PM -0400, Neil Horman wrote:
> >  
> > +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
> > +	}
> > +'
> 
> You don't need the braces. The same below.
> 
> > +
> > +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^
> > +	}
> > +'
> 
> And the expected result is that the HEAD commit is not removed, right?
> You should check for that as well.
> 
> Also, please checkout empty-branch2^0 first, in order to make the test
> independent of its predecessor.
> 

> On Fri, Apr 13, 2012 at 02:45:06PM -0400, Neil Horman wrote:
> >  
> > +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
> > +	}
> > +'
> 
> You don't need the braces. The same below.
> 

> > +
> > +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^
> > +	}
> > +'
> 
> And the expected result is that the HEAD commit is not removed, right?
> You should check for that as well.
> 
> Also, please checkout empty-branch2^0 first, in order to make the test
> independent of its predecessor.
> 

> On Sun, Apr 15, 2012 at 11:39:35AM +0200, Clemens Buchacher wrote:
> ...
>> > +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^
>> > +	}
>> > +'
>> 
>> And the expected result is that the HEAD commit is not removed, right?
>> You should check for that as well.
>> 
>> Also, please checkout empty-branch2^0 first, in order to make the test
>> independent of its predecessor.
>
> Not sure I follow what your saying here.  The expected result with both of these
> tests is that a new commit is created, referencing the current HEAD as the new
> HEAD's parent.

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > On Sun, Apr 15, 2012 at 11:39:35AM +0200, Clemens Buchacher wrote:
> > ...
> >> > +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^
> >> > +	}
> >> > +'
> >> 
> >> And the expected result is that the HEAD commit is not removed, right?
> >> You should check for that as well.
> >> 
> >> Also, please checkout empty-branch2^0 first, in order to make the test
> >> independent of its predecessor.
> >
> > Not sure I follow what your saying here.  The expected result with both of these
> > tests is that a new commit is created, referencing the current HEAD as the new
> > HEAD's parent.
> 
> If the request were "checkout master^0 first" I would understand.  The
> precondition for the second test will be different depending on the first
> one succeeds or not.  Perhaps that is what Clemens meant?
> 

> On Mon, Apr 16, 2012 at 09:35:12AM -0700, Junio C Hamano wrote:
> > Neil Horman <nhorman@tuxdriver.com> writes:
> > 
> > > On Sun, Apr 15, 2012 at 11:39:35AM +0200, Clemens Buchacher wrote:
> > > ...
> > >> > +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^
> > >> > +	}
> > >> > +'
> > >> 
> > >> And the expected result is that the HEAD commit is not removed, right?
> > >> You should check for that as well.
> > >> 
> > >> Also, please checkout empty-branch2^0 first, in order to make the test
> > >> independent of its predecessor.
> > >
> > > Not sure I follow what your saying here.  The expected result with both of these
> > > tests is that a new commit is created, referencing the current HEAD as the new
> > > HEAD's parent.
> > 
> > If the request were "checkout master^0 first" I would understand.  The
> > precondition for the second test will be different depending on the first
> > one succeeds or not.  Perhaps that is what Clemens meant?
> > 
> Perhaps, but if so, I'm still not sure how a checkout of empty-branch2^0 affects
> these tests at all, nor do I grok the relevance to ensuring that the HEAD commit
> wasn't removed (as AIUI, cherry pick never does that anyway).  Clement, can you
> clarify your thoughts here please?

> On Mon, Apr 16, 2012 at 12:50:24PM -0400, Neil Horman wrote:
> > On Mon, Apr 16, 2012 at 09:35:12AM -0700, Junio C Hamano wrote:
> > > Neil Horman <nhorman@tuxdriver.com> writes:
> > > 
> > > > On Sun, Apr 15, 2012 at 11:39:35AM +0200, Clemens Buchacher wrote:
> > > > ...
> > > >> > +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^
> > > >> > +	}
> > > >> > +'
> > > >> 
> > > >> And the expected result is that the HEAD commit is not removed, right?
> > > >> You should check for that as well.
> > > >> 
> > > >> Also, please checkout empty-branch2^0 first, in order to make the test
> > > >> independent of its predecessor.
> > > >
> > > > Not sure I follow what your saying here.  The expected result with both of these
> > > > tests is that a new commit is created, referencing the current HEAD as the new
> > > > HEAD's parent.
> > > 
> > > If the request were "checkout master^0 first" I would understand.  The
> > > precondition for the second test will be different depending on the first
> > > one succeeds or not.  Perhaps that is what Clemens meant?
> > > 
> > Perhaps, but if so, I'm still not sure how a checkout of empty-branch2^0 affects
> > these tests at all, nor do I grok the relevance to ensuring that the HEAD commit
> > wasn't removed (as AIUI, cherry pick never does that anyway).  Clement, can you
> > clarify your thoughts here please?
> 
> It seems that I was implying a lot more than I realized. What I meant
> was that master and empty-branch2 are equivalent for the purposes of
> that test (empty-branch2^ also is a non-empty commit [*1*]), but while
> master is a moving target, empty-branch2 is untouched. 
> 

> 
> And now I am also wondering why we have two tests for cherry picking an
> empty commit without --allow-empty (the one that you added and the one
> that was there before). Is the non-ff part significant and if so, how?
> And why don't we need to test fast-forward cherry-pick with
> --allow-empty?

> On Mon, Apr 16, 2012 at 11:42:49PM +0200, Clemens Buchacher wrote:
> > 
> > It seems that I was implying a lot more than I realized. What I meant
> > was that master and empty-branch2 are equivalent for the purposes of
> > that test (empty-branch2^ also is a non-empty commit [*1*]), but while
> > master is a moving target, empty-branch2 is untouched. 
> > 
> for the purposes of the --keep-redundant-commits however, the target is
> irrelevant.  The only requirement is that we cherry-pick a commit that is
> guaranteed to become empty when applied.

> > However, I just notice that empty-branch2 is also the root commit, so
> > maybe this will not work after all. But that should be easy to fix.
>
> It is easy to fix, given your clarified description above, its just that IMO,
> its not broken.

> On Tue, Apr 17, 2012 at 06:56:04AM -0400, Neil Horman wrote:
> > On Mon, Apr 16, 2012 at 11:42:49PM +0200, Clemens Buchacher wrote:
> > > 
> > > It seems that I was implying a lot more than I realized. What I meant
> > > was that master and empty-branch2 are equivalent for the purposes of
> > > that test (empty-branch2^ also is a non-empty commit [*1*]), but while
> > > master is a moving target, empty-branch2 is untouched. 
> > > 
> > for the purposes of the --keep-redundant-commits however, the target is
> > irrelevant.  The only requirement is that we cherry-pick a commit that is
> > guaranteed to become empty when applied.
> 
> That we agree on.
> 
> > We certainly could do that on empty branch2, but theres no advantage
> > to doing so,
> 
> The advantage is that I do not have to read the other tests in order to
> understand what this test does, because contrary to the master branch,
> they do not modify empty-branch2.
> 

> > and given that every other test attempts to cherry-pick to master, I
> > rather like the consistency.
> 
> We could also consistently not use the master branch.
> 

> > > However, I just notice that empty-branch2 is also the root commit, so
> > > maybe this will not work after all. But that should be easy to fix.
> >
> > It is easy to fix, given your clarified description above, its just that IMO,
> > its not broken.
> 
> Well, I don't mind too badly if this doesn't go may way. But I hope that
> I managed at least to explain my point.
> 

>
> Given that we agree on our ability to consistently not use master
> above, what if we table this discussion for now, leave the tests as
> they are, and fix them all up in a separate patch set? Does that sound
> reasonable?

> +is_empty_commit() {
> +     ptree=$(git rev-parse "$1"^{tree})
> +     pptree=$(git rev-parse "$1"^^{tree})
> +     return $(test "$ptree" = "$pptree")
> +}
> +

>  pick_one () {
>  	ff=--ff
> +
> +	if is_empty_commit $@
> +	then
> +		empty_args="--allow-empty"
> +	fi
> +
>  	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac

> @@ -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"

> On Fri, Apr 13, 2012 at 02:45:07PM -0400, Neil Horman wrote:
> >
> > 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.
> 
> Thanks. That should be useful.
> 
> > +is_empty_commit() {
> > +     ptree=$(git rev-parse "$1"^{tree})
> > +     pptree=$(git rev-parse "$1"^^{tree})
> > +     return $(test "$ptree" = "$pptree")
> > +}
> > +
> 
> What's the extra leading 'p' for? Any reason not to use 'tree' and
> 'ptree'?
> 

> >  pick_one () {
> >  	ff=--ff
> > +
> > +	if is_empty_commit $@
> > +	then
> > +		empty_args="--allow-empty"
> > +	fi
> > +
> >  	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
> 
> You do not handle the case where pick_one is called with -n. I think you
> need to move the case statement in front and then call is_empty_commit
> $sha1.
> 
> Not that it matters after this change, but in general using $@ without
> quotes looks wrong to my eyes.
> 

> > @@ -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"
> 
> How about setting comment_out="# " or comment_out="" instead and then
> 
>  printf '%s%s\n' "$comment_out" "pick $shortsha1 $rest" >> "$todo"
> 
> That would read more natural to me.

>
> +	if (!empty && !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;

> On Tue, Apr 17, 2012 at 02:20:07PM -0400, Neil Horman wrote:
> >
> > +	if (!empty && !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;
> 
> You can remove one level of indentation (yay!).
>

>
> @@ -780,9 +793,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

> On Tue, Apr 17, 2012 at 02:20:09PM -0400, Neil Horman wrote:
> >
> > @@ -780,9 +793,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
> 
> You forgot to change this to comment_out="# " and comment_out="".
> 

> 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>
> ---

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 5812222..cef290b 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() {
> +	tree=$(git rev-parse "$1"^{tree})
> +	ptree=$(git rev-parse "$1"^^{tree})
> +	return $(test "$tree" = "$ptree")
> +}

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > 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>
> > ---
> 
> The earlier one in the series seem to be getting solid enough.  Nice.
> 

> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 5812222..cef290b 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() {
> > +	tree=$(git rev-parse "$1"^{tree})
> > +	ptree=$(git rev-parse "$1"^^{tree})
> > +	return $(test "$tree" = "$ptree")
> > +}
> 
> Could "$1" ever be a root commit without a parent?
> 

>> > +is_empty_commit() {
>> > +	tree=$(git rev-parse "$1"^{tree})
>> > +	ptree=$(git rev-parse "$1"^^{tree})
>> > +	return $(test "$tree" = "$ptree")
>> > +}
>> 
>> Could "$1" ever be a root commit without a parent?
>> 
> Strictly speaking, yes.  If that happens, however, the output of git rev-parse

> diff --git a/sequencer.c b/sequencer.c
> index 71929ba..e33dfbb 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"
>  
> @@ -251,6 +252,30 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	return !clean;
>  }
>  
> +static int is_index_unchanged()
> +{

> 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>

> On 04/18/2012 09:18 PM, Neil Horman wrote:
>> 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>
>
> Hi,
> this one seems to breaks many tests when /bin/sh=dash. (Both v6 in pu
> and this v7).

>> is_empty_commit() {
>> 	tree=$(git rev-parse "$1"^{tree})
>> 	ptree=$(git rev-parse "$1"^^{tree})
>> 	return $(test "$tree" = "$ptree")
>> }

> Zbigniew Jędrzejewski-Szmek<zbyszek@in.waw.pl>  writes:
>
>> On 04/18/2012 09:18 PM, Neil Horman wrote:
>>> 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>
>>
>> Hi,
>> this one seems to breaks many tests when /bin/sh=dash. (Both v6 in pu
>> and this v7).
>
> Probably because of the strange return in this function:
>
>>> is_empty_commit() {
>>> 	tree=$(git rev-parse "$1"^{tree})
>>> 	ptree=$(git rev-parse "$1"^^{tree})
>>> 	return $(test "$tree" = "$ptree")
>>> }
>
> bash seems to pass on the exit status from $() to the caller, while dash
> doesn't.  It seems bash is actually more correct here, because POSIX
> says about 'return [n]':
>
>      EXIT STATUS
>         The value of the special parameter '?' shall be set to n, an
>         unsigned decimal integer, or to the exit status of the last
>         command executed if n is not specified.
>
> Either way, it should simply be spelled as
>
> is_empty_commit() {
> 	tree=$(git rev-parse "$1"^{tree})
> 	ptree=$(git rev-parse "$1"^^{tree})
> 	test "$tree" = "$ptree"
> }

> On 04/19/2012 01:49 PM, Thomas Rast wrote:
> >Zbigniew Jędrzejewski-Szmek<zbyszek@in.waw.pl>  writes:
> >
> >>On 04/18/2012 09:18 PM, Neil Horman wrote:
> >>>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>
> >>
> >>Hi,
> >>this one seems to breaks many tests when /bin/sh=dash. (Both v6 in pu
> >>and this v7).
> >
> >Probably because of the strange return in this function:
> >
> >>>is_empty_commit() {
> >>>	tree=$(git rev-parse "$1"^{tree})
> >>>	ptree=$(git rev-parse "$1"^^{tree})
> >>>	return $(test "$tree" = "$ptree")
> >>>}
> >
> >bash seems to pass on the exit status from $() to the caller, while dash
> >doesn't.  It seems bash is actually more correct here, because POSIX
> >says about 'return [n]':
> >
> >     EXIT STATUS
> >        The value of the special parameter '?' shall be set to n, an
> >        unsigned decimal integer, or to the exit status of the last
> >        command executed if n is not specified.
> >
> >Either way, it should simply be spelled as
> >
> >is_empty_commit() {
> >	tree=$(git rev-parse "$1"^{tree})
> >	ptree=$(git rev-parse "$1"^^{tree})
> >	test "$tree" = "$ptree"
> >}
> Yes, this change fixes the problem (all tests pass).
> 
> Thanks!
> 
> Zbyszek
> 

> Either way, it should simply be spelled as
>
> is_empty_commit() {
> 	tree=$(git rev-parse "$1"^{tree})
> 	ptree=$(git rev-parse "$1"^^{tree})
> 	test "$tree" = "$ptree"
> }

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> Either way, it should simply be spelled as
>>
>> is_empty_commit() {
>> 	tree=$(git rev-parse "$1"^{tree})
>> 	ptree=$(git rev-parse "$1"^^{tree})
>> 	test "$tree" = "$ptree"
>> }
>
> Thanks; will squash in something like this:
> ...

> Junio C Hamano <jch@google.com> writes:
> 
> > Thomas Rast <trast@student.ethz.ch> writes:
> >
> >> Either way, it should simply be spelled as
> >>
> >> is_empty_commit() {
> >> 	tree=$(git rev-parse "$1"^{tree})
> >> 	ptree=$(git rev-parse "$1"^^{tree})
> >> 	test "$tree" = "$ptree"
> >> }
> >
> > Thanks; will squash in something like this:
> > ...
> 
> Ehh, not like that.  But something like this, as we need to be able to
> pick "root" (t3412 insists on it).
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 82042b1..de71543 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -168,9 +168,11 @@ has_action () {
>  }
>  
>  is_empty_commit() {
> -	tree=$(git rev-parse "$1"^{tree})
> -	ptree=$(git rev-parse "$1"^^{tree})
> -	return $(test "$tree" = "$ptree")
> +	tree=$(git rev-parse "$1"^{tree} 2>/dev/null) ||
> +		die "$1: not a commit that can be picked"
> +	ptree=$(git rev-parse "$1"^^{tree} 2>/dev/null) ||
> +		ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
> +	test "$tree" = "$ptree"
>  }
>  
>  # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
> 

> Looks good, modulo perhaps these fixes on top.
> 
>  - It would be better to say "dropped" to stress that we inspect and
>    actively discard, instead of saying "ignored".
> 
>  - There is no need to use "changeset", the term glossary even
>    discourages, to make sense of the sentence.
> 
>  - There was still a stray keep_if_made_empty.
> 
>  - s/Add keep/add keep/ on the title as well.
> 
> No need to resend, unless you have fixes other than the following does.
> 

> 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.
> ...
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 5812222..ef263e0 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -167,6 +167,15 @@ 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)
> +
> +	return test "$tree" = "$ptree"
> +}

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > 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.
> > ...
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 5812222..ef263e0 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -167,6 +167,15 @@ 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)
> > +
> > +	return test "$tree" = "$ptree"
> > +}
> 
> I've amended the above and removed "return " from the last line.
> 
> The series is now in 'next', so if we need further enhancement or fixup,
> they need to come as incremental updates, not as replacements.
> 
> Thanks.
> 

>  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 $ff "$@"
> +       output git cherry-pick $empty_args $ff "$@"

> On Fri, Apr 20, 2012 at 7:36 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
>>  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 $ff "$@"
>> +       output git cherry-pick $empty_args $ff "$@"
> 
> The is_empty_commit check seems to mean that if $sha1 is an "empty"
> commit, we pass the --allow-empty option to cherry-pick. If it's not
> empty, we don't. The word "allow" in "allow-empty" suggests that even
> if the commit is not empty, cherry-pick would not mind. So, can we
> always pass "allow-empty" to cherry-pick (i.e. even if the commit to
> pick is not empty)?

> Am 7/18/2012 8:20, schrieb Martin von Zweigbergk:
>> On Fri, Apr 20, 2012 at 7:36 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
>>>  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 $ff "$@"
>>> +       output git cherry-pick $empty_args $ff "$@"
>>
>> The is_empty_commit check seems to mean that if $sha1 is an "empty"
>> commit, we pass the --allow-empty option to cherry-pick. If it's not
>> empty, we don't. The word "allow" in "allow-empty" suggests that even
>> if the commit is not empty, cherry-pick would not mind. So, can we
>> always pass "allow-empty" to cherry-pick (i.e. even if the commit to
>> pick is not empty)?
>
> I don't think so. If the commit is not empty, but all its changes are
> already in HEAD, then it will become "empty" when cherry-picked to HEAD.
> In such a case, we usually do not want to record an empty commit, but stop
> rebase to give to user a chance to deal with the situation.

> Am 7/18/2012 8:20, schrieb Martin von Zweigbergk:
> > On Fri, Apr 20, 2012 at 7:36 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> >>  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 $ff "$@"
> >> +       output git cherry-pick $empty_args $ff "$@"
> > 
> > The is_empty_commit check seems to mean that if $sha1 is an "empty"
> > commit, we pass the --allow-empty option to cherry-pick. If it's not
> > empty, we don't. The word "allow" in "allow-empty" suggests that even
> > if the commit is not empty, cherry-pick would not mind. So, can we
> > always pass "allow-empty" to cherry-pick (i.e. even if the commit to
> > pick is not empty)?
> 
> I don't think so. If the commit is not empty, but all its changes are
> already in HEAD, then it will become "empty" when cherry-picked to HEAD.
> In such a case, we usually do not want to record an empty commit, but stop
> rebase to give to user a chance to deal with the situation.
> 
> -- Hannes
>