{"thread":{"id":"9787","subject":"[PATCH 1/2] git-commit: Disallow unchanged tree in non-merge mode","startedAt":"2007-09-05T23:49:41Z","lastAt":"2007-09-14T19:46:51Z","messageCount":13,"participants":["Dmitry V. Levin","Shawn O. Pearce","Linus Torvalds","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"52645","messageId":"20070905234941.GA643@nomad.office.altlinux.org","threadId":"9787","inReplyTo":null,"subject":"[PATCH 1/2] git-commit: Disallow unchanged tree in non-merge mode","fromName":"Dmitry V. Levin","fromEmail":"ldv@altlinux.org","sentAt":"2007-09-05T23:49:41Z","receivedAt":"2007-09-05T23:49:41Z","isPatch":true,"sender":{"key":"ldv@altlinux.org","avatar":"https://avatars.githubusercontent.com/u/5281408?v=4"},"body":"Do not commit an unchanged tree in non-merge mode.\n\nWhile regular mode is already handled by git-runstatus, this cheap check\nallows to avoid costly git-runstatus later.  Also, amend mode needs\nspecial attention, because git-runstatus return value is ignored.\nThe idea is that amend should not commit an unchanged tree,\none should just remove the top commit using git-reset instead.\n\nSigned-off-by: Dmitry V. Levin <ldv@altlinux.org>\n---\n git-commit.sh |   10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/git-commit.sh b/git-commit.sh\nindex 1d04f1f..800f96c 100755\n--- a/git-commit.sh\n+++ b/git-commit.sh\n@@ -629,6 +629,16 @@ then\n \t\ttree=$(GIT_INDEX_FILE=\"$TMP_INDEX\" git write-tree) &&\n \t\trm -f \"$TMP_INDEX\"\n \tfi &&\n+\tif test -n \"$current\" -a ! -f \"$GIT_DIR/MERGE_HEAD\"\n+\tthen\n+\t\tcurrent_tree=\"$(git cat-file commit \"$current${amend:+^}\" 2>/dev/null |\n+\t\t\t\tsed -e '/^tree \\+/!d' -e 's///' -e q)\"\n+\t\tif test \"$tree\" = \"$current_tree\"\n+\t\tthen\n+\t\t\techo >&2 \"nothing to commit${amend:+ (use \\\"git reset HEAD^\\\" to remove the top commit)}\"\n+\t\t\tfalse\n+\t\tfi\n+\tfi &&\n \tcommit=$(git commit-tree $tree $PARENTS <\"$GIT_DIR/COMMIT_MSG\") &&\n \trlogm=$(sed -e 1q \"$GIT_DIR\"/COMMIT_MSG) &&\n \tgit update-ref -m \"$GIT_REFLOG_ACTION: $rlogm\" HEAD $commit \"$current\" &&\n-- \nldv\n"},{"id":"52672","messageId":"20070906022539.GG18160@spearce.org","threadId":"9787","inReplyTo":"20070905234941.GA643@nomad.office.altlinux.org","subject":"Re: [PATCH 1/2] git-commit: Disallow unchanged tree in non-merge mode","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-06T02:25:39Z","receivedAt":"2007-09-06T02:25:39Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Dmitry V. Levin\" <ldv@altlinux.org> wrote:\n> Do not commit an unchanged tree in non-merge mode.\n\nA laudable goal.  git-gui also does this.  Turns out the other\nchecks within git-gui prevent the user from ever getting that far.\nI probably should remove the empty tree check as it costs CPU time\nto get the old tree.  But I'd rather have the safety check.\n \n> The idea is that amend should not commit an unchanged tree,\n> one should just remove the top commit using git-reset instead.\n\nNO.  `git commit --amend` is *often* used for fixing the commit\nmessage.  Or adding additional detail.  Forcing the user to do\na `git reset --soft HEAD^ && git commit --amend` just because\nyou don't want git-commit to make an \"empty commit\" (which it\ndoesn't usually like to do now anyway!) is a major step back\nin functionality.\n\nNACK.\n \n> diff --git a/git-commit.sh b/git-commit.sh\n> index 1d04f1f..800f96c 100755\n> --- a/git-commit.sh\n> +++ b/git-commit.sh\n> @@ -629,6 +629,16 @@ then\n>  \t\ttree=$(GIT_INDEX_FILE=\"$TMP_INDEX\" git write-tree) &&\n>  \t\trm -f \"$TMP_INDEX\"\n>  \tfi &&\n> +\tif test -n \"$current\" -a ! -f \"$GIT_DIR/MERGE_HEAD\"\n> +\tthen\n> +\t\tcurrent_tree=\"$(git cat-file commit \"$current${amend:+^}\" 2>/dev/null |\n> +\t\t\t\tsed -e '/^tree \\+/!d' -e 's///' -e q)\"\n\nThe better way to get the old tree would be this:\n\n\t\tcurrent_tree=\"$(git rev-parse \"$current${amend:+^}^{tree}\" 2>/dev/null\n\nas it avoids the tool from needing to know about the internal\nrepresentation of a commit object.  It also avoids an entire\nfork+exec of a sed process.\n\n> +\t\tif test \"$tree\" = \"$current_tree\"\n> +\t\tthen\n> +\t\t\techo >&2 \"nothing to commit${amend:+ (use \\\"git reset HEAD^\\\" to remove the top commit)}\"\n\nThat message is a bad idea.  Doing a mixed mode reset will also\nreset the index, causing the user to lose any changes that had\nalready been staged.  This may actually be difficult for him/her to\nrecover from if they have used `git add -i` or git-gui to stage only\ncertain hunks of files, or if their working tree has been further\nmodified after the commit but they want to go back and amend the\nmessage only of the prior commit.\n\n-- \nShawn.\n"},{"id":"52728","messageId":"20070906101648.GD6665@basalt.office.altlinux.org","threadId":"9787","inReplyTo":"20070906022539.GG18160@spearce.org","subject":"Re: [PATCH 1/2] git-commit: Disallow unchanged tree in non-merge mode","fromName":"Dmitry V. Levin","fromEmail":"ldv@altlinux.org","sentAt":"2007-09-06T10:16:48Z","receivedAt":"2007-09-06T10:16:48Z","isPatch":true,"sender":{"key":"ldv@altlinux.org","avatar":"https://avatars.githubusercontent.com/u/5281408?v=4"},"body":"On Wed, Sep 05, 2007 at 10:25:39PM -0400, Shawn O. Pearce wrote:\n> \"Dmitry V. Levin\" <ldv@altlinux.org> wrote:\n> > Do not commit an unchanged tree in non-merge mode.\n> \n> A laudable goal.  git-gui also does this.  Turns out the other\n> checks within git-gui prevent the user from ever getting that far.\n> I probably should remove the empty tree check as it costs CPU time\n> to get the old tree.  But I'd rather have the safety check.\n>  \n> > The idea is that amend should not commit an unchanged tree,\n> > one should just remove the top commit using git-reset instead.\n> \n> NO.  `git commit --amend` is *often* used for fixing the commit\n> message.\n\nYou see, my proposed change does not affect this usage case at all.\n\n> Or adding additional detail.\n\nIf that \"additional\" detail just undoes the latest commit, why should\n\"git commit --amend\" welcome such thing?  I did not get your pint here.\n\n> Forcing the user to do\n> a `git reset --soft HEAD^ && git commit --amend` just because\n> you don't want git-commit to make an \"empty commit\" (which it\n> doesn't usually like to do now anyway!) is a major step back\n> in functionality.\n\nI suppose that helping users to avoid doing really stupid things\ndoes not look as a major step back in functionality, just otherwise.\n\n> > +\t\tcurrent_tree=\"$(git cat-file commit \"$current${amend:+^}\" 2>/dev/null |\n> > +\t\t\t\tsed -e '/^tree \\+/!d' -e 's///' -e q)\"\n> \n> The better way to get the old tree would be this:\n> \n> \t\tcurrent_tree=\"$(git rev-parse \"$current${amend:+^}^{tree}\" 2>/dev/null\n> \n> as it avoids the tool from needing to know about the internal\n> representation of a commit object.  It also avoids an entire\n> fork+exec of a sed process.\n\nAgreed.\n\n> > +\t\tif test \"$tree\" = \"$current_tree\"\n> > +\t\tthen\n> > +\t\t\techo >&2 \"nothing to commit${amend:+ (use \\\"git reset HEAD^\\\" to remove the top commit)}\"\n> \n> That message is a bad idea.  Doing a mixed mode reset will also\n> reset the index, causing the user to lose any changes that had\n> already been staged.  This may actually be difficult for him/her to\n> recover from if they have used `git add -i` or git-gui to stage only\n> certain hunks of files, or if their working tree has been further\n> modified after the commit but they want to go back and amend the\n> message only of the prior commit.\n\nWould \"git reset --soft HEAD^\" advice be better than first one?\nCould you suggest a better message, please?\n\n\n-- \nldv\n"},{"id":"53035","messageId":"alpine.LFD.0.999.0709141002360.16478@woody.linux-foundation.org","threadId":"9787","inReplyTo":"alpine.LFD.0.999.0709132123570.16478@woody.linux-foundation.org","subject":"Re: [PATCH 1/2] git-commit: Disallow unchanged tree in non-merge mode","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-14T17:14:06Z","receivedAt":"2007-09-14T17:14:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 13 Sep 2007, Linus Torvalds wrote:\n> \n> Ok, I'm downloading those tar-balls to reproduce and hopefully see what's \n> going on, but I'm not going to be able to get at it today.\n\nOk, I'm seeing it, and there are serious problems in the diffcore-rename \ncode.\n\nThe problems include things like actual overflow in 31 bits (a signed \ninteger) from the multiplication of \"num_create * num_src\".\n\nBut you'll almost certainly never even get there, because you'd long since \nhave given up on the O(n^2) behaviour of the \"cheap tests\", that aren't \nreally cheap enough, if only because even just comparing the 20-byte SHA1 \nhashes will basically take forever since they will always miss in the \ncache for lots of names.\n\nSo yes, we really *have* to have a rename limit, even if it's only to \navoid the technical bug of overflowing the multiplication.\n\nTesting this actually showed another bug too: \"git diff\" would actually \nnever call \"diff_setup_done() at all, if \"diffopt.output_format\" had been \nset explicitly to something. So doing a \"git diff --stat\" would totally \nignore all the sanity checks (and, what caused me to find it, the \ninitialization of \"diffopt.rename_limit\") that diff_setup_done() is \nsupposed to do!\n\nSo I'm going to send out two patches - one to fix the \"diff_setup_done()\" \nbug, and one that replaces the default rename_limit with something saner. \nBoth seem to be real bugs.\n\n\t\t\tLinus\n"},{"id":"53036","messageId":"alpine.LFD.0.999.0709141014130.16478@woody.linux-foundation.org","threadId":"9787","inReplyTo":"alpine.LFD.0.999.0709141002360.16478@woody.linux-foundation.org","subject":"[PATCH 1/2] Fix \"git diff\" setup code","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-14T17:17:39Z","receivedAt":"2007-09-14T17:17:39Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFor some inexplicable reason, \"git diff\" would call \"diff_setup_done()\" \niff we hadn't given an explicit output format.\n\nThat makes no sense, since much of what diff_setup_done() does is exactly \nabout checking the output format!\n\nThis just moves the call to \"diff_setup_done()\" out of the conditional, \nand to where we've actually done all of the diffopt changes.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n builtin-diff.c |    7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex f77352b..cb4743b 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -252,13 +252,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\targc = 0;\n \telse\n \t\targc = setup_revisions(argc, argv, &rev, NULL);\n-\tif (!rev.diffopt.output_format) {\n+\tif (!rev.diffopt.output_format)\n \t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n-\t\tif (diff_setup_done(&rev.diffopt) < 0)\n-\t\t\tdie(\"diff_setup_done failed\");\n-\t}\n \trev.diffopt.allow_external = 1;\n \trev.diffopt.recursive = 1;\n+\tif (diff_setup_done(&rev.diffopt) < 0)\n+\t\tdie(\"diff_setup_done failed\");\n \n \t/* If the user asked for our exit code then don't start a\n \t * pager or we would end up reporting its exit code instead.\n"},{"id":"53040","messageId":"alpine.LFD.0.999.0709141017450.16478@woody.linux-foundation.org","threadId":"9787","inReplyTo":"alpine.LFD.0.999.0709141014130.16478@woody.linux-foundation.org","subject":"[PATCH 2/2] Fix the rename detection limit checking","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-14T17:39:48Z","receivedAt":"2007-09-14T17:39:48Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis adds more proper rename detection limits. Instead of just checking \nthe limit against the number of potential rename destinations, we verify \nthat the rename matrix (which is what really matters) doesn't grow \nridiculously large, and we also make sure that we don't overflow when \ndoing the matrix size calculation.\n\nThis also changes the default limits from unlimited, to a rename matrix \nthat is limited to 100 entries on a side. You can raise it with the config \nentry, or by using the \"-l<n>\" command line flag, but at least the default \nis now a sane number that avoids spending lots of time (and memory) in \nsituations that likely don't merit it.\n\nThe choice of default value is of course very debatable. Limiting the \nrename matrix to a 100x100 size will mean that even if you have just one \nobvious rename, but you also create (or delete) 10,000 files, the rename \nmatrix will be so big that we disable the heuristics. Sounds reasonable to \nme, but let's see if people hit this (and, perhaps more importantly, \nactually *care*) in real life.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nDue to the overflow issue (and yes, Dmitry's test-case actually triggered \nan overflow even on 64-bit machines, because the math was done on \"int\" \ntypes), I really think this was a real bug.\n\nNow, whether this is necessarily the right way to fix it, I dunno, but it \nalso *does* fix the old broken limit handling that was based just on the \nnumber of target files, and didn't take the number of potential source \nfiles into account.\n\nBut the fix to that logic also means that the meaning of \"-l<n>\" changes \nsubtly. For the better, I think, but changes nonetheless.\n\nI'd also like to apologize to the change in wt-status.c, but that code \ndoesn't use the regular diffopt setup logic, so it really is a special \ncase and needs to be handled as such. Do we want to teach \"git runstatus\" \nto actually honor command line flags for diff generation etc? That's a \nseparate question, this patch just makes it use the same rename default as \nthe normal diffs now do.\n\n diff.c            |    2 +-\n diffcore-rename.c |   19 +++++++++++++++++--\n wt-status.c       |    1 +\n 3 files changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1aca5df..0ee9ea1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -17,7 +17,7 @@\n #endif\n \n static int diff_detect_rename_default;\n-static int diff_rename_limit_default = -1;\n+static int diff_rename_limit_default = 100;\n static int diff_use_color_default;\n int diff_auto_refresh_index = 1;\n \ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 6bde439..41b35c3 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -298,10 +298,25 @@ void diffcore_rename(struct diff_options *options)\n \t\telse if (detect_rename == DIFF_DETECT_COPY)\n \t\t\tregister_rename_src(p->one, 1, p->score);\n \t}\n-\tif (rename_dst_nr == 0 || rename_src_nr == 0 ||\n-\t    (0 < rename_limit && rename_limit < rename_dst_nr))\n+\tif (rename_dst_nr == 0 || rename_src_nr == 0)\n \t\tgoto cleanup; /* nothing to do */\n \n+\t/*\n+\t * This basically does a test for the rename matrix not\n+\t * growing larger than a \"rename_limit\" square matrix, ie:\n+\t *\n+\t *    rename_dst_nr * rename_src_nr > 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 (rename_dst_nr > rename_limit && rename_src_nr > rename_limit)\n+\t\tgoto cleanup;\n+\tif (rename_dst_nr * rename_src_nr > rename_limit * rename_limit)\n+\t\tgoto cleanup;\n+\n \t/* We really want to cull the candidates list early\n \t * with cheap tests in order to avoid doing deltas.\n \t * The first round matches up the up-to-date entries,\ndiff --git a/wt-status.c b/wt-status.c\nindex 5205420..10ce6ee 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -227,6 +227,7 @@ static void wt_status_print_updated(struct wt_status *s)\n \trev.diffopt.format_callback = wt_status_print_updated_cb;\n \trev.diffopt.format_callback_data = s;\n \trev.diffopt.detect_rename = 1;\n+\trev.diffopt.rename_limit = 100;\n \twt_read_cache(s);\n \trun_diff_index(&rev, 1);\n }\n"},{"id":"53045","messageId":"7v4phxaz3o.fsf@gitster.siamese.dyndns.org","threadId":"9787","inReplyTo":"alpine.LFD.0.999.0709141014130.16478@woody.linux-foundation.org","subject":"Re: [PATCH 1/2] Fix \"git diff\" setup code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-14T18:19:55Z","receivedAt":"2007-09-14T18:19:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> For some inexplicable reason, \"git diff\" would call \"diff_setup_done()\" \n> iff we hadn't given an explicit output format.\n>\n> That makes no sense, since much of what diff_setup_done() does is exactly \n> about checking the output format!\n\nWe did not even have _any_ setup_done() call in \"git diff\"\nitself in the original, because setup_revisions() has its own\ncall to setup_done(), and it always called setup_revisions()\nbefore you reached that part of the code you have in your patch.\nCommit 047fbe906b375e8a3a7564ad0e4443f62dd528a2 (\"builtin-diff:\nturn recursive on when defaulting to --patch format.\") added the\ncall to setup_done() only when we are defaulting to output\nformat, to re-validate the consistency of output format options.\n\nThe screw-up was commit fcfa33ec905fcde1c16e7cbbe00d7147b89f1f01\n(diff: make more cases implicit --no-index) that made the call\nto setup_revisions() conditional if setup_diff_no_index()\ndecides to take over, but it forgot that setup_diff_no_index()\ndoes not call setup_done().\n\nSo I tend to think the attached is a better fix.\n\n---\n diff-lib.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f5568c3..da55713 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -298,6 +298,8 @@ int setup_diff_no_index(struct rev_info *revs,\n \trevs->diffopt.nr_paths = 2;\n \trevs->diffopt.no_index = 1;\n \trevs->max_count = -2;\n+\tif (diff_setup_done(&revs->diffopt) < 0)\n+\t\tdie(\"diff_setup_done failed\");\n \treturn 0;\n }\n \n"},{"id":"53046","messageId":"7vsl5h9kc7.fsf@gitster.siamese.dyndns.org","threadId":"9787","inReplyTo":"alpine.LFD.0.999.0709132215250.16478@woody.linux-foundation.org","subject":"Re: [PATCH 1/2] git-commit: Disallow unchanged tree in non-merge mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-14T18:24:08Z","receivedAt":"2007-09-14T18:24:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Yeah, we should probably:\n>  - default to something larger but still reasonably sane (ie not 100, but \n>    perhaps 1000)\n>  - special-case the \"identical rename\", and have a higher limit for that \n>    (we already handle the identicals as a separate pass before we even \n>    start doing the similarity analysis - and it's the similarity analysis \n>    that can be the really expensive part)\n>\n> There's really no point in trying to do rename analysis for tons and tons \n> of files - even if we find perfect renames, the diff is going to be \n> unreadable by a human, so realistically nobody is ever going to care! A \n> machine won't care whether it was done as a create/delete or a rename, and \n> a human won't be bothered to read about thousands of renames, so we're \n> just wasting time trying to make it prettier.\n>\n> So quite arguably, the only case we really care about for renames is when \n> the numbers are small enough to be human-readable.\n\nI agree with that.  At the same time we might want to revisit\nthe earlier \"build a full matrix and pick the best ones\"\napproach commit 5c97558c9a813a0a775c438a79cfc438def00c22 (Detect\nrenames in diff family) introduced.\n\nA tangent.\n\nI've been thinking about updating the diffcore-rename for some\ntime to give bonus points to a filepair whose neighbors are\ndetected to be renames.  E.g. if you have this pair of preimage\nand postimage:\n\n\t(preimage)\t\t(postimage)\n\n\tarch/i386/foo.c\t\tarch/x86/foo-32.c\n\tarch/i386/bar.c\t\tarch/x86/bar-32.c\n\tarch/i386/baz.c\t\tarch/x86/baz-32.c\n\nand if foo.c and bar.c are found to be very similar to foo-32.c\nand bar-32.c while baz.c and baz-32.c are not that much, we may\nwant to take hints from the movement of neighbouring files and\nboost the similarity score between baz.c and baz-32.c pair.\n\nIt would be a quite an interesting coding challenge for anybody\nwho wants to get his hands dirty.  Would this be worth it in\npractice?  I dunno.\n\n\n\n\n        \n"},{"id":"53047","messageId":"alpine.LFD.0.999.0709141129451.16478@woody.linux-foundation.org","threadId":"9787","inReplyTo":"7v4phxaz3o.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Fix \"git diff\" setup code","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-14T18:30:53Z","receivedAt":"2007-09-14T18:30:53Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 14 Sep 2007, Junio C Hamano wrote:\n> \n> So I tend to think the attached is a better fix.\n\nAhh, yes, that explains the conditional. \n\nBut whatever gets us to actually verify our options, and fill in the right \ndefaults is ok by me!\n\n\t\tLinus\n"},{"id":"53050","messageId":"alpine.LFD.0.999.0709141132250.16478@woody.linux-foundation.org","threadId":"9787","inReplyTo":"alpine.LFD.0.999.0709141017450.16478@woody.linux-foundation.org","subject":"Re: [PATCH 2/2] Fix the rename detection limit checking","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-14T18:44:04Z","receivedAt":"2007-09-14T18:44:04Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 14 Sep 2007, Linus Torvalds wrote:\n>\n> ... and we also make sure that we don't overflow when doing the matrix \n> size calculation.\n\nSide note: by \"make sure\", I don't really mean a total guarantee.\n\nWe could be even more careful here. In particular:\n\n - we later do end up allocating the matrix with \n\n\tsizeof(*mx) * num_create * num_src\n\n   and I didn't actually fix the overflow that is possible due to the\n   \"sizeof(*mx)\" multiplication.\n\n - even after we've checked that not *both* of the source and destination \n   counts are larger than the rename_limit, we could still overflow the \n   multiplication in just the limit check.\n\n.. but with the rename_limit being set to 100, in practice neither of \nthese are really even close to realistic (ie you'd need to have less than \n100 new files, and deleted over twenty million files to overflow, or vice \nversa).\n\nSo with a rename_limit of 100, it's all good (I'm pretty sure you'd have \n*other* issues long before you'd hit the integer overflows on renames ;)\n\nBut if somebody sets the rename_limit to something bigger, it gets \nincreasingly easier to screw it up.\n\nIf somebody wants to be *really* careful, they'd need to do something like\n\n\tunsigned long max;\n\n\t/* This isn't going to overflow, since we limited 'rename_limit' */\n\tmax = rename_limit * rename_limit;\n\n\t/*\n\t * But we should also check that multiplying by \"sizeof(*mx)\" \n\t * won't make it overlof either..\n\t */\n\twhile ((sizeof(*mx) * max) / sizeof(*mx) != max)\n\t\tmax >>= 1;\n\n\t/*\n\t * And then avoid multiplying \"rename_dst_nr\" and \"rename_src_nr\"\n\t * together by turning it into a division instead\n\t */\n\tif (max / rename_dst_nr > rename_src_nr)\n\t\tgoto cleanup;\n\nbut the patch I sent out was the \"obvious\" first one that at least avoided \nthe overflow for the triggerable case that Dmitry had, and as per above \nlikely in all reasonable cases...\n\n\t\t\tLinus\n"},{"id":"53051","messageId":"alpine.LFD.0.999.0709141146110.16478@woody.linux-foundation.org","threadId":"9787","inReplyTo":"alpine.LFD.0.999.0709141132250.16478@woody.linux-foundation.org","subject":"Re: [PATCH 2/2] Fix the rename detection limit checking","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-14T18:49:58Z","receivedAt":"2007-09-14T18:49:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 14 Sep 2007, Linus Torvalds wrote:\n> \n> but the patch I sent out was the \"obvious\" first one that at least avoided \n> the overflow for the triggerable case that Dmitry had, and as per above \n> likely in all reasonable cases...\n\nFinal note (I promise): the patch I sent out took \"git runstatus\" times on \nthe workload I replicated from Dmitry down from \"so long you'd ^C it\" to \nabout two seconds..\n\nSo I wanted to point out that this was not just the correctness issue of \nthe overflow, but that the rename limiting really does need to be done for \npurely practical time reasons - doing the math in 64 bits would have \navoided the overflow, but wouldn't have avoided the real reason for not \nwanting to do these kinds of things in the first place!\n\n\t\tLinus\n"},{"id":"53052","messageId":"7vodg59i4x.fsf@gitster.siamese.dyndns.org","threadId":"9787","inReplyTo":"alpine.LFD.0.999.0709141129451.16478@woody.linux-foundation.org","subject":"Re: [PATCH 1/2] Fix \"git diff\" setup code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-14T19:11:42Z","receivedAt":"2007-09-14T19:11:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Fri, 14 Sep 2007, Junio C Hamano wrote:\n>> \n>> So I tend to think the attached is a better fix.\n>\n> Ahh, yes, that explains the conditional. \n>\n> But whatever gets us to actually verify our options, and fill in the right \n> defaults is ok by me!\n\nSorry, my explanation only explains about missing setup_done()\nwhen --no-index is used, but does not explain _if_ you actually\nfound that setup_done() was not called for you when you did a\nreal life test.  Was it only from code inspection, or did you\nhit a case where setup_done() is not run?  If the latter then\nthere is something else going on, as I cannot think of a way to\ncall setup_revisions() and not have it call setup_done()...\n"},{"id":"53054","messageId":"alpine.LFD.0.999.0709141238290.16478@woody.linux-foundation.org","threadId":"9787","inReplyTo":"7vodg59i4x.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Fix \"git diff\" setup code","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-14T19:46:51Z","receivedAt":"2007-09-14T19:46:51Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 14 Sep 2007, Junio C Hamano wrote:\n> \n> Sorry, my explanation only explains about missing setup_done()\n> when --no-index is used, but does not explain _if_ you actually\n> found that setup_done() was not called for you when you did a\n> real life test.  Was it only from code inspection, or did you\n> hit a case where setup_done() is not run?\n\nHmm. Mea culpa. What seems to have happened is that I ran things under \ngdb, and noticed that the default rename_limit hadn't been correctly set: \nbut now that I look more at it, that particular session was probably from \n\"git runstatus\", not \"git diff\".\n\nSo yeah, ignore my 1/2. It was almost certainly based on a bogus debugging \nsession, before I noticed that wt-status.c doesn't use the normal diff \nstuff at all..\n\n\t\t\tLinus\n"}]}