{"thread":{"id":"47160","subject":"[PATCH 2/4] Remove silent clamp of renameLimit","startedAt":"2017-11-10T17:45:07Z","lastAt":"2017-11-14T01:18:08Z","messageCount":15,"participants":["Elijah Newren","Stefan Beller","brian m. carlson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"332194","messageId":"20171110173956.25105-3-newren@gmail.com","threadId":"47160","inReplyTo":"20171110173956.25105-1-newren@gmail.com","subject":"[PATCH 2/4] Remove silent clamp of renameLimit","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-10T17:39:54Z","receivedAt":"2017-11-10T17:45:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit 0024a5492 (Fix the rename detection limit checking; 2007-09-14),\nthe renameLimit was clamped to 32767. This appears to have been to simply\navoid integer overflow in the following computation:\n\n num_create * num_src <= rename_limit * rename_limit\n\nalthough it also could be viewed as a hardcoded bound on the amount of CPU\ntime we're willing to allow users to tell git to spend on handling\nrenames. An upper bound may make sense, particularly as the computation\nis O(rename_limit^2), but only if the bound is documented and communicated\nto the user -- neither of which were true.\n\nIn fact, the silent clamping of the renameLimit to a smaller value and\nlack of reporting of the needed renameLimit when it was too large made it\nappear to the user as though they had used a high enough value; however,\ngit would proceed to mess up the merge or cherry-pick badly based on the\nlack of rename detection. Some hardy folks, despite the lack of feedback\non the correct limit to choose, were desperate enough to repeatedly retry\ntheir cherry-picks with increasingly larger renameLimit values (going\norders of magnitude beyond the built-in limit of 32767), but were\nconsistently met with the same failure.\n\nAlthough large limits can make things slow, we have users who would be\necstatic to have a small five file change be correctly cherry picked even\nif they have to manually specify a large limit and it took git ten minutes\nto compute it.\n\nSigned-off-by: Elijah Newren \n---\n diff.c | 2 +-\n diffcore-rename.c | 11 ++++-------\n 2 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6fd288420b..c6597e3231 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5524,7 +5524,7 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)\n \t\twarning(_(rename_limit_warning));\n \telse\n \t\treturn;\n-\tif (0 < needed && needed < 32767)\n+\tif (0 < needed)\n \t\twarning(_(rename_limit_advice), varname, needed);\n }\n \ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 0d8c3d2ee4..7f9a463f5a 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -391,14 +391,10 @@ static int too_many_rename_candidates(int num_create,\n \t * growing larger than a \"rename_limit\" square matrix, ie:\n \t *\n \t * num_create * num_src > rename_limit * rename_limit\n-\t *\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 ((num_create <= rename_limit || num_src <= rename_limit) &&\n-\t (num_create * num_src <= rename_limit * rename_limit))\n+\t ((double)num_create * (double)num_src\n+\t <= (double)rename_limit * (double)rename_limit))\n \t\treturn 0;\n \n \toptions->needed_rename_limit =\n@@ -415,7 +411,8 @@ static int too_many_rename_candidates(int num_create,\n \t\tnum_src++;\n \t}\n \tif ((num_create <= rename_limit || num_src <= rename_limit) &&\n-\t (num_create * num_src <= rename_limit * rename_limit))\n+\t ((double)num_create * (double)num_src\n+\t <= (double)rename_limit * (double)rename_limit))\n \t\treturn 2;\n \treturn 1;\n }\n-- \n2.15.0.5.g9567be9905\n\n"},{"id":"332195","messageId":"20171110173956.25105-4-newren@gmail.com","threadId":"47160","inReplyTo":"20171110173956.25105-1-newren@gmail.com","subject":"[PATCH 3/4] progress: Fix progress meters when dealing with lots of work","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-10T17:39:55Z","receivedAt":"2017-11-10T17:46:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The possibility of setting merge.renameLimit beyond 2^16 raises the\npossibility that the values passed to progress can exceed 2^32. For my\nusecase of interest, I only needed to pass a value a little over 2^31. If\nI were only interested in fixing my usecase, I could have simply changed\nlast_value from int to unsigned, and casted each of rename_dst_nr and\nrename_src_nr (in merge-recursive.c) from int to unsigned just before\nmultiplying them. However, as long as we're making changes to allow\nlarger progress meters, we may as well make a little more room in general.\nUse uint64_t, because it \"ought to be enough for anybody\". :-)\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 4 ++--\n progress.c | 22 +++++++++++-----------\n progress.h | 8 ++++----\n 3 files changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 7f9a463f5a..6ba6157c61 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -531,7 +531,7 @@ void diffcore_rename(struct diff_options *options)\n \tif (options->show_rename_progress) {\n \t\tprogress = start_delayed_progress(\n \t\t\t\t_(\"Performing inexact rename detection\"),\n-\t\t\t\trename_dst_nr * rename_src_nr);\n+\t\t\t\t(uint64_t)rename_dst_nr * (uint64_t)rename_src_nr);\n \t}\n \n \tmx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));\n@@ -568,7 +568,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, (uint64_t)(i+1)*(uint64_t)rename_src_nr);\n \t}\n \tstop_progress(&progress);\n \ndiff --git a/progress.c b/progress.c\nindex 289678d43d..7e4a2f9532 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -30,8 +30,8 @@ struct throughput {\n \n struct progress {\n \tconst char *title;\n-\tint last_value;\n-\tunsigned total;\n+\tuint64_t last_value;\n+\tuint64_t total;\n \tunsigned last_percent;\n \tunsigned delay;\n \tunsigned delayed_percent_threshold;\n@@ -79,7 +79,7 @@ static int is_foreground_fd(int fd)\n \treturn tpgrp < 0 || tpgrp == getpgid(0);\n }\n \n-static int display(struct progress *progress, unsigned n, const char *done)\n+static int display(struct progress *progress, uint64_t n, const char *done)\n {\n \tconst char *eol, *tp;\n \n@@ -106,7 +106,7 @@ static int display(struct progress *progress, unsigned n, const char *done)\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n \t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\t\tfprintf(stderr, \"%s: %3u%% (%u/%u)%s%s\",\n+\t\t\t\tfprintf(stderr, \"%s: %3u%% (%lu/%lu)%s%s\",\n \t\t\t\t\tprogress->title, percent, n,\n \t\t\t\t\tprogress->total, tp, eol);\n \t\t\t\tfflush(stderr);\n@@ -116,7 +116,7 @@ static int display(struct progress *progress, unsigned n, const char *done)\n \t\t}\n \t} else if (progress_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n-\t\t\tfprintf(stderr, \"%s: %u%s%s\",\n+\t\t\tfprintf(stderr, \"%s: %lu%s%s\",\n \t\t\t\tprogress->title, n, tp, eol);\n \t\t\tfflush(stderr);\n \t\t}\n@@ -127,7 +127,7 @@ static int display(struct progress *progress, unsigned n, const char *done)\n \treturn 0;\n }\n \n-static void throughput_string(struct strbuf *buf, off_t total,\n+static void throughput_string(struct strbuf *buf, uint64_t total,\n \t\t\t unsigned int rate)\n {\n \tstrbuf_reset(buf);\n@@ -138,7 +138,7 @@ static void throughput_string(struct strbuf *buf, off_t total,\n \tstrbuf_addstr(buf, \"/s\");\n }\n \n-void display_throughput(struct progress *progress, off_t total)\n+void display_throughput(struct progress *progress, uint64_t total)\n {\n \tstruct throughput *tp;\n \tuint64_t now_ns;\n@@ -200,12 +200,12 @@ void display_throughput(struct progress *progress, off_t total)\n \t\tdisplay(progress, progress->last_value, NULL);\n }\n \n-int display_progress(struct progress *progress, unsigned n)\n+int display_progress(struct progress *progress, uint64_t n)\n {\n \treturn progress ? display(progress, n, NULL) : 0;\n }\n \n-static struct progress *start_progress_delay(const char *title, unsigned total,\n+static struct progress *start_progress_delay(const char *title, uint64_t total,\n \t\t\t\t\t unsigned percent_threshold, unsigned delay)\n {\n \tstruct progress *progress = malloc(sizeof(*progress));\n@@ -227,12 +227,12 @@ static struct progress *start_progress_delay(const char *title, unsigned total,\n \treturn progress;\n }\n \n-struct progress *start_delayed_progress(const char *title, unsigned total)\n+struct progress *start_delayed_progress(const char *title, uint64_t total)\n {\n \treturn start_progress_delay(title, total, 0, 2);\n }\n \n-struct progress *start_progress(const char *title, unsigned total)\n+struct progress *start_progress(const char *title, uint64_t total)\n {\n \treturn start_progress_delay(title, total, 0, 0);\n }\ndiff --git a/progress.h b/progress.h\nindex 6392b63371..70a4d4a0d6 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -3,10 +3,10 @@\n \n struct progress;\n \n-void display_throughput(struct progress *progress, off_t total);\n-int display_progress(struct progress *progress, unsigned n);\n-struct progress *start_progress(const char *title, unsigned total);\n-struct progress *start_delayed_progress(const char *title, unsigned total);\n+void display_throughput(struct progress *progress, uint64_t total);\n+int display_progress(struct progress *progress, uint64_t n);\n+struct progress *start_progress(const char *title, uint64_t total);\n+struct progress *start_delayed_progress(const char *title, uint64_t total);\n void stop_progress(struct progress **progress);\n void stop_progress_msg(struct progress **progress, const char *msg);\n \n-- \n2.15.0.5.g9567be9905\n\n"},{"id":"332196","messageId":"20171110173956.25105-2-newren@gmail.com","threadId":"47160","inReplyTo":"20171110173956.25105-1-newren@gmail.com","subject":"[PATCH 1/4] sequencer: Warn when internal merge may be suboptimal due to renameLimit","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-10T17:39:53Z","receivedAt":"2017-11-10T17:46:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Having renamed files silently treated as deleted/modified conflicts\ninstead of correctly resolving the renamed/modified case has caused lots\nof pain for some users. Eventually, some of them figured out to set\nmerge.renameLimit to something higher, but without feedback about what\nvalue it should have, they were just repeatedly guessing and retrying.\n\nSigned-off-by: Elijah Newren \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f2a10cc4f2..2b4cecb617 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -462,6 +462,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tif (is_rebase_i(opts) && clean <= 0)\n \t\tfputs(o.obuf.buf, stdout);\n \tstrbuf_release(&o.obuf);\n+\tdiff_warn_rename_limit(\"merge.renamelimit\", o.needed_rename_limit, 0);\n \tif (clean < 0)\n \t\treturn clean;\n \n-- \n2.15.0.5.g9567be9905\n\n"},{"id":"332197","messageId":"20171110173956.25105-5-newren@gmail.com","threadId":"47160","inReplyTo":"20171110173956.25105-1-newren@gmail.com","subject":"[PATCH 4/4] sequencer: Show rename progress during cherry picks","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-10T17:39:56Z","receivedAt":"2017-11-10T17:49:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When trying to cherry-pick a change that has lots of renames, it is\nsomewhat unsettling to wait a really long time without any feedback.\n\nSigned-off-by: Elijah Newren \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 2b4cecb617..247d93f363 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -448,6 +448,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \to.branch2 = next ? next_label : \"(empty tree)\";\n \tif (is_rebase_i(opts))\n \t\to.buffer_output = 2;\n+\to.show_rename_progress = 1;\n \n \thead_tree = parse_tree_indirect(head);\n \tnext_tree = next ? next->tree : empty_tree();\n-- \n2.15.0.5.g9567be9905\n\n"},{"id":"332204","messageId":"20171110173956.25105-1-newren@gmail.com","threadId":"47160","inReplyTo":null,"subject":"[PATCH 0/4] Fix issues with rename detection limits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-10T17:39:52Z","receivedAt":"2017-11-10T18:13:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In a repo with around 60k files with deep directory hierarchies (due to\nbeing predominantly java code) and which had a few high level directories\nmoved around (making it appear to git as dozens of thousands of individual\nfile renames), attempts to cherry-pick commits across product versions\nresulted in non-sensical delete/modify conflicts (rather than\nrename/modify as expected). We had a few teams who were in the unenviable\nposition of having to backport hundreds or thousands of commits across\nsuch product versions, and the result was months of pain, which could have\nbeen alleviated were it not for a few small git bugs:\n\n * When you try to cherry-pick changes, unlike when you merge two\n branches, git will not notify you when you need to set a higher\n merge.renameLimit.\n\n * If you know git internals well enough, you can try to trick git into\n telling you the correct renameLimit by doing a merge instead of a\n cherry-pick. If you do that, and have a renameLimit that is too\n small, git will let you know the value you need. You can then undo\n the merge and proceed with the cherry-pick. Except that...\n\n * If you are performing a merge and specify a large renameLimit that\n isn't large enough, and the necessary renameLimit you need is greater\n than 32767, then git tells you nothing, leading you to believe that\n the limit you specified is high enough, but only to watch it still\n mess up the merge badly.\n\n * If you happen to specify a merge.renameLimit that *is* high enough,\n but just happens to be greater than 32767, then git will silently\n pretend you specified 32767, determine that 32767 is not high enough,\n not tell you anything about it's silent clamping, and then proceed to\n mess up the merge or cherry-pick badly. Not only do you get no\n feedback, the clamping to 32767 isn't documented anywhere even if you\n tried to read every manual page.\n\nFolks did discover the merge.renameLimit and tried setting it to various\nvalues, spread over the spectrum from 1000 (the default) up to 999999999\n(or maybe more, that's just the highest number I heard), completely\nunaware that most their attempts were ignored and wondering why git\ncouldn't handle things and couldn't explain why either.\n\nEventually folks wrote scripts that would run the output of format-patch\nthrough a bunch of sed commands to pretend patches were against the\nfilename on the other side of history and then pipe back through git-am.\nSuch scripts grew as more and more rename rules were added.\n\nI eventually learned of one of these scripts and said something close to,\n\"You don't need these pile of rename rules; just set merge.renameLimit to\nsomething higher.\" When they responded that merge.renameLimit didn't\nwork, I didn't believe them. This patch series, along with two others\nthat I will be sending shortly, represent my attempt to continue to not\nbelieve them. :-)\n\nElijah Newren (4):\n sequencer: Warn when internal merge may be suboptimal due to\n renameLimit\n Remove silent clamp of renameLimit\n progress: Fix progress meters when dealing with lots of work\n sequencer: Show rename progress during cherry picks\n\n diff.c | 2 +-\n diffcore-rename.c | 15 ++++++---------\n progress.c | 22 +++++++++++-----------\n progress.h | 8 ++++----\n sequencer.c | 2 ++\n 5 files changed, 24 insertions(+), 25 deletions(-)\n\n-- \n2.15.0.5.g9567be9905\n\n"},{"id":"332206","messageId":"CAGZ79kbqxA9U7EsU6_i895vgrTUwmqv8f02NCoUqPS1_nD2PtA@mail.gmail.com","threadId":"47160","inReplyTo":"20171110173956.25105-3-newren@gmail.com","subject":"Re: [PATCH 2/4] Remove silent clamp of renameLimit","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-10T18:26:15Z","receivedAt":"2017-11-10T18:26:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 10, 2017 at 9:39 AM, Elijah Newren wrote:\n> In commit 0024a5492 (Fix the rename detection limit checking; 2007-09-14),\n> the renameLimit was clamped to 32767. This appears to have been to simply\n> avoid integer overflow in the following computation:\n>\n> num_create * num_src <= rename_limit * rename_limit\n>\n> although it also could be viewed as a hardcoded bound on the amount of CPU\n> time we're willing to allow users to tell git to spend on handling\n> renames. An upper bound may make sense, particularly as the computation\n> is O(rename_limit^2), but only if the bound is documented and communicated\n> to the user -- neither of which were true.\n>\n> In fact, the silent clamping of the renameLimit to a smaller value and\n> lack of reporting of the needed renameLimit when it was too large made it\n> appear to the user as though they had used a high enough value; however,\n> git would proceed to mess up the merge or cherry-pick badly based on the\n> lack of rename detection. Some hardy folks, despite the lack of feedback\n> on the correct limit to choose, were desperate enough to repeatedly retry\n> their cherry-picks with increasingly larger renameLimit values (going\n> orders of magnitude beyond the built-in limit of 32767), but were\n> consistently met with the same failure.\n>\n> Although large limits can make things slow, we have users who would be\n> ecstatic to have a small five file change be correctly cherry picked even\n> if they have to manually specify a large limit and it took git ten minutes\n> to compute it.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> diff.c | 2 +-\n> diffcore-rename.c | 11 ++++-------\n> 2 files changed, 5 insertions(+), 8 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 6fd288420b..c6597e3231 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5524,7 +5524,7 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)\n> warning(_(rename_limit_warning));\n> else\n> return;\n> - if (0 < needed && needed < 32767)\n> + if (0 < needed)\n> warning(_(rename_limit_advice), varname, needed);\n> }\n>\n> diff --git a/diffcore-rename.c b/diffcore-rename.c\n> index 0d8c3d2ee4..7f9a463f5a 100644\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -391,14 +391,10 @@ static int too_many_rename_candidates(int num_create,\n> * growing larger than a \"rename_limit\" square matrix, ie:\n> *\n> * num_create * num_src > rename_limit * rename_limit\n> - *\n> - * but handles the potential overflow case specially (and we\n> - * assume at least 32-bit integers)\n> */\n> - if (rename_limit <= 0 || rename_limit > 32767)\n> - rename_limit = 32767;\n> if ((num_create <= rename_limit || num_src <= rename_limit) &&\n> - (num_create * num_src <= rename_limit * rename_limit))\n> + ((double)num_create * (double)num_src\n> + <= (double)rename_limit * (double)rename_limit))\n> return 0;\n\nFrom a technical perspective, I would think that if\n(num_create <= rename_limit || num_src <= rename_limit)\nholds true, that the double-cast condition would also be always true?\nCould we just remove that last check?\n\nOr phrased differently, if we can cast to double and extend the check\nhere, do we have to adapt code at other places as well?\n\n>\n> options->needed_rename_limit =\n> @@ -415,7 +411,8 @@ static int too_many_rename_candidates(int num_create,\n> num_src++;\n> }\n> if ((num_create <= rename_limit || num_src <= rename_limit) &&\n> - (num_create * num_src <= rename_limit * rename_limit))\n> + ((double)num_create * (double)num_src\n> + <= (double)rename_limit * (double)rename_limit))\n> return 2;\n> return 1;\n> }\n> --\n> 2.15.0.5.g9567be9905\n>\n"},{"id":"332209","messageId":"CABPp-BGwoduH_T7U4uN5A-h43wREfXdQgV8YCVWf8w5N09cYSw@mail.gmail.com","threadId":"47160","inReplyTo":"CAGZ79kbqxA9U7EsU6_i895vgrTUwmqv8f02NCoUqPS1_nD2PtA@mail.gmail.com","subject":"Re: [PATCH 2/4] Remove silent clamp of renameLimit","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-10T18:36:17Z","receivedAt":"2017-11-10T18:36:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Thanks for taking a look!\n\nOn Fri, Nov 10, 2017 at 10:26 AM, Stefan Beller wrote:\n\n>> - if (rename_limit <= 0 || rename_limit > 32767)\n>> - rename_limit = 32767;\n>> if ((num_create <= rename_limit || num_src <= rename_limit) &&\n>> - (num_create * num_src <= rename_limit * rename_limit))\n>> + ((double)num_create * (double)num_src\n>> + <= (double)rename_limit * (double)rename_limit))\n>> return 0;\n>\n> From a technical perspective, I would think that if\n> (num_create <= rename_limit || num_src <= rename_limit)\n> holds true, that the double-cast condition would also be always true?\n> Could we just remove that last check?\n\nNot necessarily. For example, if num_create = rename_limit-1 and\nnum_src = rename_limit+2, then the first condition will be satisfied\nbut the second won't. If it was && rather than ||, then the second\ncondition would be superfluous.\n\n> Or phrased differently, if we can cast to double and extend the check\n> here, do we have to adapt code at other places as well?\n\nGood point, and yes. Perhaps I should have re-ordered my patch series\nbecause I came back to it later and realized that the progress code\nwas broken due to overflow/wraparound, and a patch 3 fixed that.\n\nFurther, the later patch used uint64_t instead of double. While\ndouble works, perhaps I should change the double here to uint64_t for\nconsistency?\n"},{"id":"332269","messageId":"20171110234210.lojdat5zofhrdlgi@genre.crustytoothpaste.net","threadId":"47160","inReplyTo":"CABPp-BGwoduH_T7U4uN5A-h43wREfXdQgV8YCVWf8w5N09cYSw@mail.gmail.com","subject":"Re: [PATCH 2/4] Remove silent clamp of renameLimit","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-11-10T23:42:11Z","receivedAt":"2017-11-10T23:42:25Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Nov 10, 2017 at 10:36:17AM -0800, Elijah Newren wrote:\n> Thanks for taking a look!\n> \n> On Fri, Nov 10, 2017 at 10:26 AM, Stefan Beller wrote:\n> > From a technical perspective, I would think that if\n> > (num_create <= rename_limit || num_src <= rename_limit)\n> > holds true, that the double-cast condition would also be always true?\n> > Could we just remove that last check?\n> \n> Not necessarily. For example, if num_create = rename_limit-1 and\n> num_src = rename_limit+2, then the first condition will be satisfied\n> but the second won't. If it was && rather than ||, then the second\n> condition would be superfluous.\n> \n> > Or phrased differently, if we can cast to double and extend the check\n> > here, do we have to adapt code at other places as well?\n> \n> Good point, and yes. Perhaps I should have re-ordered my patch series\n> because I came back to it later and realized that the progress code\n> was broken due to overflow/wraparound, and a patch 3 fixed that.\n> \n> Further, the later patch used uint64_t instead of double. While\n> double works, perhaps I should change the double here to uint64_t for\n> consistency?\n\nI'm wondering if maybe you want to use size_t. If you end up using an\nunsigned type, you might be able to leverage unsigned_mult_overflows to\navoid having to write this by hand.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"332296","messageId":"CABPp-BHvveKW+UzTZqLq5h0bVUJLPqYpGu0CWQpYjHzzDbEWww@mail.gmail.com","threadId":"47160","inReplyTo":"20171110234210.lojdat5zofhrdlgi@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/4] Remove silent clamp of renameLimit","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-11T16:39:11Z","receivedAt":"2017-11-11T16:39:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Fri, Nov 10, 2017 at 3:42 PM, brian m. carlson\n wrote:\n> On Fri, Nov 10, 2017 at 10:36:17AM -0800, Elijah Newren wrote:\n\n>> Further, the later patch used uint64_t instead of double. While\n>> double works, perhaps I should change the double here to uint64_t for\n>> consistency?\n>\n> I'm wondering if maybe you want to use size_t. If you end up using an\n> unsigned type, you might be able to leverage unsigned_mult_overflows to\n> avoid having to write this by hand.\n\nThanks for pointing out unsigned_mult_overflows; I was unaware of it.\nI think I'd prefer to not use it though; the fact that I had a case\nthat genuinely needed a value greater than 2^31 (at least before my\nperformance patches) meant that a slightly bigger repo is going to\neventually need a value over 2^32, so I think we should just cast to a\ntype that can hold it.\n\nI'm curious why you suggest size_t, though. I have always associated\nthat with an amount of memory that will be used, and there's no\nallocation based on this result. Was I wrong to make that\nassociation, or is there another good reason here?\n"},{"id":"332299","messageId":"20171111173253.idmyypjevwqblica@genre.crustytoothpaste.net","threadId":"47160","inReplyTo":"CABPp-BHvveKW+UzTZqLq5h0bVUJLPqYpGu0CWQpYjHzzDbEWww@mail.gmail.com","subject":"Re: [PATCH 2/4] Remove silent clamp of renameLimit","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-11-11T17:32:53Z","receivedAt":"2017-11-11T17:33:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Nov 11, 2017 at 08:39:11AM -0800, Elijah Newren wrote:\n> Thanks for pointing out unsigned_mult_overflows; I was unaware of it.\n> I think I'd prefer to not use it though; the fact that I had a case\n> that genuinely needed a value greater than 2^31 (at least before my\n> performance patches) meant that a slightly bigger repo is going to\n> eventually need a value over 2^32, so I think we should just cast to a\n> type that can hold it.\n\nThat's fine. I had considered this in the context of 64-bit values, but\nI suppose that the likelihood of us hitting 2**64 iterations (and\nperforming reasonably as well) is unlikely.\n\n> I'm curious why you suggest size_t, though. I have always associated\n> that with an amount of memory that will be used, and there's no\n> allocation based on this result. Was I wrong to make that\n> association, or is there another good reason here?\n\nI usually like size_t for values that are unsigned and don't need to be\nfixed size because it's usually the largest efficient unsigned type.\nHowever, if you want something to handle items larger than 2**32, then I\nagree that it's maybe not a great idea if we want it to work on 32-bit\nsystems.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"332394","messageId":"xmqqy3na92rv.fsf@gitster.mtv.corp.google.com","threadId":"47160","inReplyTo":"20171110173956.25105-2-newren@gmail.com","subject":"Re: [PATCH 1/4] sequencer: Warn when internal merge may be suboptimal due to renameLimit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-13T05:16:36Z","receivedAt":"2017-11-13T05:16:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Subject: Re: [PATCH 1/4] sequencer: Warn when internal merge may be suboptimal due to renameLimit\n\nStyle: please downcase s/Warn/warn/ to fit this better in \"shortlog --no-merges\"\noutput.\n\n> Having renamed files silently treated as deleted/modified conflicts\n> instead of correctly resolving the renamed/modified case has caused lots\n> of pain for some users. Eventually, some of them figured out to set\n> merge.renameLimit to something higher, but without feedback about what\n> value it should have, they were just repeatedly guessing and retrying.\n\nIt would have been _much_ easier to read if you refrained from\nstating the gripes more prominently than the source of the gripe\nthat you are fixing. E.g.\n\n\tIf one side of a merge have renamed more paths than\n\tmerge.renamelimit since the sides diverged, the recursive\n\tmerge strategy stops detecting renames and instead leaves\n\tthese many paths as delete/modify conflicts, without warning\n\twhat is going on and giving hints on how to tell Git that it\n\tis allowed to spend more cycles to detect renames.\n\nperhaps.\n\nThe addition of a call to diff_warn_rename_limit looks quite\nsensible.\n\nThanks.\n\n> Signed-off-by: Elijah Newren \n> ---\n> sequencer.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index f2a10cc4f2..2b4cecb617 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -462,6 +462,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> \tif (is_rebase_i(opts) && clean <= 0)\n> \t\tfputs(o.obuf.buf, stdout);\n> \tstrbuf_release(&o.obuf);\n> +\tdiff_warn_rename_limit(\"merge.renamelimit\", o.needed_rename_limit, 0);\n> \tif (clean < 0)\n> \t\treturn clean;\n"},{"id":"332395","messageId":"xmqqtvxy92ey.fsf@gitster.mtv.corp.google.com","threadId":"47160","inReplyTo":"20171110173956.25105-4-newren@gmail.com","subject":"Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-13T05:24:21Z","receivedAt":"2017-11-13T05:24:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Subject: Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work\n\nStyle: s/Fix/fix/;\n\n> The possibility of setting merge.renameLimit beyond 2^16 raises the\n> possibility that the values passed to progress can exceed 2^32. For my\n> usecase of interest, I only needed to pass a value a little over 2^31. If\n> I were only interested in fixing my usecase, I could have simply changed\n> last_value from int to unsigned, and casted each of rename_dst_nr and\n> rename_src_nr (in merge-recursive.c) from int to unsigned just before\n> multiplying them. However, as long as we're making changes to allow\n> larger progress meters, we may as well make a little more room in general.\n> Use uint64_t, because it \"ought to be enough for anybody\". :-)\n\nThe middle part of the log message may waste more mental bandwidth\nof readers than it is worth. It might have gave you satisfaction to\nbe able to vent, but don't (the place to do so is after the three\ndash lines).\n\nI am not sure if we want all codepaths to do 64-bit math for\nprogress meter, but let's see what others would think.\n\n> -static int display(struct progress *progress, unsigned n, const char *done)\n> +static int display(struct progress *progress, uint64_t n, const char *done)\n> {\n> \tconst char *eol, *tp;\n> \n> @@ -106,7 +106,7 @@ static int display(struct progress *progress, unsigned n, const char *done)\n> \t\tif (percent != progress->last_percent || progress_update) {\n> \t\t\tprogress->last_percent = percent;\n> \t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n> -\t\t\t\tfprintf(stderr, \"%s: %3u%% (%u/%u)%s%s\",\n> +\t\t\t\tfprintf(stderr, \"%s: %3u%% (%lu/%lu)%s%s\",\n\nAre these (and there are probably other instances in this patch) %lu\ncorrect?\n"},{"id":"332396","messageId":"xmqqpo8m92cq.fsf@gitster.mtv.corp.google.com","threadId":"47160","inReplyTo":"20171110173956.25105-5-newren@gmail.com","subject":"Re: [PATCH 4/4] sequencer: Show rename progress during cherry picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-13T05:25:41Z","receivedAt":"2017-11-13T05:25:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Subject: Re: [PATCH 4/4] sequencer: Show rename progress during cherry picks\n\nStyle: s/Show/show/\n\n> When trying to cherry-pick a change that has lots of renames, it is\n> somewhat unsettling to wait a really long time without any feedback.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> sequencer.c | 1 +\n> 1 file changed, 1 insertion(+)\n\nMakes sense.\n\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 2b4cecb617..247d93f363 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -448,6 +448,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> \to.branch2 = next ? next_label : \"(empty tree)\";\n> \tif (is_rebase_i(opts))\n> \t\to.buffer_output = 2;\n> +\to.show_rename_progress = 1;\n> \n> \thead_tree = parse_tree_indirect(head);\n> \tnext_tree = next ? next->tree : empty_tree();\n"},{"id":"332438","messageId":"CABPp-BH1Cpc9UfYpmBBAHWSqadg=QuD=28qx1oV29ZdvF4NbJw@mail.gmail.com","threadId":"47160","inReplyTo":"xmqqtvxy92ey.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-13T20:05:05Z","receivedAt":"2017-11-13T20:05:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Thanks for the reviews and suggestions!\n\nOn Sun, Nov 12, 2017 at 9:24 PM, Junio C Hamano wrote:\n> Elijah Newren writes:\n>\n>> Subject: Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work\n>\n> Style: s/Fix/fix/;\n\nI also messed this up in a lot of my patches in my other patch series.\nI've fixed them all up, but I'll wait to resubmit those other series\nuntil I get some other reviews.\n\n> The middle part of the log message may waste more mental bandwidth\n> of readers than it is worth. It might have gave you satisfaction to\n> be able to vent, but don't (the place to do so is after the three\n> dash lines).\n\nCleaned it up, along with the other commit message you pointed out;\nI'll resubmit shortly.\n\n> I am not sure if we want all codepaths to do 64-bit math for\n> progress meter, but let's see what others would think.\n\nIf others don't want to do 64-bit math for the progress meter, what\nwould they like to see done instead? I can see a few options:\n 1) Have two separate progress codepaths, one for 32-bith math and\none for 64-bit math.\n 2) Instead of counting pairs of source/dest files compared, just\ncount number of dest paths completed. (Thus, we wouldn't need a value\nbig enough to hold rename_dst_nr * rename_src_nr, just big enough to\nhold rename_dst_nr).\n 3) just let the progress meter overflow and show nonsensical values\n 4) don't show the progress meter if overflow would happen\n 5) something else I'm not thinking of.\n\n>> - fprintf(stderr, \"%s: %3u%% (%u/%u)%s%s\",\n>> + fprintf(stderr, \"%s: %3u%% (%lu/%lu)%s%s\",\n>\n> Are these (and there are probably other instances in this patch) %lu\n> correct?\n\nOops, no. I think %llu is right, though looking around the code it\nappears folks use PRIuMAX and avoid %llu due to possible issues with\nold windows compilers. Not sure if that's still relevant, but I'll\ntry to remain consistent with what I see elsewhere and include that\nfix in my re-roll.\n"},{"id":"332484","messageId":"xmqqwp2t7j5i.fsf@gitster.mtv.corp.google.com","threadId":"47160","inReplyTo":"CABPp-BH1Cpc9UfYpmBBAHWSqadg=QuD=28qx1oV29ZdvF4NbJw@mail.gmail.com","subject":"Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T01:18:01Z","receivedAt":"2017-11-14T01:18:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> 2) Instead of counting pairs of source/dest files compared, just\n> count number of dest paths completed. (Thus, we wouldn't need a value\n> big enough to hold rename_dst_nr * rename_src_nr, just big enough to\n> hold rename_dst_nr).\n\nThis sounds like the most sensible thing to do even if we do switch\nto larger integer size, but that is orthogonal to the main point of\nthis series.\n\n\n"}]}