Bug 39096 - Add "tidy all" commits to a git blame ignore file
Summary: Add "tidy all" commits to a git blame ignore file
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 38664
Blocks: 39149
  Show dependency treegraph
 
Reported: 2025-02-11 15:23 UTC by Jonathan Druart
Modified: 2025-05-16 16:02 UTC (History)
3 users (show)

See Also:
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:


Attachments
Bug 39096: Add 'tidy all' commits to a git blame ignore file (1.36 KB, patch)
2025-02-11 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39096: Add 'tidy all' commits to a git blame ignore file (1.33 KB, patch)
2025-02-13 15:34 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 39096: Add 'tidy all' commits to a git blame ignore file (1.38 KB, patch)
2025-02-16 21:35 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 39096: (follow-up) Fix failing t/Makefile.t test (976 bytes, patch)
2025-02-17 16:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 39096: Add .git-blame-ignore-revs mapping to Makefile.PL (794 bytes, patch)
2025-02-17 17:17 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.