Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

Stefan Beller writes: > Maybe we can enable Michaels heuristic with the same > config/command line flag, i.e. "the flag changes its algorithm"? I think that is a very sensible proposal. After all, the name diff.compactionHeuristic only tells us what part of the diff process

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

> Is there a case where the compaction heuristic produces a better result > than this indent heuristic? AFAICT, you have not found one, and I'd be > surprised if there is one, because this _seems_ like a superset > generally. I suppose there is always the possibility that the empirical > knobs

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Sat, Aug 13, 2016 at 8:59 AM, Junio C Hamano wrote: > Jeff King writes: > >> So assuming everything I just said isn't complete bollocks, I think we >> can move to a future where nobody uses the compaction heuristic. And >> there are three ways to deal with

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

Jeff King writes: > So assuming everything I just said isn't complete bollocks, I think we > can move to a future where nobody uses the compaction heuristic. And > there are three ways to deal with that: > > 1. The knob and feature stay. It might be useful for somebody who >

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Sat, Aug 13, 2016 at 01:25:05AM +0200, Michael Haggerty wrote: > > These two flags are mutually exclusive in the xdiff code, so we should > > probably handle that here. > > > > TBH, I do not care that much what: > > > > [diff] > > compactionHeuristic = true > > indentHeuristic = true >

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On 08/04/2016 06:55 PM, Stefan Beller wrote: > [...] > I have just reread the scoring function and I think you could pull out the > `score=indent` assignment (it is always assigned except for indent <0) > > if (indent == -1) >score = 0; > else >

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On 08/04/2016 09:52 PM, Junio C Hamano wrote: > Michael Haggerty writes: >> +#define START_OF_FILE_BONUS 9 >> +#define END_OF_FILE_BONUS 46 >> +#define TOTAL_BLANK_WEIGHT 4 >> +#define PRE_BLANK_WEIGHT 16 >> +#define RELATIVE_INDENT_BONUS -1 >> +#define

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On 08/04/2016 09:56 AM, Jeff King wrote: > On Thu, Aug 04, 2016 at 12:00:36AM +0200, Michael Haggerty wrote: > >> This table shows the number of diff slider groups that were positioned >> differently than the human-generated values, for various repositories. >> "default" is the default "git diff"

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

Michael Haggerty writes: >> After >> all, somebody in this file is already scanning each and every line >> to see where it ends to split the input into records, so perhaps a >> "right" (if the "theoretical correctness" of the return value from >> this function mattered,

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On 08/04/2016 02:04 AM, Stefan Beller wrote: > On Wed, Aug 3, 2016 at 4:30 PM, Michael Haggerty wrote: >> Stefan Beller wrote: >>> [...] >>> Rather the 10 describes the ratio of "advanced magic" to pure indentation >>> based scoring in my understanding. >> >> No, it's

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On 08/04/2016 09:39 PM, Junio C Hamano wrote: > Michael Haggerty writes: > + } + /* +* We have reached the end of the line without finding any non-space +* characters; i.e., the whole line consists of trailing spaces,

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

Michael Haggerty writes: I agree with Peff about "comment on the voodoo upfront". > +#define START_OF_FILE_BONUS 9 > +#define END_OF_FILE_BONUS 46 > +#define TOTAL_BLANK_WEIGHT 4 > +#define PRE_BLANK_WEIGHT 16 > +#define RELATIVE_INDENT_BONUS -1 > +#define

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

Stefan Beller writes: > I have just reread the scoring function and I think you could pull out the > `score=indent` assignment (it is always assigned except for indent <0) > > if (indent == -1) >score = 0; > else >score =

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

Michael Haggerty writes: >>> + } >>> + /* >>> +* We have reached the end of the line without finding any non-space >>> +* characters; i.e., the whole line consists of trailing spaces, >>> which we >>> +* are not interested in. >>> +

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Thu, Aug 4, 2016 at 12:56 AM, Jeff King wrote: > On Thu, Aug 04, 2016 at 12:00:36AM +0200, Michael Haggerty wrote: > >> This table shows the number of diff slider groups that were positioned >> differently than the human-generated values, for various repositories. >> "default"

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Thu, Aug 04, 2016 at 12:00:36AM +0200, Michael Haggerty wrote: > This table shows the number of diff slider groups that were positioned > differently than the human-generated values, for various repositories. > "default" is the default "git diff" algorithm. "compaction" is Git 2.9.0 > with the

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Wed, Aug 3, 2016 at 3:36 PM, Michael Haggerty wrote: > On 08/04/2016 12:29 AM, Jacob Keller wrote: >> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty >> wrote: >> It seems odd to be that a line with "199" spaces and nothing else will >> return

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Wed, Aug 3, 2016 at 4:30 PM, Michael Haggerty wrote: > This is an important point for the optimization, but less so for the > implementation of the heuristic here. I was dynamically optimizing the > ten other variables, and everything that goes into the bonus includes >

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On 08/04/2016 12:51 AM, Stefan Beller wrote: > On Wed, Aug 3, 2016 at 3:41 PM, Michael Haggerty wrote: >> On 08/04/2016 12:30 AM, Stefan Beller wrote: >>> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty >>> wrote: >>> +return 10 * score

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Wed, Aug 3, 2016 at 3:41 PM, Michael Haggerty wrote: > On 08/04/2016 12:30 AM, Stefan Beller wrote: >> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty >> wrote: >> >>> +return 10 * score - bonus; >> >> Would it make sense to define-ify the

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On 08/04/2016 12:30 AM, Stefan Beller wrote: > On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty wrote: > >> +return 10 * score - bonus; > > Would it make sense to define-ify the 10 as well > as this is the only hardcoded number in the > scoring function? I started

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On 08/04/2016 12:29 AM, Jacob Keller wrote: > On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty wrote: >> +/* >> + * If a line is indented more than this, get_indent() just returns this >> value. >> + * This avoids having to do absurd amounts of work for data that are not >>

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty wrote: > +return 10 * score - bonus; Would it make sense to define-ify the 10 as well as this is the only hardcoded number in the scoring function? -- To unsubscribe from this list: send the line "unsubscribe git" in

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty wrote: > +/* > + * If a line is indented more than this, get_indent() just returns this > value. > + * This avoids having to do absurd amounts of work for data that are not > + * human-readable text, and also ensures that the

[PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

Some groups of added/deleted lines in diffs can be slid up or down, because lines at the edges of the group are not unique. Picking good shifts for such groups is not a matter of correctness but definitely has a big effect on aesthetics. For example, consider the following two diffs. The first is