git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Noam Postavsky]

On Tue, Apr 11, 2017 at 11:13 AM, Yuri Khan <yuri.v.khan@gmail.com> wrote:
>> I always want a merge.  So what do you recommend?  In particular what do
>> you recommend when there are conflicts?  IIUC in that case git does not
>> merge anything but waits till I have resolved the conflicts and tells me
>> to commit them when I'm done.  If, at that moment, I do commit I'm in
>> the same situation with SpecialCasing.txt as before.  Or is there any
>> difference?
>
> Well, I would recommend rebasing, which would solve the
> SpecialCasing.txt problem because your rebased branch would start
> after the problematic file has been committed.
>
> But if you insist on merging, it’s doable, too. Then I guess you
> get a merge conflict,
> resolve it,
> stage the resolved files,
> attempt to commit,
> get a whitespace policy violation,
> say an expletive of your choice,
> see that the violation was not your doing,
> optionally confirm that by looking at the history of the problematic
> file in the branch you’re merging,
> then say “Oh well” and commit bypassing the check.

Perhaps we should change the hook so that it doesn't complain about
problems from merges?

--- i/build-aux/git-hooks/pre-commit
+++ w/build-aux/git-hooks/pre-commit
@@ -25,16 +25,23 @@ LC_ALL=

 . git-sh-setup

+# If we're merging, don't flag problems that came from the other side
+# of the merge.
+head=HEAD
+if [ -f "$GIT_DIR"/MERGE_HEAD ] ; then
+    head=$(cat "$GIT_DIR"/MERGE_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`
+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
+for new_name in `$git_diff $head`; do
   case $new_name in
     -* | */-*)
       echo "$new_name: File name component begins with '-'."
@@ -53,4 +60,4 @@ nbadchars=
 # tests so that trailing spaces are generated on the fly rather than
 # being committed as source.

-exec git diff-index --check --cached HEAD --
+exec git diff-index --check --cached "$head" --

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[martin rudalics]

 > Perhaps we should change the hook so that it doesn't complain about
 > problems from merges?

If it (a) solves the problem and (b) there is no strict policy wrt
whitespace commits, I'd strongly favor such a solution.

Thanks, martin

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Paul Eggert]

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

On 04/11/2017 09:25 AM, Noam Postavsky wrote:
> Perhaps we should change the hook so that it doesn't complain about
> problems from merges?

Something like that might make sense, yes, for people in Alan's 
situation. When I do a merge, though, I'd rather see problems from the 
other side (so that I can fix them). How about an environment variable 
that captures the user's preference?

> +# If we're merging, don't flag problems that came from the other side
> +# of the merge.
> +head=HEAD
> +if [ -f "$GIT_DIR"/MERGE_HEAD ] ; then
> +    head=$(cat "$GIT_DIR"/MERGE_HEAD)
> +fi

This won't work if MERGE_HEAD file contains more than one entry, which 
can happen when doing a 3- or more-way merge. How about the attached 
(untested) patch instead? It behaves the way that you suggested, when 
doing a 2-way merge and when the GIT_MERGE_BLINDLY environment variable 
is set to 'true'. Offhand I don't see a way of supporting 3- or more-way 
merges easily, so this patch punts and ignores GIT_MERGE_BLINDLY when 
doing fancier merges.

[-- Attachment #2: 0001-Allow-bypassing-of-some-checks-when-merging.patch --]
[-- Type: application/x-patch, Size: 1803 bytes --]

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Noam Postavsky]

On Wed, Apr 12, 2017 at 2:26 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> When I do a merge, though, I'd rather see problems from the other side (so
> that I can fix them).

I would note that a usual 'git merge' skips the pre-commit hook
entirely, unless the commit is delayed somehow. That is, the hook only
triggers if $EDITOR can't run in the current terminal, there is a
merge conflict, or the --no-commit option is given. 'git rebase' runs
the hook for the other side's changes at all. I think this is why
there was so much confusion over this, Martin hits this frequently
because he runs 'git merge' in a *shell* buffer, whereas most other
people hit it very rarely or not at all.

> How about an environment variable that captures the
> user's preference?

That sounds okay. I wonder if it would be more convenient to control
this by a ./configure option instead though.

>> +    head=$(cat "$GIT_DIR"/MERGE_HEAD)
>
> This won't work if MERGE_HEAD file contains more than one entry, which can
> happen when doing a 3- or more-way merge. How about the attached (untested)
> patch instead? It behaves the way that you suggested, when doing a 2-way
> merge and when the GIT_MERGE_BLINDLY environment variable is set to 'true'.
> Offhand I don't see a way of supporting 3- or more-way merges easily, so
> this patch punts and ignores GIT_MERGE_BLINDLY when doing fancier merges.

Ah right, forgot about multiway-merges. Punting should be fine, I'm
sure anyone trying an octopus merge should have enough git expertise
to handle some commit hook errors.

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Paul Eggert]

Noam Postavsky wrote:
> I wonder if it would be more convenient to control
> this by a ./configure option instead though.

This all happens well before 'configure' is created. It could be an autogen.sh 
option, I suppose.

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Noam Postavsky]

On Wed, Apr 12, 2017 at 9:49 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> Noam Postavsky wrote:
>>
>> I wonder if it would be more convenient to control
>> this by a ./configure option instead though.
>
>
> This all happens well before 'configure' is created.

It does now, but does it need to?

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Paul Eggert]

Noam Postavsky wrote:
>>> I wonder if it would be more convenient to control
>>> this by a ./configure option instead though.
>>
>> This all happens well before 'configure' is created.
> It does now, but does it need to?

It's better if './configure' does not mess with Git settings, yes. './configure' 
has always been intended to operate independently of whether the source is under 
version control, and to operate after all the autotools have been run to set 
things up.

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Noam Postavsky]

On Thu, Apr 13, 2017 at 2:11 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> It's better if './configure' does not mess with Git settings, yes.
> './configure' has always been intended to operate independently of whether
> the source is under version control, and to operate after all the autotools
> have been run to set things up.

It seems preferable to me if the hooks could be managed by
./configure, and furthermore be updated by 'make' (as we have at least
one case of someone running with outdated hooks). Git's hooks are
unrelated to autotools regardless.

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Noam Postavsky]

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

On Wed, Apr 12, 2017 at 2:26 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>>
>> Perhaps we should change the hook so that it doesn't complain about
>> problems from merges?
>
> Something like that might make sense, yes, for people in Alan's situation.
> When I do a merge, though, I'd rather see problems from the other side (so
> that I can fix them). How about an environment variable that captures the
> user's preference?

Since git only runs the hooks on the merged changes when you pass
--no-commit to 'git merge', defaulting to non-blind merge doesn't
really work anyway. There I propose the opposite default, as in the
attached (also you had a copy-pasto "; done" in your patch).

[-- Attachment #2: v2-0001-Allow-bypassing-of-some-checks-when-merging.patch --]
[-- Type: text/x-patch, Size: 2838 bytes --]

From 824e1bac7de365708e92e22b5aa2624e6bf7bd9e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@Penguin.CS.UCLA.EDU>
Date: Wed, 12 Apr 2017 11:24:41 -0700
Subject: [PATCH v2] Allow bypassing of some checks when merging

* build-aux/git-hooks/pre-commit: Don't check merged in changes unless
GIT_MERGE_CAREFULLY is true.
* admin/notes/repo: Explain how to use GIT_MERGE_CAREFULLY.
---
 admin/notes/repo               |  7 +++++++
 build-aux/git-hooks/pre-commit | 22 +++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/admin/notes/repo b/admin/notes/repo
index 3ab3da7807..2416563f83 100644
--- a/admin/notes/repo
+++ b/admin/notes/repo
@@ -76,6 +76,13 @@ conflict by choosing either the master or branch version, then run
 'make -C lisp autoloads' to update the md5sums to the correct master
 value before committing.
 
+* Running git commit hooks on merged changes
+
+Normally git does not run the pre-commit hook on merged changes.  In
+order to run the hooks on the branch being merged in, set the
+environment variable GIT_MERGE_CAREFULLY to 'true', and the
+'--no-commit' option to the 'git merge' call.
+
 * Re-adding a file that has been removed from the repository
 
 Let's suppose you've done:
diff --git a/build-aux/git-hooks/pre-commit b/build-aux/git-hooks/pre-commit
index 6483bfc6b3..59bff79df1 100755
--- a/build-aux/git-hooks/pre-commit
+++ b/build-aux/git-hooks/pre-commit
@@ -25,16 +25,32 @@ LC_ALL=
 
 . git-sh-setup
 
+# Unless GIT_MERGE_CAREFULLY is 'true', then when doing a two-way merge,
+# ignore problems that came from the other side of the merge.
+head=HEAD
+if test -e "$GIT_DIR"/MERGE_HEAD && test "$GIT_MERGE_CAREFULLY" != true; then
+  merge_heads=`cat "$GIT_DIR"/MERGE_HEAD` || exit
+  for merge_head in $merge_heads; do
+    case $head in
+      HEAD) head=$merge_head;;
+      # For multi-head merges, there's no easy way to ignore merged in
+      # changes.  But if you're doing multi-head merges, presumably
+      # you know how to handle any ensuing problems.
+      *) head=HEAD; break;;
+    esac
+  done
+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`
+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
+for new_name in `$git_diff $head`; do
   case $new_name in
     -* | */-*)
       echo "$new_name: File name component begins with '-'."
