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

Read authorship information from last commit when squashing #152

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kellda
Copy link
Contributor

@kellda kellda commented Sep 9, 2021

Fixes #136.

This only change behaviour for @bors squash. With the current implementation, bors will still take credit when configured to keep history linear or do autosquashes.

@camelid
Copy link
Member

camelid commented Sep 9, 2021

It might be slightly better to use the authorship information of the first commit, since sometimes the reviewer will add a commit at the end to fix CI, etc.

homu/main.py Outdated
Comment on lines 1030 to 1035
commiter_name = subprocess.check_output(
git_cmd('log', '-1', '--format="%an"',
state.head_sha)).decode('ascii').strip()
commiter_email = subprocess.check_output(
git_cmd('log', '-1', '--format="%ae"',
state.head_sha)).decode('ascii').strip()
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT, these are actually getting the author's info, not the committer's, so should the variables be renamed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right

@kellda
Copy link
Contributor Author

kellda commented Sep 9, 2021

Do you have an idea of how to get the first commit ?

@camelid
Copy link
Member

camelid commented Sep 9, 2021

There might be a better way, but it looks like git log --reverse -1 master... will give it to you. (You may need to use origin/master; I'm not very knowledgeable about the bors internals.)

@kellda
Copy link
Contributor Author

kellda commented Sep 9, 2021

Should be good now.

Comment on lines -1041 to +1052
'user.name=' + git_cfg['name'],
'user.name=' + author_name,
'-c',
'user.email=' + git_cfg['email'],
'user.email=' + author_email,
Copy link
Member

Choose a reason for hiding this comment

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

Don't these need to be escaped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, I'll check. Probably not since we already give a list of arguments and not a single command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't: arguments are passed as-is. Also note that squash_msg isn't escaped either.

@camelid
Copy link
Member

camelid commented Oct 6, 2021

r? @pietroalbini

I'd love to get this issue fixed so bors doesn't take credit for people's commits :)

@@ -1027,6 +1027,15 @@ def create_merge(state, repo_cfg, branch, logger, git_cfg,
ok = False
if state.squash:
try:
first_commit = subprocess.check_output(
git_cmd('cherry', 'origin/master',
Copy link
Member

@Alexendoo Alexendoo Jul 9, 2022

Choose a reason for hiding this comment

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

We ran into this again in clippy recently so it'd be nice to take another look at this (rust-lang/rust-clippy#8356)

I believe this should be merge_base_sha rather than 'origin/master'

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.

bors takes credit for squashed PRs
3 participants