Add section on using git #890

JakobDegen · 2020-09-24T14:40:13Z

Begins to address #850 .

I am mostly looking for feedback in these areas right now:

Scope: Does the current version assume too much or too little knowledge of git? Are there any big topics missing? For now, I have only focused on the things that are likely to be necessary to contribute. But there are a lot of additional helpful things that could be added. Should they be included in this page?

One thing that was mentioned in the issue that should probably be included is some documentation on dealing with submodules. Unfortunately, I have only a basic understanding of this myself so would not be comfortable writing this. Maybe someone else wants to pick this up, or it can wait for a future PR.

Another thing is addressing the capabilities of various IDEs more directly. I'm not sure how best to do this though, so if there are any more concrete ideas on covering this topic, I should be able to expand.
Understandability: This is my first major contribution to this guide; I've mostly tried to follow the format of other pages, as well as avoiding large walls of text and similar issues. But I'm rather uncertain about the structure and some other decisions, so feedback here is appreciated.
No-Merge Policy: This section is incomplete as I'm not sure what precise points are most relevant to this project. If someone more familiar with this decision could expand on this, I'll update the PR accordingly.

I'm definitely going to do a few more passes for grammar and syntax in any case as well.

jyn514

As a general note this feels a little wordy - it's not a big deal but it would be nice to make things more concise I think. Maybe you could move some more things out into links? It's always a tradeoff between getting people to read things and doing less work though, up to you.

Thank you so much for working on this! ❤️

src/SUMMARY.md

src/git.md

jyn514 · 2020-09-24T15:39:54Z

src/git.md

+but a single commit at the end with message "fixup tidy issue" is usually
+unhelpful, and it is easier for everyone else if you combine that commit with
+another that has a more meaningful commit message. In this case, you'll want to
+run `git rebase -i HEAD~2` to edit the last two commits and merge them


This works for the most recent two commits, but I normally rebase the whole branch while I'm at it.

Suggested change

run `git rebase -i HEAD~2` to edit the last two commits and merge them

run `git rebase -i $(git merge-base HEAD master)` to edit your commits and merge them

Maybe that's too much for a beginner guide though?

I have not addressed this yet, but I'm also not quite sure how to. A slightly more general approach to this section could indeed be useful, but I don't know how to write it in a way that doesn't sacrifice clarity.

Let's hold off on the more general approach for now, then. I'm planning to add a more 'advanced' section with things like range-diff anyway, I think this would be a good fit there.

src/git.md

This section addresses the biggest issues that new contributors, especially those with limited familiarity with git, are likely to face. This is still a WIP. Thanks to jyn for the recommended improvements!

The previous iteration of the page was often wordy and occasionally unclear. This has been cleaned up in places. Additionally, the TODO in the no-merge policy section has been removed and addressed.

JakobDegen · 2020-09-24T20:57:52Z

As a general note this feels a little wordy - it's not a big deal but it would be nice to make things more concise I think. Maybe you could move some more things out into links? It's always a tradeoff between getting people to read things and doing less work though, up to you.

I've tried to cut down on the wordiness in places. I think it is better, but it also still feels too much like a wall of text to me, especially in later sections. Maybe just using more subheaders and visually breaking the text up can help partition the page into short, easy to understand units. Will think about this and revisit

jyn514

A couple minor nits, and I wrote a giant paragraph you might want to make into its own section 😆 although maybe it should wait for the 'advanced' section. Up to you, I think this is getting pretty close though :)

src/git.md

jyn514

One last nit and then I think this looks good :) we can make changes in follow-ups.

cc @Lokathor - does this address your questions about rebasing? Is there any more info we should add?

jyn514 · 2020-09-25T22:48:13Z

src/git.md

+
+You also may want to squash just the last few commits together, possibly
+because they only represent "fixups" and not real changes.
+`git rebase --interactive HEAD~2` will allow you to edit the two commits only.


Suggested change

`git rebase --interactive HEAD~2` will allow you to edit the two commits only.

For example, `git rebase --interactive HEAD~2` will allow you to edit the two commits only.

jyn514 · 2020-09-29T12:56:17Z

Hi @JakobDegen , do you think you'll get a chance to work on this soon? It's almost ready to go, I'd love to have this info in the guide :)

camelid

This looks really good so far; thank you for working on this!

src/git.md

camelid · 2020-09-29T22:01:52Z

src/git.md

+
+The advice this gives is incorrect! Because of the "no-merge" policy, running
+`git pull` will create a merge commit, defeating the point of your rebase. Use
+`git push --force` instead.


