Re: [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked
David Greene <greened@obbligato.org> writes:
Show 26 quoted lines
> From: "David A. Greene" <greened@obbligato.org>
>
> Add a "--skip-redundant-commits" option to cherry-pick to avoid
> aborting if the cherry-picked commit becomes empty due to
> conflict resolution.
>
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
> builtin/revert.c | 7 +++++++
> sequencer.c | 23 +++++++++++++++++++++++
> sequencer.h | 1 +
> 3 files changed, 31 insertions(+)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 56a2c36..befd3ce 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -91,6 +91,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> N_("option for merge strategy"), option_parse_x),
> { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> + /*
> + * There must be enough OPT_END() here to match the
> + * size of cp_extra below so that parse_options_concat
> + * will work.
> + */Show 11 quoted lines
> + OPT_END(),
> OPT_END(),
> OPT_END(),
> OPT_END(),
> @@ -106,6 +112,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
> OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
> OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
> + OPT_BOOL(0, "skip-redundant-commits", &opts->skip_redundant_commits, N_("skip redundant, empty commits")),
> OPT_END(),
> };This however makes me wonder what should happen when both are specified. Shouldn't this patch change the keep_redundant_commits field from a bool to a tristate that tells us what to do with redundant ones? int/enum opts.redundant_commit can take 0 (fail, which would be the default), 1 (keep) or 2 (skip), or something like that.
Show 9 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index 8c58fa2..12361e7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -185,6 +185,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
> else
> advise(_("after resolving the conflicts, mark the corrected paths\n"
> "with 'git add <paths>' or 'git rm <paths>'\n"
> +Show 10 quoted lines
> "and commit the result with 'git commit'"));
> }
> }
> @@ -614,6 +615,28 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> res = allow;
> goto leave;
> }
> +
> + // If told, do not try to commit things that don't make any
> + // changes.
No C++/C99 comments, please.
Show 14 quoted lines
> + if (opts->skip_redundant_commits) {
> + int index_unchanged = is_index_unchanged();
> + if (index_unchanged < 0) {
> + // Something bad happened readhing HEAD or the
> + // index. Abort.
> + res = index_unchanged;
> + goto leave;
> + }
> + if (index_unchanged) {
> + fputs(_("Skipping redundant commit "), stderr);
> + fputs(find_unique_abbrev(commit->object.oid.hash,
> + GIT_SHA1_HEXSZ),
> + stderr);
> + fputs("\n", stderr);This is a bad i18n; we do not know the sentence "Skipping commit X" is translated to have X at the end of the sentence in all languages.
fprintf(stderr, _("Skipping ... %s\n"), find_unique_abbrev(...));would allow it to be tranlated to "Commit X is getting skipped", for example.
Show 17 quoted lines
> + res = 0;
> + goto leave;
> + }
> + }
> +
> if (!opts->no_commit)
> res = run_git_commit(git_path_merge_msg(), opts, allow);
>
> diff --git a/sequencer.h b/sequencer.h
> index 5ed5cb1..ad6145d 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -34,6 +34,7 @@ struct replay_opts {
> int allow_empty;
> int allow_empty_message;
> int keep_redundant_commits;
> + int skip_redundant_commits;Continuing from the top-part of the comments, this may be better to be:
enum {
REPLAY_REDUNDANT_FAIL = 0,
REPLAY_REDUNDANT_KEEP,
REPLAY_REDUNDANT_SKIP
} redundant_commits;or something like that.
>
> int mainline;
David Greene
