-
Notifications
You must be signed in to change notification settings - Fork 184
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
REMERGE the sector-coupled model keeping the commit history #1133
Comments
Revert and then applying back is an option to test. It is advisable to check the size of the whole repo in doing so, to make sure the repo does not explode in size. I'd recommend to test it in a clean fork and test the change in size of the whole repo. @ekatef also mentioned her availability. |
Hey! You can also keep @siddharth-krishna in the loop. He is our git pro ;) |
Great @siddharth-krishna , any feedback would be welcome. Comments above. As a summary, we would like to replace a past squash and merge into a simple merge. For now options are:
As mentioned above, I wouldn't recommend force push either, but worth sharing all the options probably. @siddharth-krishna , what do you think? |
Thank you @siddharth-krishna for your suggestions. I'll review them tomorrow or Friday. In the meantime, it's important to highlight that the implemented solution must meet two critical requirements:
This is especially important because we will soon abandon the PyPSA-Earth-Sec repository (hopefully for good). So let's take the necessary time to solve this issue carefully ;) |
Hi Sid, This last suggestion actually sounds good. Very simple yet very smart. @davide-f, @ekatef, @FabianHofmann, any downsides you can foresee? Do you have suggestions for the way forward? @siddharth-krishna |
I've been experimenting on the git reset usage, mainly out of curiosity. What I did was: The result (luckily/surprisingly) does not look that bad actually. What do you think? *** if we decide to go for this, here we should add the --hard option. I didn't do that here, otherwise main and this were not easily comparable with the link above |
@davide-f I presume the option you are investigating requires us to force-push This is not a hard problem. As long as we force-push |
@hazemakhalek thanks for keeping me in the loop. I have no further suggestions and would like to leave this to the git pros. I totally support the motivation behind it though. |
To get a complete picture, have tested the initially suggested solution with reverting the merge commit and re-applying the merge properly. This exercise doesn't inflate size of the repo too much: disk usage by Have tested Option 3b, and it looks like an excellent solution:
Given that GitHub itself seems not to have any issues with displaying contributions if Option 3b is used, contributions PyPSA-Earth-Sec should be represented in an accurate way. @hazemakhalek may you have any ideas on which other metrics would be helpful to check? @siddharth-krishna thank you so much for working on this! A great solution and really amazing insights into git power 🙂 |
@davide-f thank you so much for finding time to experiment with the fix. Agree that the result looks very nice, though cherry-picking definitely requires a lot of concentration, given the need to the force-push. In any case, that is fantastic to see that the problem definitely can be resolved! ❤️ It looks like all the solutions have |
Just looking there is only one possible downside of options 1 and 3*: line contributions do not match the past repo. It is easy to check the current status in https://github.com/pypsa-meets-earth/pypsa-earth/graphs/contributors How does it appear with the other options? Do we want that? It seems conflicting to point 2 by hazem and may be conflicting also to -earth developer. The force option may be tested in a parallel branch first. Regardless of the method, I'd propose to solve this by the end of the week to normalize the approach. Current options are:
Can you express preferences/recommendations? |
Yes, it looks like this is true. The squash merged commit a898746 has
and
So I think GitHub attributed that entire commit to the author of the PR, Fabian. This can be confirmed in https://github.com/pypsa-meets-earth/pypsa-earth/graphs/contributors Testing the effect of Option 3b on GitHub's contributors pageI created a test repo https://github.com/siddharth-krishna/test-contributions to test how GitHub's contributions are counted when PRs are squash merged vs merge merged. I tried to simulate what happened here: I made some dummy commits on main (I hope you'll excuse me, Fabian, for using your name for this test):
And this resulted in the following contributors graph: I then made a branch
I then opened a PR siddharth-krishna/test-contributions#1, and did a squash merge. After the squashing, the contributors graph looked like: I then implemented Option 3b by making a redundant merge commit on the And the contributors graph looked like this: So it looks like GitHub attributes the squash merge as +1 commit to all authors involved, and also +L additions to all authors involved (where L is the number of lines added in the PR). If we then do Option 3b and make a redundant merge that brings in all the commits from the sec-branch, GitHub adds the correct number of commits and additions to each author. On the plus side, this option brings the number of commits of all contributors back to almost what it should be (pypsa-earth-sec authors will have +1 extra commit, but perhaps that's an acceptable error). It also preserves the relative ranking between pypsa-earth-sec authors in terms of additions. But on the minus side, pyspa-earth-sec authors now have double counted their additions from the sec-branch, especially compared to pypsa-earth contributors who have not contributed to pypsa-earth-sec. I suppose it all depends on which metric the contributors to both repos care about? And whether people would rather maintain an accurate "additions" metric, or force-push the main branch and potentially inconvenience people with open PRs and local branches. I am happy to support, no matter which option everyone ends up choosing. :) (If I have time tomorrow I could also investigate the effect of Option 1 on the contributors graph, but my guess would be that a revert would not result in a decrease in "additions".) |
I only now noticed the spike in line additions now. Thanks for pointing this out @davide-f @siddharth-krishna, great and insightful work! I personally only look at the number of commits. but I understand that this can be seen as unfair to the contributors of PyPSA-Earth. As a side note: The additions counter for the main pypsa-earth-sec contributors (Leon and myself) is already inflated because we had a few csvs in the repo back in the very early stages of the repo in Q1 2022. Lastly, I agree with Sid, I don't think this will automatically reduce the additions.. |
Great exploration @siddharth-krishna !!! <3 I believe we have all we need. The summary is: options that involve re-merging the merge basically bring the number of commits to the number that should be but they don't fix the line changes (additions/deletions), possibly leading to (partial) double counting of some line of code. I'd love to be able to see if with cherry-picking we could provide a better and cleaner approach without disrupting everything. I'd like to have an exchange with @hazem about it By next week, let's fix it :D |
I agree with everything above, but I had a few minutes this morning so for completeness (and my curiosity) I tested out what happens to contributions if we revert the squash merge: Testing the effect of Option 1 on the contributors graphAfter squash merging siddharth-krishna/test-contributions#3: After Option 1 (revert squash merge and merge merge the branch again): It looks like the revert commit counted as 1 commit and 3 deletions for me (Sid), and then the merge merge did the expected thing and got counted as 3 commits (2 on the branch and 1 merge commit) and 2 additions for me, and 1 commit and 1 addition for Fabian. Conclusion: I think Option 1 does not help in terms of fixing commit or additions count, and might muddy the metrics further since it also adds a bunch of deletions to the person who makes the revert commit. So, as mentioned by Hazem and Davide our options remain:
|
I tested Option 2 in this PR: https://github.com/davide-f/pypsa-earth/tree/main Using the procedure as follows: git reset 3d6365ac6af69b86fcadb1de9809ed09986e654f
git push --force
git clean -f
git fetch upstream
git status
git merge upstream/merge-pyspa-earth-sec
git cherry-pick 1777cb665ddb9d56d23aeb3f867a41ff1244742e
git cherry-pick 2350d55209bbeae09c6d019cfbf72d3e7989e80b
git cherry-pick d0fa6d6f3bccb18208c002a5d1d2353673e6a298
git cherry-pick 7b1249a20e6b56815329cc05ebf781e90b99d01f
git cherry-pick 7ef77254244e22982f4c7ba57b0fd63c7220101f
git push
git cherry-pick -m 1 ea99d748c1e7b7e6af9fd4ebad0554df70687e55
git commit --allow-empty
git cherry-pick 3142b1f71a1f73bfe753fd0107aa03c47e6740af
git cherry-pick ae4479fe3cb2405908d0962ff44de80e872873af
git cherry-pick 2cda6d768a28e79c8573be316a88e9a93c990242
git cherry-pick 3b8b952f978cf8f6c327284fc9f8e0f09c4e7d48
git cherry-pick 12edb982ef070470c08115a6ed752c0602dfa951
git cherry-pick 87d7f08f46faf9254ca8a7e31a01802a175a8a80
git cherry-pick 8fab463b47700523deefb679cca7eca75540e461
git cherry-pick a21ff8d77f2158503242c23a4cc803f8f30c046e
git push
git cherry-pick -m 1 328a32e7269853ed48c911a76e3282d5a76ed24d
git commit --allow-empty
git cherry-pick 356fb71eab053d62e9771a634c542391f5143524
git cherry-pick b64623d8d3d8127e186dbe3b9a899e326e1bcb72
git cherry-pick 75cb757f2a44464d94830812010dbd4e2c821e8c
git cherry-pick 2824af4fceff9a745e9d3ff1d02233422cd6b896
git cherry-pick 0a6f98c0104c859ba898f68a6230cf46ee401690
git cherry-pick 489ff8f9fb8b35cd4730910a23411da57ebd28e6 However, as a downside, the cherry-picks add the picker [me in this case] as co-author of each of the latest commits, though minimal. |
I'll schedule a call to finalize this issue. If you're interested to join please react to this. |
Today we met with @ekatef and @hazemakhalek and we are of common agreement that option 3b shall be the way to go. Therefore, we shall go for option 3b. Katia has given availability to do so, as she also tested it locally. |
The result of application Option 3a are here in this comparison. The new branch |
@ekatef thanks for trying it out! I took a look at your branch, and there might be 2 minor issues:
This can be seen in my test repository https://github.com/siddharth-krishna/test-contributions, where I implemented Option 3b on a branch called
Unfortunately, if you are at commit
Instead of the desired full history:
On the other hand, if we checkout
Perhaps the way to implement Option 3b is to apply the commands on a branch like |
Thanks a lot @siddharth-krishna! Regarding the difference for I have experimented with Option3a and can confirm that you are completely right on using GitHub pull requests. A pull request to Using GitHub PRsIf I'm merging the identical pull request in my fork (branch Using the local mergeWhen I'm applying Option 3b locally and push the result, the file history is brought back: Not sure if we could find a way to change parameters of Github merge... Agree that pushing local changes sound more realistically. I wonder if it could be a safer option, as compared with force-pushing the |
Have restored the history in my
The result seems to be alright:
Due to the issue with GitHub merge commit we discussed previously, the next step must be push of the fixed main to upstream. @davide-f is would be great to have your double-checking for that. |
@ekatef thanks for the dry run, and checking that the histories are preserved. I took a look at your fork and the branch 🤞 |
Preserve PyPSA-Earth-Sec history
The MERGE PR has been mistakenly squashed and merged and now the entire history of the pypsa-earth-sec repo is not shown in the main pypsa-earth repo.
Suggested solution:
Revert the merge
Remerge with preserving history
Reapply the following commits.
Important: Do not delete the merge branch
The text was updated successfully, but these errors were encountered: