bug#29197: 27.0.50; pre-commit checks for new files against "head"

bug#29197: 27.0.50; pre-commit checks for new files against "head"

[Stefan Monnier]

Package: Emacs
Version: 27.0.50


I finally figured out why recently, every time I merge changes from
master into my local branch it complains:

    File name does not consist of -+./_ or ASCII letters or digits.

It turns out it's because it's looking at the diff between master and my
local (merged) branch (i.e. it looks at my local changes) whereas before
it would look at the diff between the old version of my local branch and
the merged version of my local branch (i.e. at the changes I'm pulling
from master).

And yes, indeed, my local branch has some files with "weird" chars
in it.

I think the warning should be improved:
- I shouldn't get a warning in the above case, since this commit doesn't
  *add* those files (they weren't on origin/master admittedly but they were
  already on HEAD).
- the warning should give me some hint about which file fails the test.


        Stefan



In GNU Emacs 27.0.50 (build 1, x86_64-unknown-linux-gnu, GTK+ Version 3.22.24)
 of 2017-11-05 built on alfajor
Repository revision: 7b20a85bf651debb66742d7ed8a55bdc883ded79
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
System Description:	Debian GNU/Linux testing (buster)

Recent messages:
Undo!
Saving file /home/monnier/src/emacs/trunk/build-aux/git-hooks/pre-commit...
Wrote /home/monnier/src/emacs/trunk/build-aux/git-hooks/pre-commit
funcall-interactively: Buffer is read-only: #<buffer pre-commit | hooks>
Read-Only mode disabled in current buffer
Saving file /home/monnier/src/emacs/work/.git/hooks/pre-commit...
Wrote /home/monnier/src/emacs/work/.git/hooks/pre-commit
Saving file /home/monnier/src/emacs/trunk/build-aux/git-hooks/pre-commit...
Wrote /home/monnier/src/emacs/trunk/build-aux/git-hooks/pre-commit
Mark set [2 times]

Configured using:
 'configure -C --enable-checking --with-modules --enable-check-lisp-object-type
 'CFLAGS=-Wall -g3 -Og -Wno-pointer-sign'
 PKG_CONFIG_PATH=/home/monnier/lib/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS
GTK3 X11 MODULES LCMS2

Important settings:
  value of $LANG: fr_CH.UTF-8
  locale-coding-system: utf-8-unix

Major mode: InactiveMinibuffer

Minor modes in effect:
  diff-auto-refine-mode: t
  electric-pair-mode: t
  global-reveal-mode: t
  reveal-mode: t
  auto-insert-mode: t
  savehist-mode: t
  minibuffer-electric-default-mode: t
  global-compact-docstrings-mode: t
  url-handler-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  global-prettify-symbols-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/monnier/src/emacs/elpa/packages/svg/svg hides /home/monnier/src/emacs/work/lisp/svg
/home/monnier/src/emacs/elpa/packages/ada-mode/ada-mode hides /home/monnier/src/emacs/work/lisp/progmodes/ada-mode
/home/monnier/src/emacs/elpa/packages/ada-mode/ada-stmt hides /home/monnier/src/emacs/work/lisp/progmodes/ada-stmt
/home/monnier/src/emacs/elpa/packages/ada-mode/ada-prj hides /home/monnier/src/emacs/work/lisp/progmodes/ada-prj
/home/monnier/src/emacs/elpa/packages/ada-mode/ada-xref hides /home/monnier/src/emacs/work/lisp/progmodes/ada-xref
/home/monnier/src/emacs/elpa/packages/hyperbole/set hides /home/monnier/src/emacs/work/lisp/emacs-lisp/set
/home/monnier/src/emacs/elpa/packages/landmark/landmark hides /home/monnier/src/emacs/work/lisp/obsolete/landmark
/home/monnier/src/emacs/elpa/packages/crisp/crisp hides /home/monnier/src/emacs/work/lisp/obsolete/crisp

Features:
(sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml mml-sec epa derived epg gnus-util rmail
rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils add-log log-view pcvs-util sh-script smie
make-mode executable copyright whitespace vc vc-dispatcher smerge-mode
lisp-mnt xscheme unsafep trace testcover shadow scheme re-builder
profiler inf-lisp ielm gmm-utils ert pp find-func ewoc debug elp edebug
cl-indent cus-edit cus-start cus-load wid-edit vc-git diff-mode
filecache map cl-extra help-fns radix-tree server time-date flymake-proc
flymake compile comint ansi-color ring warnings noutline outline
easy-mmode flyspell ispell checkdoc thingatpt help-mode load-dir
elec-pair reveal autoinsert proof-site proof-autoloads cl pg-vars
savehist minibuf-eldef disp-table compact-docstrings cl-seq inline
kotl-loaddefs advice info realgud-recursive-autoloads finder-inf
url-auth package easymenu epg-config url-handlers url-parse auth-source
eieio eieio-core cl-macs eieio-loaddefs password-cache url-vars seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib bbdb-loaddefs
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting move-toolbar gtk
x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 8 219296 26772)
 (symbols 24 29613 0) (miscs 20 4050 839) (strings 16 68318 5077)
 (string-bytes 1 2005085)
 (vectors 8 38803) (vector-slots 4 1642549 23736) (floats 8 120 353)
 (intervals 28 2761 113)
 (buffers 528 30))

