{"thread":{"id":"42928","subject":"[PATCH] merge: Run commit-msg hook","startedAt":"2016-07-26T07:48:55Z","lastAt":"2016-07-27T12:35:29Z","messageCount":7,"participants":["Orgad Shaneh","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"292183","messageId":"1469519323-11420-1-git-send-email-orgad.shaneh@audiocodes.com","threadId":"42928","inReplyTo":null,"subject":"[PATCH] merge: Run commit-msg hook","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2016-07-26T07:48:43Z","receivedAt":"2016-07-26T07:48:55Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\ncommit-msg is needed to either validate the commit message or edit it.\nGerrit for instance uses this hook to append its Change-Id footer.\n\nThis is relevant to merge commit just like any other commit.\n\nSigned-off-by: Orgad Shaneh \n---\n Documentation/git-merge.txt | 6 +++++-\n builtin/merge.c | 7 ++++++-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex b758d55..59508aa 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n \t[-s ] [-X ] [-S[]]\n-\t[--[no-]allow-unrelated-histories]\n+\t[--[no-]allow-unrelated-histories] [--no-verify]\n \t[--[no-]rerere-autoupdate] [-m ] [...]\n 'git merge' HEAD ...\n 'git merge' --abort\n@@ -87,6 +87,10 @@ invocations. The automated message can include the branch description.\n \tAllow the rerere mechanism to update the index with the\n \tresult of auto-conflict resolution if possible.\n \n+--no-verify::\n+\tThis option bypasses the commit-msg hook.\n+\tSee also linkgit:githooks[5].\n+\n --abort::\n \tAbort the current conflict resolution process, and\n \ttry to reconstruct the pre-merge state.\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b555a1b..30c03c8 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -51,7 +51,7 @@ static const char * const builtin_merge_usage[] = {\n static int show_diffstat = 1, shortlog_len = -1, squash;\n static int option_commit = 1;\n static int option_edit = -1;\n-static int allow_trivial = 1, have_message, verify_signatures;\n+static int allow_trivial = 1, have_message, verify_signatures, no_verify;\n static int overwrite_ignore = 1;\n static struct strbuf merge_msg = STRBUF_INIT;\n static struct strategy **use_strategies;\n@@ -228,6 +228,7 @@ static struct option builtin_merge_options[] = {\n \t{ OPTION_STRING, 'S', \"gpg-sign\", &sign_commit, N_(\"key-id\"),\n \t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n+\tOPT_BOOL(0, \"no-verify\", &no_verify, N_(\"bypass commit-msg hook\")),\n \tOPT_END()\n };\n \n@@ -809,6 +810,10 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tif (launch_editor(git_path_merge_msg(), NULL, NULL))\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n+\tif (!no_verify &&\n+\t run_commit_hook(0 < option_edit, get_index_file(), \"commit-msg\",\n+\t\t\t git_path_merge_msg(), NULL))\n+\t\tabort_commit(remoteheads, NULL);\n \tread_merge_msg(&msg);\n \tstrbuf_stripspace(&msg, 0 < option_edit);\n \tif (!msg.len)\n-- \n2.8.2\n\n"},{"id":"292201","messageId":"alpine.DEB.2.20.1607261456480.14111@virtualbox","threadId":"42928","inReplyTo":"1469519323-11420-1-git-send-email-orgad.shaneh@audiocodes.com","subject":"Re: [PATCH] merge: Run commit-msg hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-26T13:02:50Z","receivedAt":"2016-07-26T13:03:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Orgad,\n\nOn Tue, 26 Jul 2016, Orgad Shaneh wrote:\n\n> From: Orgad Shaneh \n\nAgain, this is unnecessary if you already send the mail from the same\naddress.\n\n> commit-msg is needed to either validate the commit message or edit it.\n> Gerrit for instance uses this hook to append its Change-Id footer.\n> \n> This is relevant to merge commit just like any other commit.\n\nHmm. This is not very convincing to me, as\n\n- if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?\n\n- a merge is a different beast from a simple commit. That is why we have\n two different commands for them. A hook to edit the merge message may\n need to know the *second* parent commit, too, for example to generate\n a diffstat, or to add information about changes in an \"evil commit\".\n\n- if Gerrit is the intended user, would it not make more sense to\n introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you\n have to teach Gerrit a new trick anyway?\n\n- if Gerrit is the intended user, why does it not simply edit the merge\n message itself? After all, it executes it, and probably crafts a merge\n message mentioning that this is an automatic merge, anyway, so why not\n add the Change-Id *then*?\n\nCiao,\nDscho\n"},{"id":"292206","messageId":"CAGHpTBLr9q8h-+hVUzsTS1aS1TyZjz9gYM_T_ZBdY=o26JGaHw@mail.gmail.com","threadId":"42928","inReplyTo":"alpine.DEB.2.20.1607261456480.14111@virtualbox","subject":"Re: [PATCH] merge: Run commit-msg hook","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2016-07-26T13:50:36Z","receivedAt":"2016-07-26T13:50:42Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Hi,\n\nOn Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin\n wrote:\n> Hi Orgad,\n>\n> On Tue, 26 Jul 2016, Orgad Shaneh wrote:\n>\n>> From: Orgad Shaneh \n>\n> Again, this is unnecessary if you already send the mail from the same\n> address.\n>\n>> commit-msg is needed to either validate the commit message or edit it.\n>> Gerrit for instance uses this hook to append its Change-Id footer.\n>>\n>> This is relevant to merge commit just like any other commit.\n>\n> Hmm. This is not very convincing to me, as\n>\n> - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?\n\nprepare-commit-msg is already called, a few lines above this addition.\n\n>\n> - a merge is a different beast from a simple commit. That is why we have\n> two different commands for them. A hook to edit the merge message may\n> need to know the *second* parent commit, too, for example to generate\n> a diffstat, or to add information about changes in an \"evil commit\".\n\nThat is correct for a post-merge hook. Why should *message validation* differ\nbetween simple and merge commit?\n\n> - if Gerrit is the intended user, would it not make more sense to\n> introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you\n> have to teach Gerrit a new trick anyway?\n\nWhy is that new? Every commit in gerrit has a Change-Id footer, which is\ngenerated by commit-msg hook. What I currently do for merges that succeed\nwithout conflicts is unconditional commit --amend --no-edit just to\nrun the hook.\nThis doesn't make sense.\n\n> - if Gerrit is the intended user, why does it not simply edit the merge\n> message itself? After all, it executes it, and probably crafts a merge\n> message mentioning that this is an automatic merge, anyway, so why not\n> add the Change-Id *then*?\n\nMost Gerrit setups require Change-Id in the commit message that the user\npushes. It is possible to disable this setting, and then you don't need the\nChange-Id at all, but then you can't push a new patch set for the change\n(unless you copy the Change-Id from gerrit to your commit message).\nThat's a real pain, I'm not aware of any public gerrit server that\ndisables this :)\n\n> Ciao,\n> Dscho\n\n- Orgad\n"},{"id":"292211","messageId":"alpine.DEB.2.20.1607261614020.14111@virtualbox","threadId":"42928","inReplyTo":"CAGHpTBLr9q8h-+hVUzsTS1aS1TyZjz9gYM_T_ZBdY=o26JGaHw@mail.gmail.com","subject":"Re: [PATCH] merge: Run commit-msg hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-26T14:54:06Z","receivedAt":"2016-07-26T14:54:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Orgad,\n\nOn Tue, 26 Jul 2016, Orgad Shaneh wrote:\n\n> On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin\n> wrote:\n> >\n> > On Tue, 26 Jul 2016, Orgad Shaneh wrote:\n> >\n> >> commit-msg is needed to either validate the commit message or edit it.\n> >> Gerrit for instance uses this hook to append its Change-Id footer.\n> >>\n> >> This is relevant to merge commit just like any other commit.\n> >\n> > Hmm. This is not very convincing to me, as\n> >\n> > - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?\n> \n> prepare-commit-msg is already called, a few lines above this addition.\n\nOh. That would have made a heck of a convincing argument in the commit\nmessage. Pity it was not mentioned. (Yes, please read that as a strong\nsuggestion to fix that in the next patch iteration.)\n\nFWIW I dug out the original submission:\nhttp://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435\n\nIt seems that there was no discussion about the commit-msg. Which makes me\nwonder why nobody thought of this.\n\nNow, you could make a case that you want to run the commit-msg hook in the\nsame spirit as prepare-commit-msg (and mention that apparently nobody\nthought of that when the patch was accepted into builtin/merge.c).\n\nBut then I wonder what your argument would be for *not* running the\npre-commit and the post-commit hooks in `git merge` as well?\n\nSeems like a big inconsistency to me, one that would not be helped by a\npiecemeal patch that does only half the job of resolving the inconsistency.\n\nThere was actually a question why the post-commit hook was not run in `git\nmerge`:\nhttp://thread.gmane.org/gmane.comp.version-control.git/168716/focus=168773\n(but it seems nobody cared all that much).\n\n> > - a merge is a different beast from a simple commit. That is why we\n> > have two different commands for them. A hook to edit the merge message\n> > may need to know the *second* parent commit, too, for example to\n> > generate a diffstat, or to add information about changes in an \"evil\n> > commit\".\n> \n> That is correct for a post-merge hook. Why should *message validation*\n> differ between simple and merge commit?\n\nYou yourself do not use the hook for validation. You use it to *edit* the\nmessage. My examples do the very same thing. Why should they wait for a\n*post-merge* hook to amend the message?\n\nOtherwise, why wouldn't you use the post-merge hook to add the Change-Id:\nin your use case, too...\n\n> > - if Gerrit is the intended user, would it not make more sense to\n> > introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you\n> > have to teach Gerrit a new trick anyway?\n> \n> Why is that new? Every commit in gerrit has a Change-Id footer, which is\n> generated by commit-msg hook.\n\nSo it already works for Gerrit? Why is this patch needed, then? This is\nconfusing.\n\n> What I currently do for merges that succeed without conflicts is\n> unconditional commit --amend --no-edit just to run the hook.\n\nSo you do that manually? Or you taught Gerrit to do that? Please clarify.\n\n> > - if Gerrit is the intended user, why does it not simply edit the merge\n> > message itself? After all, it executes it, and probably crafts a merge\n> > message mentioning that this is an automatic merge, anyway, so why not\n> > add the Change-Id *then*?\n> \n> Most Gerrit setups require Change-Id in the commit message that the user\n> pushes. It is possible to disable this setting, and then you don't need the\n> Change-Id at all, but then you can't push a new patch set for the change\n> (unless you copy the Change-Id from gerrit to your commit message).\n> That's a real pain, I'm not aware of any public gerrit server that\n> disables this :)\n\nForgive me, I never used Gerrit, and neither do I intend to do so.\n\nI would appreciate a self-contained commit message that explains just\nenough about Gerrit to understand what the patch tries to solve, and in a\nmanner such that even developers who decided to ignore Gerrit understand.\n\nCiao,\nDscho\n"},{"id":"292271","messageId":"xmqq8twoyusq.fsf@gitster.mtv.corp.google.com","threadId":"42928","inReplyTo":"CAGHpTBLr9q8h-+hVUzsTS1aS1TyZjz9gYM_T_ZBdY=o26JGaHw@mail.gmail.com","subject":"Re: [PATCH] merge: Run commit-msg hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-26T21:05:25Z","receivedAt":"2016-07-26T21:07:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Orgad Shaneh writes:\n\n>> Hmm. This is not very convincing to me, as\n>>\n>> - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?\n>\n> prepare-commit-msg is already called, a few lines above this addition.\n\nI do not see such call in contrib/example/git-merge.sh; could it be\na recent bug that it calls it?\n\n>> - a merge is a different beast from a simple commit. That is why we have\n>> two different commands for them. A hook to edit the merge message may\n>> need to know the *second* parent commit, too, for example to generate\n>> a diffstat, or to add information about changes in an \"evil commit\".\n>\n> That is correct for a post-merge hook. Why should *message validation* differ\n> between simple and merge commit?\n\nHave you seen a typical merge commit message? They certainly look\ndifferent to me and different rules would apply.\n\n>> - if Gerrit is the intended user, would it not make more sense to\n>> introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you\n>> have to teach Gerrit a new trick anyway?\n>\n> Why is that new? Every commit in gerrit has a Change-Id footer, which is\n> generated by commit-msg hook.\n\nHmm, I didn't know Gerrit gave Change-ID to merges.\n\nIn any case, I would think this is completely new. Without this\nchange, commit-msg was not called for merges, so Gerrit wouldn't\nhave added Change-ID to merges with that mechanism.\n\nA new merge-msg hook would make more sense, if we were making any\nchange. I do not want to get yelled at by those who wrote their\ncommit-msg hooks long time ago and have happily been using them that\nupdated Git started calling them for their merges where their\nvalidation logic for their single-parent commit do not apply at all.\n"},{"id":"292274","messageId":"xmqq4m7cyuhj.fsf@gitster.mtv.corp.google.com","threadId":"42928","inReplyTo":"alpine.DEB.2.20.1607261614020.14111@virtualbox","subject":"Re: [PATCH] merge: Run commit-msg hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-26T21:12:08Z","receivedAt":"2016-07-26T21:13:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> FWIW I dug out the original submission:\n> http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435\n>\n> It seems that there was no discussion about the commit-msg. Which makes me\n> wonder why nobody thought of this.\n\nI actually think it was a mistaken argument.\n\nWe could have devised a mechanism to prevent \"git commit\" that\nconcludes a conflicted \"git merge\" from calling the hook to resolve\nthe inconsistency, and the solution would have been equally valid.\n\nI'd rather not make things worse by repeating that same mistake. If\nwe were to change anything, we should be adding prepare-merge-msg as\nyou suggested earlier and weaning people off of the current\nbehaviour that was added by mistake.\n\nThanks.\n\n"},{"id":"292310","messageId":"alpine.DEB.2.20.1607271413480.14111@virtualbox","threadId":"42928","inReplyTo":"CAGHpTBLN1vBv12fSBXK0taGzxynMymBWRu8FcG=miBy=raReHw@mail.gmail.com","subject":"Re: [PATCH] merge: Run commit-msg hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-27T12:35:07Z","receivedAt":"2016-07-27T12:35:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Orgad,\n\nOn Tue, 26 Jul 2016, Orgad Shaneh wrote:\n\n> On Tue, Jul 26, 2016 at 5:54 PM, Johannes Schindelin <\n> Johannes.Schindelin@gmx.de> wrote:\n> \n> > On Tue, 26 Jul 2016, Orgad Shaneh wrote:\n> >\n> > > On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin\n> > > wrote:\n> > > >\n> > > > On Tue, 26 Jul 2016, Orgad Shaneh wrote:\n> > > >\n> > > >> commit-msg is needed to either validate the commit message or\n> > > >> edit it. Gerrit for instance uses this hook to append its\n> > > >> Change-Id footer.\n> > > >>\n> > > >> This is relevant to merge commit just like any other commit.\n> > > >\n> > > > Hmm. This is not very convincing to me, as\n> > > >\n> > > > - if you call commit-msg in `git merge` now, why not\n> > `prepare-commit-msg`?\n> > >\n> > > prepare-commit-msg is already called, a few lines above this\n> > > addition.\n> >\n> > Oh. That would have made a heck of a convincing argument in the commit\n> > message. Pity it was not mentioned. (Yes, please read that as a strong\n> > suggestion to fix that in the next patch iteration.)\n> \n> Done.\n\nGood.\n\n> > FWIW I dug out the original submission:\n> > http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435\n> >\n> > It seems that there was no discussion about the commit-msg. Which\n> > makes me wonder why nobody thought of this.\n> >\n> > Now, you could make a case that you want to run the commit-msg hook in\n> > the same spirit as prepare-commit-msg (and mention that apparently\n> > nobody thought of that when the patch was accepted into\n> > builtin/merge.c).\n> >\n> > But then I wonder what your argument would be for *not* running the\n> > pre-commit and the post-commit hooks in `git merge` as well?\n> >\n> > Seems like a big inconsistency to me, one that would not be helped by\n> > a piecemeal patch that does only half the job of resolving the\n> > inconsistency.\n> >\n> > There was actually a question why the post-commit hook was not run in\n> > `git merge`:\n> > http://thread.gmane.org/gmane.comp.version-control.git/168716/focus=168773\n> > (but it seems nobody cared all that much).\n> >\n> \n> pre-commit seems to have been rejected, though I must admit I don't\n> fully understand why. post-commit might make sense, but I wouldn't\n> include it in the same patch. These are different issues.\n\nI did not mean to include that change in the patch. I meant to make a case\nfor, or against, it, in the commit message.\n\nThe commit message is an excellent place to demonstrate the usefulness of\nthe change as well as background information that lets the reader\nunderstand how thorough the patch is. A commit message can increase the\ntrust in the rigidity of the patch.\n\nThat is why I suggested to research further by providing two links with\nfurther information.\n\nNow that I know that pre-commit has been rejected, of course, I want to\nknow why. Because the same argument might well preclude commit-msg support\nfrom being added to `git merge` (and it might well lead to the\nprepare-commit-msg hook to be deprecated in `git merge`).\n\nThis is all very useful information. We are slowly getting to the full\npicture needed to assess whether to include this patch or not.\n\n> > > > - a merge is a different beast from a simple commit. That is why\n> > > > we have two different commands for them. A hook to edit the merge\n> > > > message may need to know the *second* parent commit, too, for\n> > > > example to generate a diffstat, or to add information about\n> > > > changes in an \"evil commit\".\n> > >\n> > > That is correct for a post-merge hook. Why should *message\n> > > validation* differ between simple and merge commit?\n> >\n> > You yourself do not use the hook for validation. You use it to *edit*\n> > the message. My examples do the very same thing. Why should they wait\n> > for a *post-merge* hook to amend the message?\n> >\n> \n> Because commit-msg doesn't know anything about the commit.\n\nThat is incorrect. When that hook is called, `git rev-parse HEAD` refers\nto the parent commit, and `GIT_INDEX_FILE` refers to the index that will be\nwritten as a tree object. That is pretty much all the information needed\nto know about the future commit.\n\n> > Otherwise, why wouldn't you use the post-merge hook to add the Change-Id:\n> > in your use case, too...\n> >\n> > > > - if Gerrit is the intended user, would it not make more sense to\n> > > > introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`),\n> > > > as you have to teach Gerrit a new trick anyway?\n> > >\n> > > Why is that new? Every commit in gerrit has a Change-Id footer, which is\n> > > generated by commit-msg hook.\n> >\n> > So it already works for Gerrit? Why is this patch needed, then? This is\n> > confusing.\n> >\n> \n> Gerrit is a server, the user installs a commit-msg hook provided by\n> Gerrit on the local machine. This hook currently works only for simple\n> commits and not for (trivial) merge commits. That's where this patch\n> comes to the rescue.\n\nSorry, you really need to explain this better in the commit message.\nGerrit exists for quite some time, so I would assume that other developers\nwould have had serious problems in similar situations. How do they address\nthose challenges?\n\n> > > What I currently do for merges that succeed without conflicts is\n> > > unconditional commit --amend --no-edit just to run the hook.\n> >\n> > So you do that manually? Or you taught Gerrit to do that? Please\n> > clarify.\n> \n> Gerrit can't do anything on my machine. It's a web/ssh server. I have my\n> own post-merge hook that runs commit --amend\n\nOkay, so why exactly do you need the commit-msg?\n\nAll the concerns I raised, all the confusion *need* to be addressed by a\ncommit message that makes a good case for the change. That is what commit\nmessages are for. So far, the commit message of this patch is not up to\nthe task.\n\nCiao,\nJohannes\n"}]}