Page MenuHomePhabricator

Add a git hook script that can be manually setup to run some checks on every push
ClosedPublic

Authored by mehdi_amini on Jun 1 2020, 10:41 PM.

Details

Summary

Right now it just catches arcanist noisy tags, and include a script to
automatically clean these.

Follow up on http://lists.llvm.org/pipermail/llvm-dev/2019-December/137848.html

Diff Detail

Event Timeline

mehdi_amini created this revision.Jun 1 2020, 10:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 10:41 PM
MaskRay added inline comments.Jun 1 2020, 11:14 PM
llvm/utils/git/arcfilter.sh
7

This version handles both Summay: ... (on the same line) and Summary:\nmulti lines

git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -

In addition, I added --date=now to reset author date to committer date.

llvm/utils/git/pre-push.py
2

I think we can just use python3 and drop Python 2.7 support.

Thanks for the patch. git pre-hook seems to be a clever trick. I need to play with this.

mehdi_amini marked an inline comment as done.Jun 1 2020, 11:22 PM
mehdi_amini added inline comments.
llvm/utils/git/arcfilter.sh
7

I'll update the awk, why should we add --date=now ?

Thanks! I always forget to use arcfilter before pushing the changes.

mehdi_amini marked an inline comment as done.

Limit to python3 and use the new "awk" script

mehdi_amini marked an inline comment as done.Jun 2 2020, 8:32 PM
mehdi_amini added inline comments.
llvm/utils/git/arcfilter.sh
7

Done, but still ping @MaskRay in case you missed my question? (I'm curious)

MaskRay added inline comments.Jun 2 2020, 9:00 PM
llvm/utils/git/arcfilter.sh
7

Author dates (git log default) seem to be random. They are oftentimes misleading. (For example, it is difficult to tell whether a commit was really made yesterday) I changed my git log config to display committer dates by default, but many don't. Resetting my commits will make others easier when reading my commits.

mehdi_amini marked an inline comment as done.Jun 8 2020, 12:41 PM
mehdi_amini added inline comments.
llvm/utils/git/arcfilter.sh
7

OK, does it LGTY now?

MaskRay added inline comments.Jun 9 2020, 12:32 PM
llvm/utils/git/pre-push.py
92

Are print_raw_stderr and ignore_errors and probably other arguments really used?

117

text is always True. The other code path can be dropped.

138

shutil.which

147

return reversed(...)

202

typo: find

214

No need to wrap lines.

227

spaces around =

mehdi_amini marked 3 inline comments as done.Jun 9 2020, 12:58 PM
mehdi_amini added inline comments.
llvm/utils/git/pre-push.py
92

I forked the whole file from the former git-llvm script we had in tree :)

117

this function as-is is reusable: I could strip it down the bare minimum needed in this script, but since it already existed before...

138

Does it handle the .exe suffix addition here?

I haven't reviewed the code but I like this a lot, thx! 👍

mehdi_amini marked 4 inline comments as done.

Address some comments on the python script

(mark some comments as done)

mehdi_amini marked 2 inline comments as done.Mon, Jun 15, 3:10 PM
mehdi_amini added inline comments.
llvm/utils/git/pre-push.py
138

Seems like it would work, but requires 3.3, not sure if we should require this?

This is nice! I'm pretty tired of seeing Arcanist boilerplate in commits. Let's do this!

@MaskRay any major left or can we go one with this?

MaskRay added inline comments.Thu, Jun 18, 9:47 PM
llvm/utils/git/pre-push.py
41

Not used in Python 3

50

space around =

77

space after comma

123

Quotes are very inconsistent in this file. Please stick with '

138

Python 3.4 was released 6 years ago. I think we don't need to care about 3.3 on Windows.

185

Single quotes and add spaces after commas

225

No trailing empty line

@MaskRay anything more than cosmetic left?

mehdi_amini marked an inline comment as done.Thu, Jun 18, 10:11 PM
mehdi_amini added inline comments.
llvm/utils/git/pre-push.py
147

Actually I need to revert back to commits.reverse() here:

File ".git/hooks/pre-push", line 165, in handle_push
    if len(revs) > 1:
TypeError: object of type 'list_reverseiterator' has no len()
mehdi_amini marked 7 inline comments as done.

Mostly cosmetic

@MaskRay anything more than cosmetic left?

Style-wise I cannot find more issues now. Honestly I tried this script just now.

Without git pull --rebase origin master, my local master branch was stale. This script said:

% git push origin HEAD:master                                                                                                                   
`git rev-list 4171f80d5416eccbeebe8864410d576d7dc61eaa..bb67f1a4400a19318713375655999f733418a33b` returned 128    
fatal: Invalid revision range 4171f80d5416eccbeebe8864410d576d7dc61eaa..bb67f1a4400a19318713375655999f733418a33b

The diagnostic can probably be made more helpful.

After git pull --rebase origin master:

% git push origin HEAD:master                                                                                                                     
Pushing to "refs/heads/master" on remote "git@github.com:llvm/llvm-project.git"
 - 8ffb2097cc1 [ELF] Refine LMA offset propagation rule in D76995          
Enumerating objects: 22, done.                                             
Counting objects: 100% (22/22), done.                                      
Delta compression using up to 72 threads                             
Compressing objects: 100% (11/11), done.                             
Writing objects: 100% (12/12), 2.04 KiB | 2.04 MiB/s, done.          
Total 12 (delta 9), reused 0 (delta 0), pack-reused 0                
remote: Resolving deltas: 100% (9/9), completed with 9 local objects.
To github.com:llvm/llvm-project.git
   4171f80d541..8ffb2097cc1  HEAD -> master

The first two lines are added by this script. They look good to me.

Detect when the remote revision isn't available locally

Without git pull --rebase origin master, my local master branch was stale. This script said:

% git push origin HEAD:master                                                                                                                   
`git rev-list 4171f80d5416eccbeebe8864410d576d7dc61eaa..bb67f1a4400a19318713375655999f733418a33b` returned 128    
fatal: Invalid revision range 4171f80d5416eccbeebe8864410d576d7dc61eaa..bb67f1a4400a19318713375655999f733418a33b

Thanks for catching this! I think the script should just ignore this case and let git fails. I added the check.

llvm/utils/git/pre-push.py
138

Note: this isn't about windows, using this puts the requirement everywhere

MaskRay accepted this revision.Mon, Jun 22, 3:44 PM

LGTM.

This revision is now accepted and ready to land.Mon, Jun 22, 3:44 PM

Thanks for the reviews @MaskRay !

This revision was automatically updated to reflect the committed changes.