Re: Custom merge driver with no rename detection

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 15, 2016, 08:06 UTC
Message-ID: <alpine.DEB.2.20.1602150858340.2964@virtualbox>
In-Reply-To: <CALMa68ovz=VZYkCcUDvEn1d7=xJDx__71caqsPXUFASZ1phfdw@mail.gmail.com>

Hi Felipe,

On Sun, 14 Feb 2016, Felipe Gonçalves Assis wrote:

> Attached is a quick and dirty patch that emulates the effect by
> allowing greater than 100% rename thresholds to mean "no-renames".

It is really hard to comment on attached patches.

First comment: the commit message is awfully empty, and lacks a sign-off.

Show 5 quoted lines

>         /* user says num divided by scale and we say internally that
>          * is MAX_SCORE * num / scale.
>          */
> -       return (int)((num >= scale) ? MAX_SCORE : (MAX_SCORE * num / scale));
> +       return (int)(MAX_SCORE * num / scale);

Uh oh. I suspect this opens the door pretty wide for integer overflows. I could imagine that something like

	return (int)(num > scale ? MAX_SCORE + 1 : MAX_SCORE * num / scale);

would work better, but it still would need some careful consideration.

Show 11 quoted lines

>  static int diff_scoreopt_parse(const char *opt)
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index af1fe08..7cb5a3b 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -497,7 +497,7 @@ void diffcore_rename(struct diff_options *options)
>                         register_rename_src(p);
>                 }
>         }
> -       if (rename_dst_nr == 0 || rename_src_nr == 0)
> +       if (rename_dst_nr == 0 || rename_src_nr == 0 || minimum_score > MAX_SCORE)

This line is too long now.

Ciao, Johannes