patchhooks: add signature to the top of the commit message

The sample hook to prepare the commit message before a commit allows users to opt-in to add the signature to the commit message. The signature is added at a place that isn't consistent with the "-s" option of "git commit". Further, it could go out of view in certain cases.

Add the signature to the top of the commit message as it's more appropriate and consistent with the "-s" option of git commit.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 The change might seem to be bit of an hack, but it seems
 worth it (at least to me). I hope I haven't used any
 commands that aren't portable.

 templates/hooks--prepare-commit-msg.sample | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

It does look like a hack. I was wondering if "interpret-trailers" is mature enough and can be used for this by now. Also the big comment before these examples say that this one you are updating is "rarely a good idea", though.

By the way, the one that is still actually enabled is no longer needed. The commit template generated internally was corrected some time ago not to add the "Conflicts:" section without commenting it out.

Have you tried "merge", "cherry-pick" and "commit --amend" with this patch on (meaning, with the "add sob at the top" logic in your actual hook that is enabled in your repository)?

On Fri, 2017-06-30 at 09:44 -0700, Junio C Hamano wrote:

It does look promising except for a few differences from the hook which I'll explain in the following mail.

I think the comment specifies the "editability" of the sign-off drawback but AFAIK there's no way as of now to add such trailers to commit messages. Until there's a solution for adding trailers that aren't editable manually, I think it's not a bad idea to use a hook for it and rely on the user to be true to their conscience. Moreover the script is commented out by default anyway.

The portion of the hook for adding sign-off could be removed in case any configuration to enable "-s" option for "git commit" by default existed. I'm not aware of any.

I'll send in another patch that removes it but it seems removing it would leave sample hook without anything turned on by default. That doesn't sound fine, does it? 

Any other possible tweaks that could be done to the commit message using the "prepare-commit-msg" hook, that's not done by git, and could be left uncommented by default?

How about a script that removes the "Please enter your.." message from the comments if it exists?

Yes, they don't work with this patch. I have dropped it. More on it in the following mail.

On Sat, 2017-07-01 at 19:45 +0530, Kaartic Sivaraam wrote:

interpet-trailers =================

After enabling the script I tried the following (shown here as a diff) to add the signature with "interpret-trailers",

    diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
    index 6473bcacd..9f8cbe7fd 100755
    --- a/templates/hooks--prepare-commit-msg.sample
    +++ b/templates/hooks--prepare-commit-msg.sample
    @@ -33,4 +33,4 @@ case "$2,$3" in
     esac
     
     SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
    -grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
    +git interpret-trailers --in-place --trailer "$SOB" "$1"

It adds the signature if it's not present in the following cases,

* commit
* merge
* commit --amend
* commit -F
* cherry-pick

It's pretty good in adding the signature except that it's not in line with "git commit -s" whose resulting "spacing" (new lines before and after) as shown in the editor is given below,

The spacing of "git interpret-trailers" in the editor for the relevant cases are,

commit ------

commit --amend --------------

merge -----

So, it seems that excepting for 'commit' it has quite a nice spacing. I guess we could add something like the following to fix that,

    # Add new line after SOB in case of "git commit"
    NEW_LINE='\
    '
    if [ -z "$2" ]
    then
      sed -i "1i$NEW_LINE" "$1"
    fi

sed-script ========== I also tried to add the signature that immitates the "-s" option of "git commit" using "sed" but it works only in following cases,

* commit
* commit --amend
* merge

It doesn't seem to work in cases where user doesn't edit the message using the editor. I'm not sure why.

I'm not including a patch of my manual way here as "git interpret- trailers" (with the fix added) seems quite promising (at least to me).

I'll send a typical patch that uses "git interpret-headers" as a follow-up.

The sample hook to prepare the commit message before a commit allows users to opt-in to add the signature to the commit message. The signature is added at a place that isn't consistent with the "-s" option of "git commit". Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of "git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes "git commit" without any arguments. In that case manually add a new line after the first line to ensure it's consistent with the output of "-s" option.

While at it, name the input parameters to improve readability of script.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 I've tried the various commands that I could think of and it 
 seems to work well. I guess it's safe to use.

 templates/hooks--prepare-commit-msg.sample | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Isn't "sed -i" GNUism that is not portable?

