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

Don't work on your master #31

Closed
apeyser opened this issue Jun 16, 2015 · 17 comments
Closed

Don't work on your master #31

apeyser opened this issue Jun 16, 2015 · 17 comments
Assignees

Comments

@apeyser
Copy link
Contributor

apeyser commented Jun 16, 2015

Working on your master leads to really nasty merge histories. You'll work, then merge into our master, and then continue working without pulling up to upstream, then remerging. You should keep your master pristine, work on a branch, and then do a pull request on the branch. After that, you should delete the branch and make a new branch from master.

This is something to check for in a code review --- that the history isn't crazy. If so, the author should be requested to rebase, otherwise eventually it'll become very hard to identify were bugs occurred and merges will become increasingly difficult to do correctly.

@apeyser
Copy link
Contributor Author

apeyser commented Jun 16, 2015

You can check with gitk or at https://github.com/nest/nest-simulator/network. If the branch that is pull requested has already been merged into master... then reject.

@gewaltig
Copy link
Member

"Working on your master leads to really nasty merge histories. You'll work, then merge into our master, and then continue working without pulling up to upstream, then remerging. "
I am not sure I understand what you are saying here.

@apeyser
Copy link
Contributor Author

apeyser commented Jun 16, 2015

Repositories:

  • upstream is nest/nest-simulator
  • origin is apeyser/nest-simulator
  • local is your local machine

If origin is a fork of upstream, and local is a clone of origin, then we have three masters:

  • upstream/master
  • origin/master
  • local/master

upstream/master should only be changed via pull requests which are handled by the owners team.
origin/master should be pushed to from local/master
local/master should only pull --only-ff upstream master

and if you wish to work you should branch from local/master, push it to origin and then request that it be merged into upstream master via a pull request. At that point you could delete your local branch. When you update local/master from upstream master, it will be updated with what was your local branch, indirectly.

Otherwise, your local/master branch will diverge from upstream/master, and then your origin/master will diverge from upstream/master. You will end up really confused, others will end up really confused, the same patches will end up being applied multiple times through the branch history, and even worse things may happen.

@naveau
Copy link

naveau commented Jun 16, 2015

@apeyser: is this picture illustrate your point? I was also not entirely sure to understand!
git-workflow
from http://fr.slideshare.net/rhwy/my-git-workflow

@apeyser
Copy link
Contributor Author

apeyser commented Jun 16, 2015

Mikael, thanks, very nice. That is what you are supposed to do. If you work directly on your master, then step 7 would be a problem, and step 9. If your commits become interspersed on "ProjectA" with others, then you'd have to realize this and message your master by hand.

A number of developers have already had problems because of this, and I can see from the from the branching pattern that some more are also doing this and will eventually have problems, but if you follow the graphic, you have one way information flow which makes life easier.

@gewaltig
Copy link
Member

Dear Alexander and Michael
Thanks a lot for the clarification.

Best
Marc-Oliver

Alexander Peyser notifications@github.com schrieb am Di., 16. Juni 2015
um 22:18:

Mikael, thanks, very nice. That is what you are supposed to do. If you
work directly on your master, then step 7 would be a problem, and step 9.
If your commits become interspersed on "ProjectA" with others, then you'd
have to realize this and message your master by hand.

A number of developers have already had problems because of this, and I
can see from the from the branching pattern that some more are also doing
this and will eventually have problems, but if you follow the graphic, you
have one way information flow which makes life easier.


Reply to this email directly or view it on GitHub
#31 (comment).

@wschenck
Copy link
Contributor

I understand that interspersed commits become a problem when working on
your own master and pulling from upstream (team account in the
figure). But what would happen if you don't pull (which implies merge),
but instead fetch from upstream and then rebase your master on
upstream/master?

In my so far limited understanding of git, git should detect the double
commits in upstream/master and your local master and remove the local ones?

Am 16.06.2015 22:18, schrieb Alexander Peyser:

Mikael, thanks, very nice. That is what you are supposed to do. If you
work directly on your master, then step 7 would be a problem, and step
9. If your commits become interspersed on "ProjectA" with others, then
you'd have to realize this and message your master by hand.

A number of developers have already had problems because of this, and I
can see from the from the branching pattern that some more are also
doing this and will eventually have problems, but if you follow the
graphic, you have one way information flow which makes life easier.


Reply to this email directly or view it on GitHub
#31 (comment).

Dr.-Ing. Wolfram Schenck
SimLab Neuroscience
Jülich Supercomputing Centre (JSC)
Institute for Advanced Simulation
Tel +49 2461 61-6719
www.fz-juelich.de/ias/jsc



Forschungszentrum Juelich GmbH
52425 Juelich
Sitz der Gesellschaft: Juelich
Eingetragen im Handelsregister des Amtsgerichts Dueren Nr. HR B 3498
Vorsitzender des Aufsichtsrats: MinDir Dr. Karl Eugen Huthmacher
Geschaeftsfuehrung: Prof. Dr.-Ing. Wolfgang Marquardt (Vorsitzender),
Karsten Beneke (stellv. Vorsitzender), Prof. Dr.-Ing. Harald Bolt,

Prof. Dr. Sebastian M. Schmidt


@apeyser
Copy link
Contributor Author

apeyser commented Jun 17, 2015

Yes, if you were consistent you and understand rebase, you can do exactly what @wschenck suggests.

