[Openvpn-devel] pre-commit: uncrustify based on staged changes

Message ID 20220517210121.1312072-1-heiko@ist.eigentlich.net
State Accepted
Headers show
Series
  • [Openvpn-devel] pre-commit: uncrustify based on staged changes
Related show

Commit Message

Heiko Hund May 17, 2022, 9:01 p.m.
Previously the generated patch was based on the file(s) in the working
directory. This is a problem if you have not to be commited changes
there and these changes fix formatting issues that exist in the staging
area. This effectively circumventes the script from rejecting the
commit.

An example:
   git add file.c
   git commit
   ... pre-commit hooks complains about formatting ...
   ... you fix the file manually, forget to git add ...
   git commit
   ... succeeds, even though the commit still has issues ...

Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
---
 dev-tools/git-pre-commit-uncrustify.sh | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

Comments

Frank Lichtenheld May 18, 2022, 9:41 a.m. | #1
Acked-By: Frank Lichtenheld <frank@lichtenheld.com>

Tested various combinations of workspace changes and index changes and
it seems to do what it should do.

> Heiko Hund <heiko@ist.eigentlich.net> hat am 17.05.2022 23:01 geschrieben:
> 
>  
> Previously the generated patch was based on the file(s) in the working
> directory. This is a problem if you have not to be commited changes
> there and these changes fix formatting issues that exist in the staging
> area. This effectively circumventes the script from rejecting the
> commit.
> 
> An example:
>    git add file.c
>    git commit
>    ... pre-commit hooks complains about formatting ...
>    ... you fix the file manually, forget to git add ...
>    git commit
>    ... succeeds, even though the commit still has issues ...
> 
> Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
> ---
>  dev-tools/git-pre-commit-uncrustify.sh | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/dev-tools/git-pre-commit-uncrustify.sh b/dev-tools/git-pre-commit-uncrustify.sh
> index a5cf3d41..9851c212 100755
> --- a/dev-tools/git-pre-commit-uncrustify.sh
> +++ b/dev-tools/git-pre-commit-uncrustify.sh
> @@ -97,6 +97,7 @@ fi
>  
>  # create a filename to store our generated patch
>  patch=$(mktemp /tmp/ovpn-fmt-XXXXXX)
> +tmpout=$(mktemp /tmp/uncrustify-XXXXXX)
>  
>  # create one patch containing all changes to the files
>  # sed to remove quotes around the filename, if inserted by the system
> @@ -106,21 +107,11 @@ sed -e 's/^"\(.*\)"$/\1/' | \
>  while read file
>  do
>      # ignore file if we do check for file extensions and the file
> -    # does not match any of the extensions specified in $FILE_EXTS
> +    # does not match the extensions .c or .h
>      if ! matches_extension "$file"; then
>          continue;
>      fi
>  
> -    # escape special characters in the source filename:
> -    # - '\': backslash needs to be escaped
> -    # - '*': used as matching string => '*' would mean expansion
> -    #        (curiously, '?' must not be escaped)
> -    # - '[': used as matching string => '[' would mean start of set
> -    # - '|': used as sed split char instead of '/', so it needs to be escaped
> -    #        in the filename
> -    # printf %s particularly important if the filename contains the % character
> -    file_escaped_source=$(printf "%s" "$file" | sed -e 's/[\*[|]/\\&/g')
> -
>      # escape special characters in the target filename:
>      # phase 1 (characters escaped in the output diff):
>      #     - '\': backslash needs to be escaped in the output diff
> @@ -136,15 +127,17 @@ do
>  
>      # uncrustify our sourcefile, create a patch with diff and append it to our $patch
>      # The sed call is necessary to transform the patch from
> -    #    --- $file timestamp
> -    #    +++ - timestamp
> +    #    --- - timestamp
> +    #    +++ $tmpout timestamp
>      # to both lines working on the same file and having a a/ and b/ prefix.
>      # Else it can not be applied with 'git apply'.
> -    "$UNCRUSTIFY" -q -c "$UNCRUST_CONFIG" -f "$file" | \
> -        diff -u -- "$file" - | \
> -        sed -e "1s|--- $file_escaped_source|--- \"a/$file_escaped_target\"|" -e "2s|+++ -|+++ \"b/$file_escaped_target\"|" >> "$patch"
> +    git show ":$file" | "$UNCRUSTIFY" -q -l C -c "$UNCRUST_CONFIG" -o "$tmpout"
> +    git show ":$file" | diff -u -- - "$tmpout" | \
> +        sed -e "1s|--- -|--- \"b/$file_escaped_target\"|" -e "2s|+++ $tmpout|+++ \"a/$file_escaped_target\"|" >> "$patch"
>  done
>  
> +rm -f "$tmpout"
> +
>  # if no patch has been generated all is ok, clean up the file stub and exit
>  if [ ! -s "$patch" ] ; then
>      rm -f "$patch"
> -- 
> 2.32.0
> 
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

