git-p4: Add hook p4-pre-pedit-changelist
diff mbox series

Message ID pull.698.git.git.1579555036314.gitgitgadget@gmail.com
State New
Headers show
Series
  • git-p4: Add hook p4-pre-pedit-changelist
Related show

Commit Message

Ralf Thielow via GitGitGadget Jan. 20, 2020, 9:17 p.m. UTC
From: Ben Keene <seraphire@gmail.com>

Add an additional hook to the git-p4 command to allow a hook to modify
the text of the changelist prior to displaying the p4editor command.

This hook will be called prior to checking for the flag
"--prepare-p4-only".

The hook is optional, if it does not exist, it will be skipped.

The hook takes a single parameter, the filename of the temporary file
that contains the P4 submit text.

The hook should return a zero exit code on success or a non-zero exit
code on failure.  If the hook returns a non-zero exit code, git-p4
will revert the P4 edits by calling p4_revert(f) on each file that was
flagged as edited and then it will return False so the calling method
may continue as it does in existing failure cases.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
    git-p4: Add hook p4-pre-pedit-changelist
    
    Our company's workflow requires that our P4 check-in messages have a
    specific format. A helpful feature in the GIT-P4 program would be a hook
    that occurs after the P4 change list is created but before it is
    displayed in the editor that would allow an external program to possibly
    edit the changelist text.
    
    My suggestion for the hook name is p4-pre-edit-changelist.
    
    It would take a single parameter, the full path of the temporary file.
    If the hook returns a non-zero exit code, it would cancel the current P4
    submit.
    
    The hook should be optional.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-698%2Fseraphire%2Fseraphire%2Fp4-hook-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-698/seraphire/seraphire/p4-hook-v1
Pull-Request: https://github.com/git/git/pull/698

 git-p4.py | 11 +++++++++++
 1 file changed, 11 insertions(+)


base-commit: 232378479ee6c66206d47a9be175e3a39682aea6

Comments

Junio C Hamano Jan. 21, 2020, 11:05 p.m. UTC | #1
[jc] asking for help from those who made non-trivial changes to "git
p4" in the past 18 months or so for reviewing.

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ben Keene <seraphire@gmail.com>
> Subject: Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

"git shortlog --no-merges" would show that the convention is to
downcase "Add".

