# Request for change 610e2b9240 reversal 4 messages from 2020-11-08 to 2020-11-10. Participants: Jean-Yves Avenard, René Scharfe, Barret Rhoden, Junio C Hamano. Thread: https://gitlist.dev/t/54601 ## Jean-Yves Avenard, 2020-11-08 11:15 Subject: Request for change 610e2b9240 reversal Message-ID: URL: https://gitlist.dev/e/CA%2BphgpFS_MOKOib5%2ByAE7U2QPMqMD_t%2ByXhR29UG9aSO4pgk%2BA%40mail.gmail.com ``` Hi At Mozilla we use a mercurial repository, however many developers are using a git repository; we maintain two git mirrors, mozilla-unified accessed via the cinnabar plugin and a geck-dev native git mirror. Our repositories contain a .git-blame-ignore-revs that is used for both repositories. https://searchfox.org/mozilla-central/source/.git-blame-ignore-revs That git was ignoring invalid entries (for the currently in use repo) is a requirement for our use. Following the merge of 610e2b9240 jc/blame-ignore-fix later we have lost the ability to run git blame on any of our files. Could we get that change reversed? If it ain't broken, don't fix it as they say. Thank you Jean-Yves Avenard ``` ## René Scharfe, 2020-11-10 11:38 Subject: Re: Request for change 610e2b9240 reversal Message-ID: <171b9a99-d48a-babe-168b-a7cd410e4635@web.de> URL: https://gitlist.dev/e/171b9a99-d48a-babe-168b-a7cd410e4635%40web.de In-Reply-To: ``` Am 08.11.20 um 12:15 schrieb Jean-Yves Avenard: > Hi > > At Mozilla we use a mercurial repository, however many developers are > using a git repository; we maintain two git mirrors, mozilla-unified > accessed via the cinnabar plugin and a geck-dev native git mirror. > > Our repositories contain a .git-blame-ignore-revs that is used for > both repositories. > https://searchfox.org/mozilla-central/source/.git-blame-ignore-revs > > That git was ignoring invalid entries (for the currently in use repo) > is a requirement for our use. > Following the merge of 610e2b9240 jc/blame-ignore-fix later we have > lost the ability to run git blame on any of our files. > > Could we get that change reversed? > If it ain't broken, don't fix it as they say. That looks like one tidily maintained ignore list! I saw that coming ([1], [2]), but didn't communicate my doubts clearly enough and undermined them when I wrote that I'd run into the same issue due to laziness. So I feel some responsibility for your trouble. Your actual use case is not lazy -- it uses the fundamental fact that Git object IDs are practically unique worldwide, without synchronization between repositories, i.e. they form a single flat namespace. 610e2b9240 (blame: validate and peel the object names on the ignore list, 2020-09-24) introduced two changes: Support for peeling tags and rejection of non-commits after peeling. Here's a patch for silently ignoring such non-commits read from a file. René [1] https://public-inbox.org/git/40488753-c179-4ce2-42d0-e57b5b1ec6cd@web.de/ [2] https://public-inbox.org/git/1fa730c4-eaef-2f32-e1b4-716a27ed4646@web.de/ -- >8 -- Subject: [PATCH] blame: silently ignore invalid ignore file objects Since 610e2b9240 (blame: validate and peel the object names on the ignore list, 2020-09-24) git blame reports checks if objects specified with --ignore-rev and in files loaded with --ignore-revs-file and config option blame.ignoreRevsFile are actual objects and dies if they aren't. The intent is to report typos to the user. This also breaks the ability to use a single ignore file for multiple repositories. Typos are presumably less likely in files than on the command line, so alerting is less useful here. Restore that feature by skipping non-commits without dying. Reported-by: Jean-Yves Avenard Signed-off-by: René Scharfe --- oidset.c | 5 +++-- t/t8013-blame-ignore-revs.sh | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/oidset.c b/oidset.c index 2d0ab76fb5..5aac633c1f 100644 --- a/oidset.c +++ b/oidset.c @@ -72,9 +72,10 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path, if (!sb.len) continue; - if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0' || - (fn && fn(&oid, cbdata))) + if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0') die("invalid object name: %s", sb.buf); + if (fn && fn(&oid, cbdata)) + continue; oidset_insert(set, &oid); } if (ferror(fp)) diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh index 24ae5018e8..b18633dee1 100755 --- a/t/t8013-blame-ignore-revs.sh +++ b/t/t8013-blame-ignore-revs.sh @@ -39,10 +39,10 @@ test_expect_success 'validate --ignore-rev' ' test_must_fail git blame --ignore-rev X^{tree} file ' -# Ensure bogus --ignore-revs-file requests are caught +# Ensure bogus --ignore-revs-file requests are silently accepted test_expect_success 'validate --ignore-revs-file' ' git rev-parse X^{tree} >ignore_x && - test_must_fail git blame --ignore-revs-file ignore_x file + git blame --ignore-revs-file ignore_x file ' for I in X XT -- 2.29.2 ``` ## Barret Rhoden, 2020-11-10 13:33 Subject: Re: Request for change 610e2b9240 reversal Message-ID: <416a4e32-f813-cb7f-406d-30c5d48034b3@google.com> URL: https://gitlist.dev/e/416a4e32-f813-cb7f-406d-30c5d48034b3%40google.com In-Reply-To: <171b9a99-d48a-babe-168b-a7cd410e4635@web.de> ``` Hi - On 11/10/20 6:38 AM, René Scharfe wrote: [snip] > Reported-by: Jean-Yves Avenard > Signed-off-by: René Scharfe patch looks good to me. Reviewed-by: Barret Rhoden Thanks, Barret > --- > oidset.c | 5 +++-- > t/t8013-blame-ignore-revs.sh | 4 ++-- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/oidset.c b/oidset.c > index 2d0ab76fb5..5aac633c1f 100644 > --- a/oidset.c > +++ b/oidset.c > @@ -72,9 +72,10 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path, > if (!sb.len) > continue; > > - if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0' || > - (fn && fn(&oid, cbdata))) > + if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0') > die("invalid object name: %s", sb.buf); > + if (fn && fn(&oid, cbdata)) > + continue; > oidset_insert(set, &oid); > } > if (ferror(fp)) > diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh > index 24ae5018e8..b18633dee1 100755 > --- a/t/t8013-blame-ignore-revs.sh > +++ b/t/t8013-blame-ignore-revs.sh > @@ -39,10 +39,10 @@ test_expect_success 'validate --ignore-rev' ' > test_must_fail git blame --ignore-rev X^{tree} file > ' > > -# Ensure bogus --ignore-revs-file requests are caught > +# Ensure bogus --ignore-revs-file requests are silently accepted > test_expect_success 'validate --ignore-revs-file' ' > git rev-parse X^{tree} >ignore_x && > - test_must_fail git blame --ignore-revs-file ignore_x file > + git blame --ignore-revs-file ignore_x file > ' > > for I in X XT > -- > 2.29.2 > ``` ## Junio C Hamano, 2020-11-10 17:05 Subject: Re: Request for change 610e2b9240 reversal Message-ID: URL: https://gitlist.dev/e/xmqq361h87mc.fsf%40gitster.c.googlers.com In-Reply-To: <416a4e32-f813-cb7f-406d-30c5d48034b3@google.com> ``` Barret Rhoden writes: > On 11/10/20 6:38 AM, René Scharfe wrote: > [snip] >> Reported-by: Jean-Yves Avenard >> Signed-off-by: René Scharfe > > patch looks good to me. > > Reviewed-by: Barret Rhoden > > Thanks, > > Barret Yes, I do recall that we discussed reverting half of that change and then I forgot to follow through. Thanks, all. Will queue. > > >> --- >> oidset.c | 5 +++-- >> t/t8013-blame-ignore-revs.sh | 4 ++-- >> 2 files changed, 5 insertions(+), 4 deletions(-) >> diff --git a/oidset.c b/oidset.c >> index 2d0ab76fb5..5aac633c1f 100644 >> --- a/oidset.c >> +++ b/oidset.c >> @@ -72,9 +72,10 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path, >> if (!sb.len) >> continue; >> - if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0' || >> - (fn && fn(&oid, cbdata))) >> + if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0') >> die("invalid object name: %s", sb.buf); >> + if (fn && fn(&oid, cbdata)) >> + continue; >> oidset_insert(set, &oid); >> } >> if (ferror(fp)) >> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh >> index 24ae5018e8..b18633dee1 100755 >> --- a/t/t8013-blame-ignore-revs.sh >> +++ b/t/t8013-blame-ignore-revs.sh >> @@ -39,10 +39,10 @@ test_expect_success 'validate --ignore-rev' ' >> test_must_fail git blame --ignore-rev X^{tree} file >> ' >> -# Ensure bogus --ignore-revs-file requests are caught >> +# Ensure bogus --ignore-revs-file requests are silently accepted >> test_expect_success 'validate --ignore-revs-file' ' >> git rev-parse X^{tree} >ignore_x && >> - test_must_fail git blame --ignore-revs-file ignore_x file >> + git blame --ignore-revs-file ignore_x file >> ' >> for I in X XT >> -- >> 2.29.2 >> ```