--
Frank Lichtenheld
Antonio Quartulli May 18, 2022, 1:35 p.m. | #2
Hi,

On 17/05/2022 23:01, Heiko Hund wrote:
> Previously the generated patch was based on the file(s) in the working
> directory. This is a problem if you have not to be commited changes
> there and these changes fix formatting issues that exist in the staging
> area. This effectively circumventes the script from rejecting the
> commit.
> 
> An example:
>     git add file.c
>     git commit
>     ... pre-commit hooks complains about formatting ...
>     ... you fix the file manually, forget to git add ...
>     git commit
>     ... succeeds, even though the commit still has issues ...
> 
> Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>

I tested the case described in the example (that I actually reported to 
Heiko) and I can confirm it is now fixed.

Normal workflow seems to be still working as well :-)

minor comments below

> ---
>   dev-tools/git-pre-commit-uncrustify.sh | 25 +++++++++----------------
>   1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/dev-tools/git-pre-commit-uncrustify.sh b/dev-tools/git-pre-commit-uncrustify.sh
> index a5cf3d41..9851c212 100755
> --- a/dev-tools/git-pre-commit-uncrustify.sh
> +++ b/dev-tools/git-pre-commit-uncrustify.sh
> @@ -97,6 +97,7 @@ fi
>   
>   # create a filename to store our generated patch
>   patch=$(mktemp /tmp/ovpn-fmt-XXXXXX)
> +tmpout=$(mktemp /tmp/uncrustify-XXXXXX)
>   
>   # create one patch containing all changes to the files
>   # sed to remove quotes around the filename, if inserted by the system
> @@ -106,21 +107,11 @@ sed -e 's/^"\(.*\)"$/\1/' | \
>   while read file
>   do
>       # ignore file if we do check for file extensions and the file
> -    # does not match any of the extensions specified in $FILE_EXTS
> +    # does not match the extensions .c or .h

is this unrelated?

>       if ! matches_extension "$file"; then
>           continue;
>       fi
>   
> -    # escape special characters in the source filename:
> -    # - '\': backslash needs to be escaped
> -    # - '*': used as matching string => '*' would mean expansion
> -    #        (curiously, '?' must not be escaped)
> -    # - '[': used as matching string => '[' would mean start of set
> -    # - '|': used as sed split char instead of '/', so it needs to be escaped
> -    #        in the filename
> -    # printf %s particularly important if the filename contains the % character
> -    file_escaped_source=$(printf "%s" "$file" | sed -e 's/[\*[|]/\\&/g')
> -

this seems unrelated too?

>       # escape special characters in the target filename:
>       # phase 1 (characters escaped in the output diff):
>       #     - '\': backslash needs to be escaped in the output diff
> @@ -136,15 +127,17 @@ do
>   
>       # uncrustify our sourcefile, create a patch with diff and append it to our $patch
>       # The sed call is necessary to transform the patch from
> -    #    --- $file timestamp
> -    #    +++ - timestamp
> +    #    --- - timestamp
> +    #    +++ $tmpout timestamp

same?

>       # to both lines working on the same file and having a a/ and b/ prefix.
>       # Else it can not be applied with 'git apply'.
> -    "$UNCRUSTIFY" -q -c "$UNCRUST_CONFIG" -f "$file" | \
> -        diff -u -- "$file" - | \
> -        sed -e "1s|--- $file_escaped_source|--- \"a/$file_escaped_target\"|" -e "2s|+++ -|+++ \"b/$file_escaped_target\"|" >> "$patch"
> +    git show ":$file" | "$UNCRUSTIFY" -q -l C -c "$UNCRUST_CONFIG" -o "$tmpout"
> +    git show ":$file" | diff -u -- - "$tmpout" | \
> +        sed -e "1s|--- -|--- \"b/$file_escaped_target\"|" -e "2s|+++ $tmpout|+++ \"a/$file_escaped_target\"|" >> "$patch"
>   done
>   
> +rm -f "$tmpout"
> +
>   # if no patch has been generated all is ok, clean up the file stub and exit
>   if [ ! -s "$patch" ] ; then
>       rm -f "$patch"
Heiko Hund May 18, 2022, 1:51 p.m. | #3
Hi

On Mittwoch, 18. Mai 2022 15:35:58 CEST Antonio Quartulli wrote:
> > -    # does not match any of the extensions specified in $FILE_EXTS
> > +    # does not match the extensions .c or .h
> 
> is this unrelated?

Yes, it is. The original script (collection) has a config file where $FILE_EXTS 
is defined. However, we hardcode this to C and header files only. Simply and 
oversight from the initial version drive-by fix.
 
> > -    # escape special characters in the source filename:
> > -    # - '\': backslash needs to be escaped
> > -    # - '*': used as matching string => '*' would mean expansion
> > -    #        (curiously, '?' must not be escaped)
> > -    # - '[': used as matching string => '[' would mean start of set
> > -    # - '|': used as sed split char instead of '/', so it needs to be
> > escaped -    #        in the filename
> > -    # printf %s particularly important if the filename contains the %
> > character -    file_escaped_source=$(printf "%s" "$file" | sed -e
> > 's/[\*[|]/\\&/g') -
> 
> this seems unrelated too?

