Re: [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked

Junio C Hamano [off-list ref] writes:

quoted

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

This makes good sense.

quoted

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

???

Oops.  :)

quoted

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

Will fix.

quoted

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

Ok, thank you for the guidance.

quoted

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.

Agreed.

I've also resumed work on my earlier rebase --keep-redundant-commits
change.  I think I'm going to reorganize things and send the cherry-pick
changes separate from the rebase changes since the latter depends on the
former.  Then all of the redundant commit work on rebase can be in one
series for review.

                        -David