When you say a "typical" patch, what do you exactly mean? Does anybody else send typical patches (or atypical ones for that matter) to the list?

On Mon, 2017-07-03 at 09:58 -0700, Junio C Hamano wrote:

It does seem to be the case. Then the alternative would be the following,

    if [ -z "$2" ]
    then
      sed -e "1i$NEW_LINE" "$1" >"sed-output.temp"
      mv "sed-output.temp" "$1"
    fi

Actually the GNU's sed documentation tricked me into believing '-i' wasn't a GNU extension. The '-i' option works with the '--posix' option of GNU sed which made me believe it isn't an extension.

I apologise for the inconsistent wordings. I try to mean a patch which I'm not sure is acceptable (or) not. I guess that's [PATCH/RFC] in the language of this list. I'm not acquainted to the wordings as I'm an "off-list" person trying to help and get help! I'll try to use consistent wordings as far as I could. Once again, please excuse my ignorance.

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Oh no need to apologise for "ignorance"; that goes both ways. I wasn't familiar with a phrase you used "a typical patch" (which I suspected that is something you may be used to from your involvement in other development community) and showing _my_ ignorance and asking for help to clarify, so that both of us can understand each other better.

My reading of your response is that it is a normal patch that proposes a change, as opposed to the final version of a patch meant for inclusion, after it has been discussed here and everybody supports---let me know if I am still not reading you correctly.

Thanks.

The sample hook to prepare the commit message before a commit allows users to opt-in to add the signature to the commit message. The signature is added at a place that isn't consistent with the "-s" option of "git commit". Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of "git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes "git commit" without any arguments. In that case manually add a new line after the first line to ensure it's consistent with the output of "-s" option.

While at it, name the input parameters to improve readability of script.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
  Removed the GNUism in sed command. In case no other changes are
  required this one's the final patch.

 templates/hooks--prepare-commit-msg.sample | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

The sample hook to prepare the commit message before a commit allows users to opt-in to add the signature to the commit message. The signature is added at a place that isn't consistent with the "-s" option of "git commit". Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of "git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes "git commit" without any arguments. In that case manually add a new line after the first line to ensure it's consistent with the output of "-s" option.

While at it, name the input parameters to improve readability of script.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Added a close quote that got missed in the previous patch.

 templates/hooks--prepare-commit-msg.sample | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

OK.

OK (but its execution can be simplified).

Good.

Move the latter two variable definitions to where they matter, i.e. the commented-out part that computes and adds $SOB.

In our codebase (see Documentation/CodingGuidelines) we tend to prefer "test" over "[". I.e.

	if test -z "$COMMIT_SOURCE"
	then

This looks like a more complex way to say

	{
		echo 
		cat "$COMMIT_MSG_FILE"
	} >"$SED_TEMP_FILE" &&
	mv "$SED_TEMP_FILE" "$COMMIT_MSG_FILE"

I wondered if it is safe to use a single hardcoded "$SED_TEMP_FILE", but I think it is OK.

Thanks.

On 05/07/17 18:35, Kaartic Sivaraam wrote:

I assume each occurrence of 'signature' in the commit message, including the subject, should be 'sign-off' (or Signed-off-by) instead. Yes?

(when I hear 'signature', I think GPG signature).

ATB, Ramsay Jones

On Wed, 2017-07-05 at 21:14 +0100, Ramsay Jones wrote:

Yes. Thanks for pointing out a possible way in which the message could be misinterpreted.

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Actually I was wondering if it is a good idea to remove it, as it seems to have outlived its usefulness.

From: "Junio C Hamano" <gitster@pobox.com>

Personally, I like the comfort of seeing the Conflicts: list, but if others have indicated otherwise... -- Philip

"Philip Oakley" <philipoakley@iee.org> writes:

Oh, I think you misread the discussion while arriving from the sideways. My "it" in the "remove it" refers to the sample prepare-commit-msg hook; among the three examples in that hook, only one of them is enabled but that one was to comment out the "Conflicts" section in the log message editor. These days, that section already appears in a commented-out form without the help of that hook, so there is nothing useful in there---hence a suggestion for removal of the sample.

From: "Junio C Hamano" <gitster@pobox.com>

Thanks, yes I had misread it. I hadn't managed the time to follow the details. Problem solved.

