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

Document SynthDef.writeOnce as a legacy method #1918

Merged
merged 3 commits into from
Mar 22, 2016

Conversation

nhthn
Copy link
Contributor

@nhthn nhthn commented Mar 22, 2016

As discussed in #1910.

@telephon telephon merged commit 9a0bc82 into supercollider:master Mar 22, 2016
@danstowell
Copy link
Member

What's all this "Merge pull request #1 from supercollider/master …" about? Seems like something weird happened when you created your branch. It makes for some really weird stuff in the commit history. This should have been (re)based directly on master before merging, I think

@telephon
Copy link
Member

Ah ok, I assumed that would be transparent once merged. Big issue or should we just remember this for the next time?

@nhthn
Copy link
Contributor Author

nhthn commented Mar 22, 2016

Sorry about those. I'm bad at git and I did some weird stuff while trying to update snappizz/master to match supercollider/master...

@danstowell
Copy link
Member

@telephon I don't really know...

@jamshark70
Copy link
Contributor

At this point, the history is in there... Even if you revert, it's still going to look funny.

The takeaways are:

  • If you're trying to create a PR and the history looks wrong, don't hit the Create button! Close the browser window, and ask somebody how to fix up the history locally.
  • If you're reviewing a PR and you see weird history, don't merge.
  • Reread the github help pages on forks if you're ever not sure what to do.

@bagong
Copy link
Contributor

bagong commented Mar 22, 2016

Or don't fork, hahahaha ;)

@bagong
Copy link
Contributor

bagong commented Mar 22, 2016

I think this shows that it did boil down to nothing:

git diff bc847721a 1d91c791

@jamshark70
Copy link
Contributor

Hm, Scott C. pointed out that the "merge PR" entry does not appear in the main repo's history. There's a spurious "merge remote tracking branch" entry, but no harm done.

But it's still safer to make sure everything looks right before merging permanently.

@bagong I maintain that the workflow you proposed elsewhere offers no greater insurance against error than github's recommendation. It's merely a matter of what feels better to you, but it is no safer.

@bagong
Copy link
Contributor

bagong commented Mar 23, 2016

If you promise not to get angry I'll defend my workflow, with the chance of learning something when I am shown to be wrong ;)

All this doesn't matter if you learn how to set default pull and push repos. But as long as you're not there it is safer to keep the branch names of the real origin what they are, and do your own stuff exclusively on branches you actually create yourself (with different names than master etc...)

Then pull will always bring in what you really should relate to, the data in the repo your pull request should diff from. The main advantage of "my" workflow: you don't have to manually merge/synch master here and there. I consider that safer (and significantly more practical).

I think "fork" is a Github rather than a git thing. That button alongside the badget "forked on Github" was an extremely successful marketing step, but the simplicity with which you can "fork" comes at the price that you don't synchronize automatically anymore with the source repo, and sometimes don't even realize it.

This argument that Github saves storage space etc., I don't think it's valid. For one I am not sure I want to save storage space for Github if it makes my workflow more complicated, but also: I am pretty sure the first thing Github does when data is pushed, is to see if a hash is already there, and replace it by a reference if yes.

@jamshark70
Copy link
Contributor

With no anger, real or implied:

If I understand, you're suggesting that contributors should set "origin" to point to supercollider/supercollider. Then, to make a PR, they should push the topic branch into the main SC repository.

For that, some developer has to give every potential contributor explicit write access to the repository. That's an administrative headache, not to mention that it raises the risk of an occasional contributor merging code into the main line without sufficient review.

Github's solution is that every potential contributor has her own version of the repository. GH also needs to be sure that the user's repository corresponds to the main one. Currently it does this by forks. I think it will not work if you clone SC and then push your local copy back to your own github account. It "works" for your own code maintenance, but it won't let you open a PR that way.

I could be wrong but I don't see a way to do entirely without forks. Whether to follow github's "origin" vs "upstream" nomenclature is a different issue, certainly debatable.

@crucialfelix
Copy link
Member

I agree with Mr. Jam Shark. Dangerous to set origin to supercollider. Keep
it on your personal.

Though github's hub workflow shows it as keeping origin,

Example workflow for contributing to a project:

$ git clone github/hub
$ cd hub# create a topic branch
$ git checkout -b feature
( making changes ... )
$ git commit -m "done with feature"

It's time to fork the repo!

$ git fork→ (forking repo on GitHub...)→ git remote add YOUR_USER
git://github.com/YOUR_USER/hub.git

push the changes to your new remote

$ git push YOUR_USER feature# open a pull request for the topic branch
you've just pushed
$ git pull-request→ (opens a text editor for your pull request message)

https://hub.github.com/ https://hub.github.com/

and I guess my origin is supercollider/supercollider

So I always pull my changes from there.

I push to crucialfelix/supercollider only when I am making a PR

I didn't read bagong's git workflow yet. sorry

On Wed, Mar 23, 2016 at 4:40 AM jamshark70 notifications@github.com wrote:

With no anger, real or implied:

If I understand, you're suggesting that contributors should set "origin"
to point to supercollider/supercollider. Then, to make a PR, they should
push the topic branch into the main SC repository.

For that, some developer has to give every potential contributor explicit
write access to the repository. That's an administrative headache, not to
mention that it raises the risk of an occasional contributor merging code
into the main line without sufficient review.

