git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Feb 24, 2019, 09:45 UTC
Message-ID
<20190224094545.GA11538@sigill.intra.peff.net>
In-Reply-To
<20190216065755.GB27641@sigill.intra.peff.net>
On Sat, Feb 16, 2019 at 01:57:56AM -0500, Jeff King wrote:
Show 29 quoted lines
> 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
Previous: Jeff KingNext: Junio C Hamano
Message 5 of 6 in “External tool is ignored when using difftool with --no-index”
  1. Sylvain LacazeFeb 13, 2019
  2. diff: reuse diff setup for --no-index caseJeff King, Feb 13, 2019
  3. Junio C HamanoFeb 14, 2019
  4. Jeff KingFeb 16, 2019
  5. Jeff KingFeb 24, 2019
  6. Junio C HamanoFeb 24, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.