Philip 

On Sat, 2017-07-01 at 10:36 -0700, Junio C Hamano wrote:

It does seem to be a good idea but it would leave the 'prepare-commit- msg' hook with no scripts that could be used by just activating it. That's why I thought of adding a script that removes the "Please enter your.." message from the comments if it exists. A typical patch will follow.

Enabling the "prepare-commit-msg" hook with the patch that follows would do have following result,

Before,

    # Please enter the commit message for your changes. Lines starting
    # with '#' will be ignored, and an empty message aborts the commit.
    # On branch hook-test-merge
    # Changes to be committed:
    #    	    new file:   commit-msg
    #

After,

    # On branch hook-test-merge
    # Changes to be committed:
    #    	    new file:   commit-msg
    #

A typical consequence for "git commit --amend" would be,

Before,

    Test commit

    # Please enter the commit message for your changes. Lines starting
    # with '#' will be ignored, and an empty message aborts the commit.
    #
    # Date:      Sun Jul 2 00:11:28 2017 +0530

After,

    Test commit

    #
    # Date:      Sun Jul 2 00:11:28 2017 +0530
    #

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

That sounds like a sample that is there not because it would be useful, but because we couldn't think of any useful example.

IOW, I view it just as useful as a sample that does

	#!/bin/sh
	echo "# useless cruft" >>"$1"

whose sole value is to demonstrate that you could affect what you see in the editor by modifying "$1".

On Sat, 2017-07-01 at 13:31 -0700, Junio C Hamano wrote:

I thought it would be useful as it could serve as a simple example about what could be done using hooks and further would help save some vertical spacing for users who are acquainted with that message and wouldn't want to see it anymore.

Sending a typical patch as a follow-up.

Apart from that I would like to share a patch of my attempt to give "notes" about a commit while writing the commit message itself. It was an attempt to combine three hooks at once to achieve the outcome. I just wanted to share it so that it might be of use to someone.

The pre-commit-msg hook sample has an example that comments the "Conflicts:" part of the merge commmit. It isn't relevant anymore as it's done by default since 261f315b ("merge & sequencer: turn "Conflicts:" hint into a comment", 2014-08-28).

Add an alternative example that replaces it to ensure there's atleast one example that could be used just by enabling the hook.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 This patch assumes that a previous patch that touched the commit
 template [1] has been appled. Else it could remove the information
 about the branch from the comments.

 [1]:http://public-inbox.org/git/%3C20170630121221.3327-2-kaarticsivaraam91196@gmail.com%3E/raw
 
 templates/hooks--prepare-commit-msg.sample | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

The pre-commit-msg hook sample has an example that comments the "Conflicts:" part of the merge commmit. It isn't relevant anymore as it's done by default since 261f315b ("merge & sequencer: turn "Conflicts:" hint into a comment", 2014-08-28).

Add an alternative example that replaces it. This ensures there's at the least a simple example that illustrates what could be done using the hook just by enabling it.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 This one's the final proposed patch for this simple hook. I have
 requested for suggestions for alternative scripts in the mailing
 list[1]. In case something interesting turns out, we could use that
 instead of this one.

 [1]: http://public-inbox.org/git/1499273152.16389.2.camel@gmail.com/T/#u

 templates/hooks--prepare-commit-msg.sample | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Three things that caught my eyes:

 - Between "git commit --cleanup=strip" and "git commit --cleanup=verbatim",
   lines that make up this initial instruction section are different.

 - "git grep 'Please enter the '" finds that this string is subject
   to translation, so the pattern may not match (in which case it
   will be a no-op without doing any harm, which is OK).

 - core.commentChar can be set to something other than '#', so the
   pattern may not match (I do not offhand know if that may cause a
   wrong line to match, causing harm, or not).

As merely an example, it probably is OK to say "this won't work if you are not using the C locale, and/or you are using custom core.commentChar". So if we disregard the latter two, I would think

    sed -e '/^# Please enter the commit message /,/^#$/d'

may be simpler to reason about to achieve the same goal.  

Thanks.

On Wed, 2017-07-05 at 12:50 -0700, Junio C Hamano wrote:

Thanks for enlightening me about this. I thought sed was greedy with address spaces the same way it's greedy with regex.

    sed -e '/^# Please enter the commit message /,/^#$/d'