But why? Keeping your work on a separate branch and then rebasing the branch on master would give you the same effect with the additional advantage that git would then do extra bookkeeping for you, leading to a simpler and less error prone process.

@heplesser
Copy link
Contributor

@apeyser, thanks for these explanations. I just have one question now. Assume I made changes in my branch fix1, pushed and created a pull request. It may take a little while before my PR is merged into the upstream master. In the meantime, I would like to be able to use fix1 in another branch of mine, feature2. How can I do that? Would something like

git checkout feature2
git merge fix1

work without causing problems later? I assume that my PR for feature2 would then also contain the commits included in the PR for fix1, and depending on the order in which PRs are merged, feature2 might even be merged into upstream/master before fix1. Will Git keep things tidy?

@apeyser
Copy link
Contributor Author

apeyser commented Jun 19, 2015

If I get your question:

If you merge fix1 into feature2, then merging feature2 into master will merge fix1, thus you wouldn't make (or you'd cancel) the PR for fix1 when feature2 was merged. But I expect that if you're merging feature2, you'd merge fix1 into master first to minimize the size of each single merge.

So yes, history is global and if someone else had merged fix1 into feature3, then merging feature3 into master will not case a new set of fix1 patches to be introduced. Anything with the same hash is globally equal.

You could do even nicer things using options like rebasing, but for most people this is sufficient.

@lekshmideepu lekshmideepu added ZC: Documentation DO NOT USE THIS LABEL git workflow and removed ZC: Documentation DO NOT USE THIS LABEL labels Jun 19, 2015
@abigailm
Copy link
Contributor

@apeyser and @heplesser, what is required for this issue to be closed?

@jougs
Copy link
Contributor

jougs commented Jul 11, 2016

@abigailm: I think someone needs to review our documentation of the development workflow and check if the information in this issue is contained there. After that is the case, the issue can be closed.

@sanjayankur31
Copy link
Contributor

I can check the dev workflow if you'd like to assign this to me. :)

@abigailm
Copy link
Contributor

@sanjayankur31 delighted to, thank you! :)

@sanjayankur31
Copy link
Contributor

It looks good! Some minor points/suggestions:

  • The "Getting started with Git development" section could have a link to the Git book?
  • "Create your own fork of nest" and "Setup your fork" should be subsections under the main "Making your own fork of nest" section?
  • "Development workflow" -> "Suggested development workflow"?
  • In the "Basic Workflow" section, the language could be modified slightly to stress that one must first update their fork/local repo to match the state of the central Nest repo before creating a new feature branch. The steps show this already, but people tend to skip steps and stressing on this a bit more will ensure that they don't develop on outdated commits and then run into merge issues and that sort of thing.
  • Leave out "git pull"? It'll confuse novices and advanced users will know the fetch+merge vs pull argument already - especially since the next statement starts with "never use git pull ....".
  • Before "git add", mention clang-format + compiling + testing to check that the changes actually work - even a link to the c++ coding guidelines would be useful? (We should also maybe mention that new features should be accompanied by automated tests when possible)
  • Maybe mention how to write good commit messages?? Using the -m option to write verbose git commits is not a good idea as the post above explains.
  • Remove git commit -a? Bad in the hands of newbies and not something advanced users need to be told of.
  • I think it's worth mentioning that when a pull request fixes an issue, using a commit like "Fixes #xxxx" closes the ticket automatically when the pull request is merged to master. This will reduce the housekeeping burden for the dev team (at the moment we sometimes seem to have issues that have been fixed but not yet closed)
  • The "Additional things you may do" section should be a major section - to ensure that it's not part of the "Development workflow".
  • The "rebasing on master" and "recovering from messups" should go to the "Additional things you may do" - they shouldn't ideally be part of a normal workflow.
  • Maybe a warning when using rebase could be included.
  • Maybe git bisect is worth mentioning? (For an advanced audience)
  • Ideally people in the developer team that can merge pull requests should gpg sign their commits/merges. This isn't usually enforced in the open source community but it's a good practice to initiate. Unfortunately, I don't think the github web interface permits signing merge commits either. It needs to be done using the command line :/
  • Some of the use cases are incomplete - maybe move them to a separate TODO section?

Cheers!

@jougs
Copy link
Contributor

jougs commented Jul 13, 2016

Dear @sanjayankur31, many thanks for the good work!

Regarding the use cases: they were mainly there to get things started for us and we were planning to remove them. Do you think we need them? If yes, we can also polish them instead.

Would it be possible to start a pull request containing your suggestions? The developer space pages are contained in the branch gh-pages of the nest-simulator repository, so the process is the same as it would be for changing code.

@sanjayankur31
Copy link
Contributor

Regarding the use cases: they were mainly there to get things started for us and we were planning to remove them. Do you think we need them? If yes, we can also polish them instead.

I think they can be removed then.

Would it be possible to start a pull request containing your suggestions? The developer space pages are contained in the branch gh-pages of the nest-simulator repository, so the process is the same as it would be for changing code.

Sure. I'll begin working on this now.

@jougs jougs closed this as completed in 6d2d83a Jul 14, 2016
clinssen pushed a commit to clinssen/nest-simulator that referenced this issue Oct 22, 2018
suku248 pushed a commit to suku248/nest-simulator that referenced this issue Feb 7, 2019
Removed all traces of buffers in spike_detector.
heplesser pushed a commit to heplesser/nest-simulator that referenced this issue Apr 24, 2020
Renamed files and classes related to random number generation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants