{"thread":{"id":"5971","subject":"[PATCH] cogito: Honor either post-commit script name; fail if both are executable","startedAt":"2006-10-20T16:15:51Z","lastAt":"2006-10-27T07:41:49Z","messageCount":3,"participants":["Jim Meyering","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"29383","messageId":"874ptzhsjs.fsf@rho.meyering.net","threadId":"5971","inReplyTo":null,"subject":"[PATCH] cogito: Honor either post-commit script name; fail if both are executable","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2006-10-20T16:15:51Z","receivedAt":"2006-10-20T16:15:51Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Hi,\n\nI promised this patch some time ago, made the changes,\nand then never sent them. This is slightly different\nfrom the current implementation in that it fails when both\nscripts are executable. Also, it factors out the script names and\nadds tests.\n\nSigned-off-by: Jim Meyering \n---\n cg-commit | 43 ++++++++++++++++++++++++++++++-------------\n t/t9800-hooks.sh | 37 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 67 insertions(+), 13 deletions(-)\n\ndiff --git a/cg-commit b/cg-commit\nindex 01a4eb7..ee4769e 100755\n--- a/cg-commit\n+++ b/cg-commit\n@@ -148,11 +148,11 @@ #\tIf the file exists it will be used as\n #\tthe commit message. The template file makes it possible to\n #\tautomatically add `Signed-off-by` line to the log message.\n #\n-# $GIT_DIR/hooks/commit-post::\n+# $GIT_DIR/hooks/post-commit:: (legacy: commit-post)\n #\tIf the file exists and is executable it will be executed upon\n #\tcompletion of the commit. The script is passed two arguments.\n #\tThe first argument is the commit ID and the second is the\n-#\tbranchname. A sample `commit-post` script might look like:\n+#\tbranchname. A sample `post-commit` script might look like:\n #\n #\t#!/bin/sh\n #\tid=$1\n@@ -680,18 +680,35 @@ elif [ \"$newhead\" ]; then\n \t\tbranchname=\"$(cat \"$_git/branch-name\")\"\n \tfi\n \t[ -z \"$branchname\" ] && [ \"$_git_head\" != \"master\" ] && branchname=\"$_git_head\"\n-\tif [ -x \"$_git/hooks/commit-post\" -o -x \"$_git/hooks/post-commit\" ] && [ ! \"$no_hooks\" ]; then\n-\t\tif [ \"$(git-repo-config --bool cogito.hooks.commit.post.allmerged)\" = \"true\" ]; then\n-\t\t\t# We just hope that for the initial commit, the user didn't\n-\t\t\t# manage to install the hook yet.\n-\t\t\tfor merged in $(git-rev-list $newhead ^$oldhead | tac); do\n-\t\t\t\t[ -x \"$_git/hooks/commit-post\" ] && \"$_git/hooks/commit-post\" \"$merged\" \"$branchname\"\n-\t\t\t\t[ -x \"$_git/hooks/post-commit\" ] && \"$_git/hooks/post-commit\" \"$merged\" \"$branchname\"\n-\t\t\tdone\n-\t\telse\n-\t\t\t[ -x \"$_git/hooks/commit-post\" ] && \"$_git/hooks/commit-post\" \"$newhead\" \"$branchname\"\n-\t\t\t[ -x \"$_git/hooks/post-commit\" ] && \"$_git/hooks/post-commit\" \"$newhead\" \"$branchname\"\n+\n+\tif [ \"$no_hooks\" ]; then\n+\t\texit 0\n+\tfi\n+\n+\t# Decide which spelling to use for the post-commit hook script name.\n+\tpost_commit=\"$_git/hooks/post-commit\"\n+\told_name=\"$_git/hooks/commit-post\"\n+\tif [ -x \"$post_commit\" ]; then\n+\t\tif [ -x \"$old_name\" ]; then\n+\t\t\tdie \"both $post_commit and $old_name are executable.\"\n \t\tfi\n+\t\t# This is the expected case: use $post_commit\n+\telse\n+\t\tif [ ! -x \"$old_name\" ]; then\n+\t\t\t# neither is executable: do nothing\n+\t\t\texit 0\n+\t\tfi\n+\t\tpost_commit=$old_name\n+\tfi\n+\n+\tif [ \"$(git-repo-config --bool cogito.hooks.commit.post.allmerged)\" = \"true\" ]; then\n+\t\t# We just hope that for the initial commit, the user didn't\n+\t\t# manage to install the hook yet.\n+\t\tfor merged in $(git-rev-list $newhead ^$oldhead | tac); do\n+\t\t\t\"$post_commit\" \"$merged\" \"$branchname\"\n+\t\tdone\n+\telse\n+\t\t\"$post_commit\" \"$newhead\" \"$branchname\"\n \tfi\n \n \texit 0\ndiff --git a/t/t9800-hooks.sh b/t/t9800-hooks.sh\nnew file mode 100755\nindex 0000000..a54e35f\n--- /dev/null\n+++ b/t/t9800-hooks.sh\n@@ -0,0 +1,37 @@\n+#!/usr/bin/env bash\n+#\n+# Copyright (c) 2006 Jim Meyering\n+#\n+test_description=\"Test the commit hooks.\"\n+\n+. ./test-lib.sh\n+rm -rf .git\n+\n+echo x > f\n+test_expect_success 'initialize repo' \\\n+\t'(cg-init -m\"Initial commit\")'\n+\n+commit_post=.git/hooks/commit-post\n+post_commit=.git/hooks/post-commit\n+cat > $post_commit <<\\EOF\n+#!/bin/sh\n+echo $0\n+exit 0\n+EOF\n+cp $post_commit $commit_post\n+\n+chmod a+x $post_commit\n+test_expect_success 'test the post-commit name' \\\n+\t'(echo 1 >> f; test \"`cg-commit -m x|grep git/hooks`\" = $post_commit)'\n+\n+chmod a-x $post_commit\n+chmod a+x $commit_post\n+\n+test_expect_success 'test the legacy commit-post name' \\\n+\t'(echo 2 >> f; test \"`cg-commit -m x|grep git/hooks`\" = $commit_post)'\n+\n+chmod a+x $post_commit\n+test_expect_failure 'fail when both are executable' \\\n+\t'(echo 3 >> f; cg-commit -m x)'\n+\n+test_done\n-- \n1.4.3.ge193\n"},{"id":"297495","messageId":"20061027020549.GY20017@pasky.or.cz","threadId":"5971","inReplyTo":"874ptzhsjs.fsf@rho.meyering.net","subject":"Re: [PATCH] cogito: Honor either post-commit script name; fail if both are executable","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-10-27T02:05:49Z","receivedAt":"2006-10-27T02:05:49Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":" Hi!\n\nDear diary, on Fri, Oct 20, 2006 at 06:15:51PM CEST, I got a letter\nwhere Jim Meyering said that...\n> I promised this patch some time ago, made the changes,\n> and then never sent them. This is slightly different\n> from the current implementation in that it fails when both\n> scripts are executable. Also, it factors out the script names and\n> adds tests.\n> \n> Signed-off-by: Jim Meyering \n\n I'm not sure I like this (of course, I always like additional tests,\nthough). The problem is that this loses a smooth upgrade path, things\nsuddenly break and you can't commit without having to think about fixing\nyour environment. We should always give users enough time for that with\ndeprecation warnings. So if we want to get rid of commit-post, we should\nrather start printing deprecation warnings if commit-post exists, and in\nfew months cut commit-post off.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\n#!/bin/perl -sp0777i wrote:\n> Dear diary, on Fri, Oct 20, 2006 at 06:15:51PM CEST, I got a letter\n> where Jim Meyering said that...\n>> I promised this patch some time ago, made the changes,\n>> and then never sent them. This is slightly different\n>> from the current implementation in that it fails when both\n>> scripts are executable. Also, it factors out the script names and\n>> adds tests.\n>>\n>> Signed-off-by: Jim Meyering \n>\n> I'm not sure I like this (of course, I always like additional tests,\n> though). The problem is that this loses a smooth upgrade path, things\n> suddenly break and you can't commit without having to think about fixing\n> your environment.\n\nHow would such a tiny change affect the upgrade path?\nFailing when both scripts are executable feels more like doing the\n(confused) user a favor than complicating their upgrade experience :-)\n\nIn case it wasn't clear, the only change induced by that patch is to\nmake cg-commit fail if *both* commit-post and post-commit scripts\nexist and are executable. I'll bet that is unusual enough not to matter.\n\n> deprecation warnings. So if we want to get rid of commit-post, we should\n> rather start printing deprecation warnings if commit-post exists, and in\n> few months cut commit-post off.\n\nIf you want to retire \"commit-post\", that's mostly independent.\nIMHO, the things to do:\n -- announce and document the name change (leaving mention of the old\n name at least temporarily in the documentation)\n -- later, start warning if the old name is used\n"}]}