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
