Custom merge driver with no rename detection

Custom merge driver with no rename detection

[Felipe Gonçalves Assis]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

Hi,

I would like to set up a Git repository with a custom merge driver,
and then disable rename detection when merging.

Unfortunately, the recursive strategy has no "no-renames" option. Note
that I would like to avoid rename detection even when the file
contents perfectly match.

The usual workaround is using the resolve strategy, but apparently it
ignores the custom merge driver. Besides, I would really like to use
the recursive strategy logic.

Is there any solution to this in the current version?

If not, what do you think about adding a "no-renames" option to the
recursive strategy? I could work on a patch.

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

And here is a related question on StackOverflow:
http://stackoverflow.com/questions/35135517/custom-git-merge-driver-with-no-rename-detection

Thanks in advance for any attention provided.

Regards,
Felipe Gonçalves Assis

[-- Attachment #2: 0001-diff-rename-threshold-above-100-means-no-renames.patch --]
[-- Type: text/x-patch, Size: 1208 bytes --]

From bcef6c44fac3a29afe03408ef27024776da861ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felipe=20Gon=C3=A7alves=20Assis?= <felipegassis@gmail.com>
Date: Sun, 14 Feb 2016 17:02:00 -0200
Subject: [PATCH] diff: rename threshold above 100% means no renames

---
 diff.c            | 2 +-
 diffcore-rename.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 2136b69..43b9e0a 100644
--- a/diff.c
+++ b/diff.c
@@ -4003,7 +4003,7 @@ int parse_rename_score(const char **cp_p)
 	/* 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);
 }
 
 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)
 		goto cleanup; /* nothing to do */
 
 	/*
-- 
2.7.1.287.g4943984

Re: Custom merge driver with no rename detection

[Junio C Hamano]

Felipe Gonçalves Assis <felipeg.assis@gmail.com> writes:

> The usual workaround is using the resolve strategy, but apparently it
> ignores the custom merge driver.

Hmph.

Indeed, git-merge-file seems to call xdl_merge() directly, bypassing
the ll_merge(), which is understandable as the former predates the
latter.  That needs to be fixed, I think.

Re: Custom merge driver with no rename detection

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

Hi Junio,

On Sun, 14 Feb 2016, Junio C Hamano wrote:

> Felipe Gonçalves Assis <felipeg.assis@gmail.com> writes:
> 
> > The usual workaround is using the resolve strategy, but apparently it
> > ignores the custom merge driver.
> 
> Hmph.
> 
> Indeed, git-merge-file seems to call xdl_merge() directly, bypassing
> the ll_merge(), which is understandable as the former predates the
> latter.  That needs to be fixed, I think.

I think this is by design. (Because I designed it.)

The original idea of git-merge-file was to serve as a drop-in replacement
for GNU/BSD merge when you want to avoid to be subject to the vagaries of
the GNU vs BSD implementations.

Ciao,
Dscho

Re: Custom merge driver with no rename detection

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 14 Feb 2016, Junio C Hamano wrote:
>
>> Felipe Gonçalves Assis <felipeg.assis@gmail.com> writes:
>> 
>> > The usual workaround is using the resolve strategy, but apparently it
>> > ignores the custom merge driver.
>> 
>> Hmph.
>> 
>> Indeed, git-merge-file seems to call xdl_merge() directly, bypassing
>> the ll_merge(), which is understandable as the former predates the
>> latter.  That needs to be fixed, I think.
>
> I think this is by design. (Because I designed it.)
>
> The original idea of git-merge-file was to serve as a drop-in replacement
> for GNU/BSD merge when you want to avoid to be subject to the vagaries of
> the GNU vs BSD implementations.

We did use to use "merge" from the RCS suite originally, and we did
want to use our own, but the primary reason we added our own was so
that it can be enhanced in sync with the remainder of Git in a
consistent way.

If the rest of Git can be told via the attribute system to make use
of a three-way merge driver, it should have learnt the trick to be
kept up with the rest of the system.  I see the current state of the
program merely be staying a bit behind; I do not think it was part
of the grand design to forbidd it forever from learning new optional
tricks.

Re: Custom merge driver with no rename detection

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1401 bytes --]

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.

>         /* 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.

>  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

Re: Custom merge driver with no rename detection

[Felipe Gonçalves Assis]

On 15 February 2016 at 06:06, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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.
>

Thanks. I am sorry for not submitting the patch separately in an
email, but, it was really not meant for serious review, just for
emulating the desired effect with minimal effort.

However, if you do find this approach acceptable/desirable
(rename-threshold > 100%), I can work on the issues pointed out and
propose a proper patch.

Regards,
Felipe

Re: Custom merge driver with no rename detection

[Junio C Hamano]

Felipe Gonçalves Assis <felipeg.assis@gmail.com> writes:

> However, if you do find this approach acceptable/desirable
> (rename-threshold > 100%), I can work on the issues pointed out and
> propose a proper patch.

The caller asks diffcore-rename to detect rename, and the algorithm
compares things to come up with a similarity score and match things
up.  And you add an option to the rename detection logic to forbid
finding any?

To be bluntly honest, the approach sounds like a crazy talk.

If your goal is not to allow rename detection at all, why not teach
the caller of the diff machinery not to even ask for rename
detection at all?  merge-recursive.c has a helper function called
get_renames(), and it calls into the diff machinery passing
DIFF_DETECT_RENAME option.  As a dirty hack, I think you would
achieve your desired result if you stop passing that option there.

I called that a "dirty hack", because for the purpose of not
allowing rename detection inside merge-recursive, I think an even
better thing to do is to teach the get_renames() helper to report to
its caller, under your new option, "I found no renames at all"
without doing anything.

It might be just a simple matter of teaching its callers (there
probably are two of them, one between the common ancestor and our
branch and the other between the common ancestor and their branch)
not call the get_renames() helper at all under your new option, but
I didn't check these callers closely.

Re: Custom merge driver with no rename detection

[Felipe Gonçalves Assis]

On 15 February 2016 at 09:03, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Gonçalves Assis <felipeg.assis@gmail.com> writes:
>
>> However, if you do find this approach acceptable/desirable
>> (rename-threshold > 100%), I can work on the issues pointed out and
>> propose a proper patch.
>
> The caller asks diffcore-rename to detect rename, and the algorithm
> compares things to come up with a similarity score and match things
> up.  And you add an option to the rename detection logic to forbid
> finding any?
>
> To be bluntly honest, the approach sounds like a crazy talk.
>
> If your goal is not to allow rename detection at all, why not teach
> the caller of the diff machinery not to even ask for rename
> detection at all?  merge-recursive.c has a helper function called
> get_renames(), and it calls into the diff machinery passing
> DIFF_DETECT_RENAME option.  As a dirty hack, I think you would
> achieve your desired result if you stop passing that option there.
>
> I called that a "dirty hack", because for the purpose of not
> allowing rename detection inside merge-recursive, I think an even
> better thing to do is to teach the get_renames() helper to report to
> its caller, under your new option, "I found no renames at all"
> without doing anything.
>
> It might be just a simple matter of teaching its callers (there
> probably are two of them, one between the common ancestor and our
> branch and the other between the common ancestor and their branch)
> not call the get_renames() helper at all under your new option, but
> I didn't check these callers closely.

Thanks for all the ideas. In order to have something concrete to
discuss, I submitted the patch "merge-recursive: option to disable
renames".

Is that what you had in mind? I would be happy to receive comments and
either improve it or try something else.

Felipe