Discussion:
[PATCH for comments] configure: install a whitespace checking pre-commit hook for developers
Douglas Bagnall via samba-technical
2018-04-09 12:35:07 UTC
Permalink
The problem is that the *initial* proposal is not for a checkpatch
script that people can run *before* submitting their patch. The
proposal is for something that runs at the submission gate.
This patch shows a third way, still quite authoritarian but sort of
opt-out-able and entirely local. It makes it harder to *create*
patches that break the rules, not to submit them.

It will also (I think) make it harder to `git am` a patch series that
contains whitespace errors, and will possibly annoy people who make a
lot of private WIP patches.

Douglas
Richard Sharpe via samba-technical
2018-04-09 13:53:08 UTC
Permalink
On Mon, Apr 9, 2018 at 5:35 AM, Douglas Bagnall via samba-technical
Post by Douglas Bagnall via samba-technical
The problem is that the *initial* proposal is not for a checkpatch
script that people can run *before* submitting their patch. The
proposal is for something that runs at the submission gate.
This patch shows a third way, still quite authoritarian but sort of
opt-out-able and entirely local. It makes it harder to *create*
patches that break the rules, not to submit them.
It will also (I think) make it harder to `git am` a patch series that
contains whitespace errors, and will possibly annoy people who make a
lot of private WIP patches.
LGTM! RB+
--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)(传说杜康是酒的发明者)
Jeremy Allison via samba-technical
2018-04-09 15:44:10 UTC
Permalink
Post by Douglas Bagnall via samba-technical
The problem is that the *initial* proposal is not for a checkpatch
script that people can run *before* submitting their patch. The
proposal is for something that runs at the submission gate.
This patch shows a third way, still quite authoritarian but sort of
opt-out-able and entirely local. It makes it harder to *create*
patches that break the rules, not to submit them.
It will also (I think) make it harder to `git am` a patch series that
contains whitespace errors, and will possibly annoy people who make a
lot of private WIP patches.
Thanks very much Douglas, this looks like it might be a great
solution that works for all ! Thanks very much for helping out
with this contentious issue.

I'll test it out and push if happy.

Cheers,

