Re: Custom merge driver with no rename detection
On 15 February 2016 at 09:03, Junio C Hamanowrote: > Felipe Gonçalves Assis 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Custom merge driver with no rename detection
Felipe Gonçalves Assiswrites: > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Custom merge driver with no rename detection
On 15 February 2016 at 06:06, Johannes Schindelinwrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Custom merge driver with no rename detection
Johannes Schindelinwrites: > On Sun, 14 Feb 2016, Junio C Hamano wrote: > >> Felipe Gonçalves Assis 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Custom merge driver with no rename detection
Hi Junio, On Sun, 14 Feb 2016, Junio C Hamano wrote: > Felipe Gonçalves Assiswrites: > > > 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
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 Assiswrites: > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Custom merge driver with no rename detection
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?= <felipegas...@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