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

Automatically add a pre-commit hook to git repos for running clang-fo… #537

Merged
merged 1 commit into from
Jan 27, 2017

Conversation

hernando
Copy link

…rmat.

The pre-commit hook is a bash script using only built-ins, git and
clang_format. The hook is not added if the project being configured already
has one.
The script does nothing if a .clang-format is not found and it only processes
files that are added enterily to the index.

…rmat.

The pre-commit hook is a bash script using only built-ins, git and
clang_format. The hook is not added if the project being configured already
has one.
The script does nothing if a .clang-format is not found and it only processes
files that are added enterily to the index.
break;
fi
done
done
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this applies only to fully staged files? What if I do a git commit -a?

Copy link
Author

Choose a reason for hiding this comment

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

I tested just in case, clang-format is still run on all modified files. Git seems to prepare the index before calling the pre-commit hook, and if you abort the commit it undoes the additions to the index.

if [[ $($GIT diff --name-only "$file") == "$file" ]]; then
echo "clang-format not run on partially added file $file"
else
$CLANG_FORMAT -i -style=file $PWD/$file
Copy link
Member

Choose a reason for hiding this comment

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

-style=file?

Copy link
Author

Choose a reason for hiding this comment

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

That's what man clang-format told me to do. Has this changed in a later version?

# Copyright (c) 2017, Juan Hernando <juan.hernando@epfl.ch>

GIT=${GIT_EXECUTABLE}
CLANG_FORMAT=${CLANG_FORMAT}
Copy link

Choose a reason for hiding this comment

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

this will hardcode a path at configure time, not a good idea because this file will not be overwritten once it has been created. Just use the executable from the path?

Copy link
Author

Choose a reason for hiding this comment

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

And what if your git or clang format binaries are not in the path and you detected them with CMake?

@rdumusc
Copy link

rdumusc commented Jan 26, 2017

In general I have some concerns with this approach. It is a somewhat hidden feature that changes the code behind the back of users when they just want to commit.

In terms of implementation, the .clang-format file will have to be committed in each project, and any change to our coding style will have to be replicated everywhere. If on the other hand we use a hard-coded clang-format -style {options...}, then that would be quite bad for external users who have a different coding style...

I am trying to think of a better automated solution. One option could be a per-project CMake target ${PROJECT_NAME}-clang-format that applies our coding style by default (using hard-coded clang-format -style {options...}) but will apply a ${PROJECT_SOURCE_DIR}/.clang-format instead if one is found. That way we can provide a centralized default style without impacting external users, and the formatting becomes explicit instead of implicit. This adds only one pre-commit step for external contributors who want to submit a patch to our projects. For us, we can simply configure our IDEs to apply clang-format when saving the file (e.g. Beautifier plugin in QtCreator).

@hernando
Copy link
Author

I don't know if everybody knows his IDE, but your solution means that unless you configure it for all the systems you use, you have to run the pre commit step manually to make sure you will pass CI. I see this problematic for smooth adoption inside our team.
Personally, I prefer the approach I implemented. It can be modified to ask the user interactively what to do if there are differences between the staging area and the working copy after clang-format is run (I'd give four options: accept, see diff, reject and accept for all files)

@rdumusc
Copy link

rdumusc commented Jan 26, 2017

Setting the IDE is not an overly complex task, I am sure every developer has already some presets like replace tabs with whitespace, remove trailing whitespaces, etc... sure, if you work on multiple systems you have to do it everywhere, but the same applies to you ~/.bashrc aliases, ~/.gitconfig, etc...
That being said, it is of course a matter of personal preference. Maybe we should discuss the options together to see what the majority prefers.

@tribal-tec
Copy link
Member

I'm happy with @hernando 's solution. Can we find a speedy conclusion and move forward? Looks already a bit tumbleweed here...

@rdumusc
Copy link

rdumusc commented Jan 27, 2017

Fair enough, I will accept the decision of the majority, but I let someone else approve this PR since I don't ;-)

@hernando hernando merged commit d69b903 into master Jan 27, 2017
@hernando hernando deleted the clang-format-hook branch February 8, 2017 10:04
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.

4 participants