Re: [PATCH v2] sequencer: advise if skipping cherry-picked commit

[Phillip Wood <phillip.wood123@gmail.com>]

On 10/08/2021 23:33, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>> @@ -5149,8 +5150,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>>   		oidset_insert(&interesting, &commit->object.oid);
>>   
>>   		is_empty = is_original_commit_empty(commit);
>> -		if (!is_empty && (commit->object.flags & PATCHSAME))
>> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +					_("skipped previously applied commit %s"),
>> +					short_commit_name(commit));
>> +			skipped_commit = 1;
>>   			continue;
>> +		}
>>   		if (is_empty && !keep_empty)
>>   			continue;
>>   
>> @@ -5214,6 +5220,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>>   		oidcpy(&entry->entry.oid, &commit->object.oid);
>>   		oidmap_put(&commit2todo, entry);
>>   	}
>> +	if (skipped_commit)
>> +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +				  _("use --reapply-cherry-picks to include skipped commits"));
> 
> I agree with the change in this hunk that advanced users may want to
> squelch this "what to do" hint.
> 
> I am not sure about the earlier hunk that reports when some commits
> have actually been skipped.  When --no-reapply-cherry-picks is in
> effect, the user is expecting that some commits are cherry-picks
> among other (hopefully the majority of) commits, and even those
> users who do not want to be taught how to use the command would want
> to learn the fact that some commits were skipped (and which ones).
> 
> Using two separate advice configuration variables feel way overkill
> for this.  I wonder if the previous hunk should use warning(), i.e.
> 
>> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +			warning(_("skipped previously applied commit %s"),
>> +				short_commit_name(commit));
>> +			skipped_commit = 1;
>>   			continue;
>> +		}
> 
> possibly squelched by "git rebase --quiet".

I think that would be nicer. Using advise_if_enabled() in the second 
hunk makes sense but I think a printing "warning" rather than a "hint" 
in the first hunk.

>> @@ -5369,8 +5379,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>>   	while ((commit = get_revision(&revs))) {
>>   		int is_empty = is_original_commit_empty(commit);
>>   
>> -		if (!is_empty && (commit->object.flags & PATCHSAME))
>> +		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>> +			advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +					  _("skipped previously applied commit %s"),
>> +					  short_commit_name(commit));
>> +			skipped_commit = 1;
>>   			continue;
>> +		}
> 
> Likewise.  I wonder why we have two so-much-similar codepaths that
> we need to touch, though.

--recreate-merges implemented the main loop for its todo list generation 
in a new function as it is much more involved that for the linear case.

Best Wishes

Phillip

> 
>>   		if (is_empty && !keep_empty)
>>   			continue;
>>   		strbuf_addf(out, "%s %s ", insn,
>> @@ -5380,6 +5395,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>>   			strbuf_addf(out, " %c empty", comment_line_char);
>>   		strbuf_addch(out, '\n');
>>   	}
>> +	if (skipped_commit)
>> +		advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS,
>> +				  _("use --reapply-cherry-picks to include skipped commits"));
>>   	return 0;
>>   }