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