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

Improve handling of multiple authors #27

Open
hugovdm opened this issue Dec 29, 2020 · 4 comments
Open

Improve handling of multiple authors #27

hugovdm opened this issue Dec 29, 2020 · 4 comments

Comments

@hugovdm
Copy link

hugovdm commented Dec 29, 2020

Currently if two authors push to a branch, it looks like jira-github-pr-check squashes to a commit attributed to the last author. A small fix by a reviewer can thus result in the reviewer getting full blame - thus discouraging direct fixes.

Options:

  • refusing to squash when there's more than one author?
  • using the first author, not the last?
  • or at least warning very clearly when there's more than one?
  • some way of explicitly choosing which author?
  • (only squashing successive commits by the same author?) <- not recommended, when the same author might also commit with different email addresses. And the end-result isn't a single commit.

I like the first option above: I think it's reasonable to just let people squash manually when there's more than one author.

@sffc
Copy link
Member

sffc commented Jan 11, 2021

The choice of author is rather arbitrary. It does look like it is based on the last commit:

async function writeSquashCommit(githubToken, { owner, repo, parentSha, headSha, message }) {

My preferred solutions, in order, would be:

  1. Add the author to the form in the squash UI, defaulted to the author of the last commit, but allowing the person running the squash to change it
  2. Set the author based on the first commit

I don't like refusing to squash if there are mixed authors, because:

  1. It's surprisingly common for the same person to have multiple different email addresses used for commits (laptop, desktop, GitHub UI, etc), which would get flagged as different authors
  2. By saying that people should squash manually, we are saying that there is indeed a correct answer; the tool, which is designed to be helpful, should therefore be capable of performing that action
  3. The final approval of the PR should take into account the correctness of the author, catching any potential errors

@sffc
Copy link
Member

sffc commented Jan 11, 2021

thus discouraging direct fixes

An alternative to direct fixes is to use suggestions in the PR review: https://haacked.com/archive/2019/06/03/suggested-changes/

@markusicu
Copy link
Member

First author makes sense to me. That's who started the PR and probably did most of the work.

PR suggestions are useful, but I think that git-savvy people (e.g., Andy) will sometimes still offer direct commits to help fix stuff or contribute. But normally the original author should get credit and blame. If someone else ends up doing the majority of the work, then they should clone the PR and make it their own.

@hugovdm
Copy link
Author

hugovdm commented Jan 27, 2021

An example of what github suggestions does: unicode-org/icu@fbb1ad2

It adds additional authors in the commit description, in forms like this:

Co-authored-by: Shane F. Carr [email protected]

This can be reviewed and adjusted in squashbot's squash form.

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

No branches or pull requests

3 participants