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

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

[Øystein Walle]

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(-)

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

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

[Eric Sunshine]

On Mon, Nov 24, 2014 at 6:21 AM, Øystein Walle <oystwa@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 <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.

> +
> +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

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

[Jeff King]

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

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

[Junio C Hamano]

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?

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

[Jeff King]

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

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

[Mark Levedahl]

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 <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

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

[Jeff King]

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

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

[Mark Levedahl]

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

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

[Junio C Hamano]

Mark Levedahl <mlevedahl@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.