With two consecutive non-words (i.e. 'pre' and "pedit'), it really
feels an unpronounceable mouthful to a non-perforce person like me.

On the core Git side, "git commit", which is the primary command
that is used to create a new commit, has two hooks that helps to
enforce consistency to the commit log messages:

 - The "prepare-commit-msg" hook prepares the message to be further
   edited by the end-user in the editor

 - The "commit-msg" hook takes what the end-user edited in the
   editor, and can audit and/or tweaks it.

Having a matching pair of hooks and making sure the new hooks have
similar names to these existing ones may help experienced Git users
adopt the new hooks "git p4" learns here.

What makes "p4-pre-pedit-changelist" a good name for this hook?  "In
pure Perforce without Git, there is 'pre-pedit-changelist' hook that
Perforce users are already familiar with" would be a good answer but
not being P4 user myself, I do not know if that is true.

Also, "git commit" has a mechanism (i.e. "--no-verify") to suppress
the "auditing" hook, and it serves as an escape hatch.  The new hook
"git p4" learns may want to have a similar mechanism, to keep its
users productive even when they have broken/stale/bogus hook rejects
their legitimate log message, by allowing them to bypass the
offending hook(s).


> Add an additional hook to the git-p4 command to allow a hook to modify
> the text of the changelist prior to displaying the p4editor command.
>
> This hook will be called prior to checking for the flag
> "--prepare-p4-only".
>
> The hook is optional, if it does not exist, it will be skipped.
>
> The hook takes a single parameter, the filename of the temporary file
> that contains the P4 submit text.
>
> The hook should return a zero exit code on success or a non-zero exit
> code on failure.  If the hook returns a non-zero exit code, git-p4
> will revert the P4 edits by calling p4_revert(f) on each file that was
> flagged as edited and then it will return False so the calling method
> may continue as it does in existing failure cases.

The githooks(5) page should talk about some of these, I would think.

>  git-p4.py | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index 40d9e7c594..1f8c7383df 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
>          tmpFile.write(submitTemplate)
>          tmpFile.close()
>  
> +        # Run the pre-edit hook to allow programmatic update to the changelist
> +        hooks_path = gitConfig("core.hooksPath")
> +        if len(hooks_path) <= 0:
> +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
> +
> +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
> +            for f in editedFiles:
> +                p4_revert(f)
> +            return False
> +
>          if self.prepare_p4_only:
>              #
>              # Leave the p4 tree prepared, and the submit template around
>
> base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
Luke Diamand Jan. 29, 2020, 10:13 a.m. UTC | #2
On Tue, 21 Jan 2020 at 23:05, Junio C Hamano <gitster@pobox.com> wrote:
>
> [jc] asking for help from those who made non-trivial changes to "git
> p4" in the past 18 months or so for reviewing.

This looks fine to me. I've not actually tested it.

Ack.

Luke


>
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Ben Keene <seraphire@gmail.com>
> > Subject: Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist
>
> "git shortlog --no-merges" would show that the convention is to
> downcase "Add".
>
> With two consecutive non-words (i.e. 'pre' and "pedit'), it really
> feels an unpronounceable mouthful to a non-perforce person like me.
>
> On the core Git side, "git commit", which is the primary command
> that is used to create a new commit, has two hooks that helps to
> enforce consistency to the commit log messages:
>
>  - The "prepare-commit-msg" hook prepares the message to be further
>    edited by the end-user in the editor
>
>  - The "commit-msg" hook takes what the end-user edited in the
>    editor, and can audit and/or tweaks it.
>
> Having a matching pair of hooks and making sure the new hooks have
> similar names to these existing ones may help experienced Git users
> adopt the new hooks "git p4" learns here.
>
> What makes "p4-pre-pedit-changelist" a good name for this hook?  "In
> pure Perforce without Git, there is 'pre-pedit-changelist' hook that
> Perforce users are already familiar with" would be a good answer but
> not being P4 user myself, I do not know if that is true.
>
> Also, "git commit" has a mechanism (i.e. "--no-verify") to suppress
> the "auditing" hook, and it serves as an escape hatch.  The new hook
> "git p4" learns may want to have a similar mechanism, to keep its
> users productive even when they have broken/stale/bogus hook rejects
> their legitimate log message, by allowing them to bypass the
> offending hook(s).
>
>
> > Add an additional hook to the git-p4 command to allow a hook to modify
> > the text of the changelist prior to displaying the p4editor command.
> >
> > This hook will be called prior to checking for the flag
> > "--prepare-p4-only".
> >
> > The hook is optional, if it does not exist, it will be skipped.
> >
> > The hook takes a single parameter, the filename of the temporary file
> > that contains the P4 submit text.
> >
> > The hook should return a zero exit code on success or a non-zero exit
> > code on failure.  If the hook returns a non-zero exit code, git-p4
> > will revert the P4 edits by calling p4_revert(f) on each file that was
> > flagged as edited and then it will return False so the calling method
> > may continue as it does in existing failure cases.
>
> The githooks(5) page should talk about some of these, I would think.
>
> >  git-p4.py | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/git-p4.py b/git-p4.py
> > index 40d9e7c594..1f8c7383df 100755
> > --- a/git-p4.py
> > +++ b/git-p4.py
> > @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
> >          tmpFile.write(submitTemplate)
> >          tmpFile.close()
> >
> > +        # Run the pre-edit hook to allow programmatic update to the changelist
> > +        hooks_path = gitConfig("core.hooksPath")
> > +        if len(hooks_path) <= 0:
> > +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
> > +
> > +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
> > +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
> > +            for f in editedFiles:
> > +                p4_revert(f)
> > +            return False
> > +
> >          if self.prepare_p4_only:
> >              #
> >              # Leave the p4 tree prepared, and the submit template around
> >
> > base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
Junio C Hamano Jan. 29, 2020, 7:05 p.m. UTC | #3
Luke Diamand <luke@diamand.org> writes:

> On Tue, 21 Jan 2020 at 23:05, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> [jc] asking for help from those who made non-trivial changes to "git
>> p4" in the past 18 months or so for reviewing.
>
> This looks fine to me. I've not actually tested it.
>
> Ack.

Thanks, but it wasn't very helpful to see an Ack (i.e. "an expert
says this is good") without seeing any of my "why is this good?"
answered by either the original author or the expert X-<.

>> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > From: Ben Keene <seraphire@gmail.com>
>> > Subject: Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist
>>
>> "git shortlog --no-merges" would show that the convention is to
>> downcase "Add".
>>
>> With two consecutive non-words (i.e. 'pre' and "pedit'), it really
>> feels an unpronounceable mouthful to a non-perforce person like me.
>>
>> On the core Git side, "git commit", which is the primary command
>> that is used to create a new commit, has two hooks that helps to
>> enforce consistency to the commit log messages:
>>
>>  - The "prepare-commit-msg" hook prepares the message to be further
>>    edited by the end-user in the editor
>>
>>  - The "commit-msg" hook takes what the end-user edited in the
>>    editor, and can audit and/or tweaks it.
>>
>> Having a matching pair of hooks and making sure the new hooks have
>> similar names to these existing ones may help experienced Git users
>> adopt the new hooks "git p4" learns here.
>>
>> What makes "p4-pre-pedit-changelist" a good name for this hook?  "In
>> pure Perforce without Git, there is 'pre-pedit-changelist' hook that
>> Perforce users are already familiar with" would be a good answer but
>> not being P4 user myself, I do not know if that is true.
>>
>> Also, "git commit" has a mechanism (i.e. "--no-verify") to suppress
>> the "auditing" hook, and it serves as an escape hatch.  The new hook
>> "git p4" learns may want to have a similar mechanism, to keep its
>> users productive even when they have broken/stale/bogus hook rejects
>> their legitimate log message, by allowing them to bypass the
>> offending hook(s).
>>
>>
>> > Add an additional hook to the git-p4 command to allow a hook to modify
>> > the text of the changelist prior to displaying the p4editor command.
>> >
>> > This hook will be called prior to checking for the flag
>> > "--prepare-p4-only".
>> >
>> > The hook is optional, if it does not exist, it will be skipped.
>> >
>> > The hook takes a single parameter, the filename of the temporary file
>> > that contains the P4 submit text.
>> >
>> > The hook should return a zero exit code on success or a non-zero exit
>> > code on failure.  If the hook returns a non-zero exit code, git-p4
>> > will revert the P4 edits by calling p4_revert(f) on each file that was
>> > flagged as edited and then it will return False so the calling method
>> > may continue as it does in existing failure cases.
>>
>> The githooks(5) page should talk about some of these, I would think.
>>
>> >  git-p4.py | 11 +++++++++++
>> >  1 file changed, 11 insertions(+)
>> >
>> > diff --git a/git-p4.py b/git-p4.py
>> > index 40d9e7c594..1f8c7383df 100755
>> > --- a/git-p4.py
>> > +++ b/git-p4.py
>> > @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
>> >          tmpFile.write(submitTemplate)
>> >          tmpFile.close()
>> >
>> > +        # Run the pre-edit hook to allow programmatic update to the changelist
>> > +        hooks_path = gitConfig("core.hooksPath")
>> > +        if len(hooks_path) <= 0:
>> > +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>> > +
>> > +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
>> > +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
>> > +            for f in editedFiles:
>> > +                p4_revert(f)
>> > +            return False
>> > +
>> >          if self.prepare_p4_only:
>> >              #
>> >              # Leave the p4 tree prepared, and the submit template around
>> >
>> > base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
Luke Diamand Jan. 29, 2020, 9:23 p.m. UTC | #4
On Wed, 29 Jan 2020 at 19:05, Junio C Hamano <gitster@pobox.com> wrote:
>
> Luke Diamand <luke@diamand.org> writes:
>
> > On Tue, 21 Jan 2020 at 23:05, Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> [jc] asking for help from those who made non-trivial changes to "git
> >> p4" in the past 18 months or so for reviewing.
> >
> > This looks fine to me. I've not actually tested it.
> >
> > Ack.
>
> Thanks, but it wasn't very helpful to see an Ack (i.e. "an expert
> says this is good") without seeing any of my "why is this good?"
> answered by either the original author or the expert X-<.

You're right.

Revisiting the code, there is already a p4-pre-submit hook. However, I
don't think that would suffice as it doesn't get given the actual
commits, and trying to figure out the commit messages would be quite
tricky.

However, a regular git pre-submit hook could do this.

If the commit-message-checking relied on talking to the Perforce
server though, this proposed hook would be necessary.


>
> >> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >>
> >> > From: Ben Keene <seraphire@gmail.com>
> >> > Subject: Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist
> >>
> >> "git shortlog --no-merges" would show that the convention is to
> >> downcase "Add".
> >>
> >> With two consecutive non-words (i.e. 'pre' and "pedit'), it really
> >> feels an unpronounceable mouthful to a non-perforce person like me.
> >>
> >> On the core Git side, "git commit", which is the primary command
> >> that is used to create a new commit, has two hooks that helps to
> >> enforce consistency to the commit log messages:
> >>
> >>  - The "prepare-commit-msg" hook prepares the message to be further
> >>    edited by the end-user in the editor
> >>
> >>  - The "commit-msg" hook takes what the end-user edited in the
> >>    editor, and can audit and/or tweaks it.
> >>
> >> Having a matching pair of hooks and making sure the new hooks have
> >> similar names to these existing ones may help experienced Git users
> >> adopt the new hooks "git p4" learns here.
> >>
> >> What makes "p4-pre-pedit-changelist" a good name for this hook?  "In
> >> pure Perforce without Git, there is 'pre-pedit-changelist' hook that
> >> Perforce users are already familiar with" would be a good answer but
> >> not being P4 user myself, I do not know if that is true.
> >>
> >> Also, "git commit" has a mechanism (i.e. "--no-verify") to suppress
> >> the "auditing" hook, and it serves as an escape hatch.  The new hook
> >> "git p4" learns may want to have a similar mechanism, to keep its
> >> users productive even when they have broken/stale/bogus hook rejects
> >> their legitimate log message, by allowing them to bypass the
> >> offending hook(s).
> >>
> >>
> >> > Add an additional hook to the git-p4 command to allow a hook to modify
> >> > the text of the changelist prior to displaying the p4editor command.
> >> >
> >> > This hook will be called prior to checking for the flag
> >> > "--prepare-p4-only".
> >> >
> >> > The hook is optional, if it does not exist, it will be skipped.
> >> >
> >> > The hook takes a single parameter, the filename of the temporary file
> >> > that contains the P4 submit text.
> >> >
> >> > The hook should return a zero exit code on success or a non-zero exit
> >> > code on failure.  If the hook returns a non-zero exit code, git-p4
> >> > will revert the P4 edits by calling p4_revert(f) on each file that was
> >> > flagged as edited and then it will return False so the calling method
> >> > may continue as it does in existing failure cases.
> >>
> >> The githooks(5) page should talk about some of these, I would think.
> >>
> >> >  git-p4.py | 11 +++++++++++
> >> >  1 file changed, 11 insertions(+)
> >> >
> >> > diff --git a/git-p4.py b/git-p4.py
> >> > index 40d9e7c594..1f8c7383df 100755
> >> > --- a/git-p4.py
> >> > +++ b/git-p4.py
> >> > @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
> >> >          tmpFile.write(submitTemplate)
> >> >          tmpFile.close()
> >> >
> >> > +        # Run the pre-edit hook to allow programmatic update to the changelist
> >> > +        hooks_path = gitConfig("core.hooksPath")
> >> > +        if len(hooks_path) <= 0:
> >> > +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
> >> > +
> >> > +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
> >> > +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
> >> > +            for f in editedFiles:
> >> > +                p4_revert(f)
> >> > +            return False
> >> > +
> >> >          if self.prepare_p4_only:
> >> >              #
> >> >              # Leave the p4 tree prepared, and the submit template around
> >> >
> >> > base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
Junio C Hamano Jan. 30, 2020, 1:37 a.m. UTC | #5
Junio C Hamano <gitster@pobox.com> writes:

> Thanks, but it wasn't very helpful to see an Ack (i.e. "an expert
> says this is good") without seeing any of my "why is this good?"
> answered by either the original author or the expert X-<.

More specifically, to summarize the issues I raised:

 * Is the proposed name of the hook a reasonable one?  If so, the
   log message should explain why it is a reasonable one.  If not,
   it should be given a more reasonable name and the log message
   should justify the new name.

 * Given that "git commit" has a pair of hooks for log message, is
   adding one new hook a reasonable thing?  If so, the log mesasge
   should explain why (e.g. perhaps the other one already is there,
   or perhaps the other one is not applicable in the context of
   interacting with P4 with such and such reasons).)

 * Is it reasonable not to have a mechanism to disable/skip the
   hook, like "git commit" does?  If not, the log message should
   explain why such an escape hatch, which is needed for "git
   commit", is not needed.

 * githooks(5) manual page is supposed to list all hooks, so a patch
   that adds a new one should add a description for it in there.

Thanks.

>>> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>> > From: Ben Keene <seraphire@gmail.com>
>>> > Subject: Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist
>>>
>>> "git shortlog --no-merges" would show that the convention is to
>>> downcase "Add".
>>>
>>> With two consecutive non-words (i.e. 'pre' and "pedit'), it really
>>> feels an unpronounceable mouthful to a non-perforce person like me.
>>>
>>> On the core Git side, "git commit", which is the primary command
>>> that is used to create a new commit, has two hooks that helps to
>>> enforce consistency to the commit log messages:
>>>
>>>  - The "prepare-commit-msg" hook prepares the message to be further
>>>    edited by the end-user in the editor
>>>
>>>  - The "commit-msg" hook takes what the end-user edited in the
>>>    editor, and can audit and/or tweaks it.
>>>
>>> Having a matching pair of hooks and making sure the new hooks have
>>> similar names to these existing ones may help experienced Git users
>>> adopt the new hooks "git p4" learns here.
>>>
>>> What makes "p4-pre-pedit-changelist" a good name for this hook?  "In
>>> pure Perforce without Git, there is 'pre-pedit-changelist' hook that
>>> Perforce users are already familiar with" would be a good answer but
>>> not being P4 user myself, I do not know if that is true.
>>>
>>> Also, "git commit" has a mechanism (i.e. "--no-verify") to suppress
>>> the "auditing" hook, and it serves as an escape hatch.  The new hook
>>> "git p4" learns may want to have a similar mechanism, to keep its
>>> users productive even when they have broken/stale/bogus hook rejects
>>> their legitimate log message, by allowing them to bypass the
>>> offending hook(s).
>>>
>>>
>>> > Add an additional hook to the git-p4 command to allow a hook to modify
>>> > the text of the changelist prior to displaying the p4editor command.
>>> >
>>> > This hook will be called prior to checking for the flag
>>> > "--prepare-p4-only".
>>> >
>>> > The hook is optional, if it does not exist, it will be skipped.
>>> >
>>> > The hook takes a single parameter, the filename of the temporary file
>>> > that contains the P4 submit text.
>>> >
>>> > The hook should return a zero exit code on success or a non-zero exit
>>> > code on failure.  If the hook returns a non-zero exit code, git-p4
>>> > will revert the P4 edits by calling p4_revert(f) on each file that was
>>> > flagged as edited and then it will return False so the calling method
>>> > may continue as it does in existing failure cases.
>>>
>>> The githooks(5) page should talk about some of these, I would think.
>>>
>>> >  git-p4.py | 11 +++++++++++
>>> >  1 file changed, 11 insertions(+)
>>> >
>>> > diff --git a/git-p4.py b/git-p4.py
>>> > index 40d9e7c594..1f8c7383df 100755
>>> > --- a/git-p4.py
>>> > +++ b/git-p4.py
>>> > @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
>>> >          tmpFile.write(submitTemplate)
>>> >          tmpFile.close()
>>> >
>>> > +        # Run the pre-edit hook to allow programmatic update to the changelist
>>> > +        hooks_path = gitConfig("core.hooksPath")
>>> > +        if len(hooks_path) <= 0:
>>> > +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>>> > +
>>> > +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
>>> > +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
>>> > +            for f in editedFiles:
>>> > +                p4_revert(f)
>>> > +            return False
>>> > +
>>> >          if self.prepare_p4_only:
>>> >              #
>>> >              # Leave the p4 tree prepared, and the submit template around
>>> >
>>> > base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
Bryan Turner Jan. 30, 2020, 1:51 a.m. UTC | #6
On Mon, Jan 20, 2020 at 1:17 PM Ben Keene via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Ben Keene <seraphire@gmail.com>
>
> Add an additional hook to the git-p4 command to allow a hook to modify
> the text of the changelist prior to displaying the p4editor command.
>
> This hook will be called prior to checking for the flag
> "--prepare-p4-only".
>
> The hook is optional, if it does not exist, it will be skipped.
>
> The hook takes a single parameter, the filename of the temporary file
> that contains the P4 submit text.
>
> The hook should return a zero exit code on success or a non-zero exit
> code on failure.  If the hook returns a non-zero exit code, git-p4
> will revert the P4 edits by calling p4_revert(f) on each file that was
> flagged as edited and then it will return False so the calling method
> may continue as it does in existing failure cases.
>
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>     git-p4: Add hook p4-pre-pedit-changelist
>
>     Our company's workflow requires that our P4 check-in messages have a
>     specific format. A helpful feature in the GIT-P4 program would be a hook
>     that occurs after the P4 change list is created but before it is
>     displayed in the editor that would allow an external program to possibly
>     edit the changelist text.
>
>     My suggestion for the hook name is p4-pre-edit-changelist.
>
>     It would take a single parameter, the full path of the temporary file.
>     If the hook returns a non-zero exit code, it would cancel the current P4
>     submit.
>
>     The hook should be optional.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-698%2Fseraphire%2Fseraphire%2Fp4-hook-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-698/seraphire/seraphire/p4-hook-v1
> Pull-Request: https://github.com/git/git/pull/698
>
>  git-p4.py | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index 40d9e7c594..1f8c7383df 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
>          tmpFile.write(submitTemplate)
>          tmpFile.close()
>
> +        # Run the pre-edit hook to allow programmatic update to the changelist
> +        hooks_path = gitConfig("core.hooksPath")
> +        if len(hooks_path) <= 0:
> +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
> +
> +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")

The commit subject and the resulting email say "p4-pre-pedit", and I
see Junio is asking about "p4-pre-pedit", but the actual code, and the
scissors message, both say "p4-pre-edit". So which is it intended to
be? "edit", or "pedit"?

> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
> +            for f in editedFiles:
> +                p4_revert(f)
> +            return False
> +
>          if self.prepare_p4_only:
>              #
>              # Leave the p4 tree prepared, and the submit template around
>
> base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
> --
> gitgitgadget
Ben Keene Jan. 30, 2020, 1:45 p.m. UTC | #7
On 1/29/2020 8:51 PM, Bryan Turner wrote:
> On Mon, Jan 20, 2020 at 1:17 PM Ben Keene via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> From: Ben Keene <seraphire@gmail.com>
>>
>> Add an additional hook to the git-p4 command to allow a hook to modify
>> the text of the changelist prior to displaying the p4editor command.
>>
>> This hook will be called prior to checking for the flag
>> "--prepare-p4-only".
>>
>> The hook is optional, if it does not exist, it will be skipped.
>>
>> The hook takes a single parameter, the filename of the temporary file
>> that contains the P4 submit text.
>>
>> The hook should return a zero exit code on success or a non-zero exit
>> code on failure.  If the hook returns a non-zero exit code, git-p4
>> will revert the P4 edits by calling p4_revert(f) on each file that was
>> flagged as edited and then it will return False so the calling method
>> may continue as it does in existing failure cases.
>>
>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> ---
>>      git-p4: Add hook p4-pre-pedit-changelist
>>
>>      Our company's workflow requires that our P4 check-in messages have a
>>      specific format. A helpful feature in the GIT-P4 program would be a hook
>>      that occurs after the P4 change list is created but before it is
>>      displayed in the editor that would allow an external program to possibly
>>      edit the changelist text.
>>
>>      My suggestion for the hook name is p4-pre-edit-changelist.
>>
>>      It would take a single parameter, the full path of the temporary file.
>>      If the hook returns a non-zero exit code, it would cancel the current P4
>>      submit.
>>
>>      The hook should be optional.
>>
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-698%2Fseraphire%2Fseraphire%2Fp4-hook-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-698/seraphire/seraphire/p4-hook-v1
>> Pull-Request: https://github.com/git/git/pull/698
>>
>>   git-p4.py | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 40d9e7c594..1f8c7383df 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
>>           tmpFile.write(submitTemplate)
>>           tmpFile.close()
>>
>> +        # Run the pre-edit hook to allow programmatic update to the changelist
>> +        hooks_path = gitConfig("core.hooksPath")
>> +        if len(hooks_path) <= 0:
>> +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>> +
>> +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
> The commit subject and the resulting email say "p4-pre-pedit", and I
> see Junio is asking about "p4-pre-pedit", but the actual code, and the
> scissors message, both say "p4-pre-edit". So which is it intended to
> be? "edit", or "pedit"?


"pedit" was a typo in the gitgitgadget submission.  I have since
changed the submission title to the corrected text "edit".

>> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
>> +            for f in editedFiles:
>> +                p4_revert(f)
>> +            return False
>> +
>>           if self.prepare_p4_only:
>>               #
>>               # Leave the p4 tree prepared, and the submit template around
>>
>> base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
>> --
>> gitgitgadget
Ben Keene Jan. 30, 2020, 2:20 p.m. UTC | #8
On 1/29/2020 8:37 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thanks, but it wasn't very helpful to see an Ack (i.e. "an expert
>> says this is good") without seeing any of my "why is this good?"
>> answered by either the original author or the expert X-<.
> More specifically, to summarize the issues I raised:
Thanks for summarizing your questions, below are my thoughts.
>
>   * Is the proposed name of the hook a reasonable one?  If so, the
>     log message should explain why it is a reasonable one.  If not,
>     it should be given a more reasonable name and the log message
>     should justify the new name.

Having re-read your original comments, no, I think that I should change
the name of the hook from "p4-pre-edit-changelist" to follow the
git standard hooks:

* "p4-prepare-changelist" - This will replace the proposed hook but still
   take only the filename. This hook will be called, even if the
   prepare-p4-only option is selected.

* "p4-changelist" - this is a new hook that will be added after the
   user edit of the changelist text, but prior to the actual submission.
   This hook will also take the temporary file as it's only parameter
   and a failed response will fail the submission.

>   * Given that "git commit" has a pair of hooks for log message, is
>     adding one new hook a reasonable thing?  If so, the log mesasge
>     should explain why (e.g. perhaps the other one already is there,
>     or perhaps the other one is not applicable in the context of
>     interacting with P4 with such and such reasons).)

I agree with your suggestion.

>   * Is it reasonable not to have a mechanism to disable/skip the
>     hook, like "git commit" does?  If not, the log message should
>     explain why such an escape hatch, which is needed for "git
>     commit", is not needed.
The existing hook, p4-pre-submit, does not have an escape hatch,
so I did not add one to this method, but I can certainly add one.

I am amenable to adding an escape hatch, I'll add --no-verify.

>
>   * githooks(5) manual page is supposed to list all hooks, so a patch
>     that adds a new one should add a description for it in there.

I'll add text for these files (githooks and the git-p4 pages).

I'll make a new submission soon.


>
> Thanks.
>
>>>> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>>
>>>>> From: Ben Keene <seraphire@gmail.com>
>>>>> Subject: Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist
>>>> "git shortlog --no-merges" would show that the convention is to
>>>> downcase "Add".
>>>>
>>>> With two consecutive non-words (i.e. 'pre' and "pedit'), it really
>>>> feels an unpronounceable mouthful to a non-perforce person like me.
>>>>
>>>> On the core Git side, "git commit", which is the primary command
>>>> that is used to create a new commit, has two hooks that helps to
>>>> enforce consistency to the commit log messages:
>>>>
>>>>   - The "prepare-commit-msg" hook prepares the message to be further
>>>>     edited by the end-user in the editor
>>>>
>>>>   - The "commit-msg" hook takes what the end-user edited in the
>>>>     editor, and can audit and/or tweaks it.
>>>>
>>>> Having a matching pair of hooks and making sure the new hooks have
>>>> similar names to these existing ones may help experienced Git users
>>>> adopt the new hooks "git p4" learns here.
>>>>
>>>> What makes "p4-pre-pedit-changelist" a good name for this hook?  "In
>>>> pure Perforce without Git, there is 'pre-pedit-changelist' hook that
>>>> Perforce users are already familiar with" would be a good answer but
>>>> not being P4 user myself, I do not know if that is true.
>>>>
>>>> Also, "git commit" has a mechanism (i.e. "--no-verify") to suppress
>>>> the "auditing" hook, and it serves as an escape hatch.  The new hook
>>>> "git p4" learns may want to have a similar mechanism, to keep its
>>>> users productive even when they have broken/stale/bogus hook rejects
>>>> their legitimate log message, by allowing them to bypass the
>>>> offending hook(s).
>>>>
>>>>
>>>>> Add an additional hook to the git-p4 command to allow a hook to modify
>>>>> the text of the changelist prior to displaying the p4editor command.
>>>>>
>>>>> This hook will be called prior to checking for the flag
>>>>> "--prepare-p4-only".
>>>>>
>>>>> The hook is optional, if it does not exist, it will be skipped.
>>>>>
>>>>> The hook takes a single parameter, the filename of the temporary file
>>>>> that contains the P4 submit text.
>>>>>
>>>>> The hook should return a zero exit code on success or a non-zero exit
>>>>> code on failure.  If the hook returns a non-zero exit code, git-p4
>>>>> will revert the P4 edits by calling p4_revert(f) on each file that was
>>>>> flagged as edited and then it will return False so the calling method
>>>>> may continue as it does in existing failure cases.
>>>> The githooks(5) page should talk about some of these, I would think.
>>>>
>>>>>   git-p4.py | 11 +++++++++++
>>>>>   1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/git-p4.py b/git-p4.py
>>>>> index 40d9e7c594..1f8c7383df 100755
>>>>> --- a/git-p4.py
>>>>> +++ b/git-p4.py
>>>>> @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
>>>>>           tmpFile.write(submitTemplate)
>>>>>           tmpFile.close()
>>>>>
>>>>> +        # Run the pre-edit hook to allow programmatic update to the changelist
>>>>> +        hooks_path = gitConfig("core.hooksPath")
>>>>> +        if len(hooks_path) <= 0:
>>>>> +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>>>>> +
>>>>> +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
>>>>> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
>>>>> +            for f in editedFiles:
>>>>> +                p4_revert(f)
>>>>> +            return False
>>>>> +
>>>>>           if self.prepare_p4_only:
>>>>>               #
>>>>>               # Leave the p4 tree prepared, and the submit template around
>>>>>
>>>>> base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
Junio C Hamano Jan. 30, 2020, 6:39 p.m. UTC | #9
Ben Keene <seraphire@gmail.com> writes:

> ... the name of the hook from "p4-pre-edit-changelist" to follow the
> git standard hooks:
>
> * "p4-prepare-changelist" - This will replace the proposed hook but still
>   take only the filename. This hook will be called, even if the
>   prepare-p4-only option is selected.

With "to follow", I presume that this corresponds to the
"prepare-commit-msg" hook?  Does "changelist" in P4 lingo roughly
correspond to "commit", or to "commit message"?

> * "p4-changelist" - this is a new hook that will be added after the
>   user edit of the changelist text, but prior to the actual submission.
>   This hook will also take the temporary file as it's only parameter
>   and a failed response will fail the submission.

And I presume that this corresponds to "commit-msg" hook?

>>   * Given that "git commit" has a pair of hooks for log message, is
>>     adding one new hook a reasonable thing?  If so, the log mesasge
>>     should explain why (e.g. perhaps the other one already is there,
>>     or perhaps the other one is not applicable in the context of
>>     interacting with P4 with such and such reasons).)
>
> I agree with your suggestion.

OK.  Let's find them in the updated patch ;-)

>>   * Is it reasonable not to have a mechanism to disable/skip the
>>     hook, like "git commit" does?  If not, the log message should
>>     explain why such an escape hatch, which is needed for "git
>>     commit", is not needed.
> The existing hook, p4-pre-submit, does not have an escape hatch,
> so I did not add one to this method, but I can certainly add one.

It is probably a good idea in the longer term, but we can certainly
punt and then revisit to cover them with --no-verify later (as long
as we do document our intention to do so in the log message).

>>   * githooks(5) manual page is supposed to list all hooks, so a patch
>>     that adds a new one should add a description for it in there.
>
> I'll add text for these files (githooks and the git-p4 pages).
>
> I'll make a new submission soon.

Thanks.

Patch
diff mbox series

diff --git a/git-p4.py b/git-p4.py
index 40d9e7c594..1f8c7383df 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2026,6 +2026,17 @@  def applyCommit(self, id):
         tmpFile.write(submitTemplate)
         tmpFile.close()
 
+        # Run the pre-edit hook to allow programmatic update to the changelist
+        hooks_path = gitConfig("core.hooksPath")
+        if len(hooks_path) <= 0:
+            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
+
+        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
+        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
+            for f in editedFiles:
+                p4_revert(f)
+            return False
+
         if self.prepare_p4_only:
             #
             # Leave the p4 tree prepared, and the submit template around