Re: [PATCH 1/2] diffcore-break: don't divide by zero

John Keeping [off-list ref] writes:

When the source file is empty, the calculation of the merge score
results in a division by zero.  Since the merge score is initialized to
zero, it makes sense to just leave it as it is if the source size is
zero.  This means that we still use the extent of damage metric to
decide whether to break the filepair.

Well spotted.  An empty blob to another blob that is larger than 400
bytes will trigger div0, and it makes sense to leave merge-score to
0 (i.e. do not show as whole-delete-then-whole-add after rename
detection is done and the broken filepair is merged back).

Actually, if src->size is 0, we probably shouldn't break the filepair
in the first place.  That is, if your preimage and postimage looked
like these:


     == preimage ==		== postimage ==

     F (empty file)		F (a large file)
			        E (a new empty file)

do we want to see F renamed to E and then a new file created as F
while running "git diff -B -M"?  I doubt it.

So in that sense, this might be a better solution.  I dunno.

 diffcore-break.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/diffcore-break.c b/diffcore-break.c
index 44f8678..eabafd5 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c

@@ -67,6 +67,8 @@ static int should_break(struct diff_filespec *src,
 	max_size = ((src->size > dst->size) ? src->size : dst->size);
 	if (max_size < MINIMUM_BREAK_SIZE)
 		return 0; /* we do not break too small filepair */
+	if (src->size == 0)
+		return 0; /* we do not let empty files get renamed */
 
 	if (diffcore_count_changes(src, dst,
 				   &src->cnt_data, &dst->cnt_data,

On Tue, Apr 02, 2013 at 02:15:17PM -0700, Junio C Hamano wrote:

John Keeping [off-list ref] writes:

quoted

When the source file is empty, the calculation of the merge score
results in a division by zero.  Since the merge score is initialized to
zero, it makes sense to just leave it as it is if the source size is
zero.  This means that we still use the extent of damage metric to
decide whether to break the filepair.

Well spotted.  An empty blob to another blob that is larger than 400
bytes will trigger div0, and it makes sense to leave merge-score to
0 (i.e. do not show as whole-delete-then-whole-add after rename
detection is done and the broken filepair is merged back).

Actually, if src->size is 0, we probably shouldn't break the filepair
in the first place.  That is, if your preimage and postimage looked
like these:


     == preimage ==		== postimage ==

     F (empty file)		F (a large file)
			        E (a new empty file)

do we want to see F renamed to E and then a new file created as F
while running "git diff -B -M"?  I doubt it.

So in that sense, this might be a better solution.  I dunno.

I considered this, but didn't quite understand the code well enough to
be sure that was the right thing to do.  Does it make sense to bail out
early if dst->size is zero as well?

The message for commit 6dd4b66 (Fix diffcore-break total breakage)
indicates that "don't bother to break small files" is wrong in some
cases, but it I wonder if "don't bother to break empty files" is okay.