@@ -53,4 +69,4 @@ nbadchars=
 # tests so that trailing spaces are generated on the fly rather than
 # being committed as source.
 
-exec git diff-index --check --cached HEAD --
+exec git diff-index --check --cached $head --
-- 
2.11.1

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Paul Eggert]

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

Noam Postavsky wrote:
> Since git only runs the hooks on the merged changes when you pass
> --no-commit to 'git merge', defaulting to non-blind merge doesn't
> really work anyway.

Thanks for looking into this. I guess I'll have to rethink how I merge. In the 
meantime I installed the attached simpler patch, which doesn't bother with an 
environment variable and simply skips the checks in a two-way merge.

[-- Attachment #2: 0001-Allow-bypassing-of-some-checks-when-merging.patch --]
[-- Type: text/x-diff, Size: 1899 bytes --]

From 8d323317768aaebfc53e01338dd0a9933d1d7de4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 29 Apr 2017 11:28:52 -0700
Subject: [PATCH] Allow bypassing of some checks when merging

* build-aux/git-hooks/pre-commit: Don't check merged-in changes.
---
 build-aux/git-hooks/pre-commit | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/build-aux/git-hooks/pre-commit b/build-aux/git-hooks/pre-commit
index 6483bfc..548bf93 100755
--- a/build-aux/git-hooks/pre-commit
+++ b/build-aux/git-hooks/pre-commit
@@ -25,16 +25,32 @@ LC_ALL=
 
 . git-sh-setup
 
+# When doing a two-way merge, ignore problems that came from the other
+# side of the merge.
+head=HEAD
+if test -e "$GIT_DIR"/MERGE_HEAD; then
+  merge_heads=`cat "$GIT_DIR"/MERGE_HEAD` || exit
+  for merge_head in $merge_heads; do
+    case $head in
+      HEAD) head=$merge_head;;
+      # For multi-head merges, there's no easy way to ignore merged-in
+      # changes.  But if you're doing multi-head merges, presumably
+      # you know how to handle any ensuing problems.
+      *) head=HEAD; break;;
+    esac
+  done
+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`
+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
+for new_name in `$git_diff $head`; do
   case $new_name in
     -* | */-*)
       echo "$new_name: File name component begins with '-'."
@@ -53,4 +69,4 @@ nbadchars=
 # tests so that trailing spaces are generated on the fly rather than
 # being committed as source.
 
-exec git diff-index --check --cached HEAD --
+exec git diff-index --check --cached $head --
-- 
2.7.4

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 29 Apr 2017 11:44:06 -0700
> Cc: martin rudalics <rudalics@gmx.at>, emacs-devel <emacs-devel@gnu.org>,
> 	Andreas Schwab <schwab@suse.de>, Yuri Khan <yuri.v.khan@gmail.com>
> 
> Noam Postavsky wrote:
> > Since git only runs the hooks on the merged changes when you pass
> > --no-commit to 'git merge', defaulting to non-blind merge doesn't
> > really work anyway.
> 
> Thanks for looking into this. I guess I'll have to rethink how I merge.

Not sure it matters in the context of this discussion, but please keep
in mind that "git cherry-pick" also merges.

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Noam Postavsky]

On Sat, Apr 29, 2017 at 3:15 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> > Since git only runs the hooks on the merged changes when you pass
>> > --no-commit to 'git merge', defaulting to non-blind merge doesn't
>> > really work anyway.
>>
>> Thanks for looking into this. I guess I'll have to rethink how I merge.
>
> Not sure it matters in the context of this discussion, but please keep
> in mind that "git cherry-pick" also merges.

My intuition is that getting a commit hook failure from the commit
being cherry picked will be less confusing than getting hook failures
from one of the  many commits being merged from a branch, so I don't
think it's worth updating the hook to avoid this case.

'git cherry-pick' does have the same issue as 'git merge': it won't
rerun the commit hook, unless '--no-commit' is passed (or something
else prevents the auto-commit, e.g., a conflict, or $EDITOR failing
due to lack of terminal).

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sat, 29 Apr 2017 15:54:41 -0400
> Cc: Paul Eggert <eggert@cs.ucla.edu>, martin rudalics <rudalics@gmx.at>, 
> 	Emacs developers <emacs-devel@gnu.org>, Andreas Schwab <schwab@suse.de>, Yuri Khan <yuri.v.khan@gmail.com>
> 
> On Sat, Apr 29, 2017 at 3:15 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> >> > Since git only runs the hooks on the merged changes when you pass
> >> > --no-commit to 'git merge', defaulting to non-blind merge doesn't
> >> > really work anyway.
> >>
> >> Thanks for looking into this. I guess I'll have to rethink how I merge.
> >
> > Not sure it matters in the context of this discussion, but please keep
> > in mind that "git cherry-pick" also merges.
> 
> My intuition is that getting a commit hook failure from the commit
> being cherry picked will be less confusing than getting hook failures
> from one of the  many commits being merged from a branch, so I don't
> think it's worth updating the hook to avoid this case.

I don't think I agree.  In fact, I think many users will not even
realize that cherry-pick merges, and will be surprised.

> 'git cherry-pick' does have the same issue as 'git merge': it won't
> rerun the commit hook, unless '--no-commit' is passed

Too bad.  I wish this could be fixed as well.

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Noam Postavsky]

On Sat, Apr 29, 2017 at 4:04 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>
>> My intuition is that getting a commit hook failure from the commit
>> being cherry picked will be less confusing than getting hook failures
>> from one of the  many commits being merged from a branch, so I don't
>> think it's worth updating the hook to avoid this case.
>
> I don't think I agree.  In fact, I think many users will not even
> realize that cherry-pick merges, and will be surprised.

Hmm, I'm not sure I entirely understand what you're getting at. What
exactly do you mean by "merges" in this context?

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sat, 29 Apr 2017 19:25:17 -0400
> Cc: Paul Eggert <eggert@cs.ucla.edu>, martin rudalics <rudalics@gmx.at>, 
> 	Emacs developers <emacs-devel@gnu.org>, Andreas Schwab <schwab@suse.de>, Yuri Khan <yuri.v.khan@gmail.com>
> 
> On Sat, Apr 29, 2017 at 4:04 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> >>
> >> My intuition is that getting a commit hook failure from the commit
> >> being cherry picked will be less confusing than getting hook failures
> >> from one of the  many commits being merged from a branch, so I don't
> >> think it's worth updating the hook to avoid this case.
> >
> > I don't think I agree.  In fact, I think many users will not even
> > realize that cherry-pick merges, and will be surprised.
> 
> Hmm, I'm not sure I entirely understand what you're getting at. What
> exactly do you mean by "merges" in this context?

I mean that users will be surprised to see the hooks invoked when
cherry-picking.  I know I was surprised to see the git-merge-changelog
utility to be invoked the first time I cherry-picked.

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Noam Postavsky]

On Sat, Apr 29, 2017 at 10:34 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> I mean that users will be surprised to see the hooks invoked when
> cherry-picking.  I know I was surprised to see the git-merge-changelog
> utility to be invoked the first time I cherry-picked.

I don't see where git-merge-changelog is being called from the hooks.

With respect to the pre-commit hook, it seems that we can't play the
same trick as with 'git merge', because CHERRY_PICK_HEAD doesn't exist
when picking only one commit.

Re: git pre-commit hook for merges (WAS: master has switched from Automake to GNU Make)

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sun, 30 Apr 2017 15:35:55 -0400
> Cc: Paul Eggert <eggert@cs.ucla.edu>, martin rudalics <rudalics@gmx.at>, 
> 	Emacs developers <emacs-devel@gnu.org>, Andreas Schwab <schwab@suse.de>, Yuri Khan <yuri.v.khan@gmail.com>
> 
> On Sat, Apr 29, 2017 at 10:34 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > I mean that users will be surprised to see the hooks invoked when
> > cherry-picking.  I know I was surprised to see the git-merge-changelog
> > utility to be invoked the first time I cherry-picked.
> 
> I don't see where git-merge-changelog is being called from the hooks.

It isn't.  The fact that it's invoked during cherry-picking is an
indication that cherry-picking involves a merge.  I'm saying that the
first time I realized that, it was a surprise.  Which means others
might be surprised to see the hooks invoked when they cherry-pick.

> With respect to the pre-commit hook, it seems that we can't play the
> same trick as with 'git merge', because CHERRY_PICK_HEAD doesn't exist
> when picking only one commit.

Yeah, figured that much.  Too bad.