Re: [PATCHv2 3/3] Teach --dirstat to not completely ignore rearranged lines within a file

From: Johan Herland <johan@herland.net>
Date: Apr 11, 2011, 21:56 UTC
Message-ID: <201104112356.09408.johan@herland.net>
In-Reply-To: <7vtye4tqmk.fsf@alter.siamese.dyndns.org>

On Monday 11 April 2011, Junio C Hamano wrote:

Show 8 quoted lines

> Johan Herland <johan@herland.net> writes:
> > Currently, the --dirstat analysis fails to detect when lines within a
> > file are rearranged, because the "damage" calculated by show_dirstat()
> > is 0. However, if the SHA1 sum has changed, we already now that there
> > should be at least some minimum amount of damage.
> 
> This logic is sensible, modulo that "fails to detect" is actually
> "ignores mere line movements on purpose".

I apologize for my commit message not having caught up with discussion around this issue. I came into the discussion from the POV of "--dirstat does not pick up what --stat picks up; there must be a bug in --dirstat", and my original objective was therefore to "fix" --dirstat to be "more like --stat". Obviously, I now know exactly why --dirstat is different, and that we don't want to fundamentally change it. My commit message should have been rephrased in a more positive light as a result. Feel free to fix before applying.

> In any case, if the object names are different, we already know that
> there is _some_ damage, and it is very unintiutive to claim that there
> is _no_ damage.

Agreed.

Show 12 quoted lines

> > This patch teaches show_dirstat() to assign a minimum amount of damage
> > (== 1) to entries for which the analysis otherwise yields zero damage.
> 
> So it is perfectly in line with the above logic to give a minimum here.
> Zero was simply just unintuitive, and this is a good fix to the problem.
> 
> > Obviously this is not a complete fix, but it's at least better to
> 
> I however do not understand what "a complete fix" means in this context.
> You've fixed the unintuitiveness, and as far as the description in the
> introductory paragraph of the problem goes, I think this already is a
> complete fix.

I still feel that a file with 1000 rearranged lines should somehow count "more" than a file with only 1 rearranged line, but it's hard to get there without futzing with diffcore_count_changes(), probably making the whole thing considerably more expensive... So in that sense, I agree that the current solution is probably as complete as we can get.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net