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

GitHub Contribution Workflow #5

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

OpenJohnMacGregor
Copy link
Collaborator

@OpenJohnMacGregor OpenJohnMacGregor commented Sep 29, 2021

Work related to Issue #4 (! Numbering changed...)

This addresses Issue elisa-tech#2

- Updated the nav bar structure
- Added a link to the GitHub contribution document in the nav bar
- Wrote an initial version of the contribution document

Signed-off-by: John MacGregor <open.john.macgregor@gmail.com>
…/orientation into githubWorkflow

Apparently the merge on the ELISA repo put this branch one commit behind.
This work contributes to Issue elisa-tech#2

-	After trying it in GitHub found out that the hashtag syntax
	works in GitHub, but the syntax used in the document was
	misleading - changed, hopefully improved
-	In the Update localRepo section, two code snippets weren't
	displayed properly

Signed-off-by: John MacGregor <open.john.macgregor@gmail.com>
This work contributes to Issue elisa-tech#2

-	above
-	also fixed missing code snippet highlighting in the Create
	a newFeature branch section

Signed-off-by: John MacGregor <open.john.macgregor@gmail.com>
This work contributes to Issue elisa-tech#2

Simplified the workflow to sync the newFeature branch
with the projectRepo/master from pulling to newFeature/master
and then merging newFeature/master into newFeature to
fetching userRepo and rebasing newFeature.

Signed-off-by: John MacGregor <open.john.macgregor@gmail.com>
This work contributes to Issue elisa-tech#2

- to call it before creating a new branch
- to make it a default switch in the git pull command

Signed-off-by: John MacGregor <open.john.macgregor@gmail.com>
Copy link

@reiterative reiterative left a comment

Choose a reason for hiding this comment

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

Comments / suggestions from first pass

docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Show resolved Hide resolved
$ git pull origin master --ff-only

~~~

Choose a reason for hiding this comment

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

We can also fetch and base the feature branch directly on origin/master to minimize mess up potential even more as Paul suggested, see elisa-tech/wg-automotive#21

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm..... this one's as clear as mud to me.

Couldn't find Paul's comment in the automotive WG pull request. Maybe you mean his e-mail?

In the Automotive WG description, you've used git checkout with the --no-track option with origin/master as argument.

  • It's not clear to me why git would assume that the tracking branch on origin would be "master" and not the new branch
  • Does this have to be done every time the user switches to another branch and tries to switch back?
  • The git branch and git checkout documentation just say that it doesn't set up a new tracking branch configuration on origin, nothing about origin/master

In his e-mail, Paul used git pull for this action. I'm not so sure why a fetch at the position you've marked is better.

In the "Update localRepo from userRepo" section (in "Create Content"), I've mentioned using git fetch without providing a command. Fixed this. Please check.

