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

[WIP] Adding QMK Git Conventions doc; formatting cleanup on Learning Resources doc #3902

Merged
merged 8 commits into from
Oct 18, 2018

Conversation

noroadsleft
Copy link
Member

Please advise if I should separate these into two PRs. I added the QMK Git Conventions doc on a branch which already had the Learning Resources cleanup.

@drashna suggested adding a doc that went over best practices for using Git with QMK, so this is the initial attempt at that. I'm not sure I've written this in the proper scope, and ideally I'll be working with him to fix that if necessary and flesh this document out.

PR is mainly to get eyeballs on these changes.


```
git checkout master
git pull upstream master
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have the git fetch upstream here.

I know you mentioned it above, but it should be here explicitly.

Without it, tags won't get updated properly, for instance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is rebasing.... may want to add --rebase to the pull? Not sure if it's 100% needed.

But what we do need is a section on conflict resolution. Because it's not if, but when a merge conflict will happen.

git status is great for finding what file it is. But a GUI is helpful for finding the merge conflicts.

And git add . && git rebase --continue to add the changes and continue rebasing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always been under the impression that git pull runs a git fetch automatically. I nearly never use a git fetch on its own and I've never had any unintended behavior.

I was actually trying to avoid getting into git rebase in this section, as I feel it's a bit advanced for someone brand new to Git. For a later section, or maybe another page if this turns into a subsection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does, mostly, but it does not pull down the tags when it does this. To get the tags (eg 0.6.100), you need to run fetch first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it before I replied.

git hist is an alias for a --pretty-formatted git log. The git pull operation pulls down two PRs (3868 and 3879), and the tags are present in the second pass of git hist.

https://gist.github.com/noroadsleft/162887cf995798d4642187550b66c3b2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I had my aliases in git.... I'd show you that I was using git pull --rebase upstream master for a while. But it NEVER got the tags. I had to add git fetch before the pull command to get it to ACTUALLY get the tags.

So, there is something in there that isn't quite right. Or something with how git on MSYS works.

But I'm still saying that myself and others have HAD to use git fetch upstream to properly update the tags.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's the --rebase argument, which I don't think I've ever used.

Come to think of it, we might be talking about two different tasks here. The goal for this section is only about how to update the master branch. No need for --rebase here because we're not rebasing anything.

Though this doesn't do anything for dealing with merge conflicts, which I want to cover in a separate section or document (as part of a series).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested this. Does NOT pull the tags:

48a992f1c037658bbacccefd2709ffdcda8bb345 (HEAD -> master, upstream/master) Zeal60/Zeal65/M60-A implementation (#3879)
170de1273cde78d93b3f955aca133a4d690742b2 Add an easy way to create new keymaps for your favorite keyboard (#3868)
6d6d91c834ef3415425e21d895d4ec91c67fd4b8 (tag: 0.6.117, origin/master, origin/HEAD) rgblight.[ch] more configurable (#3582)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full output: https://gist.github.com/drashna/e414cc7a76bb7d98e87c32a27ff0baea

Notice how it only even showed the tags when I did git fetch upstream? it ignored them otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is Git Bash via Windows, or the fact that I'm using --graph. Those are the only reasons I can think that mine would work differently.

I'll be changing this.

docs/newbs_learn_more_resources.md Show resolved Hide resolved
@noroadsleft
Copy link
Member Author

Also @drashna, I think I'm going to title this "Best Practices," though I've grown attached to the idea of giving it the subtitle "Or, How I Learned to Stop Worrying and Love Git." 😆

Probably going to rename the file as well.

@drashna
Copy link
Member

drashna commented Sep 13, 2018

lol, have that as the tagline under the title. :D

```
git checkout master
git pull upstream master
git push origin master
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if the remote is set correctly, you don't need origin master here.

To make changes, create a new branch by entering `git checkout -b dev_branch`, which creates a new branch named `dev_branch` and checks it out. You can name your branch nearly anything you want, though it is recommended to name it something related to the changes you are going to make. By default `git checkout -b` will base your new branch on your currently-checked-out branch. You can base your new branch on an existing branch that is not checked out by adding the name of the existing branch to the command:

