threads / discuss / 41409

Custom merge driver with no rename detection

Subject: Custom merge driver with no rename detection

## tl;dr

8 messages between Feb 14, 2016 and Feb 16, 2016.

replies: 7people: 3as markdown or json

Felipe Gonçalves Assis· Feb 14, 2016, 19:51 UTC · lore

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

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

Junio C Hamano· Feb 15, 2016, 05:10 UTC · re: Felipe Gonçalves Assis · lore

Re: Custom merge driver with no rename detection

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.

Johannes Schindelin· Feb 15, 2016, 08:06 UTC · re: Junio C Hamano · lore

Re: Custom merge driver with no rename detection

Hi Junio,

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

Show 10 quoted lines

> 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

Junio C Hamano· Feb 15, 2016, 09:57 UTC · re: Johannes Schindelin · lore

Re: Custom merge driver with no rename detection

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

Show 18 quoted lines

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

Johannes Schindelin· Feb 15, 2016, 08:06 UTC · re: Felipe Gonçalves Assis · lore

Re: Custom merge driver with no rename detection

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

Felipe Gonçalves Assis· Feb 15, 2016, 10:42 UTC · re: Johannes Schindelin · lore

Re: Custom merge driver with no rename detection

On 15 February 2016 at 06:06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 11 quoted lines

> 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

Junio C Hamano· Feb 15, 2016, 11:03 UTC · re: Felipe Gonçalves Assis · lore

Re: Custom merge driver with no rename detection

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.

Felipe Gonçalves Assis· Feb 16, 2016, 01:04 UTC · re: Junio C Hamano · lore

Re: Custom merge driver with no rename detection

On 15 February 2016 at 09:03, Junio C Hamano <gitster@pobox.com> wrote:

Show 31 quoted lines

> 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