This command does seem to work regardless of the cleanup mode used.

That said, in case my interpretation that "'prepare-commit-msg' hook is not to be shipped due to it's uselessness" is correct, the reply of this mail as a whole seems to contradict it.

Should I work on this patch and another related one (he one that modifies the signature part of the hook) or should I drop it ?

IOW, would this patch likely make the hook useful again?

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

That is because I wear multiple hats, because I try to help in different ways, and because open source is not a battle to see whose idea is more right, but is a cooperative process to find a better solution together.

As a fellow contributor, I do not think that removing the hint that is commented out, which is meant to be helpful to users while in their editor and which will be removed after the editor finishes anyway, is a useful enough example to keep the now otherwise useless sample hook. But as the maintainer, I can see that you are still making sincere efforts to come up with a useful example and improve the end-user experience, and more importantly, I haven't heard from other people what they think---the only thing I have are different opinions from two people. That is why I am not deciding and telling you to go find another area to hack in.

At the same time, I found that your implementation of the idea, i.e. removal of the commented-out hint, can be improved. With an improved implementation of the proposed solution, it may have a better chance to be supported by others on the list, and equally importantly, if it turns out that other people do support what this patch tries to do, i.e. keep the sample hook alive by replacing the now useless examples with this one, we would have a better implementation of it. And that is something I can help with, while I, the maintainer, is waiting.

Oh, by the way, what the maintainer is waiting for is not just "me too"s; this is not exactly a "having more people wins" democracy.

On Fri, 2017-07-07 at 08:05 -0700, Junio C Hamano wrote:

Thanks for helping and being kind!

So, I'll improve it and of course wait for any replies. BTW, thanks for clearing off the little confusion I had. I'm not used to these multiple-hat replies from single person. Thanks for exposing me to it.

Got it.

The pre-commit-msg hook sample has an example that comments the "Conflicts:" part of the merge commmit. It isn't relevant anymore as it's done by default since 261f315b ("merge & sequencer: turn "Conflicts:" hint into a comment", 2014-08-28).

Add an alternative example that replaces it. This ensures there's at the least a simple example that illustrates what could be done using the hook just by enabling it.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 I have made the second patch depend on the first one to avoid
 conflicts that may occur. Further, it was meaningful to join
 the two patches as they would go together (or) not at all.

 templates/hooks--prepare-commit-msg.sample | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

The sample hook to prepare the commit message before a commit allows users to opt-in to add the signature to the commit message. The signature is added at a place that isn't consistent with the "-s" option of "git commit". Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of "git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes "git commit" without any arguments. In that case manually add a new line after the first line to ensure it's consistent with the output of "-s" option.

While at it, name the input parameters to improve readability of script.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 templates/hooks--prepare-commit-msg.sample | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Whether the two samples these patches implement are useful ones or not, the early part of 2/2 that starts using named variables instead of positional ones is an improvement. It makes it easier to see what is going on.

Patch 2/2 as posted forgets to update some references to $1, $2 and $3, both in the actual code and also commented out part, e.g.

    sed -e ... "$COMMIT_MSG_FILE" >"$SED_OUTPUT_TEMP" && mv "$SED_OUTPUT_TEMP" "$1"
    # case "$2,$3" in

In the first one, $COMMIT_MSG_FILE and $1 refer to the same thing; they should be consistent.

If these updates were to be done as multiple patches, the change to use named not positional variables, without changing anything else, should come second, I think, after the one that simply removes the now-useless "Conflicts:" sample from the script, as a preparatory clean-up step. You can build other things like hints-removal and sign-off with interpret-trailers on top of these two steps.

Have you considered using "@PERL_PATH@ -i" instead of "sed" in this step, by the way? That would allow you not to worry about the temporary file left behind (e.g. what happens when somebody else runs this in a shared repository setting, creating and leaving the temporary file that you may not be able to write into later because her umask is tighter, and then you try to make a commit).

Thanks.

Prepare the 'preare-commit-msg' sample script for upcoming changes. Preparation includes removal of an example that has outlived it's purpose. The example is the one that comments the "Conflicts:" part of a merge commit message. It isn't relevant anymore as it's done by default since 261f315b ("merge & sequencer: turn "Conflicts:" hint into a comment", 2014-08-28).

