{"thread":{"id":"40792","subject":"[RFC] rename detection: allow more renames","startedAt":"2015-11-13T16:35:06Z","lastAt":"2015-11-30T18:25:18Z","messageCount":5,"participants":["Andreas Krey","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"273282","messageId":"20151113163506.GD16219@inner.h.apk.li","threadId":"40792","inReplyTo":null,"subject":"[RFC] rename detection: allow more renames","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2015-11-13T16:35:06Z","receivedAt":"2015-11-13T16:35:06Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"Hi all,\n\nwe also ran into the maximum rename limit\nin the rename detection. (Yes, we get a lot\nof rename candidates when cherry-picking\nbetween two specific releases.)\n\nThe code talks about limiting the size\nof the rename matrix, but as far as I\ncan see, the matrix itself never exists\nas such, and the only thing that could\nactually overflow is the computation\nfor the progress indication. This\ncan be fixed by reporting just the\ndestinations checked instead of the\ncombinations, since we only update\nthe progress once per destination\nanyway.\n\nIf the direction is good, I will\nshape it up (and obtain the\nproper signoffs).\n\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 749a35d..279f24e 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -368,7 +368,7 @@ static int too_many_rename_candidates(int num_create,\n \tint rename_limit = options->rename_limit;\n \tint num_src = rename_src_nr;\n \tint i;\n-\n+\tstatic int max_rename_limit = 100000;\n \toptions->needed_rename_limit = 0;\n \n \t/*\n@@ -380,8 +380,8 @@ static int too_many_rename_candidates(int num_create,\n \t * but handles the potential overflow case specially (and we\n \t * assume at least 32-bit integers)\n \t */\n-\tif (rename_limit <= 0 || rename_limit > 32767)\n-\t\trename_limit = 32767;\n+\tif (rename_limit <= 0 || rename_limit > max_rename_limit)\n+\t\trename_limit = max_rename_limit;\n \tif ((num_create <= rename_limit || num_src <= rename_limit) &&\n \t    (num_create * num_src <= rename_limit * rename_limit))\n \t\treturn 0;\n@@ -515,7 +515,7 @@ void diffcore_rename(struct diff_options *options)\n \tif (options->show_rename_progress) {\n \t\tprogress = start_progress_delay(\n \t\t\t\t_(\"Performing inexact rename detection\"),\n-\t\t\t\trename_dst_nr * rename_src_nr, 50, 1);\n+\t\t\t\trename_dst_nr, 50, 1);\n \t}\n \n \tmx = xcalloc(num_create * NUM_CANDIDATE_PER_DST, sizeof(*mx));\n@@ -552,7 +552,7 @@ void diffcore_rename(struct diff_options *options)\n \t\t\tdiff_free_filespec_blob(two);\n \t\t}\n \t\tdst_cnt++;\n-\t\tdisplay_progress(progress, (i+1)*rename_src_nr);\n+\t\tdisplay_progress(progress, (i+1));\n \t}\n \tstop_progress(&progress);\n\n\nAnd we would also like to see progress when doing\na cherry pick - in our case this takes a few minutes:\n\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 3c060e0..8fce028 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -282,6 +282,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \n \tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n \t\tparse_merge_opt(&o, *xopt);\n+\to.show_rename_progress = isatty(2);\n \n \tclean = merge_trees(&o,\n \t\t\t    head_tree,\n\n\nAndreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds <torvalds@*.org>\nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"273673","messageId":"20151124233328.GA13872@sigill.intra.peff.net","threadId":"40792","inReplyTo":"20151113163506.GD16219@inner.h.apk.li","subject":"Re: [RFC] rename detection: allow more renames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-24T23:33:28Z","receivedAt":"2015-11-24T23:33:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 13, 2015 at 05:35:06PM +0100, Andreas Krey wrote:\n\n> The code talks about limiting the size\n> of the rename matrix, but as far as I\n> can see, the matrix itself never exists\n> as such, and the only thing that could\n> actually overflow is the computation\n> for the progress indication. This\n> can be fixed by reporting just the\n> destinations checked instead of the\n> combinations, since we only update\n> the progress once per destination\n> anyway.\n\nI didn't dig in the archive, but I think we discussed the \"just show\nprogress for destinations\" before. The problem you run into is that the\nitems aren't a good indication of the amount of work. You really are\ndoing n*m work, so if you just count \"m\", it can be very misleading if\n\"n\" is high (and vice versa).\n\nMight it make more sense just to move to a larger integer size? Or\nperhaps to allow a higher limit for each side as long as the product of\nthe sides does not overflow?\n\n> And we would also like to see progress when doing\n> a cherry pick - in our case this takes a few minutes:\n> \n>  \n> diff --git a/sequencer.c b/sequencer.c\n> index 3c060e0..8fce028 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -282,6 +282,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n>  \n>  \tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n>  \t\tparse_merge_opt(&o, *xopt);\n> +\to.show_rename_progress = isatty(2);\n>  \n>  \tclean = merge_trees(&o,\n>  \t\t\t    head_tree,\n\nI think that's sensible, for the same reasons that \"merge\" shows\nprogress. I suspect the patch you show above catches \"git revert\", too,\nbut if not, it should probably get the same treatment.\n\n-Peff\n"},{"id":"273725","messageId":"20151125120315.GC11192@inner.h.apk.li","threadId":"40792","inReplyTo":"20151124233328.GA13872@sigill.intra.peff.net","subject":"Re: [RFC] rename detection: allow more renames","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2015-11-25T12:03:15Z","receivedAt":"2015-11-25T12:03:15Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"Hi Jeff,\n\nthanks for the reply!\n\nOn Tue, 24 Nov 2015 18:33:28 +0000, Jeff King wrote:\n...\n> I didn't dig in the archive, but I think we discussed the \"just show\n> progress for destinations\" before. The problem you run into is that the\n> items aren't a good indication of the amount of work. You really are\n> doing n*m work, so if you just count \"m\", it can be very misleading if\n> \"n\" is high (and vice versa).\n\nTrue, but the loops do progress indication for destinations only anyway.\nSo if you only have three destinations and a zillion sources, you\nwill still get only three progress updates, even if they say\n'one zillion (33%)', 'two zillion (67%)', ...\n\nI think as long as this is the case we can as well report the destination\ncount; maybe put the source count somewhere in the progress text.\n\n> Might it make more sense just to move to a larger integer size?\n\nWhich would be? I'd venture into the progress code to identify\nthe necessary changes.\n\n> Or\n> perhaps to allow a higher limit for each side as long as the product of\n> the sides does not overflow?\n\nWe're somewhat close to getting there. The rename detection runs\nfor several minutes in our cases.\n\nAndreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds <torvalds@*.org>\nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"273730","messageId":"20151125123625.GA4504@sigill.intra.peff.net","threadId":"40792","inReplyTo":"20151125120315.GC11192@inner.h.apk.li","subject":"Re: [RFC] rename detection: allow more renames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-25T12:36:25Z","receivedAt":"2015-11-25T12:36:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 25, 2015 at 01:03:15PM +0100, Andreas Krey wrote:\n\n> On Tue, 24 Nov 2015 18:33:28 +0000, Jeff King wrote:\n> ...\n> > I didn't dig in the archive, but I think we discussed the \"just show\n> > progress for destinations\" before. The problem you run into is that the\n> > items aren't a good indication of the amount of work. You really are\n> > doing n*m work, so if you just count \"m\", it can be very misleading if\n> > \"n\" is high (and vice versa).\n> \n> True, but the loops do progress indication for destinations only anyway.\n> So if you only have three destinations and a zillion sources, you\n> will still get only three progress updates, even if they say\n> 'one zillion (33%)', 'two zillion (67%)', ...\n> \n> I think as long as this is the case we can as well report the destination\n> count; maybe put the source count somewhere in the progress text.\n\nAh, true. Though maybe that is something we should be fixing, rather\nthan building on. I dunno. I cannot recall the last time I saw rename\ndetection progress myself.\n\n> > Might it make more sense just to move to a larger integer size?\n> \n> Which would be? I'd venture into the progress code to identify\n> the necessary changes.\n\nI was thinking of uint64_t; the progress code would need to change, but\nI don't see a big problem with that.\n\n> We're somewhat close to getting there. The rename detection runs\n> for several minutes in our cases.\n\nYikes. :)\n\n-Peff\n"},{"id":"273829","messageId":"xmqqegf78hv5.fsf@gitster.mtv.corp.google.com","threadId":"40792","inReplyTo":"20151124233328.GA13872@sigill.intra.peff.net","subject":"Re: [RFC] rename detection: allow more renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-30T18:25:18Z","receivedAt":"2015-11-30T18:25:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Nov 13, 2015 at 05:35:06PM +0100, Andreas Krey wrote:\n>\n>> The code talks about limiting the size\n>> of the rename matrix, but as far as I\n>> can see, the matrix itself never exists\n>> as such, and the only thing that could\n>> actually overflow is the computation\n>> for the progress indication. This\n>> can be fixed by reporting just the\n>> destinations checked instead of the\n>> combinations, since we only update\n>> the progress once per destination\n>> anyway.\n>\n> I didn't dig in the archive, but I think we discussed the \"just show\n> progress for destinations\" before. The problem you run into is that the\n> items aren't a good indication of the amount of work. You really are\n> doing n*m work, so if you just count \"m\", it can be very misleading if\n> \"n\" is high (and vice versa).\n\nRight.\n\nWith s/never exists/no longer exists/ in the above observation, I\nagree that this topic is sensible that it revisits the stale comment\nfrom days back when we did use the matrix.\n"}]}