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

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

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

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

> > +		if (parse_commit(head_commit) < 0)
> > +			return error(_("could not parse commit %s\n"),
> > +				     sha1_to_hex(head_commit->object.sha1));
> > +
> > +		if (!active_cache_tree) {
> > +			active_cache_tree = cache_tree();
> > +			need_free = 1;
> > +		}
> 
> I think this is wrong.  If for whatever reason the process does not have
> the index it is about to tell "git commit" to use to make a commit in-core,
> it should first be doing read_cache().

> > +
> > +		if (!cache_tree_fully_valid(active_cache_tree))
> > +			cache_tree_update(active_cache_tree, active_cache,
> > +					  active_nr, 0);
> 
> I do not recall offhand if cache_tree_update() can give you an error, but
> if it does, it should be checked here.
> 

> > +		rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
> > +
> > +		if (need_free)
> > +			cache_tree_free(&active_cache_tree);
> 
> and I don't see a need for nuking the cache tree here, unless you are
> discarding the_index as well.
> 

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