Further remove the relevant comments from the sample script and update the documentation.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Documentation/githooks.txt                 |  3 ---
 templates/hooks--prepare-commit-msg.sample | 20 ++++++++------------
 2 files changed, 8 insertions(+), 15 deletions(-)

It's always nice to have named variables instead of positional variables as they communicate their purpose well.

Appropriately name the positional variables of the hook to make it easier to see what's going on.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 templates/hooks--prepare-commit-msg.sample | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Makes sense.  Thanks.

The sample hook to prepare the commit message before a commit allows users to opt-in to add the signature to the commit message. The signature is added at a place that isn't consistent with the "-s" option of "git commit". Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of "git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes "git commit" without any arguments. In that case manually add a new line after the first line to ensure it's consistent with the output of "-s" option.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 templates/hooks--prepare-commit-msg.sample | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

On 10/07/17 15:17, Kaartic Sivaraam wrote:

Again, s/signature/sign-off/g, or similar (including subject line).

ATB, Ramsay Jones

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

Thanks for being a careful reader.

I think we should do

	print "\n" if !$first_line++

for brevity (and also avoid "if(" that lacks SP) here.

Prepare the 'preare-commit-msg' sample script for upcoming changes. Preparation includes removal of an example that has outlived it's purpose. The example is the one that comments the "Conflicts:" part of a merge commit message. It isn't relevant anymore as it's done by default since 261f315b ("merge & sequencer: turn "Conflicts:" hint into a comment", 2014-08-28).

Further update the relevant comments from the sample script and update the documentation.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Documentation/githooks.txt                 |  3 ---
 templates/hooks--prepare-commit-msg.sample | 24 ++++++++++--------------
 2 files changed, 10 insertions(+), 17 deletions(-)

The sample hook to prepare the commit message before a commit allows users to opt-in to add the sign-off to the commit message. The sign-off is added at a place that isn't consistent with the "-s" option of "git commit". Further, it could go out of view in certain cases.

Add the sign-off in a way similar to "-s" option of "git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes "git commit" without any arguments. In that case manually add a new line after the first line to ensure it's consistent with the output of "-s" option.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 templates/hooks--prepare-commit-msg.sample | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Sign-off added should be that of the "committer" not that of the "commit's author".

Use the correct logical variable that identifies the committer.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 This fixes a small issue when trying to do the following with the script enabled,

    $ git commit --amend -s

 If the commit being amended was signed off by the commit's author then the above command
 would *append* the sign-off of the committer followed by that of the commit's author.
 That' because the script is invoked only after the sign-off is added by the '-s' option AND
 the default of 'trailer.ifexists' for interpret-trailers currently defaults to the 'addIfDifferentNeighbor'
 thus interpret-trailer fails to identify the existing sign-off of the commit's author and adds it.

 Anyways, it doesn't make sense for a script to add the sign-off of the commit's author. So,
 fixing it seemed correct to me.

 templates/hooks--prepare-commit-msg.sample | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Mon, Aug 14, 2017 at 1:46 AM, Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> wrote:

The background knowledge provided up to here seems like a valuable information that we'd want to preserve in the commit history, i.e. make it part of the commit message?

Code looks good.

Thanks, Stefan

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

I tend to agree with "Anyways" above ;-) simply because I found the long paragraph more confusing than enlightening, leaving me wonder "why is the user using different settings for author and committer name" at the end, which _is_ an irrelevant point in the issue being addressed.

The real issue is that this commented-out sample hook uses "author" ident for the sign-off, while all the rest of Git (i.e. all callers of append_signoff() in sequencer.c) use committer ident.

If anything to be changed to this patch, I would say its "should be" in the log message can be clarified with "why", perhaps like:

    Sign-off added should be that of the "committer", not that of
    the "commit's author"; that is how the rest of Git adds sign-off
    using sequencer.c::append_signoff().

Thanks.

On Monday 14 August 2017 11:24 PM, Stefan Beller wrote:

I didn't do that previously expecting a few people would get confused by this (it did turn out to be true). I could have made it more clearer but didn't attempt as I thought it wasn't worth the effort. Yeah, it sometimes takes time to *simplify* things.

