{"thread":{"id":"54601","subject":"Request for change 610e2b9240 reversal","startedAt":"2020-11-08T11:19:57Z","lastAt":"2020-11-10T17:05:57Z","messageCount":4,"participants":["Jean-Yves Avenard","René Scharfe","Barret Rhoden","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"409334","messageId":"CA+phgpFS_MOKOib5+yAE7U2QPMqMD_t+yXhR29UG9aSO4pgk+A@mail.gmail.com","threadId":"54601","inReplyTo":null,"subject":"Request for change 610e2b9240 reversal","fromName":"Jean-Yves Avenard","fromEmail":"jyavenard@mozilla.com","sentAt":"2020-11-08T11:15:04Z","receivedAt":"2020-11-08T11:19:57Z","isPatch":false,"sender":{"key":"jyavenard@mozilla.com","avatar":null},"body":"Hi\n\nAt Mozilla we use a mercurial repository, however many developers are\nusing a git repository; we maintain two git mirrors, mozilla-unified\naccessed via the cinnabar plugin and a geck-dev native git mirror.\n\nOur repositories contain a .git-blame-ignore-revs that is used for\nboth repositories.\nhttps://searchfox.org/mozilla-central/source/.git-blame-ignore-revs\n\nThat git was ignoring invalid entries (for the currently in use repo)\nis a requirement for our use.\nFollowing the merge of 610e2b9240 jc/blame-ignore-fix later we have\nlost the ability to run git blame on any of our files.\n\nCould we get that change reversed?\nIf it ain't broken, don't fix it as they say.\n\nThank you\nJean-Yves Avenard\n"},{"id":"409473","messageId":"171b9a99-d48a-babe-168b-a7cd410e4635@web.de","threadId":"54601","inReplyTo":"CA+phgpFS_MOKOib5+yAE7U2QPMqMD_t+yXhR29UG9aSO4pgk+A@mail.gmail.com","subject":"Re: Request for change 610e2b9240 reversal","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-10T11:38:27Z","receivedAt":"2020-11-10T11:38:36Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.11.20 um 12:15 schrieb Jean-Yves Avenard:\n> Hi\n>\n> At Mozilla we use a mercurial repository, however many developers are\n> using a git repository; we maintain two git mirrors, mozilla-unified\n> accessed via the cinnabar plugin and a geck-dev native git mirror.\n>\n> Our repositories contain a .git-blame-ignore-revs that is used for\n> both repositories.\n> https://searchfox.org/mozilla-central/source/.git-blame-ignore-revs\n>\n> That git was ignoring invalid entries (for the currently in use repo)\n> is a requirement for our use.\n> Following the merge of 610e2b9240 jc/blame-ignore-fix later we have\n> lost the ability to run git blame on any of our files.\n>\n> Could we get that change reversed?\n> If it ain't broken, don't fix it as they say.\n\nThat looks like one tidily maintained ignore list!\n\nI saw that coming ([1], [2]), but didn't communicate my doubts clearly\nenough and undermined them when I wrote that I'd run into the same issue\ndue to laziness. So I feel some responsibility for your trouble. Your\nactual use case is not lazy -- it uses the fundamental fact that Git\nobject IDs are practically unique worldwide, without synchronization\nbetween repositories, i.e. they form a single flat namespace.\n\n610e2b9240 (blame: validate and peel the object names on the ignore\nlist, 2020-09-24) introduced two changes: Support for peeling tags and\nrejection of non-commits after peeling. Here's a patch for silently\nignoring such non-commits read from a file.\n\nRené\n\n\n[1] https://public-inbox.org/git/40488753-c179-4ce2-42d0-e57b5b1ec6cd@web.de/\n[2] https://public-inbox.org/git/1fa730c4-eaef-2f32-e1b4-716a27ed4646@web.de/\n\n-- >8 --\nSubject: [PATCH] blame: silently ignore invalid ignore file objects\n\nSince 610e2b9240 (blame: validate and peel the object names on the\nignore list, 2020-09-24) git blame reports checks if objects specified\nwith --ignore-rev and in files loaded with --ignore-revs-file and config\noption blame.ignoreRevsFile are actual objects and dies if they aren't.\nThe intent is to report typos to the user.\n\nThis also breaks the ability to use a single ignore file for multiple\nrepositories. Typos are presumably less likely in files than on the\ncommand line, so alerting is less useful here. Restore that feature by\nskipping non-commits without dying.\n\nReported-by: Jean-Yves Avenard \nSigned-off-by: René Scharfe \n---\n oidset.c | 5 +++--\n t/t8013-blame-ignore-revs.sh | 4 ++--\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/oidset.c b/oidset.c\nindex 2d0ab76fb5..5aac633c1f 100644\n--- a/oidset.c\n+++ b/oidset.c\n@@ -72,9 +72,10 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,\n \t\tif (!sb.len)\n \t\t\tcontinue;\n\n-\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0' ||\n-\t\t (fn && fn(&oid, cbdata)))\n+\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n \t\t\tdie(\"invalid object name: %s\", sb.buf);\n+\t\tif (fn && fn(&oid, cbdata))\n+\t\t\tcontinue;\n \t\toidset_insert(set, &oid);\n \t}\n \tif (ferror(fp))\ndiff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh\nindex 24ae5018e8..b18633dee1 100755\n--- a/t/t8013-blame-ignore-revs.sh\n+++ b/t/t8013-blame-ignore-revs.sh\n@@ -39,10 +39,10 @@ test_expect_success 'validate --ignore-rev' '\n \ttest_must_fail git blame --ignore-rev X^{tree} file\n '\n\n-# Ensure bogus --ignore-revs-file requests are caught\n+# Ensure bogus --ignore-revs-file requests are silently accepted\n test_expect_success 'validate --ignore-revs-file' '\n \tgit rev-parse X^{tree} >ignore_x &&\n-\ttest_must_fail git blame --ignore-revs-file ignore_x file\n+\tgit blame --ignore-revs-file ignore_x file\n '\n\n for I in X XT\n--\n2.29.2\n"},{"id":"409484","messageId":"416a4e32-f813-cb7f-406d-30c5d48034b3@google.com","threadId":"54601","inReplyTo":"171b9a99-d48a-babe-168b-a7cd410e4635@web.de","subject":"Re: Request for change 610e2b9240 reversal","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2020-11-10T13:33:53Z","receivedAt":"2020-11-10T13:33:58Z","isPatch":false,"sender":{"key":"brho@google.com","avatar":null},"body":"Hi -\n\nOn 11/10/20 6:38 AM, René Scharfe wrote:\n[snip]\n> Reported-by: Jean-Yves Avenard \n> Signed-off-by: René Scharfe \n\npatch looks good to me.\n\nReviewed-by: Barret Rhoden \n\nThanks,\n\nBarret\n\n\n> ---\n> oidset.c | 5 +++--\n> t/t8013-blame-ignore-revs.sh | 4 ++--\n> 2 files changed, 5 insertions(+), 4 deletions(-)\n> \n> diff --git a/oidset.c b/oidset.c\n> index 2d0ab76fb5..5aac633c1f 100644\n> --- a/oidset.c\n> +++ b/oidset.c\n> @@ -72,9 +72,10 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,\n> \t\tif (!sb.len)\n> \t\t\tcontinue;\n> \n> -\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0' ||\n> -\t\t (fn && fn(&oid, cbdata)))\n> +\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n> \t\t\tdie(\"invalid object name: %s\", sb.buf);\n> +\t\tif (fn && fn(&oid, cbdata))\n> +\t\t\tcontinue;\n> \t\toidset_insert(set, &oid);\n> \t}\n> \tif (ferror(fp))\n> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh\n> index 24ae5018e8..b18633dee1 100755\n> --- a/t/t8013-blame-ignore-revs.sh\n> +++ b/t/t8013-blame-ignore-revs.sh\n> @@ -39,10 +39,10 @@ test_expect_success 'validate --ignore-rev' '\n> \ttest_must_fail git blame --ignore-rev X^{tree} file\n> '\n> \n> -# Ensure bogus --ignore-revs-file requests are caught\n> +# Ensure bogus --ignore-revs-file requests are silently accepted\n> test_expect_success 'validate --ignore-revs-file' '\n> \tgit rev-parse X^{tree} >ignore_x &&\n> -\ttest_must_fail git blame --ignore-revs-file ignore_x file\n> +\tgit blame --ignore-revs-file ignore_x file\n> '\n> \n> for I in X XT\n> --\n> 2.29.2\n> \n\n"},{"id":"409500","messageId":"xmqq361h87mc.fsf@gitster.c.googlers.com","threadId":"54601","inReplyTo":"416a4e32-f813-cb7f-406d-30c5d48034b3@google.com","subject":"Re: Request for change 610e2b9240 reversal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-10T17:05:47Z","receivedAt":"2020-11-10T17:05:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Barret Rhoden writes:\n\n> On 11/10/20 6:38 AM, René Scharfe wrote:\n> [snip]\n>> Reported-by: Jean-Yves Avenard \n>> Signed-off-by: René Scharfe \n>\n> patch looks good to me.\n>\n> Reviewed-by: Barret Rhoden \n>\n> Thanks,\n>\n> Barret\n\nYes, I do recall that we discussed reverting half of that change and\nthen I forgot to follow through.\n\nThanks, all. Will queue.\n\n>\n>\n>> ---\n>> oidset.c | 5 +++--\n>> t/t8013-blame-ignore-revs.sh | 4 ++--\n>> 2 files changed, 5 insertions(+), 4 deletions(-)\n>> diff --git a/oidset.c b/oidset.c\n>> index 2d0ab76fb5..5aac633c1f 100644\n>> --- a/oidset.c\n>> +++ b/oidset.c\n>> @@ -72,9 +72,10 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,\n>> \t\tif (!sb.len)\n>> \t\t\tcontinue;\n>> -\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0' ||\n>> -\t\t (fn && fn(&oid, cbdata)))\n>> +\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n>> \t\t\tdie(\"invalid object name: %s\", sb.buf);\n>> +\t\tif (fn && fn(&oid, cbdata))\n>> +\t\t\tcontinue;\n>> \t\toidset_insert(set, &oid);\n>> \t}\n>> \tif (ferror(fp))\n>> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh\n>> index 24ae5018e8..b18633dee1 100755\n>> --- a/t/t8013-blame-ignore-revs.sh\n>> +++ b/t/t8013-blame-ignore-revs.sh\n>> @@ -39,10 +39,10 @@ test_expect_success 'validate --ignore-rev' '\n>> \ttest_must_fail git blame --ignore-rev X^{tree} file\n>> '\n>> -# Ensure bogus --ignore-revs-file requests are caught\n>> +# Ensure bogus --ignore-revs-file requests are silently accepted\n>> test_expect_success 'validate --ignore-revs-file' '\n>> \tgit rev-parse X^{tree} >ignore_x &&\n>> -\ttest_must_fail git blame --ignore-revs-file ignore_x file\n>> +\tgit blame --ignore-revs-file ignore_x file\n>> '\n>> for I in X XT\n>> --\n>> 2.29.2\n>> \n"}]}