git workflow conventions for SasView

[pbeaucage]

Describe the bug
All CI tests, installers, and doc builds are running twice on commits to branches once a PR is created. Simple origin, most of our GH actions scripts use a trigger of [push,pull_request]

I propose that we run tests on [push], installers on [pull_request] and docs probably also on [push].

To Reproduce
PR commits have 24 run tests instead of the proper 10.

Expected behavior
Tests should not be duplicated.

[llimeht]

Running the unit tests only on push would potentially mean that the merged version doesn't ever get tested. Except in the situation where the merge will be a fast-forward, they are running adding different code.

For work in progress branches, potentially all three sets of tests need to be run. For merges, all three also need to be run. I'm not sure how to reduce the runs, or if it's even worth doing.

[andyfaff]

In many projects the preferred git workflow is for everyone to fork the main repository, then clone their fork to their local computer, submitting PRs to the main repository from a feature branch on their fork. (see https://numpy.org/doc/stable/dev/index.html#devindex).
If one does this, and you check in the CI configuration that the run is against sasview/sasview, then there will be no duplication of CI runs.
Moreover, this kind of workflow is inherently safer as pushes to the remote repository (e.g. git push origin <branch>) have less chance of either ruining someone else's work, or damaging the main repository. It also makes it easier to see the wood for the trees as there will be a lot less branches on the main repository.

[pkienzle]

@andyfaff does the fork style make it harder to collaborate on PRs? Can I easily switch my local repo to your fork:branch, do some changes, then push back to it? Or do I have to fork your fork and do a PR on your repo to share code with you?

[llimeht]

Looking back through the project's history for the last few years, the number of branches with more than one author is vanishingly small.

But yes, github has the ability to control access to individual branches on other forks. On every PR, there's a nice little tickbox that says "Allow edits and access to secrets by maintainers" with the help text "If checked, users with write access to SasView/sasview can add new commits to your «branch name» branch."

Or another way of putting it: of the several dozen different projects I contribute too, SasView is the only one using git like it is subversion.

Lots of branches within the one project mostly works... but it does mean that things don't get polished up prior to being made public (meaning that the git history is much messier and therefore harder to bisect if needed). There's also 337 stale remote branches here, 320 of which need not exist.

[andyfaff]

Let's say A and B have both forked a repository, and have then cloned their fork to their local computer. This means that their 'origin' is their own fork:

$ git remote -v
origin	https://github.com/B/sasview.git (fetch)
origin	https://github.com/B/sasview.git (push)

This saves accidental git push origin <x> from being too dangerous to the sasview repository and also means you're never going to overwrite other peoples branches because their branches don't live on your fork. (I believe the sasview repository has disabled direct pushes to the main branch).
If one wants to get updates from the sasview repository then they add that as a remote: git remote add sasview https://github.com/sasview/sasview.git. Keeping up to date with the sasview repository is then done by git checkout main; git fetch sasview; git rebase sasview/main; git push origin main.

Now let's say B creates a local feature branch and pushes it to their fork on github. B then submits a PR against the sasview repository from their fork.

A wants to contribute to the changes that B is making in their feature branch. A is also a maintainer to the sasview project. A has two ways of doing this, but they both involve a common first step.

A adds a remote to B's repository: git remote add B https://github.com/B/sasview.git. A can then checkout the feature branch that B created:

git fetch B     #  get the state of B's repository
git checkout feature_branch

A then has the same state as B's feature_branch on their local computer.

1. A can then create their own branch off of B's feature_branch, make changes, commit them, and push them to A's fork. On GitHub A then creates a PR against the feature_branch of B's forked repository. If B likes the changes A is making then they can merge those changes.
   -ALTERNATIVELY-
2. Because A is a maintainer of sasview they can checkout B's feature_branch, make changes, commit them. A can then directly push to B's branch if B allows it (i.e what @llimeht said above). git push B feature_branch.

Either way B's feature_branch PR against the sasview repository is then updated with the changes A made.
Using this style embraces the decentralisation that is git's central feature. It also encourages more branching because it's easier to experiment without messing up.

Let's consider how CI runs as well. On many projects the CI configurations are often setup to only trigger if it's the main sasview/sasview repository asking for them to be run (and often specialising against specific branches). If everyone's work lives in branches on the sasview/sasview repository then everytime a push is made the CI is triggered. CI is then working overtime. If people keep their working branches on their own forks, then the CIs won't be run nearly as often. The only time it will be triggered is if changes are pushed to a branch that is subject to a PR against the sasview repository.

[andyfaff]

Also, the 'everyone fork' approach has to be taken anyway by those who aren't part of the sasview maintainer team because they have to push their branches to their fork as they don't have permissions to push directly to sasview.
If instructions on this kind of git workflow were available it may encourage more users to contribute to the project.

[pkienzle]

also means you're never going to overwrite other peoples branches because their branches don't live on your fork.

@andyfaff What you consider a problem I consider a feature. I like working together on a branch to solve problems. It is much more efficient than working through code reviews. Also, git+vscode makes it easy to switch context between branches. I don't have a convenient workflow for separate forks.

Re: CI, I want to see tests passing/failing on the PR regardless of whether it is on sasview or on a fork, and whenever main is modified. There's no value running it on a branch that isn't in a PR since the result isn't reported anywhere. That is, I don't see the overall CI workload changing in the branch vs fork model if the CI is setup to do only what it needs to do.

@llimeht I don't understand why fork vs. branch will have any effect on git history. You are going to have the separate block chains for the different commit paths unless you are doing squash merges with rebase, and you can do that just as easily with branches as with forks.

Yes, cleaning up branches on merge would be useful. There's even a button for it on the PR page.

[llimeht]

Re: CI, I want to see tests passing/failing on the PR regardless of whether it is on sasview or on a fork, and whenever main is modified.

Agreed. That is currently the case and it should remain so. The CI status of every PR is shown within the PR regardless of the origin of the PR.

There's no value running it on a branch that isn't in a PR since the result isn't reported anywhere. That is, I don't see the overall CI workload changing in the branch vs fork model if the CI is setup to do only what it needs to do.

That's not correct. The CI outcome of every push is visible in the branch's history and in the 'Actions' tab. The details of the CI run can be inspected to look at the failures, artifacts downloaded etc. Yes, I run the tests locally, but I've also got the CI coverage across different platforms that I have no access to, and different versions of dependencies. That's how I know that the CI will pass before I open a PR — I treat the PR as "here is a request to review because I think it's ready to merge", not "it's ready for review, oh CI doesn't pass, oops, let me now start adding fixup commits, I wonder if this fixes it, no, oops, sorry for the noise, how about now, looks good, now you can review".

@llimeht I don't understand why fork vs. branch will have any effect on git history. You are going to have the separate block chains for the different commit paths unless you are doing squash merges with rebase, and you can do that just as easily with branches as with forks.

I wouldn't advocate for squashing all commits in the branch during the merge process. Some like that idea but I'm not a big fan. That doesn't make the history simpler as there is the same number of parallel branches in it, and in any large piece of work, having the work lumped into one big chunk makes both understanding it and bisecting it harder. It makes some sense if there's one main commit and then a trail of fixups behind it, but I'd rather that weren't turned into a PR in the first place.

However, the author of a branch rebasing it onto the current main branch means that there are fewer parallel branches in the final merged product. Rebasing rather than merging main into the feature branch or having a long-lived feature branch makes the merge simpler to understand and any later bisection simpler. Rebasing also means that any fixup commits that do happen to be needed can be squashed, once again making the history simpler. Squashing fixup commits is really important to be able to bisect; whenever I git bisect on sasview, I have to spend excessive time looking for commits that don't contain lots of unrelated errors that are then dealt with in a fixup commit later in the same branch.

When I'm working in my own fork and it's not a branch that I have 'published' (as in, I've not asked other people to work on it with me and not made a PR using it), I will freely rebase that branch. That's why any merges you see from me are normally on what look like short-lived branches and there are no fixup commits in them. I can only do that if I'm not going to ruin someone's day by rewriting the history of the branch; temporary branches in my own fork I can do that to but it would be poor practice to do that to branches in the shared project.

[pkienzle]

Back to the ticket:

Tests are running on both push and pull_request. Propose running them only on push. Concern is that push doesn't catch merge requests.

Looking at the docs we should should ignore most pull_request triggers. Just adding a comment to the discussion should not cause a rebuild.

The merge action isn't even listed in the pull_request activity types, so does it even solve the merge problem?

We shouldn't need to run tests on the pull_request open activity since it should have been built and tested on the push. However, I wouldn't be surprised if it didn't show the last build activity before the PR, so this might be the most expedient way of having the test results appear.

The git fork vs. branch workflow doesn't seem relevant to the issue.

[pkienzle]

Note: the git fork vs. branch workflow discussion does not belong here. Feel free to delete all my comments on the matter from this ticket, including this comment.

[butlerpd]

Was just thinking today that maybe this should be moved to discussions rather than be an issue at this point?

[pkienzle]

@lucas-wilkins I cleaned the off-topic bits out of #2307. Can you please unlock it so that we can resume discussion of the actual ticket?

[lucas-wilkins]

@pkienzle I don't think I or anyone else has that power. It's either a ticket or a discussion as far as I can tell.

[lucas-wilkins]

You can create a new ticket from here if you like. It should fill it in sensibly.

[llimeht]

For the purposes of discussion, the more important workflow change to consider is actually "merge main into feature branch" vs "rebase feature branch onto current main". The upside of rebasing is that the history is much simpler, bisecting is easier, fixups can be squashed so that history is not full of breakage+unbreakage, unittests that are run locally or on the branch on push are more reflective of the merged situation. The downside is that rewriting history is not a nice thing to do to a public branch, only something that you'd generally do to branches that no-one else has checked out.

To an extent this is like asking "vi or emacs" and there is no right answer. However, having done git bisects on this code base in the past and having had them take hours longer than they needed to because of the convoluted and broken history, I can attest that there are also practical considerations.

Some related reading:

• https://www.atlassian.com/git/tutorials/merging-vs-rebasing
• https://www.atlassian.com/git/articles/git-team-workflows-merge-or-rebase

[pkienzle]

Posted by @butlerpd in #2370 (comment)

For now there seems to be general agreement that moving towards linked issues and PR for everything with names that match along with scrupulously deleting old branches is a good ideas. @lucas-wilkins points out that the github webtool makes this trivial (creating a branch directly from the issue will both link it and give it the branch an appropriate name). One worry for the command line folks is the length of the string to type when checking out a branch.

For now however the consensus seems to be that we should strongly encourage these practices by training new people this way and disseminating this to the existing developers somehow. We can revisit various enforcement mechanisms once we have more experience and decide that we are ready to take that step.

For more details see that thread.

[llimeht]

None of that is actually addressing merging vs rebasing, which is really what the discussion was about.