Github's solution is that every potential contributor has her own version
of the repository. GH also needs to be sure that the user's repository
corresponds to the main one. Currently it does this by forks. I think it
will not work if you clone SC and then push your local copy back to your
own github account. It "works" for your own code maintenance, but it won't
let you open a PR that way.

I could be wrong but I don't see a way to do entirely without forks.
Whether to follow github's "origin" vs "upstream" nomenclature is a
different issue, certainly debatable.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1918 (comment)

https://twitter.com/crucialfelix
https://medium.com/@crucialfelix
http://www.mattermind.com/

@bagong
Copy link
Contributor

bagong commented Mar 23, 2016

Chris, you say you're agreeing with James, but I think de facto you are agreeing with me ;) You say it's dangerous to set origin to supercollider, but from all I can see you are doing exactly that ;) I admit, there is a risk of accidentally pushing to supercollider/supercollider if you have write access, but making it a top priority rule to always make your push target explicit reduces that significantly. So "never-never-never":

git push

But always:

git push myRepo (and make sure git is set to push the current branch only)

@james, it's not really a terminological thing (although I do believe "origin" is misleading and "upstream" is obscure), but it's about where you pull from by default. Of course I don't advocate pushing to supercollider/supercollider, the repo would be a mess in a week. And that's not talking about write access.

If we step down further in that attempt to uncover hidden implications, the crucial point, I think, is here:

It "works" for your own code maintenance, but it won't let you open a PR that way.

Well, it will, but it's less obvious. You can press the green "create pull request" button and then choose another repository (supercollider/supercollider) to direct that PR to. And you can even choose the individual branches that are compared in the PR if you press "compare between forks".

So forking after all does have it's use, it makes directing a PR a little bit more simple (but it also makes it appear like something "magic", and then people direct the PRs to the wrong branch). So by all means, "fork", but do it like Chris ;), only use the fork to push to, but not as your default pull "origin", otherwise you'll have to manually sync your master and supercolliders master, which is very error prone.

The messy situation starts when people clone their own fork and then find it doesn't sync with supercollider/supercollider automagically. If you clone supercollider/supercollider and then learn how to push to your own repo that sync problem never occurs.

@crucialfelix
Copy link
Member

Yes, I know my origin is set to supercollider. I make sure never to push to
there, but accidents do happen.

I admit, there is a risk of accidentally pushing to supercollider/supercollider if you have write access

Like when Julian was trying out the github desktop client and it f*#$%ing
pushes to origin when you hit the "sync" button.

On Wed, Mar 23, 2016 at 10:27 AM Rainer Schütz notifications@github.com
wrote:

Chris, you say you're agreeing with James, but I think de facto you are
agreeing with me ;) You say it's dangerous to set origin to supercollider,
but from all I can see you are doing exactly that ;) I admit, there is a
risk of accidentally pushing to supercollider/supercollider if you have
write access, but making it a top priority rule to always make your push
target explicit reduces that significantly. So "never-never-never":

git push

But always:

git push myRepo (and make sure git is set to push the current branch only)

@james https://github.com/James, it's not really a terminological thing
(although I do believe "origin" is misleading and "upstream" is obscure),
but it's about where you pull from by default. Of course I don't
advocate pushing to supercollider/supercollider, the repo would be a mess
in a week. And that's not talking about write access.

If we step down further in that attempt to uncover hidden implications,
the crucial point, I think, is here:

It "works" for your own code maintenance, but it won't let you open a PR
that way.

Well, it will, but it's less obvious. You can press the green "create pull
request" button and then choose another repository
(supercollider/supercollider) to direct that PR to. And you can even choose
the individual branches that are compared in the PR if you press "compare
between forks".

So forking after all does have it's use, it makes directing a PR a little
bit more simple (but it also makes it appear like something "magic", and
then people direct the PRs to the wrong branch). So by all means, "fork",
but do it like Chris ;), only use the fork to push to, but not as your
default pull "origin", otherwise you'll have to manually sync your master
and supercolliders master, which is very error prone.

The messy situation starts when people clone their own fork and then find
it doesn't sync with supercollider/supercollider automagically. If you
clone supercollider/supercollider and then learn how to push to your own
repo that sync problem never occurs.


You are receiving this because you commented.

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

https://twitter.com/crucialfelix
https://medium.com/@crucialfelix
http://www.mattermind.com/

@jamshark70
Copy link
Contributor

@bagong let me see if I understand your approach:

  1. On github web, fork the main SC repository. (As noted, when making a PR, you can compare across forks... but that would seem to require that you've made a fork at some point.)
  2. Do not clone your forked repository. Clone the main one instead. (Now "origin" points to the main repo, and it should be used for pulling by default.)
  3. Add a remote, under whatever name you like. Use this for pushing.

If I understand correctly, I have no specific objection to the procedure. I question some of the discussion of it:

  • Saying "don't fork" when, as far as I can see, you'll still have to fork.
  • The claim that it protects against error. It protects against some errors that are likely using github's recommended procedure, but raises the risk of other errors that are no less dangerous. That was my point initially: in terms of risk management, it's a shift but not an improvement.

@bagong
Copy link
Contributor

bagong commented Mar 23, 2016

Yes, @jamshark70 , thanks for spelling out clearly and succinctly. I could try to split a hair on what a fork actually is, and what to weigh stronger, errors that occur every day 5 times vs. errors that are pretty rare (but dangerous), but basically I agree to every point you make.

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

Successfully merging this pull request may close these issues.

7 participants