See below.
 
> > -    #    --- $file timestamp
> > -    #    +++ - timestamp
> > +    #    --- - timestamp
> > +    #    +++ $tmpout timestamp
> 
> same?

See below.

> > +    git show ":$file" | diff -u -- - "$tmpout" | \
> > +        sed -e "1s|--- -|--- \"b/$file_escaped_target\"|" -e "2s|+++
> > $tmpout|+++ \"a/$file_escaped_target\"|" >> "$patch"> 

Since the patch is now done differently $file_escaped_source is no longer 
needed. Instead we use $tmpout (the uncrustify output), which has a well 
defined (no need to being escaped) filename. 

The file contents we diff from stdin has changed as well. It is no longer the 
uncrustify output, but instead the file contents from the index (staged), thus 
the --- and +++ lines, we need to fix, switched as well.

Hope that explains it.

Heiko
Antonio Quartulli May 18, 2022, 1:59 p.m. | #4
Hi,

On 18/05/2022 15:51, Heiko Hund wrote:
[CUT]
> Hope that explains it.
Thanks, it makes sense.

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering May 22, 2022, 3:20 p.m. | #5
I have not tested this much - will use it soon :-) - but I'm fine
if Frank and Antonio says this works now for "with stage" usage,
too.

Your patch has been applied to the master branch.

commit d78c2a73a8f631f082ec03bb21b329d3e6fd00dd
Author: Heiko Hund
Date:   Tue May 17 23:01:21 2022 +0200

     pre-commit: uncrustify based on staged changes

     Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Acked-by: Antonio Quartulli <a@unstable.cc>
     Message-Id: <20220517210121.1312072-1-heiko@ist.eigentlich.net>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24376.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/dev-tools/git-pre-commit-uncrustify.sh b/dev-tools/git-pre-commit-uncrustify.sh
index a5cf3d41..9851c212 100755
--- a/dev-tools/git-pre-commit-uncrustify.sh
+++ b/dev-tools/git-pre-commit-uncrustify.sh
@@ -97,6 +97,7 @@  fi
 
 # create a filename to store our generated patch
 patch=$(mktemp /tmp/ovpn-fmt-XXXXXX)
+tmpout=$(mktemp /tmp/uncrustify-XXXXXX)
 
 # create one patch containing all changes to the files
 # sed to remove quotes around the filename, if inserted by the system
@@ -106,21 +107,11 @@  sed -e 's/^"\(.*\)"$/\1/' | \
 while read file
 do
     # ignore file if we do check for file extensions and the file
-    # does not match any of the extensions specified in $FILE_EXTS
+    # does not match the extensions .c or .h
     if ! matches_extension "$file"; then
         continue;
     fi
 
-    # escape special characters in the source filename:
-    # - '\': backslash needs to be escaped
-    # - '*': used as matching string => '*' would mean expansion
-    #        (curiously, '?' must not be escaped)
-    # - '[': used as matching string => '[' would mean start of set
-    # - '|': used as sed split char instead of '/', so it needs to be escaped
-    #        in the filename
-    # printf %s particularly important if the filename contains the % character
-    file_escaped_source=$(printf "%s" "$file" | sed -e 's/[\*[|]/\\&/g')
-
     # escape special characters in the target filename:
     # phase 1 (characters escaped in the output diff):
     #     - '\': backslash needs to be escaped in the output diff
@@ -136,15 +127,17 @@  do
 
     # uncrustify our sourcefile, create a patch with diff and append it to our $patch
     # The sed call is necessary to transform the patch from
-    #    --- $file timestamp
-    #    +++ - timestamp
+    #    --- - timestamp
+    #    +++ $tmpout timestamp
     # to both lines working on the same file and having a a/ and b/ prefix.
     # Else it can not be applied with 'git apply'.
-    "$UNCRUSTIFY" -q -c "$UNCRUST_CONFIG" -f "$file" | \
-        diff -u -- "$file" - | \
-        sed -e "1s|--- $file_escaped_source|--- \"a/$file_escaped_target\"|" -e "2s|+++ -|+++ \"b/$file_escaped_target\"|" >> "$patch"
+    git show ":$file" | "$UNCRUSTIFY" -q -l C -c "$UNCRUST_CONFIG" -o "$tmpout"
+    git show ":$file" | diff -u -- - "$tmpout" | \
+        sed -e "1s|--- -|--- \"b/$file_escaped_target\"|" -e "2s|+++ $tmpout|+++ \"a/$file_escaped_target\"|" >> "$patch"
 done
 
+rm -f "$tmpout"
+
 # if no patch has been generated all is ok, clean up the file stub and exit
 if [ ! -s "$patch" ] ; then
     rm -f "$patch"