On Mon, Jun 5, 2017 at 6:10 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> writes: > >>> "git diff" has been taught to optionally paint new lines that are >>> the same as deleted lines elsewhere differently from genuinely new >>> lines. >>> >>> Are we happy with these changes? >> >> I advertised this series e.g. for reviewing Brandons >> repo object refactoring series and used it myself to inspect >> some patches there[1]. I am certainly happy (but biased) with >> what we have available there. >> >> Jacob intended to use this series >> for review as well, but has given no opinion yet. >> >> You seemed to have used it for js/blame-lib? >> >> -- >> Those patches had a wide reviewer audience cc'd, >> so I would think people are aware of this series. > > I tried to, yes. I haven't had a chance to see how well the current > iteration fares "does the externally-visible goal make sense?" test. > > I do not think I saw a negative "an approach to show this kind of > output would not be useful" reaction, so I assume at least people > would want an alternative output format that would help reviewing a > change that moves blocks of lines around. > > In any case, that is not a review. A patch series wanting to do a > good thing, and people agreeing that the externally visible effect > it produces matches that good thing, is one thing. A review that > makes sure the code achieves the externally visible effect well > (e.g. without overly inefficient algorithm, without buffer overflows > or underflows, off-by-ones, etc.) is another thing, and I haven't > seen anybody going with fine toothed comb to do that kind of review, > hence my "are we happy?" inquiry. > I will try to find some time tomorrow to go over it in detail. Thanks, Jake