threads / rfc / 12400

RFC patchrename: warn user when we have turned off rename detection

Subject: [RFC/PATCH] rename: warn user when we have turned off rename detection

## tl;dr

4 messages between Mar 1, 2008 and Mar 1, 2008. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jeff King· Mar 1, 2008, 06:14 UTC · lore

Signed-off-by: Jeff King <peff@peff.net>
---
This was another patch from late in the freeze period. It was in
response to a user getting confused about why rename detection wasn't
happening in a large merge. Is it appropriate to print this for every
rename we try? Or should it just be for merges?

Perhaps we should also bump the default limit from 100, which I think was just arbitrarily chosen. I posted some numbers in:

  http://mid.gmane.org/20080211113516.GB6344@coredump.intra.peff.net

which imply that 200-400 is probably a more reasonable value. Thoughts?

 diffcore-rename.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

Show changes to diffcore-rename.c +4 −3

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 3d37725..31941bc 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -468,10 +468,11 @@ void diffcore_rename(struct diff_options *options)
 	 */
 	if (rename_limit <= 0 || rename_limit > 32767)
 		rename_limit = 32767;
-	if (num_create > rename_limit && num_src > rename_limit)
-		goto cleanup;
-	if (num_create * num_src > rename_limit * rename_limit)
+	if ((num_create > rename_limit && num_src > rename_limit) ||
+	    (num_create * num_src > rename_limit * rename_limit)) {
+		warning("too many files, skipping inexact rename detection");
 		goto cleanup;
+	}
 
 	mx = xmalloc(sizeof(*mx) * num_create * num_src);
 	for (dst_cnt = i = 0; i < rename_dst_nr; i++) {

-- 
1.5.4.3.422.g55194

Junio C Hamano· Mar 1, 2008, 07:35 UTC · re: Jeff King · lore

Re: [RFC/PATCH] rename: warn user when we have turned off rename detection

Jeff King <peff@peff.net> writes:

Show 18 quoted lines

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This was another patch from late in the freeze period. It was in
> response to a user getting confused about why rename detection wasn't
> happening in a large merge. Is it appropriate to print this for every
> rename we try? Or should it just be for merges?
>
> Perhaps we should also bump the default limit from 100, which I think
> was just arbitrarily chosen.
> ...
> +	if ((num_create > rename_limit && num_src > rename_limit) ||
> +	    (num_create * num_src > rename_limit * rename_limit)) {
> +		warning("too many files, skipping inexact rename detection");
>  		goto cleanup;
> +	}
> 
>  	mx = xmalloc(sizeof(*mx) * num_create * num_src);
>  	for (dst_cnt = i = 0; i < rename_dst_nr; i++) {

This reminds me of the 6d24ad9 (Optimize rename detection for a huge diff) topic that reduces the above allocation greatly. Some benching with the patch may prove useful to establish much higher limits, I suspect.

Jeff King· Mar 1, 2008, 08:18 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] rename: warn user when we have turned off rename detection

On Fri, Feb 29, 2008 at 11:35:00PM -0800, Junio C Hamano wrote:

Show 12 quoted lines

> > +	if ((num_create > rename_limit && num_src > rename_limit) ||
> > +	    (num_create * num_src > rename_limit * rename_limit)) {
> > +		warning("too many files, skipping inexact rename detection");
> >  		goto cleanup;
> > +	}
> > 
> >  	mx = xmalloc(sizeof(*mx) * num_create * num_src);
> >  	for (dst_cnt = i = 0; i < rename_dst_nr; i++) {
> 
> This reminds me of the 6d24ad9 (Optimize rename detection for a huge diff)
> topic that reduces the above allocation greatly.  Some benching with the
> patch may prove useful to establish much higher limits, I suspect.

Actually, my timings are about the same. While the patch you mentioned is nice for reducing the memory consumption, we still have an O(n^2) loop.

-Peff

Junio C Hamano· Mar 1, 2008, 09:30 UTC · re: Jeff King · lore

Re: [RFC/PATCH] rename: warn user when we have turned off rename detection

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

>> This reminds me of the 6d24ad9 (Optimize rename detection for a huge diff)
>> topic that reduces the above allocation greatly.  Some benching with the
>> patch may prove useful to establish much higher limits, I suspect.
>
> Actually, my timings are about the same. While the patch you mentioned
> is nice for reducing the memory consumption, we still have an O(n^2)
> loop.

Yeah, that one was not about the time but about the space, so it is expected.