haddocking / pdb-tools

A dependency-free cross-platform swiss army knife for PDB files.
https://haddocking.github.io/pdb-tools/
Apache License 2.0
372 stars 113 forks source link

[SKIP] updates CONTRIBUTING #78

Closed joaomcteixeira closed 3 years ago

joaomcteixeira commented 3 years ago

With the current GitHub Actions, if a developer does the normal contributing process as described in the CONTRIBUTING file, the merge step from the upstream to the origin will trigger the GitHub actions and commits can become messy in the fork as it happened to me.

I believe this can be avoided if in the commit title of the merging step from the upstream/master to the origin/master the [SKIP] tag is added.

codecov[bot] commented 3 years ago

Codecov Report

Merging #78 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #78   +/-   ##
=======================================
  Coverage   82.09%   82.09%           
=======================================
  Files          45       45           
  Lines        3574     3574           
  Branches      748      748           
=======================================
  Hits         2934     2934           
  Misses        451      451           
  Partials      189      189           

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 109b11e...ea922c3. Read the comment docs.

JoaoRodrigues commented 3 years ago

I don't understand how this is happening to you - I never have that problem. You can see in my branches when I make PRs, they never trigger actions. This will only happen if you push to the master branch of your own fork, which is a no-no anyway. We should get to the bottom of this - either something is wrong with the action or with your workflow πŸ˜›

joaomcteixeira commented 3 years ago

The problem comes with this pipeline:

(on my fork)
git checkout master
git remote add upstream [haddock/pdb-tools]
git fetch upstream
git merge (--no-ff) upstream/master
git push origin master

I can work with a different pipeline that gives no problem, being:

git clone [haddock/pdbtools]
git checkout -b new_branch
git remote add fork [my personal fork]
git push fork new_branch

and then PR.

But the question is: How to update the fork's master branch and have it synced with the upstream master branch without trigering the actions?

To my experience, with the workflow in the contribution.md it gives the problem. I am missing something?

JoaoRodrigues commented 3 years ago

OK, here's what I do:

(on master)
git pull origin master  # I use origin for the main fork
git checkout -B new_branch
# do stuff...
git push joaor new_branch
# do PR

I never have merge commits like this. If there are developments in master when I'm doing stuff (rarely) I can cherry-pick them to avoid the merge commit.

See also: https://stackoverflow.com/questions/16358418/how-to-avoid-merge-commit-hell-on-github-bitbucket

mtrellet commented 3 years ago

As the article says if there are new commits on the master while you're working on your new branch locally, "just" rebase your local repo to the master. We do that in the team on a daily basis and this works like a charm.

One thing the most upvoted answer forgets to add is to update the master before rebasing, that seems obvious but without that, you won't have much to rebase...

JoaoRodrigues commented 3 years ago

@mtrellet mind pasting here a quick example of the commands you would use to do that? Just for future reference!

mtrellet commented 3 years ago

Sure, I was a bit lazy, sorry for that!

So, kind of inspired by your workflow but I preferred to keep upstream as the main repo (haddocking one):

(on master)
git pull upstream master  # I use origin for the main fork
git checkout -B new_branch
# do stuff... commit...
git commit -m "wonderful new feature"
# sync origin master with upstream one (needed if upstream one has been updated)
git checkout master
git pull upsteam master
# replay commits on top of the master branch
git checkout new_branch
git rebase master # <-- HERE you could have conflicts. Just solve them and process for each commit you've made
# Push your commits
git push origin new_branch
# do PR
joaomcteixeira commented 3 years ago

@mtrellet I believe when you do git pull upsteam master because this comes with a commit to the master, it will trigger the github action as configured currently. Can you try it?

Also, on @JoaoRodrigues reply, what is origiin what is joaor ? I think in your example is just like my second example, origin is the official haddock repo and joaor is your fork. That works, but how do you sync your joaor/master to the haddocking/pdb-tools/master ?

@JoaoRodrigues The master in your fork is now 22 commits behind haddocking/pdb-tools/master. can you sync your fork's master to the haddocking/pdb-tools/master without triggering the github action? If you can, how?

JoaoRodrigues commented 3 years ago

On my local machine, origin point to haddocking and joaor to my fork. I find it more explicit than origin and upstream.

@joaomcteixeira I just did that. Because the last commit message has a [SKIP] tag, it doesn't trigger the action. In fact, unless the last commit message has the tag, the haddocking fork will trigger the version action and that will add a [SKIP]-tagged commit. So, it should never trigger an action on your end.

I think the problem is that when you merge without fast-forwarding, you get the 'merge' commit and that indeed triggers the action. The solution here is to just not to do that :)

joaomcteixeira commented 3 years ago

I just tested the git merge upstream/master adding [SKIP] in the commit message. It just works fine. Regardless of the strategy to merge, the key point is the need for the [SKIP]. And with the strategy you're proposing, if you remove the [SKIP] it triggers the action also.

So, bottom line :wink:, the strategy in the CONTRIBUTING.md needs to be updated in order to avoid the problem. Would you write an appropriate strategy? I believe the one in this PR solves it, but indeed yours is cleaner. Still, which command have you used to sync your master? I don't believe merging with --no-ff is bad anyway.

JoaoRodrigues commented 3 years ago

In the strategy I am proposing, you never have to edit messages :) I don't understand why you would have to do merges. Why doesn't git pull upstream master work fine with fast-forwarding?

To sync my master I just do what I showed before:

git checkout master
git pull origin master  # where origin points to haddocking
git push joaor master  # update my fork

There are no merge commits this way!

EDIT: OK, I tried it locally your way. First thing: git pull is git fetch followed by git merge. So what I'm doing can be rewritten as:

git checkout master
git fetch origin
git merge origin/master

Or something like this. If I use git merge --no-ff then I get the merge commit because it's done recursively. You want to avoid this! Is there a very specific reason why you are doing git fetch and git merge, instead of just git pull? Or, why would you use --no-ff?

JoaoRodrigues commented 3 years ago

Having concluded it's up to the merge (some people like pretty pictures on github git histories), I think we can close this, unless @joaomcteixeira thinks the wording can be improved? 😝

joaomcteixeira commented 3 years ago

:stuck_out_tongue_closed_eyes: I understood the conflict now. When I merge between my branches (forks) and branches and master in private repositories I always do the --no-ff tag because I like to keep the lines well represented in the git history :wink: However, I see that to update a master in fork from the upstream master it is better to just do fast forward. Yes, I knew pull is fetch + merge and now I remember I stopped using pull because I like the no-ff :ice_cream: Okay. Got a new strategy now. Something learned.

Let's close :+1: Thx @JoaoRodrigues :wink:

mtrellet commented 3 years ago

Just tried with my outdated fork, fetch + merge did work perfectly ;) This was FFed so no merge commit.