Jeremy.
Post by Douglas Bagnall via samba-technical
From 126836d41d4154594805890557fbbb9f7d0ac04b Mon Sep 17 00:00:00 2001
Date: Tue, 10 Apr 2018 00:03:55 +1200
Subject: [PATCH] configure: install a whitespace checking pre-commit hook for
developers
If you don't already have a pre-commit git hook,
configure --enable-developer
will give you one that checks for whitespace errors in changed lines
of Python, Perl and C.
---
examples/git-hooks/pre-commit | 15 +++++++++++++++
wscript | 9 ++++++++-
2 files changed, 23 insertions(+), 1 deletion(-)
create mode 100755 examples/git-hooks/pre-commit
diff --git a/examples/git-hooks/pre-commit b/examples/git-hooks/pre-commit
new file mode 100755
index 00000000000..68c6150cde6
--- /dev/null
+++ b/examples/git-hooks/pre-commit
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+git diff-index --check HEAD -- :/*.[ch] :/*.p[ylm]
+
+if [ $? != 0 ]; then
+ echo
+ echo "The commit failed because it seems to introduce trailing whitespace"
+ echo "into C, Perl, or Python code."
+ echo
+ echo "If you are sure you want to do this, repeat the commit with the "
+ echo "--no-verify, like this:"
+ echo
+ echo " git commit --no-verify"
+ exit 1
+fi
diff --git a/wscript b/wscript
index 0985aa94867..102b772428b 100644
--- a/wscript
+++ b/wscript
@@ -10,7 +10,7 @@ import sys, os, tempfile
sys.path.insert(0, srcdir+"/buildtools/wafsamba")
import wafsamba, Options, samba_dist, samba_git, Scripting, Utils, samba_version
import Logs, samba_utils
-
+import shutil
samba_dist.DIST_DIRS('.')
samba_dist.DIST_BLACKLIST('.gitignore .bzrignore source4/selftest/provisions')
conf.ADD_CFLAGS('-DDEVELOPER -DDEBUG_PASSWORD')
conf.env.DEVELOPER = True
+ # if we are in a git tree without a pre-commit hook, install a
+ # simple default.
+ pre_commit_hook = os.path.join(srcdir, '.git/hooks/pre-commit')
+ if (os.path.isdir(os.path.dirname(pre_commit_hook)) and
+ shutil.copy(os.path.join(srcdir, 'examples/git-hooks/pre-commit'),
+ pre_commit_hook)
conf.ADD_EXTRA_INCLUDES('#include/public #source4 #lib #source4/lib #source4/include #include #lib/replace')
--
2.11.0
Ralph Böhme via samba-technical
2018-04-10 06:30:53 UTC
Permalink
Post by Jeremy Allison via samba-technical
Thanks very much Douglas, this looks like it might be a great
solution that works for all ! Thanks very much for helping out
with this contentious issue.
I'll test it out and push if happy.
+1 from my side.

Wonder whether we still need the more complex checking tool from Simo given that
the original issue that started the outrage would be addressed by Douglas'
patch.

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Douglas Bagnall via samba-technical
2018-04-10 09:03:19 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Jeremy Allison via samba-technical
Thanks very much Douglas, this looks like it might be a great
solution that works for all ! Thanks very much for helping out
with this contentious issue.
I'll test it out and push if happy.
+1 from my side.
Wonder whether we still need the more complex checking tool from Simo given that
the original issue that started the outrage would be addressed by Douglas'
patch.
I think there is a place for a more sophisticated tool that can be run manually.
I haven't looked closely at Simo's patch, but it sounds like it has pedigree.

When I am reviewing or working on python files, I quite often run flake8 to see
what it says. I don't always agree. It would be nice to have a similar thing for C.
It is sometimes more interesting for a tool to be opinionated than authoritative.

I also agree that the git-commit hook could do more than check whitespace.

cheers,
Douglas
Uri Simchoni via samba-technical
2018-04-10 09:39:06 UTC
Permalink
Post by Douglas Bagnall via samba-technical
Post by Ralph Böhme via samba-technical
Post by Jeremy Allison via samba-technical
Thanks very much Douglas, this looks like it might be a great
solution that works for all ! Thanks very much for helping out
with this contentious issue.
I'll test it out and push if happy.
+1 from my side.
Wonder whether we still need the more complex checking tool from Simo given that
the original issue that started the outrage would be addressed by Douglas'
patch.
I think there is a place for a more sophisticated tool that can be run manually.
I haven't looked closely at Simo's patch, but it sounds like it has pedigree.
When I am reviewing or working on python files, I quite often run flake8 to see
what it says. I don't always agree. It would be nice to have a similar thing for C.
It is sometimes more interesting for a tool to be opinionated than authoritative.
IMHO the one-stop-shop for an opinionated formatting tool for C is
clang-format.

- The samba-conformant configuration is in README.Coding
- It does whitespace (end-of-line, tab), alignment, 80-column, space
between variables and operators, you name it. Being based on a compiler,
it probably does a better job than any hand-crafted script.
- Combined with the git-clang-format git add-on, doing "git
clang-format" will only re-format the changed lines.
- I mostly take its edits as-is. Only place where I consistently
disagree is in initialized arrays of popt options - it's not necessarily
incorrect but the new option has to be consistent with other options.
This is an example of where the automatic formatting doesn't work though.

So my workflow for a single commit is:
- add all changes - everything is in the index
- run "git clang-format" - clang-format reformats the patches that are
in the index
- run "git add -i" to accept or reject git-clang-format edits.

Thanks,
Uri.
Post by Douglas Bagnall via samba-technical
I also agree that the git-commit hook could do more than check whitespace.
cheers,
Douglas
Douglas Bagnall via samba-technical
2018-04-11 00:08:53 UTC
Permalink
Post by Uri Simchoni via samba-technical
Post by Douglas Bagnall via samba-technical
When I am reviewing or working on python files, I quite often run flake8 to see
what it says. I don't always agree. It would be nice to have a similar thing for C.
It is sometimes more interesting for a tool to be opinionated than authoritative.
IMHO the one-stop-shop for an opinionated formatting tool for C is
clang-format.
- The samba-conformant configuration is in README.Coding
- It does whitespace (end-of-line, tab), alignment, 80-column, space
between variables and operators, you name it. Being based on a compiler,
it probably does a better job than any hand-crafted script.
- Combined with the git-clang-format git add-on, doing "git
clang-format" will only re-format the changed lines.
- I mostly take its edits as-is. Only place where I consistently
disagree is in initialized arrays of popt options - it's not necessarily
incorrect but the new option has to be consistent with other options.
This is an example of where the automatic formatting doesn't work though.
- add all changes - everything is in the index
- run "git clang-format" - clang-format reformats the patches that are
in the index
- run "git add -i" to accept or reject git-clang-format edits.
Thanks Uri,

I wasn't aware of git clang-format. I have got as far as this:

git config clangFormat.style "{BasedOnStyle: LLVM, IndentWidth: 8, UseTab: true, BreakBeforeBraces: Linux, AllowShortIfStatementsOnASingleLine: false, IndentCaseLabels: false, BinPackParameters: false, BinPackArguments: false}"

which leaves me wanting to patch some C.

Douglas
Uri Simchoni via samba-technical
2018-04-11 01:11:53 UTC
Permalink
Post by Douglas Bagnall via samba-technical
Post by Uri Simchoni via samba-technical
Post by Douglas Bagnall via samba-technical
When I am reviewing or working on python files, I quite often run flake8 to see
what it says. I don't always agree. It would be nice to have a similar thing for C.
It is sometimes more interesting for a tool to be opinionated than authoritative.
IMHO the one-stop-shop for an opinionated formatting tool for C is
clang-format.
- The samba-conformant configuration is in README.Coding
- It does whitespace (end-of-line, tab), alignment, 80-column, space
between variables and operators, you name it. Being based on a compiler,
it probably does a better job than any hand-crafted script.
- Combined with the git-clang-format git add-on, doing "git
clang-format" will only re-format the changed lines.
- I mostly take its edits as-is. Only place where I consistently
disagree is in initialized arrays of popt options - it's not necessarily
incorrect but the new option has to be consistent with other options.
This is an example of where the automatic formatting doesn't work though.
- add all changes - everything is in the index
- run "git clang-format" - clang-format reformats the patches that are
in the index
- run "git add -i" to accept or reject git-clang-format edits.
Thanks Uri,
git config clangFormat.style "{BasedOnStyle: LLVM, IndentWidth: 8, UseTab: true, BreakBeforeBraces: Linux, AllowShortIfStatementsOnASingleLine: false, IndentCaseLabels: false, BinPackParameters: false, BinPackArguments: false}"
which leaves me wanting to patch some C.
Douglas
That might work. My setup is:
clangformat.style=file

...and then I have a .clang-format file in my homedir with:
BasedOnStyle: LLVM
IndentWidth: 8
UseTab: true
BreakBeforeBraces: Linux
AllowShortIfStatementsOnASingleLine: false
IndentCaseLabels: false
BinPackParameters: false
BinPackArguments: false

Uri.
Ralph Böhme via samba-technical
2018-04-09 15:47:37 UTC
Permalink
Hi Douglas,
Post by Douglas Bagnall via samba-technical
The problem is that the *initial* proposal is not for a checkpatch
script that people can run *before* submitting their patch. The
proposal is for something that runs at the submission gate.
This patch shows a third way, still quite authoritarian but sort of
opt-out-able and entirely local. It makes it harder to *create*
patches that break the rules, not to submit them.
It will also (I think) make it harder to `git am` a patch series that
contains whitespace errors, and will possibly annoy people who make a
lot of private WIP patches.
this looks like as if it could be it. I'll give it a whirl. Thanks!

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Ralph Böhme via samba-technical
2018-04-10 06:20:55 UTC
Permalink
Post by Douglas Bagnall via samba-technical
The problem is that the *initial* proposal is not for a checkpatch
script that people can run *before* submitting their patch. The
proposal is for something that runs at the submission gate.
This patch shows a third way, still quite authoritarian but sort of
opt-out-able and entirely local. It makes it harder to *create*
patches that break the rules, not to submit them.
It will also (I think) make it harder to `git am` a patch series that
contains whitespace errors, and will possibly annoy people who make a
lot of private WIP patches.
[***@kazak scratch]$ git am ~/patches/0001-configure-install-a-whitespace-checking-pre-commit-h.patch
Applying: configure: install a whitespace checking pre-commit hook for developers
.git/rebase-apply/patch:46: trailing whitespace.
# simple default.
warning: 1 line adds whitespace errors.
[***@kazak scratch]$

:)))

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Ralph Böhme via samba-technical
2018-04-10 06:36:41 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Douglas Bagnall via samba-technical
The problem is that the *initial* proposal is not for a checkpatch
script that people can run *before* submitting their patch. The
proposal is for something that runs at the submission gate.
This patch shows a third way, still quite authoritarian but sort of
opt-out-able and entirely local. It makes it harder to *create*
patches that break the rules, not to submit them.
It will also (I think) make it harder to `git am` a patch series that
contains whitespace errors, and will possibly annoy people who make a
lot of private WIP patches.
Applying: configure: install a whitespace checking pre-commit hook for developers
.git/rebase-apply/patch:46: trailing whitespace.
# simple default.
warning: 1 line adds whitespace errors.
:)))
besides that, this is really, really nice. Thanks Douglas!

Updated patch with fixed whitespace attached.

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Swen Schillig via samba-technical
2018-04-10 07:31:43 UTC
Permalink
On Tue, 2018-04-10 at 08:36 +0200, Ralph Böhme via samba-technical
On Tue, Apr 10, 2018 at 08:20:55AM +0200, Ralph Böhme via samba-
Post by Ralph Böhme via samba-technical
Post by Douglas Bagnall via samba-technical
The problem is that the *initial* proposal is not for a
checkpatch
script that people can run *before* submitting their
patch. The
proposal is for something that runs at the submission gate.
This patch shows a third way, still quite authoritarian but sort of
opt-out-able and entirely local. It makes it harder to *create*
patches that break the rules, not to submit them.
It will also (I think) make it harder to `git am` a patch series that
contains whitespace errors, and will possibly annoy people who make a
lot of private WIP patches.
whitespace-checking-pre-commit-h.patch
Applying: configure: install a whitespace checking pre-commit hook for developers
.git/rebase-apply/patch:46: trailing whitespace.
# simple default.
warning: 1 line adds whitespace errors.
:)))
besides that, this is really, really nice. Thanks Douglas!
I really like Douglas' work as well, especially the part where
the pre-commit hook gets installed auto-magically if there isn't one.

But, is trailing white-spaces all we want to check for ?
Why not go a bit further and have a few more of those "annoying"
violations flagged as well ?

Like
- leading white-space
- code indent should use tabs where possible
- space required before the open parenthesis '('
- line over 80 characters
- Missing a blank line after declarations
- space prohibited before that close parenthesis ')'
- ...

especially if all those checks can be turned off, if required for
whatever reason, with a simple "git --no-check ...."

Cheers Swen
Stefan Metzmacher via samba-technical
2018-04-10 07:53:11 UTC
Permalink
On Tue, 2018-04-10 at 08:36 +0200, Ralph Böhme via samba-technical
On Tue, Apr 10, 2018 at 08:20:55AM +0200, Ralph Böhme via samba-
Post by Ralph Böhme via samba-technical
Post by Douglas Bagnall via samba-technical
The problem is that the *initial* proposal is not for a
checkpatch
script that people can run *before* submitting their
patch. The
proposal is for something that runs at the submission gate.
This patch shows a third way, still quite authoritarian but sort of
opt-out-able and entirely local. It makes it harder to *create*
patches that break the rules, not to submit them.
It will also (I think) make it harder to `git am` a patch series that
contains whitespace errors, and will possibly annoy people who make a
lot of private WIP patches.
whitespace-checking-pre-commit-h.patch
Applying: configure: install a whitespace checking pre-commit hook for developers
.git/rebase-apply/patch:46: trailing whitespace.
# simple default.
warning: 1 line adds whitespace errors.
:)))
besides that, this is really, really nice. Thanks Douglas!
I really like Douglas' work as well, especially the part where
the pre-commit hook gets installed auto-magically if there isn't one.
But, is trailing white-spaces all we want to check for ?
Why not go a bit further and have a few more of those "annoying"
violations flagged as well ?
Like
- leading white-space
- code indent should use tabs where possible
- space required before the open parenthesis '('
- line over 80 characters
- Missing a blank line after declarations
- space prohibited before that close parenthesis ')'
- ...
especially if all those checks can be turned off, if required for
whatever reason, with a simple "git --no-check ...."
I was also wondering about that.

I think we should use some like wireshark,
were the real script[s] are part of the checkout and the pre-commit hook
is just a wrapper arround it, that way we can improve the script step by
step.

See
https://code.wireshark.org/review/gitweb?p=wireshark.git;a=blob;f=tools/pre-commit

metze
Swen Schillig via samba-technical
2018-04-10 09:36:04 UTC
Permalink
On Tue, 2018-04-10 at 09:53 +0200, Stefan Metzmacher via samba-
Post by Stefan Metzmacher via samba-technical
Post by Swen Schillig via samba-technical
I really like Douglas' work as well, especially the part where
the pre-commit hook gets installed auto-magically if there isn't one.
But, is trailing white-spaces all we want to check for ?
Why not go a bit further and have a few more of those "annoying"
violations flagged as well ?
Like
- leading white-space
- code indent should use tabs where possible
- space required before the open parenthesis '('
- line over 80 characters
- Missing a blank line after declarations
- space prohibited before that close parenthesis ')'
- ...
especially if all those checks can be turned off, if required for
whatever reason, with a simple "git --no-check ...."
I was also wondering about that.
I think we should use some like wireshark,
were the real script[s] are part of the checkout and the pre-commit hook
is just a wrapper arround it, that way we can improve the script step by
step.
See
https://code.wireshark.org/review/gitweb?p=wireshark.git;a=blob;f=too
ls/pre-commit
metze
I think this is all going into the right direction (more automatic checks),
but I'm not sure why we should invest development efforts while there is
already a really matured tool available which only needs to be configured.

I promise I will mention it only this one time again, but what about using
checkpatch instead of reinventing the wheel ?

checkpatch is used in a variety of other non-kernel projects,
like GANESHA (user space NFS server) which is a project with
very similar requirements.

So why not having a look at least ?

Cheers Swen
Ralph Böhme via samba-technical
2018-04-10 10:50:11 UTC
Permalink
Post by Swen Schillig via samba-technical
I promise I will mention it only this one time again, but what about using
checkpatch instead of reinventing the wheel ?
checkpatch is used in a variety of other non-kernel projects,
like GANESHA (user space NFS server) which is a project with
very similar requirements.
So why not having a look at least ?
heavens, that's 6k+ lines of Perl and a lot of it is really kernel
specific.

Simo's proposed patch looks a lot simpler and seems to do what we want as well,
so *if* we adopt such a swiss-army knife tool, I'd prefer Simo's proposal based
on MIT krb5.

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Swen Schillig via samba-technical
2018-04-10 11:09:38 UTC
Permalink
On Tue, 2018-04-10 at 12:50 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
I promise I will mention it only this one time again, but what about using
checkpatch instead of reinventing the wheel ?
checkpatch is used in a variety of other non-kernel projects,
like GANESHA (user space NFS server) which is a project with
very similar requirements.
So why not having a look at least ?
heavens, that's 6k+ lines of Perl and a lot of it is really kernel
specific.
True, but is it not better to have matured 6k LOC which
we don't have to maintain vs 500 LOC where we do ?

Besides, the SAMBA project's README.Coding refers explicitly
to the kernel coding style so why not use the utility "they" use ?

Cheers Swen
Ralph Böhme via samba-technical
2018-04-10 11:24:37 UTC
Permalink
Post by Swen Schillig via samba-technical
On Tue, 2018-04-10 at 12:50 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
I promise I will mention it only this one time again, but what about using
checkpatch instead of reinventing the wheel ?
checkpatch is used in a variety of other non-kernel projects,
like GANESHA (user space NFS server) which is a project with
very similar requirements.
So why not having a look at least ?
heavens, that's 6k+ lines of Perl and a lot of it is really kernel
specific.
True, but is it not better to have matured 6k LOC which
we don't have to maintain vs 500 LOC where we do ?
I guess in the end we will have to maintain a copy as well. I haven't looked in
detail, but I guess we will need changes to checkpatch as well. So it means
maintaining a few hundred clean Python lines of a few thousand Perl.

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Andrew Bartlett via samba-technical
2018-04-10 11:28:31 UTC
Permalink
On Tue, 2018-04-10 at 13:24 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
On Tue, 2018-04-10 at 12:50 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
I promise I will mention it only this one time again, but what about using
checkpatch instead of reinventing the wheel ?
checkpatch is used in a variety of other non-kernel projects,
like GANESHA (user space NFS server) which is a project with
very similar requirements.
So why not having a look at least ?
heavens, that's 6k+ lines of Perl and a lot of it is really kernel
specific.
True, but is it not better to have matured 6k LOC which
we don't have to maintain vs 500 LOC where we do ?
I guess in the end we will have to maintain a copy as well. I haven't looked in
detail, but I guess we will need changes to checkpatch as well. So it means
maintaining a few hundred clean Python lines of a few thousand Perl.
-slow
The python check-file.py script looked practical to adapt into a
standard Samba test for if/when we choose to make some files in samba
permanently comply with some/all of the results. If adapted to speak
subunit it would also fit into the flapping/knownfail/skip system we
have.

I suspect the linux kernel and Samba styles (as found in current 'good'
code, not just the rule-book) have diverged also, but I've not checked
specifically.

Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
Douglas Bagnall via samba-technical
2018-04-10 09:51:07 UTC
Permalink
Post by Stefan Metzmacher via samba-technical
Post by Swen Schillig via samba-technical
But, is trailing white-spaces all we want to check for ?
Why not go a bit further and have a few more of those "annoying"
violations flagged as well ?
Like
- leading white-space
- code indent should use tabs where possible
- space required before the open parenthesis '('
- line over 80 characters
- Missing a blank line after declarations
- space prohibited before that close parenthesis ')'
- ...
especially if all those checks can be turned off, if required for
whatever reason, with a simple "git --no-check ...."
I was also wondering about that.
I think we should use some like wireshark,
were the real script[s] are part of the checkout and the pre-commit hook
is just a wrapper arround it, that way we can improve the script step by
step.
See
https://code.wireshark.org/review/gitweb?p=wireshark.git;a=blob;f=tools/pre-commit
Yes.

Of course, a difficulty we have that wireshark and other projects
don't is 17655 lines of C with trailing whitespace and nearly 50k
lines over 80 characters, which we don't want to change all at once. I
think the wireshark scripts check the whole of the affected file on
the assumption that everything you didn't change will still be
perfectly formatted. Our scripts will have to fiddle around with
diffs.

Douglas
Stefan Metzmacher via samba-technical
2018-04-10 09:58:18 UTC
Permalink
Post by Douglas Bagnall via samba-technical
Post by Stefan Metzmacher via samba-technical
Post by Swen Schillig via samba-technical
But, is trailing white-spaces all we want to check for ?
Why not go a bit further and have a few more of those "annoying"
violations flagged as well ?
Like
- leading white-space
- code indent should use tabs where possible
- space required before the open parenthesis '('
- line over 80 characters
- Missing a blank line after declarations
- space prohibited before that close parenthesis ')'
- ...
especially if all those checks can be turned off, if required for
whatever reason, with a simple "git --no-check ...."
I was also wondering about that.
I think we should use some like wireshark,
were the real script[s] are part of the checkout and the pre-commit hook
is just a wrapper arround it, that way we can improve the script step by
step.
See
https://code.wireshark.org/review/gitweb?p=wireshark.git;a=blob;f=tools/pre-commit
Yes.
Of course, a difficulty we have that wireshark and other projects
don't is 17655 lines of C with trailing whitespace and nearly 50k
lines over 80 characters, which we don't want to change all at once. I
think the wireshark scripts check the whole of the affected file on
the assumption that everything you didn't change will still be
perfectly formatted. Our scripts will have to fiddle around with
diffs.
I'm not proposing to use the wireshark scripts, I haven't looked at them
in details.

The logic for using a script that is part of the checkout and having a
.git/hook/pre-commit script that just calls into the working tree
is what I'd like to have (and maybe the logic to detect an outdated script)

metze
Martin Schwenke via samba-technical
2018-04-10 11:32:24 UTC
Permalink
On Tue, 10 Apr 2018 11:58:18 +0200, Stefan Metzmacher via
Post by Stefan Metzmacher via samba-technical
The logic for using a script that is part of the checkout and having a
.git/hook/pre-commit script that just calls into the working tree
is what I'd like to have (and maybe the logic to detect an outdated script)
I think this is a good approach. This allows us to refine the tool or
even a choice/combination of tools. There is a question about how to do
this neatly...

The "git commit --no-verify" option to avoid the pre-commit hook can
even be easily done when committing using magit under Emacs (and perhaps
also with the git support for vim).

Hmmm... that said, Douglas' pre-commit hook doesn't work under magit.
It works if I don't specify any paths. For some reason it doesn't
trigger if I do add the paths that match C and/or Python files. I
think there's some subtlety in the way the paths are specified in the
environment magit is running git in. :-(

Any other magit users on the team who might have a clue about this?

peace & happiness,
martin
Swen Schillig via samba-technical
2018-04-10 11:43:02 UTC
Permalink
On Tue, 2018-04-10 at 21:32 +1000, Martin Schwenke via samba-technical
Post by Martin Schwenke via samba-technical
On Tue, 10 Apr 2018 11:58:18 +0200, Stefan Metzmacher via
Post by Stefan Metzmacher via samba-technical
The logic for using a script that is part of the checkout and having a
.git/hook/pre-commit script that just calls into the working tree
is what I'd like to have (and maybe the logic to detect an outdated script)
I think this is a good approach. This allows us to refine the tool or
even a choice/combination of tools. There is a question about how to do
this neatly...
The "git commit --no-verify" option to avoid the pre-commit hook can
even be easily done when committing using magit under Emacs (and perhaps
also with the git support for vim).
Hmmm... that said, Douglas' pre-commit hook doesn't work under magit.
It works if I don't specify any paths. For some reason it doesn't
trigger if I do add the paths that match C and/or Python files. I
think there's some subtlety in the way the paths are specified in the
environment magit is running git in. :-(
I don''t know anything about magit but I think the "--cached" parameter
is missed in Douglas' git command line.
Otherwise all changed files will be "checked" while one would only want
the "added" files to be verified.

But I'm afraid that won't help to solve your issue.

Cheers Swen.
Ralph Böhme via samba-technical
2018-04-10 12:16:41 UTC
Permalink
Post by Martin Schwenke via samba-technical
On Tue, 10 Apr 2018 11:58:18 +0200, Stefan Metzmacher via
Post by Stefan Metzmacher via samba-technical
The logic for using a script that is part of the checkout and having a
.git/hook/pre-commit script that just calls into the working tree
is what I'd like to have (and maybe the logic to detect an outdated script)
I think this is a good approach. This allows us to refine the tool or
even a choice/combination of tools. There is a question about how to do
this neatly...
hold right there, I'll post something in a minute... :)

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Ralph Böhme via samba-technical
2018-04-10 12:20:47 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Martin Schwenke via samba-technical
On Tue, 10 Apr 2018 11:58:18 +0200, Stefan Metzmacher via
Post by Stefan Metzmacher via samba-technical
The logic for using a script that is part of the checkout and having a
.git/hook/pre-commit script that just calls into the working tree
is what I'd like to have (and maybe the logic to detect an outdated script)
I think this is a good approach. This allows us to refine the tool or
even a choice/combination of tools. There is a question about how to do
this neatly...
hold right there, I'll post something in a minute... :)
here we go.

With the attached patchset I get this:

[***@kazak scratch]$ git diff --cached
diff --git a/source4/torture/smb2/durable_v2_open.c b/source4/torture/smb2/durable_v2_open.c
index 13316eb3f8b..f4df0a97983 100644
--- a/source4/torture/smb2/durable_v2_open.c
+++ b/source4/torture/smb2/durable_v2_open.c
@@ -2121,7 +2121,7 @@ static bool test_persistent_reconnect_lease(struct torture_context *tctx,
*/
// TALLOC_FREE(tree);
torture_comment(tctx, "Got PH, waiting for cluster kill...\n");
- getchar();
+ getchar();
torture_comment(tctx, "Trying to reconnect PH on new connection\n");

