[all] Pull Request methodology

[Roman-Manevich]

I'm hoping that we can agree on a methodology for using git/github to publish new features. Here is my attempt.

[relokin]

Thanks @Roman-Manevich!

This is a very good starting point. This is very similar to the flow I would use but before we recommend that everyone adopts a flow that allows a linear history we need a wider consensus.

[maranget]

Hi all, thanks for this initiative. I'd like to be clear about what history we have in the end:

A linear history, with PR being merged one after the other, with explicit merge messages, without any overlap. This can be checked as git lg with the following lines in one's .gitconf file

[alias]
        lg = log --color --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr) %C(bold blue)<%an>%Creset' --abbrev-commit

We also want the tests (make test) to succeed after every commit (again to facilitate bisecting). A relatively light technique to achieve this is to squash the PR commits into one or a few ones (another technique is to run make all test after each commit, which is arguably heavier).

Hence, once the PR is ready, that is, once merge has been agreed, the PR authors can proceed as follows, from their branch (if I am not wrong):

1. Rebase on top of master with git rebase master.
2. Squash commits into one or a few with git rebase -i.
3. Force push with git push --force.

It is to be noticed that the merger can do the same, but this means pushing to the author's clone and slightly alters authorship.

Finally, the merger has to fill the merge message, it is often convenient to copy the initial message of the conversation, which usually describes the effect and purpose of the PR.

[relokin]

Thanks for this @maranget. I agree with everything just some questions/comments.

We also want the tests (make test) to succeed after every commit (again to facilitate bisecting). A relatively light technique to achieve this is to squash the PR commits into one or a few ones (another technique is to run make all test after each commit, which is arguably heavier).

I think there is still value in keeping multiple commits in a PR if:

• Each of the commit in the PR passes make test as you pointed out, and
• Each commit implements a logical change.
  Some PRs implement large features and being able to split into different logical changes, I think helps. In fact, it's way easier for the reviewer or anyone trying to understand a PR, if the reviewing load can be split in reasonably sized meaningful changes. And there is no downside, if one wants to see only merge commits, they can do so with git lg --merges. So squashing is not only reducing information.

On the other hand, sometime this is not information worth keeping. If the commit history in the PR is not clean, with fixes on top of each, I would argue that these sequence of changes is not worth keeping in the project's history, they only add noise and squashing is the right strategy.

It is to be noticed that the merger can do the same, but this means pushing to the author's clone and slightly alters authorship.

I agree it's not ideal, but when I squash commits together, I retain the Author information. git automatically changes the Commit field.

Finally, the merger has to fill the merge message, it is often convenient to copy the initial message of the conversation, which usually describes the effect and purpose of the PR.

That's a very good point. I will start doing it.

[artkhyzha]

LGTM -- this is more or less what I do too.

I added two nitpick-y comments, to check if I understood the flow correctly than anything else.

[artkhyzha]

Step 6 as I understand it assumes that the remote pf the fork is called "origin" and does not rely on there being a name for the fork. Step 7 as I understand it assumes that "upstream" is indeed 'herd/herdtools7" on Github.

So, maybe this could be something like "Create a fork and clone it. Add a remote for the original repository, call it upstream"?

[artkhyzha]

Strictly speaking not necessary for the sequence of steps here, but I wonder if it is useful to keep origin's master branch up to date with upstream's master branch. Presumably, step 2 is good to do branching off up-to-date master.

I usually press a button on the web interface for this ("Sync fork" on the forked page).

[relokin]

To make this more clear I think you can break it down into further steps
2. Clone your fork of herdtools7: git clone [email protected]:${GITHUB_USERNAME}/herdtools7
1 and 2 are steps need only the first time.

The one option would be to add herd/herdtools7 as a remote:
4. From the folder in which the repository is cloned add herd/herdtools7 as a remote: git remote add upstream https://github.com/herd/herdtools7.git
5. Fetch the latest refs form herd/herdtools7 git fetch upstream

Otherwise and as Artem suggested:
4. Sync your fork with the upstream repository using github's "Sync fork" (not sure of the details of this, I've never used it)
5. Fetch the latest refs of your remote repository with git fetch

[relokin]

Suggested change

	2. Create a git branch: `git checkout -b feature`
	2. Create a git branch: `git checkout upstream/master -b <feature_name>`

or if you follow Artem's approach
git checkout origin/master -b <feature_name>

[relokin]

As Artem said this is the default behaviour, if you want you can add a comment to this extend when you instruct users to add the upstream remote, but here this is unnecessary.

[HadrienRenaud]

Hi @Roman-Manevich, thank you for writing this.

I agree with the comments that most of those steps need more explanation.
I would like to add that external references can (and probably should) be used to provide more detailed and up-to-date explanations. For example, the steps on rebasing are confusing, and we could point to an external tutorial, for example git-scm.

A fear I have is that such a text would deter occasional contributors. For example, while rebasing is good for people that do 5 PR a week (me? 🙄), it might not be the easiest if you don't have a good knowledge of the code base and you only occasionally work on it, in which case a merge commit is probably fine.