Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option
Neil Horman <nhorman@tuxdriver.com> writes:
Show 27 quoted lines
> 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 toIt 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.
Show 5 quoted lines
> +--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`
Show 64 quoted lines
> 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.
Show 8 quoted lines
> + 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.
Show 30 quoted lines
> + 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?
Neil Horman