We should recommend using --force-with-lease, as it won't overwrite new commits that haven't been pulled locally:

Suggested change

`git push --force` instead.

`git push --force-with-lease` instead.

I didn't know that existed! That's really useful :D

Yes, and it's even better with

git config --global alias.pushf "push --force-with-lease"

which makes it less likely that you do git push -f by mistake (instead you use git pushf). Maybe we should add that too? (I found it here.)

I'm not going to include this last thing just yet because I'm not quite sure how best to do this in a way that avoids introducing extra complexity. But the --force-with-lease thing is certainly right and a good idea.

src/SUMMARY.md

src/git.md

The Advanced Rebasing section has been mostly rewritten to include both a major suggestion from jyn and a general rewrite. Additional thanks to camelid for some suggestions!

JakobDegen · 2020-09-30T03:53:53Z

Sorry about the delay on this, some personal stuff got in the way over the weekend. If there's any residual issues let me know. Also feel free to ping me whenever related issues crop up.

jyn514

Thanks so much for working on this!

jyn514 self-assigned this Sep 24, 2020

jyn514 reviewed Sep 24, 2020

View reviewed changes

JakobDegen added 2 commits September 24, 2020 15:04

Add a section on using git.

7885733

This section addresses the biggest issues that new contributors, especially those with limited familiarity with git, are likely to face. This is still a WIP. Thanks to jyn for the recommended improvements!

Edit using git page for brevity and clarity.

360d4a5

The previous iteration of the page was often wordy and occasionally unclear. This has been cleaned up in places. Additionally, the TODO in the no-merge policy section has been removed and addressed.

JakobDegen force-pushed the git-instructions branch from aedcdcf to 360d4a5 Compare September 24, 2020 20:51

jyn514 reviewed Sep 24, 2020

View reviewed changes

jyn514 reviewed Sep 25, 2020

View reviewed changes

LeSeulArtichaut added the S-waiting-on-author Status: this PR is waiting for additional action by the OP label Sep 28, 2020

jyn514 mentioned this pull request Sep 29, 2020

ICEs should print the top of the query stack rust-lang/rust#76920

Closed

camelid mentioned this pull request Sep 29, 2020

Link to Git guide in rustc-dev-guide on merge conflicts rust-lang/homu#103

Merged

camelid reviewed Sep 29, 2020

View reviewed changes

Incorporated suggestions into git page and edit for more brevity.

074371e

The Advanced Rebasing section has been mostly rewritten to include both a major suggestion from jyn and a general rewrite. Additional thanks to camelid for some suggestions!

JakobDegen force-pushed the git-instructions branch from 49ac43f to 074371e Compare September 30, 2020 03:48

JakobDegen marked this pull request as ready for review September 30, 2020 03:49

JakobDegen changed the title ~~[WIP] Add section on using git~~ Add section on using git Sep 30, 2020

jyn514 approved these changes Sep 30, 2020

View reviewed changes

jyn514 merged commit c26ab70 into rust-lang:master Sep 30, 2020

This was referenced Sep 30, 2020

Add a section on how to use git #850

Closed

Difficult to find 'How to structure your PR' #831

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add section on using git #890

Add section on using git #890

JakobDegen commented Sep 24, 2020 •

edited

Loading

jyn514 left a comment

jyn514 Sep 24, 2020

JakobDegen Sep 24, 2020

jyn514 Sep 24, 2020

JakobDegen commented Sep 24, 2020

jyn514 left a comment

jyn514 left a comment

jyn514 Sep 25, 2020

jyn514 commented Sep 29, 2020

camelid left a comment

camelid Sep 29, 2020

jyn514 Sep 29, 2020

camelid Sep 29, 2020

JakobDegen Sep 30, 2020

JakobDegen commented Sep 30, 2020

jyn514 left a comment

	run `git rebase -i HEAD~2` to edit the last two commits and merge them
	run `git rebase -i $(git merge-base HEAD master)` to edit your commits and merge them

	`git rebase --interactive HEAD~2` will allow you to edit the two commits only.
	For example, `git rebase --interactive HEAD~2` will allow you to edit the two commits only.

	`git push --force` instead.
	`git push --force-with-lease` instead.

Add section on using git #890

Add section on using git #890

Conversation

JakobDegen commented Sep 24, 2020 • edited Loading

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobDegen commented Sep 24, 2020

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Sep 29, 2020

camelid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobDegen commented Sep 30, 2020

jyn514 left a comment

Choose a reason for hiding this comment

JakobDegen commented Sep 24, 2020 •

edited

Loading