Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrong loc after author change or repo import #85

Closed
KrisThielemans opened this issue Jan 8, 2023 · 3 comments · Fixed by #88
Closed

wrong loc after author change or repo import #85

KrisThielemans opened this issue Jan 8, 2023 · 3 comments · Fixed by #88
Assignees
Labels

Comments

@KrisThielemans
Copy link

A while ago, I needed to incorporate someone else's code into our STIR repo. Unfortunately, I had commited his code in the original repo. So I manipulated history to assign some commits to him. (Sadly, I didn't record exactly what I did but I followed roughly https://stackoverflow.com/a/28845565/15030207). I then incorporated the repo into our STIR repo and moved files. (Roughly along the lines of https://stackoverflow.com/a/1684694/15030207). Checking now output of git fame, the relevant author doesn't get the credit he (=Carles Falcon) deserves. Am I doing something wrong with the call to git fame?

As running git fame -wMC on STIR take a very long time, I've tried to show an example with one of the relevant files is https://github.com/UCL/STIR/blob/master/src/recon_buildblock/PinholeSPECTUB_Weight3d.cxx, original name was wm_SPECT_mph2/weight3d_SPECT_mph.cpp`. For that I get

$ git fame  -wMC '--incl=PinholeSPECTUB_Weight3d.cxx|weight3d_SPECT_mph.cpp'
Total commits: 8021
Total ctimes: 30
Total files: 2
Total loc: 73
| Author                 |   loc |   coms |   fils |  distribution   |
|:-----------------------|------:|-------:|-------:|:----------------|
| Matthew Strugari       |    69 |     15 |      1 | 94.5/ 0.2/50.0  |
| Kris Thielemans        |     4 |   5625 |      1 | 5.5/70.1/50.0   |
| Alaleh Rashidnasab     |     0 |      1 |      0 | 0.0/ 0.0/ 0.0   |
| Alexander C. Whitehead |     0 |      5 |      0 | 0.0/ 0.1/ 0.0   |
| Alexey Zverovich       |     0 |      6 |      0 | 0.0/ 0.1/ 0.0   |
| Ander Biguri           |     0 |     63 |      0 | 0.0/ 0.8/ 0.0   |
| Ashley Gillman         |     0 |     50 |      0 | 0.0/ 0.6/ 0.0   |
| Benjamin Thomas        |     0 |     22 |      0 | 0.0/ 0.3/ 0.0   |
| C. Ross Schmidtlein    |     0 |      2 |      0 | 0.0/ 0.0/ 0.0   |
| Carles Falcon          |     0 |      2 |      0 | 0.0/ 0.0/ 0.0   |
...

Note that the "Total loc" reported is 73, while actually the file has some 1100 lines. All the ones from Carles are excluded for some reason. Of course, I could be making a mistake with the --incl option but Carles gets no credit when I don't specify an include.

git blame reports the correct thing, see here. Carles gets the correct number of commits, so maybe it's the way that I merged the original repo into STIR? (e.g. the commit does not "follow" from the first STIR commit).

The "re-authored" commit is UCL/STIR@dd6fdee. The PR with the move (and other fixes) is UCL/STIR#1100

@KrisThielemans
Copy link
Author

Something else that I notice is that the number of commits reports is not to the files included, but the whole repo. That might be intentional though.

@KrisThielemans
Copy link
Author

KrisThielemans commented Jan 8, 2023

A simpler case is probably https://github.com/UCL/STIR/blame/master/src/include/stir/NumericType.h where I didn't do the author change. The initial revision was by Alexei Zverovich, UCL/STIR@691d037, and some of his lines survive, (Weirdly, he's listed as a committer in the output above, but not when I don't have the --incl line). . This goes back 23 years though, and I remember having to do some filter-branch stuff to remove non-open source files, so again the history isn't quite standard.

(Edited: I've now re-run git fame -wMC on STIR without the --since and Alexei is credited ok, so forget about the "simpler example". sorry for the confusion!)

@casperdcl casperdcl self-assigned this Mar 9, 2023
@casperdcl casperdcl added the bug label Mar 9, 2023
casperdcl added a commit that referenced this issue Mar 9, 2023
- fixes #85
- fixes regression from #32
@casperdcl casperdcl linked a pull request Mar 9, 2023 that will close this issue
@casperdcl
Copy link
Owner

Sorry for the delay; thanks for reporting this. Should be fixed in git-fame>=2.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants