{"thread":{"id":"60966","subject":"Bug: impure renames may be reported as pure renames","startedAt":"2024-02-20T20:58:42Z","lastAt":"2024-02-21T21:00:54Z","messageCount":4,"participants":["Mário Guimarães","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"489026","messageId":"CAF7CYk6dERu7Lb0iKeq7zwtZVVd_bG2FMZReSDeejcu1JGiaFw@mail.gmail.com","threadId":"60966","inReplyTo":null,"subject":"Bug: impure renames may be reported as pure renames","fromName":"Mário Guimarães","fromEmail":"mario.luis.guimaraes@gmail.com","sentAt":"2024-02-20T20:58:30Z","receivedAt":"2024-02-20T20:58:42Z","isPatch":false,"sender":{"key":"mario.luis.guimaraes@gmail.com","avatar":"https://gravatar.com/avatar/8e173e2e6a2c6632ccc7553f6528a149c83142191584c6edb1d6f34f91ff8a2b?d=mp&s=160"},"body":"Hello Git community,\n\nplease see the report below of what may be a bug.\n\nYours sincerely\nMário Guimarães\n\n======================================================\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\nIn the rust-lang/rust repository (I cloned today from GitHub), if we\nrun the command\n\n    git diff-tree -r -M a04e649^2 a04e649 --\ntests/ui/issues/issue-83190.rs\ntests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n\nwe get this result\n\n    :100644 100644 da931c3edaf6f9de6805e771f2b3b28edd27001f\n11b659eec97323ea5190dac1771c7ca3241861e7 R100\ntests/ui/issues/issue-83190.rs\ntests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n\nHowever, the source and destination files of the rename are not 100%\nequal. If we run this other command\n\n    git diff -M a04e649^2 a04e649 -- tests/ui/issues/issue-83190.rs\ntests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n\nwe get the following result\n\n    diff --git a/tests/ui/issues/issue-83190.rs\nb/tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n    similarity index 100%\n    rename from tests/ui/issues/issue-83190.rs\n    rename to tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n    index da931c3edaf..11b659eec97 100644\n    --- a/tests/ui/issues/issue-83190.rs\n    +++ b/tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n    @@ -1,7 +1,7 @@\n    -// check-pass\n    -\n     // Regression test for issue #83190, triggering an ICE in borrowck.\n\n    +// check-pass\n    +\n     pub trait Any {}\n     impl<T> Any for T {}\n\nWhat did you expect to happen? (Expected behavior)\n\nI was expecting to get from the `git diff-tree ...` command above a\nstatus of `R099`, but never `R100` which should be reserved for pure\nrenames (i.e., renames with no content modifications).\n\nIn fact, if we search for pure renames only, by using the `-M100%`\noption, such as running this command\n\n    git diff-tree -r -M100% a04e649^2 a04e649 --\ntests/ui/issues/issue-83190.rs\ntests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n\nwe get this output\n\n    :100644 000000 da931c3edaf6f9de6805e771f2b3b28edd27001f\n0000000000000000000000000000000000000000 D\ntests/ui/issues/issue-83190.rs\n    :000000 100644 0000000000000000000000000000000000000000\n11b659eec97323ea5190dac1771c7ca3241861e7 A\ntests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n\nThis suggests that the initial command in this report cannot return an\n`R100` status, as doing so it contradicts this last command.\n\nNote that we only get a rename in case we lower the similarity\npercentage, as demonstrated by this other command using the `-M99%`\noption\n\n    git diff-tree -r -M99% a04e649^2 a04e649 --\ntests/ui/issues/issue-83190.rs\ntests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n\nwhich outputs\n\n    :100644 100644 da931c3edaf6f9de6805e771f2b3b28edd27001f\n11b659eec97323ea5190dac1771c7ca3241861e7 R100\ntests/ui/issues/issue-83190.rs\ntests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n\nin which we see again `R100`, whereas the correct status should be `R099`.\n\nWhat happened instead? (Actual behavior)\n\nWhat's different between what you expected and what actually happened?\n\nAnything else you want to add:\n\nThe rename detection machinery should be fixed to report pure renames\n(`R100`) only when the source and destination files have exactly the\nsame contents.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[Info de sistema]\nversão git:\ngit version 2.42.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Darwin 20.6.0 Darwin Kernel Version 20.6.0: Thu Jul  6 22:12:47\nPDT 2023; root:xnu-7195.141.49.702.12~1/RELEASE_X86_64 x86_64\ninfo de compilador: clang: 13.0.0 (clang-1300.0.29.30)\ninfo de libc: informação libc indisponível\n"},{"id":"489047","messageId":"CABPp-BG2m3_fyxFL6Hw4o5HhoKVd7_tYchDxCdTaXxdxZke-9g@mail.gmail.com","threadId":"60966","inReplyTo":"CAF7CYk6dERu7Lb0iKeq7zwtZVVd_bG2FMZReSDeejcu1JGiaFw@mail.gmail.com","subject":"Re: Bug: impure renames may be reported as pure renames","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-02-21T02:12:00Z","receivedAt":"2024-02-21T02:13:55Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Feb 20, 2024 at 12:58 PM Mário Guimarães\n<mario.luis.guimaraes@gmail.com> wrote:\n>\n> Hello Git community,\n>\n> please see the report below of what may be a bug.\n>\n> Yours sincerely\n> Mário Guimarães\n>\n> ======================================================\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n> In the rust-lang/rust repository (I cloned today from GitHub), if we\n> run the command\n>\n>     git diff-tree -r -M a04e649^2 a04e649 --\n> tests/ui/issues/issue-83190.rs\n> tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n>\n> we get this result\n>\n>     :100644 100644 da931c3edaf6f9de6805e771f2b3b28edd27001f\n> 11b659eec97323ea5190dac1771c7ca3241861e7 R100\n> tests/ui/issues/issue-83190.rs\n> tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n>\n> However, the source and destination files of the rename are not 100%\n> equal. If we run this other command\n>\n>     git diff -M a04e649^2 a04e649 -- tests/ui/issues/issue-83190.rs\n> tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n>\n> we get the following result\n>\n>     diff --git a/tests/ui/issues/issue-83190.rs\n> b/tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n>     similarity index 100%\n>     rename from tests/ui/issues/issue-83190.rs\n>     rename to tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n>     index da931c3edaf..11b659eec97 100644\n>     --- a/tests/ui/issues/issue-83190.rs\n>     +++ b/tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs\n>     @@ -1,7 +1,7 @@\n>     -// check-pass\n>     -\n>      // Regression test for issue #83190, triggering an ICE in borrowck.\n>\n>     +// check-pass\n>     +\n>      pub trait Any {}\n>      impl<T> Any for T {}\n\nHeh, good point.  And more generally, due to how the similarity checks\nwork (split the file into lines/chunks, hash each to an integer, then\nsort the list of integers and compare the list of integers between two\nfiles), whenever you keep all the original lines of a file but permute\ntheir order, you will see a 100% match.\n\nMaybe a simple hack like the below would suffice to fix (untested --\nanyone want to test it out for me?):\n\ndiff --git a/diff.c b/diff.c\nindex a2def45644b..6b7b3a8b9af 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4405,7 +4405,7 @@ static void run_external_diff(const char *pgm,\n\n static int similarity_index(struct diff_filepair *p)\n {\n-       return p->score * 100 / MAX_SCORE;\n+       return p->score * 100 / (MAX_SCORE+1);\n }\n\n static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 5a6e2bcac71..3228a898e0b 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -314,7 +314,7 @@ static int find_identical_files(struct hashmap *srcs,\n                        break;\n        }\n        if (best) {\n-               record_rename_pair(dst_index, best->index, MAX_SCORE);\n+               record_rename_pair(dst_index, best->index, MAX_SCORE+1);\n                renames++;\n        }\n        return renames;\n\nWhich is based on the premise that 100% of source lines can match 100%\nof destination lines and result in a score of MAX_SCORE, so we\nmanually set the scoring to MAX_SCORE+1 when we detect exact renames\nbased on matching hashes and then use that higher value as the divisor\nwhen computing percentages.  (Note that there are several other places\nwhere \"MAX_SCORE\" is used and which I'm pretty sure we would _not_\nwant to replace with \"MAX_SCORE+1\", so if you don't like my patch and\nwant to redefine MAX_SCORE, then all those other locations would need\nto be adjusted instead.)\n"},{"id":"489089","messageId":"xmqqttm1pwsv.fsf@gitster.g","threadId":"60966","inReplyTo":"CABPp-BG2m3_fyxFL6Hw4o5HhoKVd7_tYchDxCdTaXxdxZke-9g@mail.gmail.com","subject":"Re: Bug: impure renames may be reported as pure renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-21T17:11:12Z","receivedAt":"2024-02-21T17:11:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Heh, good point.  And more generally, due to how the similarity checks\n> work (split the file into lines/chunks, hash each to an integer, then\n> sort the list of integers and compare the list of integers between two\n> files), whenever you keep all the original lines of a file but permute\n> their order, you will see a 100% match.\n\nStepping back a bit, there internally is a distinction between\n\"identical files\" and \"similar but not identical files\".  The\nfile with permuted lines are not identical but are very similar.\n\nThe decision not to surface these two to end-users was made long\nago, and after finding identical renames, we skip similarity\ncomputation when minimum_score is set to -M100 from the command\nline, which is a documented UI that cannot change.\n\nSo you are right to say that similarity index of an inexact rename\nshould be capped at 99 and never reach 100.  I do not know offhand\nif your \"how about\" computation ...\n\n>  static int similarity_index(struct diff_filepair *p)\n>  {\n> -       return p->score * 100 / MAX_SCORE;\n> +       return p->score * 100 / (MAX_SCORE+1);\n>  }\n\n... is the way to go, though.  We have filepair, so shouldn't we be\ndoing more like\n\n\tif (identical_rename(p))\n\t\treturn 100;\n\telse\n\t\treturn 99 * p->score / MAX_SCORE;\n\ninstead?\n\nIt will also keep working when a minor pet peeve of mine is fixed.\nIf you have a file with 1000 lines, all hashing to distinct values,\nand if you change only one line, even though the similarity is\ncomputed as 99% by the current logic, it actually is 99.9% that is\nmuch closer to 100%.  But we always round down, which feels wrong.\n\nAlternatively, we could cap p->score to (MAX_SCORE-1) when\nsimilarity is _computed_ for filepair between non-identical blobs.\nThat should happen in diffcore-rename.c:estimate_similarity().\nIt would allow us to still _show_ \"1 line changed among 1000 lines\"\ncase as R100 and still reject inexact renames via \"-M100\" from the\ncommand line, I think, as the \"exact renames only\" short-cut works\nwith the finer-grained \"score\" values and not the coarser \"percent\"\nvalues, which might give us a better approach.\n\n\n\n\n"},{"id":"489107","messageId":"xmqqzfvtmt1b.fsf@gitster.g","threadId":"60966","inReplyTo":"xmqqttm1pwsv.fsf@gitster.g","subject":"Re: Bug: impure renames may be reported as pure renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-21T21:00:48Z","receivedAt":"2024-02-21T21:00:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Alternatively, we could cap p->score to (MAX_SCORE-1) when\n> similarity is _computed_ for filepair between non-identical blobs.\n> That should happen in diffcore-rename.c:estimate_similarity().\n> It would allow us to still _show_ \"1 line changed among 1000 lines\"\n> case as R100 and still reject inexact renames via \"-M100\" from the\n> command line, I think, as the \"exact renames only\" short-cut works\n> with the finer-grained \"score\" values and not the coarser \"percent\"\n> values, which might give us a better approach.\n\n... the above may be \"better\" under a specific assumption: in the\nfuture, we would correct the rounding of scaling from 0..MAX_SCORE\n(or 0..MAX_SCORE-1) to 0..100 from the current \"always round down\"\nto \"below 0.5 is rounded down, 0.5 and above is rounded up\".  If\nthat happens, we would still honor the \"Give -M100 to limit the\nrename detection to exact ones\", and will exclude a change that\nmodifies a single line in 1000, but if you say -M99, we would show\nsuch a change and it would be labelled as R100 as correctly rounded.\n\n"}]}