threads / patch / 38035

patchcommit: inform pre-commit if --amend is used

Subject: [PATCH] commit: inform pre-commit if --amend is used

## tl;dr

9 messages between Nov 24, 2014 and Dec 1, 2014. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Øystein Walle· Nov 24, 2014, 11:21 UTC · lore

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 <oystwa@gmail.com>
---
 Documentation/githooks.txt |  3 ++-
 builtin/commit.c           |  3 ++-
 t/t7503-pre-commit-hook.sh | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)
Show changes to 3 files +18 −2

Documentation/githooks.txt, builtin/commit.c, t/t7503-pre-commit-hook.sh

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
Eric Sunshine· Nov 24, 2014, 23:14 UTC · re: Øystein Walle · lore

Re: [PATCH] commit: inform pre-commit if --amend is used

On Mon, Nov 24, 2014 at 6:21 AM, Øystein Walle <oystwa@gmail.com> wrote:
Show 30 quoted lines
> 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 <oystwa@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.
Show 7 quoted lines
> +
> +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
Jeff King· Nov 25, 2014, 03:44 UTC · re: Øystein Walle · lore

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.
Show 5 quoted lines
> +# 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
Junio C Hamano· Nov 25, 2014, 04:58 UTC · re: Jeff King · lore

Re: [PATCH] commit: inform pre-commit if --amend is used

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
>   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?

Jeff King· Nov 25, 2014, 05:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] commit: inform pre-commit if --amend is used

On Mon, Nov 24, 2014 at 08:58:37PM -0800, Junio C Hamano wrote:
Show 19 quoted lines
> Jeff King <peff@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
Mark Levedahl· Nov 27, 2014, 14:40 UTC · re: Jeff King · lore

Re: [PATCH] commit: inform pre-commit if --amend is used

On 11/25/2014 12:03 AM, Jeff King wrote:
Show 32 quoted lines
> On Mon, Nov 24, 2014 at 08:58:37PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@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
Jeff King· Nov 28, 2014, 05:18 UTC · re: Mark Levedahl · lore

Re: [PATCH] commit: inform pre-commit if --amend is used

On Thu, Nov 27, 2014 at 09:40:08AM -0500, Mark Levedahl wrote:
Show 13 quoted lines
> >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
Mark Levedahl· Nov 28, 2014, 15:49 UTC · re: Jeff King · lore

Re: [PATCH] commit: inform pre-commit if --amend is used

On 11/28/2014 12:18 AM, Jeff King wrote:
Show 23 quoted lines
> 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
Junio C Hamano· Dec 1, 2014, 00:56 UTC · re: Mark Levedahl · lore

Re: [PATCH] commit: inform pre-commit if --amend is used

Mark Levedahl <mlevedahl@gmail.com> writes:
Show 20 quoted lines
> 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.

← back to recent threads