Request for change 610e2b9240 reversal

Request for change 610e2b9240 reversal

[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.

Thank you
Jean-Yves Avenard

Re: Request for change 610e2b9240 reversal

[René Scharfe]

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 <jyavenard@mozilla.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 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

Re: Request for change 610e2b9240 reversal

[Barret Rhoden]

Hi -

On 11/10/20 6:38 AM, René Scharfe wrote:
[snip]
> Reported-by: Jean-Yves Avenard <jyavenard@mozilla.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>

patch looks good to me.

Reviewed-by: Barret Rhoden <brho@google.com>

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
> 

Re: Request for change 610e2b9240 reversal

[Junio C Hamano]

Barret Rhoden <brho@google.com> writes:

> On 11/10/20 6:38 AM, René Scharfe wrote:
> [snip]
>> Reported-by: Jean-Yves Avenard <jyavenard@mozilla.com>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>
> patch looks good to me.
>
> Reviewed-by: Barret Rhoden <brho@google.com>
>
> 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
>>