threads / discuss / 54601

Request for change 610e2b9240 reversal

Subject: Request for change 610e2b9240 reversal

## tl;dr

4 messages between Nov 8, 2020 and Nov 10, 2020.

replies: 3people: 4as markdown or json

Jean-Yves Avenard· Nov 8, 2020, 11:15 UTC · lore
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· Nov 10, 2020, 11:38 UTC · re: Jean-Yves Avenard · lore

Re: Request for change 610e2b9240 reversal

Am 08.11.20 um 12:15 schrieb Jean-Yves Avenard:
Show 17 quoted lines
> 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
Barret Rhoden· Nov 10, 2020, 13:33 UTC · re: René Scharfe · lore

Re: Request for change 610e2b9240 reversal

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
Show 42 quoted lines
> ---
>   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· Nov 10, 2020, 17:05 UTC · re: Barret Rhoden · lore

Re: Request for change 610e2b9240 reversal

Barret Rhoden <brho@google.com> writes:
Show 12 quoted lines
> 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.
Show 40 quoted lines
>
>
>> ---
>>   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
>> 

← back to recent threads