Re: [PATCH] diff: reuse diff setup for --no-index case

[Jeff King <peff@peff.net>]

On Sat, Feb 16, 2019 at 01:57:56AM -0500, Jeff King wrote:

> On Thu, Feb 14, 2019 at 11:47:02AM -0800, Junio C Hamano wrote:
> 
> > > +	if (no_index)
> > > +		/* If this is a no-index diff, just run it and exit there. */
> > > +		diff_no_index(&rev, argc, argv);
> > > +
> > >  	if (nongit)
> > >  		die(_("Not a git repository"));
> > >  	argc = setup_revisions(argc, argv, &rev, NULL);
> > 
> > To summarize, I would suspect that two further improvements on this
> > patch are:
> > 
> >  (1) move "Otherwise" comment to the right place
> > 
> >  (2) make the two assignments that are irrelevant to "--no-index"
> >      after we jumped to diff_no_index().
> > 
> > The latter is optional, but may be good for code health by making
> > developers _think_ if an option is applicable to "--no-index" mode.
> > I dunno.
> 
> Yeah, I very much agree with (1). For (2), I intentionally left it as
> one mixed block, because I didn't want people to accidentally add new
> setup lines to the wrong block. But maybe with sufficient comments, it
> would be clear (and even make the code flow a bit more obvious).
> 
> Here's an attempt at that.  I did drop a few comments that seemed
> pointless to me, as they're just re-stating the lines they describe.

It looks like the original got merged to next. I think we at least need
to deal with the "Otherwise..." comment, but I think the layout I posted
in my followup is nicer overall. Was it a mistake to merge the first
one?

If so, do you want an incremental, or do you just want to drop it and
redo as part of the post-2.21 rewind?

-Peff