bug#29197: 27.0.50; pre-commit checks for new files against "head"

[Noam Postavsky]

On Tue, Nov 7, 2017 at 2:18 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> I finally figured out why recently, every time I merge changes from
> master into my local branch it complains:
>
>     File name does not consist of -+./_ or ASCII letters or digits.
>
> It turns out it's because it's looking at the diff between master and my
> local (merged) branch (i.e. it looks at my local changes) whereas before
> it would look at the diff between the old version of my local branch and
> the merged version of my local branch (i.e. at the changes I'm pulling
> from master).

See [1] and followups. Getting warnings about other people's changes
was causing some confusion and frustration.

[1]: https://lists.gnu.org/archive/html/emacs-devel/2017-04/msg00299.html

> And yes, indeed, my local branch has some files with "weird" chars
> in it.
>
> I think the warning should be improved:
> - I shouldn't get a warning in the above case, since this commit doesn't
>   *add* those files (they weren't on origin/master admittedly but they were
>   already on HEAD).

We could choose which side of the merge to check based on an
environment var (that was considered in the thread I referenced above,
but we didn't see much of a use case at the time). Or is it possible
to check only changes from the merge itself (i.e., in case of conflict
resolution)?

> - the warning should give me some hint about which file fails the test.

Yeah, that would make sense.

bug#29197: 27.0.50; pre-commit checks for new files against "head"

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

>> I think the warning should be improved:
>> - I shouldn't get a warning in the above case, since this commit doesn't
>>   *add* those files (they weren't on origin/master admittedly but they were
>>   already on HEAD).
>
> We could choose which side of the merge to check based on an
> environment var (that was considered in the thread I referenced above,
> but we didn't see much of a use case at the time). Or is it possible
> to check only changes from the merge itself (i.e., in case of conflict
> resolution)?
>
>> - the warning should give me some hint about which file fails the test.
>
> Yeah, that would make sense.

This seems fairly difficult to do while keeping in standard bourne shell
syntax.  Not sure how to catch file names with whitespace.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2187 bytes --]

From fa94e7452be5ffb4d63ad2a40fc5b9fdeed3fed0 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 14 Feb 2018 19:58:07 -0500
Subject: [PATCH v1] ; Let pre-commit git hook check merged in changes
 (Bug#29197)

* build-aux/git-hooks/pre-commit: If GIT_MERGE_CHECK_OTHER is 'true',
check changes against the merge target, rather than the current
branch.  Include file name when giving error message about
non-standard characters.
---
 build-aux/git-hooks/pre-commit | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/build-aux/git-hooks/pre-commit b/build-aux/git-hooks/pre-commit
index 5e42dab233..33d8c91168 100755
--- a/build-aux/git-hooks/pre-commit
+++ b/build-aux/git-hooks/pre-commit
@@ -28,7 +28,7 @@ LC_ALL=
 # When doing a two-way merge, ignore problems that came from the other
 # side of the merge.
 head=HEAD
-if test -r "$GIT_DIR"/MERGE_HEAD; then
+if test -r "$GIT_DIR"/MERGE_HEAD && test "$GIT_MERGE_CHECK_OTHER" != true; then
   merge_heads=`cat "$GIT_DIR"/MERGE_HEAD` || exit
   for merge_head in $merge_heads; do
     case $head in
@@ -42,13 +42,6 @@ head=
 fi
 
 git_diff='git diff --cached --name-only --diff-filter=A'
-ok_chars='\0+[=-=]./0-9A-Z_a-z'
-nbadchars=`$git_diff -z $head | tr -d "$ok_chars" | wc -c`
-
-if test "$nbadchars" -ne 0; then
-  echo "File name does not consist of -+./_ or ASCII letters or digits."
-  exit 1
-fi
 
 for new_name in `$git_diff $head`; do
   case $new_name in
@@ -58,9 +51,20 @@ nbadchars=
     ChangeLog | */ChangeLog)
       echo "$new_name: Please use git commit messages, not ChangeLog files."
       exit 1;;
+    *[^-+./_0-9A-Z_a-z]*)
+      echo "$new_name: File name does not consist of -+./_ or ASCII letters or digits."
+      exit 1;;
   esac
 done
 
+ok_chars='\0+[=-=]./0-9A-Z_a-z'
+nbadchars=`$git_diff -z $head | tr -d "$ok_chars" | wc -c`
+
+if test "$nbadchars" -ne 0; then
+  echo "A file name has whitespace."
+  exit 1
+fi
+
 # The '--check' option of git diff-index makes Git complain if changes
 # introduce whitespace errors.  This can be a pain when editing test
 # files that deliberately contain lines with trailing whitespace.
-- 
2.11.0

bug#29197: 27.0.50; pre-commit checks for new files against "head"

[Noam Postavsky]

tags 29197 fixed
close 29197
quit

Noam Postavsky <npostavs@gmail.com> writes:

> This seems fairly difficult to do while keeping in standard bourne shell
> syntax.  Not sure how to catch file names with whitespace.

Acually, it turns out that 'git diff' does escaping, so it's pretty
easy.

Done in emacs-26.

[1: fca935e4ab]: 2018-08-28 08:04:17 -0400
  ; Let pre-commit git hook check merged in changes (Bug#29197)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fca935e4abe817130abb2676ec2f37b73e4f45f4