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" algorithm. "compaction" is Git 2.9.0 >> with the `--compaction-heuristic` option "indent" is an earlier, > > s/option/&./ Thanks, will change. >> static int diff_detect_rename_default; >> +static int diff_indent_heuristic; /* experimental */ >> static int diff_compaction_heuristic; /* experimental */ > > 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 > > does. But right now: > > git config diff.compactionHeuristic true > git show --indent-heuristic > > still prefers the compaction heuristic, which I think is objectively > wrong. I wasn't worrying about that yet, given that these two features are both still experimental. I also have a strong inkling that at most one of them needs to be made permanent. I propose that I repair the semantics in the simplest way possible for now while we decide on the long-term plan, which might conceivably be: * keep both options permanently * keep only one option permanently * choose one heuristic and use it always (i.e., make it part of the new standard one-and-only diff algorithm) * discard both heuristics (I hope not!) After we've decided on that, *then* let's decide on a suitable UI and implement it before we declare either feature non-experimental. > [...] > Speaking of absurd amounts of work, I was curious if there was a > noticeable performance penalty for using this heuristic [...] I included some performance numbers in my response to Junio [1]. >> +#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 RELATIVE_INDENT_HAS_BLANK_BONUS 15 >> +#define RELATIVE_OUTDENT_BONUS -19 >> +#define RELATIVE_OUTDENT_HAS_BLANK_BONUS 2 >> +#define RELATIVE_DEDENT_BONUS -63 >> +#define RELATIVE_DEDENT_HAS_BLANK_BONUS 50 > > I see there is a comment below here mentioning that these are empirical > voodoo, but it might be worth one at the top (or just moving these below > the comment) because the comment looks like it's just associated with > the function (and these are sufficiently bizarre that anybody reading is > going to double-take on them). Good idea. >> + return 10 * score - bonus; > > I don't mind this not "10" not being a #define constant, but after > reading the exchange between you and Stefan, I think it would be nice to > describe what it is in a comment. The rest of the function is commented > so nicely that this one left me thinking "huh?" upon seeing the "10". Done. Thanks for your review. Michael [1] http://public-inbox.org/git/5fe0edbc-3659-058f-3328-639d1343fa05@xxxxxxxxxxxx/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html