Re: [PATCH] commit: inform pre-commit if --amend is used
Mark Levedahl mleved...@gmail.com writes: On 11/28/2014 12:18 AM, Jeff King wrote: Thanks for the links; I had no recollection of that thread. Unsurprisingly, I like the HEAD/HEAD~1 suggestion. That peff guy seems really clever (and handsome, too, I'll bet). I'd still be OK with any of the suggestions given in this thread, though. -Peff ars Apparently our combined handsome-foo was insufficient to get this accepted way back when, hopefully the current submitter has more :^) In any event, I've carried the patches using HEAD/HEAD~1 in my tree for the last 4+ years, have a widely used pre-commit script that depends upon those. So, I personally would be very happy to see this finally show up in Junio's tree, would prefer HEAD/HEAD~1 but can adapt to whatever. One thing to be careful about is that the approach HEAD, HEAD~, etc. does allow this to be extended to cover merge cases as the old thread speculated, it will make it impossible to pass any kind of information, other than here are the parents of the results, to the hook. Of course, there are ways to make sure that we won't paint us into an unescapable corner, e.g. an obvious example (not necessarily the best) being to pass HEAD, HEAD~, b76b088 b260d26 etc., in other words, passing these parents still as a single argument, multiple parents concatenated with some delimiters, so that $1 will always be who are the parent(s) even when we needed to later pass other sorts of information. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: inform pre-commit if --amend is used
On 11/28/2014 12:18 AM, Jeff King wrote: On Thu, Nov 27, 2014 at 09:40:08AM -0500, Mark Levedahl wrote: Then when you add new arguments, the hook has to search through the parameters looking for one that matches, rather than just checking $1 for amend (and $2 for the new option, and so on). As long as the set of options remains relatively small, I think that is preferable. We could also just pass them through the environment, which gives nice named parameters. See http://comments.gmane.org/gmane.comp.version-control.git/148479 for an earlier conversation on this exact topic. Also, see http://permalink.gmane.org/gmane.comp.version-control.git/148480 for a similar change in git-gui. Thanks for the links; I had no recollection of that thread. Unsurprisingly, I like the HEAD/HEAD~1 suggestion. That peff guy seems really clever (and handsome, too, I'll bet). I'd still be OK with any of the suggestions given in this thread, though. -Peff ars Apparently our combined handsome-foo was insufficient to get this accepted way back when, hopefully the current submitter has more :^) In any event, I've carried the patches using HEAD/HEAD~1 in my tree for the last 4+ years, have a widely used pre-commit script that depends upon those. So, I personally would be very happy to see this finally show up in Junio's tree, would prefer HEAD/HEAD~1 but can adapt to whatever. Mark -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: inform pre-commit if --amend is used
On 11/25/2014 12:03 AM, Jeff King wrote: On Mon, Nov 24, 2014 at 08:58:37PM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: 1. It is a bit more obvious when debugging or dumping arguments (e.g., via GIT_TRACE), especially if new options are added after the first. 2. It makes it easier for a script to work on old and new versions of git. It sees either amend or noamend for the two obvious cases, and if it sees no argument, then it knows that it does not know either way (it is running on an old version of git). Technically one can tell the difference in shell between an empty string and a missing argument, but it is sufficiently subtle that I think noamend is a better route. If we ever add more info, would we want to keep piling on new arguments, though? Wouldn't it a viable option to use amend vs not giving anything (not even an empty string), so that normal case there won't be no parameter? Then when you add new arguments, the hook has to search through the parameters looking for one that matches, rather than just checking $1 for amend (and $2 for the new option, and so on). As long as the set of options remains relatively small, I think that is preferable. We could also just pass them through the environment, which gives nice named parameters. -Peff See http://comments.gmane.org/gmane.comp.version-control.git/148479 for an earlier conversation on this exact topic. Also, see http://permalink.gmane.org/gmane.comp.version-control.git/148480 for a similar change in git-gui. -Mark -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: inform pre-commit if --amend is used
On Thu, Nov 27, 2014 at 09:40:08AM -0500, Mark Levedahl wrote: Then when you add new arguments, the hook has to search through the parameters looking for one that matches, rather than just checking $1 for amend (and $2 for the new option, and so on). As long as the set of options remains relatively small, I think that is preferable. We could also just pass them through the environment, which gives nice named parameters. See http://comments.gmane.org/gmane.comp.version-control.git/148479 for an earlier conversation on this exact topic. Also, see http://permalink.gmane.org/gmane.comp.version-control.git/148480 for a similar change in git-gui. Thanks for the links; I had no recollection of that thread. Unsurprisingly, I like the HEAD/HEAD~1 suggestion. That peff guy seems really clever (and handsome, too, I'll bet). I'd still be OK with any of the suggestions given in this thread, though. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] commit: inform pre-commit if --amend is used
When a commit is amended a pre-commit hook that verifies the commit's
contents might not find what it's looking for if for example it looks at
the differences against HEAD when HEAD~1 might be more appropriate.
Inform the commit hook that --amend is being used so that hook authors
can do e.g.
if test $1 = amend
then
...
else
...
fi
to handle these situations.
Signed-off-by: Øystein Walle oys...@gmail.com
---
Documentation/githooks.txt | 3 ++-
builtin/commit.c | 3 ++-
t/t7503-pre-commit-hook.sh | 14 ++
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 9ef2469..e113027 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -73,7 +73,8 @@ pre-commit
~~
This hook is invoked by 'git commit', and can be bypassed
-with `--no-verify` option. It takes no parameter, and is
+with `--no-verify` option. It takes one parameter which is amend if
+`--amend` was used when committing and empty otherwise. It is
invoked before obtaining the proposed commit log message and
making a commit. Exiting with non-zero status from this script
causes the 'git commit' to abort.
diff --git a/builtin/commit.c b/builtin/commit.c
index e108c53..e38dd4a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -694,7 +694,8 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
- if (!no_verify run_commit_hook(use_editor, index_file, pre-commit,
NULL))
+ if (!no_verify run_commit_hook(use_editor, index_file,
+ pre-commit, amend ? amend : ,
NULL))
return 0;
if (squash_message) {
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b..be97676 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -136,4 +136,18 @@ test_expect_success 'check the author in hook' '
git show -s
'
+# a hook that checks if amend is passed as an argument
+cat $HOOK EOF
+#!/bin/sh
+test \$1 = amend
+EOF
+
+test_expect_success 'check that amend argument is given' '
+ git commit --amend --allow-empty
+'
+
+test_expect_success 'check that amend argument is NOT given' '
+ ! git commit --allow-empty
+'
+
test_done
--
2.0.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: inform pre-commit if --amend is used
On Mon, Nov 24, 2014 at 6:21 AM, Øystein Walle oys...@gmail.com wrote: When a commit is amended a pre-commit hook that verifies the commit's contents might not find what it's looking for if for example it looks at the differences against HEAD when HEAD~1 might be more appropriate. Inform the commit hook that --amend is being used so that hook authors can do e.g. if test $1 = amend then ... else ... fi to handle these situations. Signed-off-by: Øystein Walle oys...@gmail.com --- diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh index 984889b..be97676 100755 --- a/t/t7503-pre-commit-hook.sh +++ b/t/t7503-pre-commit-hook.sh @@ -136,4 +136,18 @@ test_expect_success 'check the author in hook' ' git show -s ' +# a hook that checks if amend is passed as an argument +cat $HOOK EOF +#!/bin/sh +test \$1 = amend +EOF write_script would be the modern way to create this hook. + +test_expect_success 'check that amend argument is given' ' + git commit --amend --allow-empty +' + +test_expect_success 'check that amend argument is NOT given' ' + ! git commit --allow-empty You probably want test_must_fail rather than '!' [1]. +' + test_done -- [1]: http://thread.gmane.org/gmane.comp.version-control.git/259871/focus=260092 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: inform pre-commit if --amend is used
On Mon, Nov 24, 2014 at 12:21:51PM +0100, Øystein Walle wrote: This hook is invoked by 'git commit', and can be bypassed -with `--no-verify` option. It takes no parameter, and is +with `--no-verify` option. It takes one parameter which is amend if +`--amend` was used when committing and empty otherwise. It is This interface change is OK for backwards compatibility, since existing scripts would not look at the arguments (which are always empty currently). And I think it is OK for adding new options in the future, too, because the option is always present (just sometimes as an empty string). Can we make the non-amend option something more verbose than the empty string (like noamend)? I have two reasons: 1. It is a bit more obvious when debugging or dumping arguments (e.g., via GIT_TRACE), especially if new options are added after the first. 2. It makes it easier for a script to work on old and new versions of git. It sees either amend or noamend for the two obvious cases, and if it sees no argument, then it knows that it does not know either way (it is running on an old version of git). Technically one can tell the difference in shell between an empty string and a missing argument, but it is sufficiently subtle that I think noamend is a better route. +# a hook that checks if amend is passed as an argument +cat $HOOK EOF +#!/bin/sh +test \$1 = amend +EOF Eric mentioned write_script already (and it would be nice to convert the existing uses in t7503 to use it). You may also want to use \EOF to inhibit interpolation in the here-document, which means you do not have to quote variables inside the script. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: inform pre-commit if --amend is used
Jeff King p...@peff.net writes: 1. It is a bit more obvious when debugging or dumping arguments (e.g., via GIT_TRACE), especially if new options are added after the first. 2. It makes it easier for a script to work on old and new versions of git. It sees either amend or noamend for the two obvious cases, and if it sees no argument, then it knows that it does not know either way (it is running on an old version of git). Technically one can tell the difference in shell between an empty string and a missing argument, but it is sufficiently subtle that I think noamend is a better route. If we ever add more info, would we want to keep piling on new arguments, though? Wouldn't it a viable option to use amend vs not giving anything (not even an empty string), so that normal case there won't be no parameter? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: inform pre-commit if --amend is used
On Mon, Nov 24, 2014 at 08:58:37PM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: 1. It is a bit more obvious when debugging or dumping arguments (e.g., via GIT_TRACE), especially if new options are added after the first. 2. It makes it easier for a script to work on old and new versions of git. It sees either amend or noamend for the two obvious cases, and if it sees no argument, then it knows that it does not know either way (it is running on an old version of git). Technically one can tell the difference in shell between an empty string and a missing argument, but it is sufficiently subtle that I think noamend is a better route. If we ever add more info, would we want to keep piling on new arguments, though? Wouldn't it a viable option to use amend vs not giving anything (not even an empty string), so that normal case there won't be no parameter? Then when you add new arguments, the hook has to search through the parameters looking for one that matches, rather than just checking $1 for amend (and $2 for the new option, and so on). As long as the set of options remains relatively small, I think that is preferable. We could also just pass them through the environment, which gives nice named parameters. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