```
git checkout -b dev_branch master
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add git push --set-upstream origin dev_branch here.

This sets the upstream info, so only git push is needed in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this under the next section "Publishing Changes," but perhaps it is best covered here instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of keeping things "in the workflow". So if you'd set the upstream after creating the branch, then that's where it should be, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't push until my commits are ready, so my workflow is more

git checkout -b newbranch
[edit files here]
git add path/to/file
git commit
[maybe more edit/add/commit cycles]
git push

Sometimes my first git push on a branch occurs less than five minutes before a PR. 🤷‍♂️

@drashna
Copy link
Member

drashna commented Sep 15, 2018

Don't forget to add a link to the newbs.md file, as well as the sidebar/summary files!

@noroadsleft
Copy link
Member Author

Still need to write a walk-through on how to fix merge conflicts.

@drashna
Copy link
Member

drashna commented Sep 29, 2018

For the merge conflicts. git status to see what files are a problem.

Then a GUI editor is VERY nice to use. .... .... .... like VS Code. (and if you have the terminal set to bash or MSYS, you could control click the file)

And then git add . && git rebase --continue, or git add . && git merge --continue, depending on how you updated

@drashna
Copy link
Member

drashna commented Oct 3, 2018

@noroadsleft you can thank skully for it, but it may be worth to include info about "hub".
https://hub.github.com/

Among other things, it lets you open PRs from command line. :)

@noroadsleft
Copy link
Member Author

Irony: having to solve merge conflicts with a PR before you can write a walk-through of how to solve merge conflicts. 🤣

New Merge Conflict section probably needs a bit of a rewrite and some more detail and/or tips. Also nothing on Hub because I've never used it.

@drashna
Copy link
Member

drashna commented Oct 14, 2018

Well, there are a couple of reasons to install HUB.
Namely, better integration with github. The last ~10 PRs, I opened using hub, not github. Also, hub am lets you use a URL to the PR to download the commits and apply them to your branch. So, very useful things.
https://hub.github.com/hub.1.html

Also, it may be a good idea to see about going over how to sign git commits, as an optional thing.
https://help.github.com/articles/generating-a-new-gpg-key/

@noroadsleft
Copy link
Member Author

noroadsleft commented Oct 14, 2018

Re: Hub

It's more that having never used it, I don't feel qualified to write anything about it. If I do use and learn it though, then that's fine.

As for signing commits, how important is that? I don't think any of mine are signed.

@drashna
Copy link
Member

drashna commented Oct 14, 2018

Hub is basically a replacement for git. it does all the stuff that git does, but enhances some functionality, and adds new functionality. It's worth checking out, actually.

As for signing, I'm not really sure. But it's nice to see the "verified" label next to commits.

@noroadsleft
Copy link
Member Author

Re: Hub

Well I tried to install Hub and didn't even get that far. Hub apparently requires installing Go on Windows, and installing Go failed at basically every step. Seems the instructions that Go gives for installation are both incomplete and wrong in places. Plus it seems that it integrates with Windows' built-in Command Prompt... which I don't use basically ever (maybe a dozen times in the nearly four years I've had this computer).

I don't care to change my whole workflow to integrate this tool. I'm already set-up comfortably using Git Bash and MSYS2 (which, by the way, is what the Docs currently say to use for a Windows environment). If you care enough about this to write it up, that's fine. But I'm currently against this change.

@drashna
Copy link
Member

drashna commented Oct 15, 2018

For MSYS, the best way to install hub, is to drop the exe into (MSYS)\usr\bin, and just that.

And you can add alias git=hub to your .bashrc file. This causes the MSYS terminal to use hub instead of git, and then most everything works exactly the same as before. Plus more features.

@drashna
Copy link
Member

drashna commented Oct 18, 2018

Aside from the additional stuff that I mentioned, this looks like it's good to go, as is.

If there are no objections, I'm going to merge this.

@noroadsleft
Copy link
Member Author

Maybe a "Suggested Tools" doc as a separate PR?

@drashna
Copy link
Member

drashna commented Oct 18, 2018

Yeah, exactly. I think this is useful enough, as is. And we can add more advanced stuff like the recommended tools later.

@drashna drashna merged commit 480651c into qmk:master Oct 18, 2018
@noroadsleft noroadsleft deleted the nrl-docs branch October 18, 2018 22:12
zer09 pushed a commit to zer09/qmk_firmware that referenced this pull request Oct 27, 2018
…Resources doc (qmk#3902)

* Docs: newbs_learn_more_resources.md: formatting

* Added QMK Git Conventions doc, initial version

* Renamed contributing_qmk.md to newbs_best_practices.md

* Updated per review by @drashna

* Added navigation links

* Updated to Best Practices doc

* Minor updates to Learn More Resources doc

Markdown formatting consistency because I'm particular about it.

* Added Merge Conflict section to Best Practices doc
rseymour pushed a commit to rseymour/qmk_firmware that referenced this pull request Mar 13, 2019
…Resources doc (qmk#3902)

* Docs: newbs_learn_more_resources.md: formatting

* Added QMK Git Conventions doc, initial version

* Renamed contributing_qmk.md to newbs_best_practices.md

* Updated per review by @drashna

* Added navigation links

* Updated to Best Practices doc

* Minor updates to Learn More Resources doc

Markdown formatting consistency because I'm particular about it.

* Added Merge Conflict section to Best Practices doc
yamad pushed a commit to yamad/qmk_firmware that referenced this pull request Apr 10, 2019
…Resources doc (qmk#3902)

* Docs: newbs_learn_more_resources.md: formatting

* Added QMK Git Conventions doc, initial version

* Renamed contributing_qmk.md to newbs_best_practices.md

* Updated per review by @drashna

* Added navigation links

* Updated to Best Practices doc

* Minor updates to Learn More Resources doc

Markdown formatting consistency because I'm particular about it.

* Added Merge Conflict section to Best Practices doc
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.

2 participants