Bug 39096

Summary: Add "tidy all" commits to a git blame ignore file
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to main --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: schodkowy.omegi-0r, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:
Bug Depends on: 38664    
Bug Blocks: 39149    
Attachments: Bug 39096: Add 'tidy all' commits to a git blame ignore file
Bug 39096: Add 'tidy all' commits to a git blame ignore file
Bug 39096: Add 'tidy all' commits to a git blame ignore file
Bug 39096: (follow-up) Fix failing t/Makefile.t test
Bug 39096: Add .git-blame-ignore-revs mapping to Makefile.PL

Description Jonathan Druart 2025-02-11 15:23:45 UTC
To get a relevant blame we want to exclude some commits from bug 38664.
Comment 1 Jonathan Druart 2025-02-11 15:28:26 UTC
Created attachment 177739 [details] [review]
Bug 39096: Add 'tidy all' commits to a git blame ignore file

To get a relevant blame we want to exclude some commits from bug 38664.

Test plan:
0. Do not apply this patch
1.
% git blame C4/Auth.pm (or other file)
Notice that there is a lot of reference to bug 38664 (you can see it
with the author "Koha Development Team")
2. Apply this patch
3. Tell git to use this new file:
 % git config blame.ignoreRevsFile .git-blame-ignore-revs

Repeat 1.
=> There are way less refs to "Koha Development Team"

Before:
% git blame C4/Auth.pm|grep "Koha Development Team" | wc -l
454
After:
% git blame C4/Auth.pm|grep "Koha Development Team" | wc -l
29

QA: Do we add the `git config` command that to ktd so devs won't need to do it?
Comment 2 Owen Leonard 2025-02-13 15:34:47 UTC
Created attachment 178027 [details] [review]
Bug 39096: Add 'tidy all' commits to a git blame ignore file

To get a relevant blame we want to exclude some commits from bug 38664.

Test plan:
0. Do not apply this patch
1.
% git blame C4/Auth.pm (or other file)
Notice that there is a lot of reference to bug 38664 (you can see it
with the author "Koha Development Team")
2. Apply this patch
3. Tell git to use this new file:
 % git config blame.ignoreRevsFile .git-blame-ignore-revs

Repeat 1.
=> There are way less refs to "Koha Development Team"

Before:
% git blame C4/Auth.pm|grep "Koha Development Team" | wc -l
454
After:
% git blame C4/Auth.pm|grep "Koha Development Team" | wc -l
29

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Owen Leonard 2025-02-13 15:35:19 UTC
(In reply to Jonathan Druart from comment #1)
> Created attachment 177739 [details] [review] [review]
> QA: Do we add the `git config` command that to ktd so devs won't need to do
> it?

I would think so
Comment 4 Michał 2025-02-13 17:19:02 UTC
Yeah I think adding it to ktd would be a nice QoL benefit, I can't think of downsides.

But I suggest trying to check if setting it as --global option would work, to avoid changing the configuration of the mounted repo. But even doing it on the level of git repo wouldn't be without advantages, since then the file would be recognized even if someone opens the repo in some IDE outside of ktd! So actually it might be better this way.

The only other thing I'm thinking is that some more of old tidy commits could be added here (like a6c9bd0eb55c32d5632625144775271f20aa15f7?). But I'm not sure of a good way to find and identify large commits right now to reasonably find all/most of the past ones, so don't mind it much, could always be added later.
Comment 5 Victor Grousset/tuxayo 2025-02-16 21:35:07 UTC
Created attachment 178128 [details] [review]
Bug 39096: Add 'tidy all' commits to a git blame ignore file

To get a relevant blame we want to exclude some commits from bug 38664.

Test plan:
0. Do not apply this patch
1.
% git blame C4/Auth.pm (or other file)
Notice that there is a lot of reference to bug 38664 (you can see it
with the author "Koha Development Team")
2. Apply this patch
3. Tell git to use this new file:
 % git config blame.ignoreRevsFile .git-blame-ignore-revs

Repeat 1.
=> There are way less refs to "Koha Development Team"

Before:
% git blame C4/Auth.pm|grep "Koha Development Team" | wc -l
454
After:
% git blame C4/Auth.pm|grep "Koha Development Team" | wc -l
29

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2025-02-16 21:37:02 UTC
(In reply to Jonathan Druart from comment #1)
> QA: Do we add the `git config` command that to ktd so devs won't need to do
> it?

Yes. As far as I searched, there isn't a default blame.ignoreRevsFile that is picked up. So a `git config` command in KTD to enable it seems reasonable.
Comment 7 Victor Grousset/tuxayo 2025-02-16 21:38:37 UTC
Tested on a vue file also. Works as expected. The remaining blame lines related to bug 38664 are those that are entirely new from bug 38664.

Works, makes sense, QA script happy, code looks good, passing QA :)
Comment 8 Katrin Fischer 2025-02-17 12:10:54 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 9 Katrin Fischer 2025-02-17 14:09:02 UTC
Looks like we are missing something here:
t_Makefile_t.All directories should be mapped: .git-blame-ignore-revs
Comment 10 Katrin Fischer 2025-02-17 16:26:18 UTC
(In reply to Katrin Fischer from comment #9)
> Looks like we are missing something here:
> t_Makefile_t.All directories should be mapped: .git-blame-ignore-revs

Can someone confirm this is the right fix?

- Add .git-blame-ignore-revs to MANIFEST.SKIP
- Modify t/Make.t to exclude .git-blame-ignore-revs

This would be about the same we do for .gitignore if I researched correctly.
Comment 11 Katrin Fischer 2025-02-17 16:29:54 UTC
Created attachment 178144 [details] [review]
Bug 39096: (follow-up) Fix failing t/Makefile.t test

Ignore .git-blame-ignore-revs for Makefile.t, like we do for .gitignore.
Comment 12 Tomás Cohen Arazi (tcohen) 2025-02-17 17:08:27 UTC
(In reply to Katrin Fischer from comment #11)
> Created attachment 178144 [details] [review] [review]
> Bug 39096: (follow-up) Fix failing t/Makefile.t test
> 
> Ignore .git-blame-ignore-revs for Makefile.t, like we do for .gitignore.

This was not correct.
Comment 13 Tomás Cohen Arazi (tcohen) 2025-02-17 17:17:14 UTC
Created attachment 178147 [details] [review]
Bug 39096: Add .git-blame-ignore-revs mapping to Makefile.PL

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Katrin Fischer 2025-02-18 09:03:24 UTC
Picking follow-up for main.