Navigation Menu

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

Fix missing ending newline in db structure dump: #8651

Merged
merged 1 commit into from Dec 30, 2012
Merged

Fix missing ending newline in db structure dump: #8651

merged 1 commit into from Dec 30, 2012

Conversation

ghost
Copy link

@ghost ghost commented Dec 29, 2012

Fix missing ending newline in db structure dump:

  When dumping database structure with `rake db:structure:dump` and
using migrations, the resulting file will not end with a newline char.
Although it's not mandatory, it breaks a lot of simple use cases with
programs like cat, more, grep, etc.

  This changes use `puts' instead of `<<' to append migration versions
data to the dump.

It would also help if future changes need to append more data in this
dump.

What do you think of it?

@jeremy
Copy link
Member

jeremy commented Dec 29, 2012

Looks good! Please squash to a single commit.

@ghost
Copy link
Author

ghost commented Dec 29, 2012

I did not found how to add the squashed branch here (which is
activerecord-structure_dump-ending_nl-squashed on my github clone),
but here his the squashed commit:

tjouan/rails@a87cd4d47ccff684a410c3bf6e1ee05506458326

@senny
Copy link
Member

senny commented Dec 30, 2012

the Pull Requests on github are reflect the contents of your branch. This means, you can put whatever you want on the branch you created the PR from and it will be updated to reflect that content. For your situation you should be able to push the squashed commit to the branch like so (involves a force push):

git push -f <remote-name-for-your-fork> a87cd4d47ccff684a410c3bf6e1ee05506458326:activerecord-structure_dump-ending_nl

While <remote-name-for-your-fork> is almost certainly "origin". This will push the commit a87cd4d to the branch "activerecord-structure_dump-ending_nl", which is the source of this PR.

  When dumping database structure with `rake db:structure:dump` and
using migrations, the resulting file will not end with a newline char.
Although it's not mandatory, it breaks a lot of simple use cases with
programs like cat, more, grep, etc.

  This changes use `puts' instead of `<<' to append migration versions
data to the dump and also split the line where this is happening as it
was a bit long.
@ghost
Copy link
Author

ghost commented Dec 30, 2012

Amazing, it worked!

@senny thank you for the detailed clarification. To be honest I must
admit I read that a few hours ago, but couldn't believe it was the
right way to do it as it seems to me that it violates a strong
principle when using DVCS like git: never rewrite a public branch!
So it feels kind of "wrong" to me :-)

However I now kind of understand it plays better with how GitHub
handles pull requests.

As a side note, the hash of the commit changed because I fixed a typo
in the commit message and rebased the change on latest master.

@senny
Copy link
Member

senny commented Dec 30, 2012

never rewrite a public branch!

This is a good principle to follow. I think it's important though to think about what you consider a public branch. The problems start when other people depend on your work. I would consider a branch in your own fork, which is open for discussion to still be a private branch. If someone depends on it, they need a good reason and should be able to fix the situation when your branch changes. I consider PR-Branches to be exposed local branches that you can reason about and you should be able to change them upside down.

@ghost
Copy link
Author

ghost commented Dec 30, 2012

Thank you again for more details, I agree it makes sense in the
context of pull requests.

rafaelfranca added a commit that referenced this pull request Dec 30, 2012
…ng_nl

Fix missing ending newline in db structure dump:
@rafaelfranca rafaelfranca merged commit 1202170 into rails:master Dec 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants