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

[Josh Steadmon <steadmon@google.com>]

On 2021.08.10 15:33, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > +	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1},
> 
> No need to resend to only fix this, but I'll add SP after "1" to
> match surrounding lines while queuing the patch.

Ack. Fixed in V3. Do you also want me to rebase this on
ab/retire-advice-config?

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

Switched these to warnings, squelched by "--quiet" as suggested below.


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