| Message ID | 20220517210121.1312072-1-heiko@ist.eigentlich.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] pre-commit: uncrustify based on staged changes | expand |
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
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"
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
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>
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
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"
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(-)