I guess Junio's suggestion found below seems concise enough although it doesn't capture the reason I did the change.

         Sign-off added should be that of the "committer", not that of
         the "commit's author"; that is how the rest of Git adds sign-off
         using sequencer.c::append_signoff().

--- Kaartic

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

I did shoot for conciseness, but what is a lot more important is to record what is at the core of the issue. "I found it by doing A" can hint to careful readers why doing A leads to an undesirable behaviour, but when there are other ways to trigger problems that come from the same cause, "I found it by doing A" is less useful unless we also record "Doing A reveals the underlying problem X" that can be shared by other ways B, C, ... to trigger it. The careful readers need to guess what the X is.

And once you identify the underlying problem X and record _that_ in the log message, I and A in "I found it by doing A" becomes much less interesting and the readers do not have to guess.

Your "A" is 'git commit --amend -s' with the disabled part of hook enabled. But I think 'git commit' without "--amend" and "-s" would also show an issue that come from the same root cause. The hook will add SoB that is based on the author, not the committer. That resulting commit would be different from 'git commit -s' without the hook enabled, which would add SoB based on the commiter name (that would be a "B", that causes a related but different problem that comes from the same underlying issue "X" which is "we should consistently use the committer info like other parts of the system").

In any case, thanks for a fix-up. Let's move this forward quickly, as it is an update to a topic that is already in 'master'.

Thanks.

On Tue, 2017-08-15 at 10:28 -0700, Junio C Hamano wrote:

Seeing the reply, I changed my opinion that "it isn't worth the effort to write a better commit message for the change". I've given a try but it got a little off-hand. Let me know if there's ways in which it could be simplified and/or improved.

In general, a 'Sign-off' added should be that of the *committer* and not that of the *commit's author*. As the 3rd part of the 'prepare-commit-msg' hook appended the sign-off of the *commit's author* it worked weirdly in some cases. For example 'git commit --amend -s' when coupled with that part of the script woked weirdly as illustrated by the following scenario in which the last commit's log message has the following trailer,

    ...

    Signed-off-by: Random Developer <developer@example.com>

and the commit's author is "Random Developer <developer@example.com>". Assume that the commit is trying to be amended by another developer who's identity is "Another Developer <another-dev@example.com>". When he tries to do

    $ git commit --amend -s

with the 3rd part of the hook enabled then the trailer he would see in his editor would be,

    ...

    Signed-off-by: Random Developer <developer@example.com>
    Signed-off-by: Another Developer <another-dev@example.com>
    Signed-off-by: Random Developer <developer@example.com>

This is because,

  * the hook is invoked only after the sign-off is appended by the '-s' option

  * the script tries to add the sign-off of the *commit's author* using interpret-trailers
    and 'interpret-trailers' in it's default configuration tries to adds the trailer
    when the *neighbouring* trailer isn't the same as the one trying to be added.

This is just an example and this kind of issue could repeat if similar conditions are satisified for other cases.

Moreover the rest of Git adds the sign-off of the *committer* using sequencer.c::append_signoff(). So, use the correct logical variable that identifies the committer to append the sign-off in the sample hook script.

Bottom line: Being consistent prevents all sorts of weird issues.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Changes in v2:

    - updated the commit message
 
 Suggestions regarding ways to improve the message are most welcome.

 templates/hooks--prepare-commit-msg.sample | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Monday 14 August 2017 11:49 PM, Junio C Hamano wrote:

This is typically the reason I didn't add the big paragraph to the log message.

--- Kaartic

It's always nice to have named variables instead of positional variables as they communicate their purpose well.

Appropriately name the positional variables of the hook to make it easier to see what's going on.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 templates/hooks--prepare-commit-msg.sample | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Add a simple example that replaces an outdated example that was removed. This ensures that there's at the least a simple example that illustrates what could be done using the hook just by enabling it.

Also, update the documentation.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Documentation/githooks.txt                 | 3 +++
 templates/hooks--prepare-commit-msg.sample | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Add a simple example that replaces an outdated example that was removed. This ensures that there's at the least a simple example that illustrates what could be done using the hook just by enabling it.

Also, update the documentation.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Documentation/githooks.txt                 | 3 +++
 templates/hooks--prepare-commit-msg.sample | 9 ++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

