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

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

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

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

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

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