Leaving this open. Maybe we should discuss this with Paul (don't see how to cc him...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops!

@reiterative I didn't read the comments close enough. I really wanted you to look at this... Sorry

Choose a reason for hiding this comment

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

There's no 'right way' to do this.

When creating a feature branch, I think the following sequence is easiest to remember (apart form the --ff-only flag, but you can make this the default behaviour)

  1. Checkout my local copy of master
    git checkout master
  2. Update that local copy from the remote (but only if I can do fast-forward merges)
    git pull --ff-only origin master
  3. Create a new branch from the updated master branch
    git checkout -b my_feature_branch

However, this runs the risk that the user will inadvertently omit step 3, make changes in master, and then end up in trouble when they later pull some changes from origin. (The trouble is even more likely to happen if they didn't specify --ff-only, because they may find themselves dealing with a merge conflict when they call git pull)

The alternative is to avoid switching to master altogether by following this sequence:

  1. Fetch all the latest changes from the remote
    git fetch origin
  2. Create a new branch from the remote branch, but don't track the upstream branch
    git checkout -b my_feature --no-track origin/master

However this syntax is not very memorable.

It is especially confusing if you don't understand the significance of origin/master, and what 'track' means in this context. You can read all the gory details here, but the TLDR is:

  • When you call git fetch remote_name, git creates (or updates) special copies of all the branches in remote_name within your local repository. (Conventionally, the default remote name is origin, but this name has no special significance)
  • These branches are referred to in your local repository using the name of the remote i.e. remote_name/branch_name and are treated differently to local branches. It's safest to think of them as 'bookmarks' or 'references' to the state of the branch in the remote repository.
  • By default, when you perform a git checkout on a remote branch (either git checkout remote_name branch_name or git checkout remote_name/branch_name), the new local branch that is created (the tracking branch) is set up with a default relationship with the remote branch (the upstream branch). Using the --no-track flag when creating a branch via git checkout -b omits this default behaviour
  • This means that the upstream branch is used as the default target for push and pull operations when you have the tracking branch checked out. In other words, if you call git push without specifying a remote or a remote branch, then git will push your changes to the upstream branch associated with your current branch.

The reason git fetch is arguably preferable is that git pull is actually two operations (fetch and merge) wrapped in one, with some default behaviour that can terrify the newbie. Encouraging new git users to use explicit fetch and merge operations can help them to grok this more quickly. It also allows them to review the changes that they have fetched before deciding whether to merge them into their local branch.

Copy link

@reiterative reiterative Oct 21, 2021

Choose a reason for hiding this comment

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

Answering your questions more directly:

  • It's not clear to me why git would assume that the tracking branch on origin would
    be "master" and not the new branch

It would assume that because you are explicitly telling it to create the new branch from origin/master (i.e. the remote copy of master), and the default behaviour when git checkout -b is called on a remote branch is to track that remote branch.

  • Does this have to be done every time the user switches to another branch and tries
    to switch back?

No. The --no-track is only needed when creating the branch.

  • The git branch and git checkout documentation just say that it doesn't set up a
    new tracking branch configuration on origin, nothing about origin/master

origin/master is a local copy of the remote branch that represents the last-fetched state of that branch

In his e-mail, Paul used git pull for this action. I'm not so sure why a fetch at
the position you've marked is better.

See previous comment. It's arguably better to git fetch because git pull may result in an inadvertent merge.

docs/GitHubContributionWorkflow.md Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
docs/GitHubContributionWorkflow.md Outdated Show resolved Hide resolved
This work contributes to Issue elisa-tech#2

Fixed various issues identfied by
- Paul Albertella on the 30th of September 2021
- Jochen Kall on the 30th of September 2021

Signed-off-by: John MacGregor <open.john.macgregor@gmail.com>
This work contributes to Issue elisa-tech#2

- typo mastter
- Added a link to a general explanation of the fork/clone/pull request model in GitHub docs.
- Consistently put the link to the documentation directly behind a command reference
- Added a link to an explanation of the DCO

Signed-off-by: John MacGregor <open.john.macgregor@gmail.com>
@OpenJohnMacGregor OpenJohnMacGregor changed the title GitHub workflow GitHub Contribution Workflow Oct 19, 2021

You can also configure **newFeature** to be the default to be pushed to by using:
* `git push -u origin newFeature`{:.language-shell .highlight} when you first push it, or
* `git branch --set-upstream newFeature origin/newFeature`{:.language-shell .highlight} if you forget
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried it out. Got the following error message:
fatal: the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.
Git version: 2.33.1

Tried "set-upstream-to". Got the following error message:
fatal: branch 'origin/newFeature' does not exist

OK, mea culpa. i didn't push the branch to origin before creating content. Am now in the conundrum that I'd have to push an incomplete version to userRepo to create the tracking branch...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@reiterative Paul can you take a look at the above comment?

I just found the @mention facility and I'm trying it out. Can you confirm that you received a notification? Thx

Choose a reason for hiding this comment

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

Can confirm!

@OpenJohnMacGregor OpenJohnMacGregor linked an issue Oct 21, 2021 that may be closed by this pull request
Copy link

@Jochen-Kall Jochen-Kall left a comment

Choose a reason for hiding this comment

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

Testing how approval manifests ^^


$ git cs -a -m "commit message"

~~~
Copy link
Collaborator Author

@OpenJohnMacGregor OpenJohnMacGregor Nov 2, 2021

Choose a reason for hiding this comment

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

This section has to be looked at more closely, possibly after a discussion in the weekly sync.

Commits are marked as verified in the PR. See About commit signature verification.

a) Commits which are submitted from a local copy and signed with only name and email have no verification status displayed ==> implicitly not verified and cannot be verified by the maintainer.
b) Developers can set "Vigilant mode" to mark all of their commits as verified. We should probably recommend that they set it. Vigilant mode is in beta, however.

Adding @Jochen-Kall and @reiterative on cc

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.

Documentation for a Workflow to Contribute on GitHub
3 participants