On Mon, 2017-07-10 at 16:13 +0100, Ramsay Jones wrote:

Thanks! Forgot that in the course of splitting the patches and modifying them.

Note: Re-attempting to send mail due to rejection
On Mon, 2017-07-10 at 16:13 +0100, Ramsay Jones wrote:

Thanks! Forgot that in the course of splitting the patches and modifying them.

Add a simple example that replaces an outdated example that was removed. This ensures that there's at the least a simple example that illustrates what could be done using the hook just by enabling it.

Also, update the documentation.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 I made an attempt to make the second example work with amending 
 with the aim of making it suitable for usage out of the box. It
 seems that it's not easy to make it work as the status of a file
 cannot be determined correctly when the index while amending
 introduces changes to a file that has a change in the commit being
 amended.

 Is there any way in which the second example could be made to work with
 amending without much effort? I'm asking this assuming something might
 have happened, since the script was added, that could ease the task.

 Documentation/githooks.txt                 | 3 +++
 templates/hooks--prepare-commit-msg.sample | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Sorry, but I do not understand what you are asking here.

Ahh, do you mean if we can avoid doing one half of the 1/4 (i.e. the part that removes the commented out 'diff --name-status') and instead make it a useful example (while still removing the thing that comments out the "conflicts:")?

After going back and checking 1/4, I realize that I misread the patch. you did keep the commented out 'diff --name-status' thing, so it still has three---it just lost one half of the original "first" example. So please disregard my earlier "do we still have three, not two?"

On Mon, 2017-07-10 at 13:02 -0700, Junio C Hamano wrote:

I'm was trying to ask, "Is there any way to change the second example (diff --name-status) to make it work with "commit --amend" so that it could be uncommented by default ?" 

If there was a way then the patch 4/4 could be dropped as the name status example would be enough make the script live (I think).

Actually speaking, I did think of promoting the second to the first to make the sub-patches independent of each other. I held myself as I thought it would be overkill. Anyways, I'll just overkill it!

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

But does it even be useful to enable it? The commented out "git status" output already lists the modified paths, so I do not think anything is gained by adding 'diff --cached --name-status' there.

On Tue, 2017-07-11 at 09:03 -0700, Junio C Hamano wrote:

The script *doesn't* add the output of "diff --cached --name-status" near the status output. It adds it above the first comment and of course in an uncommented form.

So, should we remove it (diff --cached --name-status) altogether?

On Tue, 2017-07-11 at 09:03 -0700, Junio C Hamano wrote:

Just for the note, I thought it was considered useful (at least to someone) due to it's presence in the sample script.

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Makes sense.

Didn't we just remove one, reducing the total number to two? If so, the second and the third below would need to be promoted to the first and the second, I think.

This script is a little different from others in that it uses THREE hooks to acheive it's goal, which is to allow users to add notes for a commit while writing the commit message. It's working isn't guaranteed even if one of the hooks aren't executed.

It currently works in the following scenartios,

* commit
* commit --amend
* commit -t

From what I'm aware of it's not possible to make it work with 'merge' as there isn't any hook that checks the message of a merge commit. The notes are extracted from the commit message using the hook that checks the commit message.

Use cases other than those specified here aren't tested. It might/ might not be possible to use this script in other cases (with/without modifications).

The usage of various hooks are as follows,

prepare-commit-msg: Used to add the template in which the user
                    enters the notes for the commit. Leaving it
                    doesn't add any notes for the commit.

commit-msg: Used to extract the notes from the commit message if
            it exists and save it to a file. It removes the the
            content between the template to avoid it being added
            to the commit message.

post-commit: Used to check if the file with notes for the last commit
             exists and if it does, uses it to add notes for the commit.

             Note: It's often times an file with empty lines when notes
             aren't given for a commit. It would not be added as a note
             for the commit as 'git notes' doesn't accept files with
             empty lines.
---
 templates/hooks--commit-msg.sample         | 28 +++++++++++++++++++
 templates/hooks--post-commit.sample        | 18 +++++++++++++
 templates/hooks--prepare-commit-msg.sample | 43 +++++++++++++++++++++++-------
 3 files changed, 80 insertions(+), 9 deletions(-)
 create mode 100644 templates/hooks--post-commit.sample