/*
[***@kazak scratch]$ git commit
source4/torture/smb2/durable_v2_open.c:2124: trailing whitespace.
+ getchar();

The commit failed because it seems to introduce trailing whitespace
into C, Perl, or Python code.

If you are sure you want to do this, repeat the commit with the
--no-verify, like this:

git commit --no-verify

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Swen Schillig via samba-technical
2018-04-10 12:29:41 UTC
Permalink
On Tue, 2018-04-10 at 14:20 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Martin Schwenke via samba-technical
On Tue, 10 Apr 2018 11:58:18 +0200, Stefan Metzmacher via
Post by Stefan Metzmacher via samba-technical
The logic for using a script that is part of the checkout and having a
.git/hook/pre-commit script that just calls into the working tree
is what I'd like to have (and maybe the logic to detect an outdated script)
I think this is a good approach. This allows us to refine the tool or
even a choice/combination of tools. There is a question about how to do
this neatly...
hold right there, I'll post something in a minute... :)
here we go.
diff --git a/source4/torture/smb2/durable_v2_open.c
b/source4/torture/smb2/durable_v2_open.c
index 13316eb3f8b..f4df0a97983 100644
--- a/source4/torture/smb2/durable_v2_open.c
+++ b/source4/torture/smb2/durable_v2_open.c
@@ -2121,7 +2121,7 @@ static bool
test_persistent_reconnect_lease(struct torture_context *tctx,
*/
// TALLOC_FREE(tree);
torture_comment(tctx, "Got PH, waiting for cluster kill...\n");
- getchar();
+ getchar();
torture_comment(tctx, "Trying to reconnect PH on new connection\n");
/*
Ralph, don't you need to add that "--cached" parameter to your script
as well ?
As stated in one of my earlier mails, I believe you need that 'cause
otherwise all modified files are checked and not just the ones which
are part of the commit (were add'ed before).

Cheers Swen
Ralph Böhme via samba-technical
2018-04-10 12:38:36 UTC
Permalink
Post by Swen Schillig via samba-technical
Ralph, don't you need to add that "--cached" parameter to your script
as well ?
As stated in one of my earlier mails, I believe you need that 'cause
otherwise all modified files are checked and not just the ones which
are part of the commit (were add'ed before).
are you sure? It's a commit hook after all so my expectation was it only check
what I commit. I'll check...

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Ralph Böhme via samba-technical
2018-04-10 12:45:24 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
Ralph, don't you need to add that "--cached" parameter to your script
as well ?
As stated in one of my earlier mails, I believe you need that 'cause
otherwise all modified files are checked and not just the ones which
are part of the commit (were add'ed before).
are you sure? It's a commit hook after all so my expectation was it only check
what I commit. I'll check...
d'oh! You're obviously right...

Updated patch attached.

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Jeremy Allison via samba-technical
2018-04-10 16:27:50 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
Ralph, don't you need to add that "--cached" parameter to your script
as well ?
As stated in one of my earlier mails, I believe you need that 'cause
otherwise all modified files are checked and not just the ones which
are part of the commit (were add'ed before).
are you sure? It's a commit hook after all so my expectation was it only check
what I commit. I'll check...
d'oh! You're obviously right...
Updated patch attached.
OK, where are we with this ? Are we able to commit this
as it seems to solve the immediate problem ?

Sure we can add more stuff later, but consider this
a RB+ from me (and when can we push ? :-).

Cheers,

Jeremy.
Post by Ralph Böhme via samba-technical
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
From 76fb1110a62d25497252c863d2d6f6e33530dabe Mon Sep 17 00:00:00 2001
Date: Tue, 10 Apr 2018 13:04:27 +0200
Subject: [PATCH 1/2] Add a wrapper script as git pre-commit hook
When developer mode is enabled, the wrapper script
"script/git-hooks/pre-commit-hook" gets installed as
.git/hooks/pre-commit
and calls "script/git-hooks/pre-commit-script".
This way we can later modify the "script/git-hooks/pre-commit-script"
without the need to ever change the installed commit hook itself.
---
script/git-hooks/pre-commit-hook | 7 +++++++
script/git-hooks/pre-commit-script | 9 +++++++++
wscript | 9 ++++++++-
3 files changed, 24 insertions(+), 1 deletion(-)
create mode 100755 script/git-hooks/pre-commit-hook
create mode 100755 script/git-hooks/pre-commit-script
diff --git a/script/git-hooks/pre-commit-hook b/script/git-hooks/pre-commit-hook
new file mode 100755
index 00000000000..223a30dc53d
--- /dev/null
+++ b/script/git-hooks/pre-commit-hook
@@ -0,0 +1,7 @@
+#!/bin/sh
+
+gitdir=$(git rev-parse --show-toplevel)
+
+${gitdir}/script/git-hooks/pre-commit-script || exit $?
+
+exit 0
diff --git a/script/git-hooks/pre-commit-script b/script/git-hooks/pre-commit-script
new file mode 100755
index 00000000000..eb4114e197d
--- /dev/null
+++ b/script/git-hooks/pre-commit-script
@@ -0,0 +1,9 @@
+#!/bin/sh
+
+gitdir=$(git rev-parse --show-toplevel)
+if [ $? -ne 0 ] ; then
+ echo "git rev-parse --show-toplevel failed"
+ exit 1
+fi
+
+exit 0
diff --git a/wscript b/wscript
index 0985aa94867..9c42f9a6ef6 100644
--- a/wscript
+++ b/wscript
@@ -10,7 +10,7 @@ import sys, os, tempfile
sys.path.insert(0, srcdir+"/buildtools/wafsamba")
import wafsamba, Options, samba_dist, samba_git, Scripting, Utils, samba_version
import Logs, samba_utils
-
+import shutil
samba_dist.DIST_DIRS('.')
samba_dist.DIST_BLACKLIST('.gitignore .bzrignore source4/selftest/provisions')
@@ -105,6 +105,13 @@ default_prefix = Options.default_prefix = '/usr/local/samba'
conf.ADD_CFLAGS('-DDEVELOPER -DDEBUG_PASSWORD')
conf.env.DEVELOPER = True
+ # if we are in a git tree without a pre-commit hook, install a
+ # simple default.
+ pre_commit_hook = os.path.join(srcdir, '.git/hooks/pre-commit')
+ if (os.path.isdir(os.path.dirname(pre_commit_hook)) and
+ shutil.copy(os.path.join(srcdir, 'script/git-hooks/pre-commit-hook'),
+ pre_commit_hook)
conf.ADD_EXTRA_INCLUDES('#include/public #source4 #lib #source4/lib #source4/include #include #lib/replace')
--
2.13.6
From 2aefd14ed47d46b4f362763caa12c13c76c292b4 Mon Sep 17 00:00:00 2001
Date: Tue, 10 Apr 2018 13:19:09 +0200
Subject: [PATCH 2/2] script/git-hooks: add check-trailing-whitespace
---
script/git-hooks/check-trailing-whitespace | 17 +++++++++++++++++
script/git-hooks/pre-commit-script | 2 ++
2 files changed, 19 insertions(+)
create mode 100755 script/git-hooks/check-trailing-whitespace
diff --git a/script/git-hooks/check-trailing-whitespace b/script/git-hooks/check-trailing-whitespace
new file mode 100755
index 00000000000..5303f1fcefa
--- /dev/null
+++ b/script/git-hooks/check-trailing-whitespace
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+git diff-index --cached --check HEAD -- :/*.[ch] :/*.p[ylm]
+
+if [ $? != 0 ]; then
+ echo
+ echo "The commit failed because it seems to introduce trailing whitespace"
+ echo "into C, Perl, or Python code."
+ echo
+ echo "If you are sure you want to do this, repeat the commit with the "
+ echo "--no-verify, like this:"
+ echo
+ echo " git commit --no-verify"
+ exit 1
+fi
+
+exit 0
diff --git a/script/git-hooks/pre-commit-script b/script/git-hooks/pre-commit-script
index eb4114e197d..98e1bd5269e 100755
--- a/script/git-hooks/pre-commit-script
+++ b/script/git-hooks/pre-commit-script
@@ -6,4 +6,6 @@ if [ $? -ne 0 ] ; then
exit 1
fi
+${gitdir}/script/git-hooks/check-trailing-whitespace || exit $?
+
exit 0
--
2.13.6
Ralph Böhme via samba-technical
2018-04-10 19:54:00 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
Ralph, don't you need to add that "--cached" parameter to your script
as well ?
As stated in one of my earlier mails, I believe you need that 'cause
otherwise all modified files are checked and not just the ones which
are part of the commit (were add'ed before).
are you sure? It's a commit hook after all so my expectation was it only check
what I commit. I'll check...
d'oh! You're obviously right...
Updated patch attached.
OK, where are we with this ? Are we able to commit this
as it seems to solve the immediate problem ?
Sure we can add more stuff later, but consider this
a RB+ from me (and when can we push ? :-).
Thanks! I'd like to see broader consensus before we push this. metze? Douglas?
Martin? Andrew? Volker?

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Andrew Bartlett via samba-technical
2018-04-11 01:41:28 UTC
Permalink
On Tue, 2018-04-10 at 21:54 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Jeremy Allison via samba-technical
Post by Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
Ralph, don't you need to add that "--cached" parameter to your script
as well ?
As stated in one of my earlier mails, I believe you need that 'cause
otherwise all modified files are checked and not just the ones which
are part of the commit (were add'ed before).
are you sure? It's a commit hook after all so my expectation was it only check
what I commit. I'll check...
d'oh! You're obviously right...
Updated patch attached.
OK, where are we with this ? Are we able to commit this
as it seems to solve the immediate problem ?
Sure we can add more stuff later, but consider this
a RB+ from me (and when can we push ? :-).
Thanks! I'd like to see broader consensus before we push this. metze? Douglas?
Martin? Andrew? Volker?
I'll give it a try but the concept looks good, being local, immediate,
softly enforced and automated.

Making the broader tree comply will be a longer task, but one I think
we should take on also.

Thanks Douglas,

Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
Douglas Bagnall via samba-technical
2018-04-10 22:41:20 UTC
Permalink
hi Ralph,
Post by Ralph Böhme via samba-technical
diff --git a/script/git-hooks/pre-commit-hook b/script/git-hooks/pre-commit-hook
new file mode 100755
index 00000000000..223a30dc53d
--- /dev/null
+++ b/script/git-hooks/pre-commit-hook
@@ -0,0 +1,7 @@
+#!/bin/sh
+
+gitdir=$(git rev-parse --show-toplevel)
If the above fails for some reason, then the next line will be calling
/script/git-hooks/pre-commit-script ...
Post by Ralph Böhme via samba-technical
+${gitdir}/script/git-hooks/pre-commit-script || exit $?
+
+exit 0
diff --git a/script/git-hooks/pre-commit-script b/script/git-hooks/pre-commit-script
new file mode 100755
index 00000000000..eb4114e197d
--- /dev/null
+++ b/script/git-hooks/pre-commit-script
@@ -0,0 +1,9 @@
+#!/bin/sh
+gitdir=$(git rev-parse --show-toplevel)
+if [ $? -ne 0 ] ; then
+ echo "git rev-parse --show-toplevel failed"
+ exit 1
+fi
therefore I think we need the rev-parse failure check in
pre-commit-hook. (or a `set -e` for the same effect).

Otherwise good.

Douglas
Ralph Böhme via samba-technical
2018-04-11 05:48:43 UTC
Permalink
Hi Douglas,
Post by Douglas Bagnall via samba-technical
Post by Ralph Böhme via samba-technical
diff --git a/script/git-hooks/pre-commit-hook b/script/git-hooks/pre-commit-hook
new file mode 100755
index 00000000000..223a30dc53d
--- /dev/null
+++ b/script/git-hooks/pre-commit-hook
@@ -0,0 +1,7 @@
+#!/bin/sh
+
+gitdir=$(git rev-parse --show-toplevel)
If the above fails for some reason, then the next line will be calling
/script/git-hooks/pre-commit-script ...
Post by Ralph Böhme via samba-technical
+${gitdir}/script/git-hooks/pre-commit-script || exit $?
+
+exit 0
diff --git a/script/git-hooks/pre-commit-script b/script/git-hooks/pre-commit-script
new file mode 100755
index 00000000000..eb4114e197d
--- /dev/null
+++ b/script/git-hooks/pre-commit-script
@@ -0,0 +1,9 @@
+#!/bin/sh
+gitdir=$(git rev-parse --show-toplevel)
+if [ $? -ne 0 ] ; then
+ echo "git rev-parse --show-toplevel failed"
+ exit 1
+fi
therefore I think we need the rev-parse failure check in
pre-commit-hook. (or a `set -e` for the same effect).
oh, good catch, thanks. I thought I had added the check in both places...

Attached is an updated version with a fix for that and including the "unset
GIT_LITERAL_PATHSPECS" for emacs/magit.

This has several acks and no nack, so this version could be pushed if it passes
a final review. Thanks!

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Ralph Böhme via samba-technical
2018-04-11 06:24:56 UTC
Permalink
Post by Ralph Böhme via samba-technical
This has several acks and no nack, so this version could be pushed if it passes
a final review. Thanks!
just noticed that this version will fail if you git rebase to a version of the
tree that doesn't have the patchset but the git-hook is in place.

Attached patchset has an additional check in the hook:

+if [ ! -f ${gitdir}/script/git-hooks/pre-commit-script ] ; then
+ exit 0
+fi
+

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Martin Schwenke via samba-technical
2018-04-11 06:45:26 UTC
Permalink
On Wed, 11 Apr 2018 08:24:56 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
This has several acks and no nack, so this version could be pushed if it passes
a final review. Thanks!
just noticed that this version will fail if you git rebase to a version of the
tree that doesn't have the patchset but the git-hook is in place.
+if [ ! -f ${gitdir}/script/git-hooks/pre-commit-script ] ; then
+ exit 0
+fi
+
A thing of beauty! Ship it! :-)


peace & happiness,
martin
Stefan Metzmacher via samba-technical
2018-04-11 08:04:32 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
This has several acks and no nack, so this version could be pushed if it passes
a final review. Thanks!
just noticed that this version will fail if you git rebase to a version of the
tree that doesn't have the patchset but the git-hook is in place.
+if [ ! -f ${gitdir}/script/git-hooks/pre-commit-script ] ; then
+ exit 0
+fi
+
Why is unset GIT_LITERAL_PATHSPECS not needed in 'pre-commit-script',
but not in 'pre-commit-hook'?

I'd also prefer to have 'set -e' and 'set -u' in (at least)
pre-commit-hook, before it gets install widely.

metze
Ralph Böhme via samba-technical
2018-04-11 09:59:45 UTC
Permalink
Post by Stefan Metzmacher via samba-technical
Post by Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
This has several acks and no nack, so this version could be pushed if it passes
a final review. Thanks!
just noticed that this version will fail if you git rebase to a version of the
tree that doesn't have the patchset but the git-hook is in place.
+if [ ! -f ${gitdir}/script/git-hooks/pre-commit-script ] ; then
+ exit 0
+fi
+
Why is unset GIT_LITERAL_PATHSPECS not needed in 'pre-commit-script',
??? I guess you meant "why is it needed in 'pre-commit-script'" ? It's needed
there because we want to remove it from the environment that later gets passed
to any other shell script.
Post by Stefan Metzmacher via samba-technical
but not in 'pre-commit-hook'?
Because git rev-parse doesn't suffer from the issue described in the issue
tracker.

Being in the script and not in the installed hook, we can later revisit this
workaroung for the magit issue.
Post by Stefan Metzmacher via samba-technical
I'd also prefer to have 'set -e' and 'set -u' in (at least)
pre-commit-hook, before it gets install widely.
Added to both hook and script.

Updated patch attached, please push if happy. Thanks!

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Martin Schwenke via samba-technical
2018-04-17 23:14:07 UTC
Permalink
On Wed, 11 Apr 2018 11:59:45 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
[...]
Post by Stefan Metzmacher via samba-technical
Post by Ralph Böhme via samba-technical
just noticed that this version will fail if you git rebase to a version of the
tree that doesn't have the patchset but the git-hook is in place.
+if [ ! -f ${gitdir}/script/git-hooks/pre-commit-script ] ; then
+ exit 0
+fi
+
Why is unset GIT_LITERAL_PATHSPECS not needed in 'pre-commit-script',
??? I guess you meant "why is it needed in 'pre-commit-script'" ? It's needed
there because we want to remove it from the environment that later gets passed
to any other shell script.
Post by Stefan Metzmacher via samba-technical
but not in 'pre-commit-hook'?
Because git rev-parse doesn't suffer from the issue described in the issue
tracker.
Being in the script and not in the installed hook, we can later revisit this
workaroung for the magit issue.
Post by Stefan Metzmacher via samba-technical
I'd also prefer to have 'set -e' and 'set -u' in (at least)
pre-commit-hook, before it gets install widely.
Added to both hook and script.
Updated patch attached, please push if happy. Thanks!
I notice this hasn't gone in yet. Metze, are you happy with it?

Thanks...

peace & happiness,
martin
Ralph Böhme via samba-technical
2018-04-25 06:03:49 UTC
Permalink
Post by Martin Schwenke via samba-technical
On Wed, 11 Apr 2018 11:59:45 +0200, Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Stefan Metzmacher via samba-technical
I'd also prefer to have 'set -e' and 'set -u' in (at least)
pre-commit-hook, before it gets install widely.
Added to both hook and script.
Updated patch attached, please push if happy. Thanks!
I notice this hasn't gone in yet. Metze, are you happy with it?
also, how does/will this align with what Simo is proposing?

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Martin Schwenke via samba-technical
2018-04-25 08:40:48 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Martin Schwenke via samba-technical
On Wed, 11 Apr 2018 11:59:45 +0200, Ralph Böhme via samba-technical
[...]
Post by Martin Schwenke via samba-technical
Post by Ralph Böhme via samba-technical
Added to both hook and script.
Updated patch attached, please push if happy. Thanks!
I notice this hasn't gone in yet. Metze, are you happy with it?
also, how does/will this align with what Simo is proposing?
I think this should go in now to avoid the problem we were actually
seeing.

I think Simo's tool should go in too so that we can play with it. I've
already used it to clean up some of my commits. Those who have enough
courage can add it to their commit hook (possibly ignoring the result
and using it to provide warnings). Eventually we can even look at
adding it to tests.

However, I think we need something simple now, while we work on the
long term solution and long term approach.

peace & happiness,
martin
Jeremy Allison via samba-technical
2018-04-25 15:42:25 UTC
Permalink
Post by Martin Schwenke via samba-technical
Post by Ralph Böhme via samba-technical
Post by Martin Schwenke via samba-technical
On Wed, 11 Apr 2018 11:59:45 +0200, Ralph Böhme via samba-technical
[...]
Post by Martin Schwenke via samba-technical
Post by Ralph Böhme via samba-technical
Added to both hook and script.
Updated patch attached, please push if happy. Thanks!
I notice this hasn't gone in yet. Metze, are you happy with it?
also, how does/will this align with what Simo is proposing?
I think this should go in now to avoid the problem we were actually
seeing.
I think Simo's tool should go in too so that we can play with it. I've
already used it to clean up some of my commits. Those who have enough
courage can add it to their commit hook (possibly ignoring the result
and using it to provide warnings). Eventually we can even look at
adding it to tests.
However, I think we need something simple now, while we work on the
long term solution and long term approach.
Yes, I agree. What is the blocker on this immediate fix ? Metze,
are you OK with this ? If not can you articulate the problem ?
Stefan Metzmacher via samba-technical
2018-04-25 18:03:16 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Martin Schwenke via samba-technical
Post by Ralph Böhme via samba-technical
On Wed, 11 Apr 2018 11:59:45 +0200, Ralph Böhme via samba-technical
[...]
Post by Ralph Böhme via samba-technical
Added to both hook and script.
Updated patch attached, please push if happy. Thanks!
I notice this hasn't gone in yet. Metze, are you happy with it?
also, how does/will this align with what Simo is proposing?
I think this should go in now to avoid the problem we were actually
seeing.
I think Simo's tool should go in too so that we can play with it. I've
already used it to clean up some of my commits. Those who have enough
courage can add it to their commit hook (possibly ignoring the result
and using it to provide warnings). Eventually we can even look at
adding it to tests.
However, I think we need something simple now, while we work on the
long term solution and long term approach.
Yes, I agree. What is the blocker on this immediate fix ? Metze,
are you OK with this ? If not can you articulate the problem ?
No objections...

metze

Martin Schwenke via samba-technical
2018-04-11 02:10:18 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Ralph Böhme via samba-technical
Post by Swen Schillig via samba-technical
Ralph, don't you need to add that "--cached" parameter to your script
as well ?
As stated in one of my earlier mails, I believe you need that 'cause
otherwise all modified files are checked and not just the ones which
are part of the commit (were add'ed before).
are you sure? It's a commit hook after all so my expectation was it only check
what I commit. I'll check...
d'oh! You're obviously right...
Updated patch attached.
Looks good:

Reviewed-by: Martin Schwenke <***@meltin.net>

We can followup with a patch to make it work under magit, probbaly via:

unset GIT_LITERAL_PATHSPECS

Alternatively, if others agree then this can go in now.

peace & happiness,
martin
Ralph Böhme via samba-technical
2018-04-10 12:22:18 UTC
Permalink
Post by Martin Schwenke via samba-technical
Hmmm... that said, Douglas' pre-commit hook doesn't work under magit.
It works if I don't specify any paths. For some reason it doesn't
trigger if I do add the paths that match C and/or Python files. I
think there's some subtlety in the way the paths are specified in the
environment magit is running git in. :-(
yeah, I noticed that as well but haven't looked into it. As a heavy magit user
myself, I'll ensure to get this working from within magit. :)

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Martin Schwenke via samba-technical
2018-04-10 12:34:18 UTC
Permalink
Post by Ralph Böhme via samba-technical
Post by Martin Schwenke via samba-technical
Hmmm... that said, Douglas' pre-commit hook doesn't work under magit.
It works if I don't specify any paths. For some reason it doesn't
trigger if I do add the paths that match C and/or Python files. I
think there's some subtlety in the way the paths are specified in the
environment magit is running git in. :-(
yeah, I noticed that as well but haven't looked into it. As a heavy magit user
myself, I'll ensure to get this working from within magit. :)
Cool! :-)

I opened this and got an initial response:

https://github.com/magit/magit/issues/3419

peace & happiness,
martin
Martin Schwenke via samba-technical
2018-04-10 21:44:18 UTC
Permalink
Post by Martin Schwenke via samba-technical
Post by Ralph Böhme via samba-technical
Post by Martin Schwenke via samba-technical
Hmmm... that said, Douglas' pre-commit hook doesn't work under magit.
It works if I don't specify any paths. For some reason it doesn't
trigger if I do add the paths that match C and/or Python files. I
think there's some subtlety in the way the paths are specified in the
environment magit is running git in. :-(
yeah, I noticed that as well but haven't looked into it. As a heavy magit user
myself, I'll ensure to get this working from within magit. :)
Cool! :-)
https://github.com/magit/magit/issues/3419
As per that issue, the neatest solution seems to be to do this near the
top of the script:

unset GIT_LITERAL_PATHSPECS

peace & happiness,
martin
Douglas Bagnall via samba-technical
2018-04-10 19:50:47 UTC
Permalink
Post by Stefan Metzmacher via samba-technical
Post by Douglas Bagnall via samba-technical
Post by Stefan Metzmacher via samba-technical
I think we should use some like wireshark,
were the real script[s] are part of the checkout and the pre-commit hook
is just a wrapper arround it, that way we can improve the script step by
step.
See
https://code.wireshark.org/review/gitweb?p=wireshark.git;a=blob;f=tools/pre-commit
Yes.
Of course, a difficulty we have that wireshark and other projects
don't is 17655 lines of C with trailing whitespace and nearly 50k
lines over 80 characters, which we don't want to change all at once. I
think the wireshark scripts check the whole of the affected file on
the assumption that everything you didn't change will still be
perfectly formatted. Our scripts will have to fiddle around with
diffs.
I'm not proposing to use the wireshark scripts, I haven't looked at them
in details.
Yeah, I got that and agree -- I was just drifting off on a tangent.
Post by Stefan Metzmacher via samba-technical
The logic for using a script that is part of the checkout and having a
.git/hook/pre-commit script that just calls into the working tree
is what I'd like to have (and maybe the logic to detect an outdated script)
If this hasn't been fixed overnight, could we just make the hook a
symlink into the tree rather than a copy?

Douglas
Ralph Böhme via samba-technical
2018-04-10 19:55:35 UTC
Permalink
Post by Douglas Bagnall via samba-technical
Post by Stefan Metzmacher via samba-technical
The logic for using a script that is part of the checkout and having a
.git/hook/pre-commit script that just calls into the working tree
is what I'd like to have (and maybe the logic to detect an outdated script)
If this hasn't been fixed overnight, could we just make the hook a
symlink into the tree rather than a copy?
cf one of the other branches in this thread. :)

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Ralph Böhme via samba-technical
2018-04-10 10:44:31 UTC
Permalink
Hi metze,
Post by Stefan Metzmacher via samba-technical
Post by Swen Schillig via samba-technical
I really like Douglas' work as well, especially the part where
the pre-commit hook gets installed auto-magically if there isn't one.
But, is trailing white-spaces all we want to check for ?
Why not go a bit further and have a few more of those "annoying"
violations flagged as well ?
Like
- leading white-space
- code indent should use tabs where possible
- space required before the open parenthesis '('
- line over 80 characters
- Missing a blank line after declarations
- space prohibited before that close parenthesis ')'
- ...
especially if all those checks can be turned off, if required for
whatever reason, with a simple "git --no-check ...."
I was also wondering about that.
well, the problem that caused the whole discussion was trailing whitespace, so
maybe we should, at least for now, just fix the problem at hand and defer
more sophisticated stuff for later. I don't think there will be quick consensus
on such a tool that correctly checks for everything in README.Coding and then
there's also stuff in README.Coding that is dubious like formatting in for
loops: we seem to expect

i = 0;

if (i < 10) {
i += 1;
}

but deem

for (i=0; i<10; i+=1)

acceptable.

I guess there will be more cases like this.

Fwiw, you have seen this parallel thread:
https://lists.samba.org/archive/samba-technical/2018-April/126906.html

-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Stefan Metzmacher via samba-technical
2018-04-10 10:48:49 UTC
Permalink
Post by Ralph Böhme via samba-technical
Hi metze,
Post by Stefan Metzmacher via samba-technical
Post by Swen Schillig via samba-technical
I really like Douglas' work as well, especially the part where
the pre-commit hook gets installed auto-magically if there isn't one.
But, is trailing white-spaces all we want to check for ?
Why not go a bit further and have a few more of those "annoying"
violations flagged as well ?
Like
- leading white-space
- code indent should use tabs where possible
- space required before the open parenthesis '('
- line over 80 characters
- Missing a blank line after declarations
- space prohibited before that close parenthesis ')'
- ...
especially if all those checks can be turned off, if required for
whatever reason, with a simple "git --no-check ...."
I was also wondering about that.
well, the problem that caused the whole discussion was trailing whitespace, so
maybe we should, at least for now, just fix the problem at hand and defer
more sophisticated stuff for later. I don't think there will be quick consensus
on such a tool that correctly checks for everything in README.Coding and then
there's also stuff in README.Coding that is dubious like formatting in for
loops: we seem to expect
Sure, we should start simple, but we should have a way to extend it
easily. And if we have a the check logic directly in
.git/hooks/pre-commit, then we can't update it easily.

metze
Loading...