[PATCH] git-p4: Add hook p4-pre-pedit-changelist

[PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Ben Keene via GitGitGadget]

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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Junio C Hamano]

[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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Luke Diamand]

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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Junio C Hamano]

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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Luke Diamand]

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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Junio C Hamano]

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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Bryan Turner]

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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Ben Keene]

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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Ben Keene]

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

Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

[Junio C Hamano]

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 v2 0/4] git-p4: add hook p4-pre-edit-changelist

[Ben Keene via GitGitGadget]

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.

v1: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.

v2:Instead of a single hook, p4-pre-edit-changelist, follow the git
convention for hook names and add the trio of hooks that work together,
similar to git commit.

The hook names are: 

 * p4-prepare-changelist
 * p4-changelist
 * p4-post-changelist

The hooks should follow the same convention as git commit, so a new command
line option for the git-p4 submit function --no-verify should also be added.

Ben Keene (4):
  git-p4: rewrite prompt to be Windows compatible
  git-p4: create new method gitRunHook
  git-p4: add hook p4-pre-edit-changelist
  git-p4: add p4 submit hooks

 Documentation/git-p4.txt   |  44 ++++++++-
 Documentation/githooks.txt |  46 +++++++++
 git-p4.py                  | 191 ++++++++++++++++++++++++++-----------
 3 files changed, 225 insertions(+), 56 deletions(-)


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

Range-diff vs v1:

 -:  ---------- > 1:  585bdd51b2 git-p4: rewrite prompt to be Windows compatible
 -:  ---------- > 2:  f1f9fdc542 git-p4: create new method gitRunHook
 1:  bb3ce3a795 ! 3:  1bdcdc4c18 git-p4: Add hook p4-pre-pedit-changelist
     @@ -1,6 +1,6 @@
      Author: Ben Keene <seraphire@gmail.com>
      
     -    git-p4: Add hook p4-pre-pedit-changelist
     +    git-p4: add hook p4-pre-edit-changelist
      
          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.
 -:  ---------- > 4:  3d34dd042e git-p4: add p4 submit hooks

-- 
gitgitgadget

[PATCH v2 1/4] git-p4: rewrite prompt to be Windows compatible

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The existing function prompt(prompt_text) does not work correctly when
run on Windows 10 bash terminal when launched from the sourcetree
GUI application. The stdout is not flushed properly so the prompt text
is not displayed to the user until the next flush of stdout, which is
quite confusing.

Change this method by:
* Adding flush to stderr, stdout, and stdin
* Use readline from sys.stdin instead of raw_input.

The existing strip().lower() are retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 40d9e7c594..7d8a5ee788 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -175,7 +175,11 @@ def prompt(prompt_text):
     """
     choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))
     while True:
-        response = raw_input(prompt_text).strip().lower()
+        sys.stderr.flush()
+        sys.stdout.write(prompt_text)
+        sys.stdout.flush()
+        sys.stdin.flush()
+        response=sys.stdin.readline().strip().lower()
         if not response:
             continue
         response = response[0]
-- 
gitgitgadget

[PATCH v2 2/4] git-p4: create new method gitRunHook

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

This commit is in preparation of introducing new p4 submit hooks.

The current code in the python script git-p4.py makes the assumption
that the git hooks can be executed by subprocess.call() method. However,
when git is run on Windows, this may not work as expected.

The subprocess.call() does not execute SH.EXE implictly under Windows,
so the scripts may fail. In other words, the hooks do not execute under
windows because the shell interpreter is not automatically loaded.

Add a new function, gitRunHook, that takes 2 parameters:
* the filename of an optionally registered git hook
* an optional list of parameters

The gitRunHook function will honor the existing behavior seen in the
current code for executing the p4-pre-submit hook:

* Hooks are looked for in core.hooksPath directory.
* If core.hooksPath is not set, then the current .git/hooks directory
  is checked.
* If the hook does not exist, the function returns True.
* If the hook file is not accessible, the function returns True.
* If the hook returns a zero exit code when executed, the function
  return True.
* If the hook returns a non-zero exit code, the function returns False.

Add new conditional behavior for Windows:
* Check for an evironment variable 'EXEPATH' which should be set by
  git when git-p4.py is envoked.
* If EXEPATH is None - treat it as an empty string.
* If EXEPATH is set, look for sh.exe in the bin/ directory located
  in EXEPATH.
* If EXEPATH is not set, attempt to resolve against "bin/sh.exe"
* Add a new test for Windows that checks to see of sh.exe can be
  located. If not, return True.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 7d8a5ee788..4e481b3b55 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -4125,6 +4125,35 @@ def printUsage(commands):
     "unshelve" : P4Unshelve,
 }
 
+def gitRunHook(cmd, param=[]):
+    """Execute a hook if the hook exists."""
+    if verbose:
+        sys.stderr.write("Looking for hook: %s\n" % cmd)
+        sys.stderr.flush()
+
+    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, cmd)
+    if isinstance(param,basestring):
+        param=[param]
+
+    if platform.system() == 'Windows':
+        exepath = os.environ.get("EXEPATH")
+        if exepath is None:
+            exepath = ""
+        shexe = os.path.join(exepath, "bin", "sh.exe")
+        if os.path.isfile(shexe) \
+            and os.path.isfile(hook_file) \
+            and os.access(hook_file, os.X_OK) \
+            and subprocess.call([shexe, hook_file] + param) != 0:
+            return False
+
+    else:
+        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file] + param) != 0:
+            return False
+    return True
 
 def main():
     if len(sys.argv[1:]) == 0:
-- 
gitgitgadget

[PATCH v2 3/4] git-p4: add hook p4-pre-edit-changelist

[Ben Keene via GitGitGadget]

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.py | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 4e481b3b55..61cfd1c1ae 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2030,6 +2030,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
-- 
gitgitgadget

[PATCH v2 4/4] git-p4: add p4 submit hooks

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The git command "commit" supports a number of hooks that support
changing the behavior of the commit command.  The git-p4.py program only
has one existing hook, "p4-p4-pre-submit".  This command occurs early
in the process.  There are no hooks in the process flow for modifying
the P4 changelist text programmatically.

Adds 3 new hooks and 1 new option to git-p4.py to the submit option.

The new command line option --no-verify:

Add a new command line option "--no-verify" to the Submit command of
git-p4.py.  This option will function in the spirit of the existing
--no-verify command line option found in git commit. It will cause the
P4 Submit function to ignore the existing p4-pre-submit and the new
p4-changelist hooks.

The new hooks are:
* p4-prepare-changelist - Execute this hook after the changelist file
  has been created. The hook will be executed even if the
  --prepare-p4-only option is set.  This hook ignores the --no-verify
  option in keeping with the existing behavior of git commit.

* p4-changelist - Execute this hook after the user has edited the
  changelist. Do not execute this hook if the user has selected the
  --prepare-p4-only option. This hook will honor the --no-verify,
  following the conventions of git commit.

* p4-post-changelist - Execute this hook after the P4 submission process
  has completed successfully. This hook takes no parameters and is
  executed regardless of the --no-verify option.  It's return value will
  not be checked.

Change the execution of the existing trigger p4-pre-submit to honor the
--no-verify option. Before exiting on failure of this hook, display
text to the user explaining which hook has failed and the impact
of using the --no-verify option.

Move all the code after creating the submitTemplate text into the
try-finally block in the applyCommit() method.  By restructuring the
code, the P4 workspace is protected against otherwise unhandled
exceptions and other early exits.

The calls to the new hooks: p4-prepare-changelist, p4-changelist,
and p4-post-changelist should all be called inside the try-finally
block.

Existing flow control can remain as defined - the if-block for
prepare-p4-only sets the local variable "submitted" to True and exits
the function. All other early exits, leave submitted set to False so the
Finally block will undo changes to the P4 workspace.

Make the small usability change of adding an empty string to the
print statements displayed to the user when the prepare-p4-only option
is selected.  On Windows, the command print() may display a set of
parentheses () to the user when the print() function is called with no
parameters. By supplying an empty string, the intended blank line will
print as expected.

Fix a small bug when the submittedTemplate is edited by the user
and all content in the file is removed. The existing code will throw
an exception if the separateLine is not found in the file.  Change this
code to test for the separator line using a find() test first and only
split on the separator if it is found.

Additionally, add the new behavior that if the changelist file has been
completely emptied that the Submit action for this changelist will be
aborted.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   |  44 +++++++++-
 Documentation/githooks.txt |  46 ++++++++++
 git-p4.py                  | 167 +++++++++++++++++++++++--------------
 3 files changed, 191 insertions(+), 66 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3494a1db3e..f17c1d0561 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -374,14 +374,54 @@ These options can be used to modify 'git p4 submit' behavior.
     been submitted. Implies --disable-rebase. Can also be set with
     git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
 
-Hook for submit
-~~~~~~~~~~~~~~~
+Hooks for submit
+----------------
+
+p4-pre-submit
+~~~~~~~~~~~~~
+
 The `p4-pre-submit` hook is executed if it exists and is executable.
 The hook takes no parameters and nothing from standard input. Exiting with
 non-zero status from this script prevents `git-p4 submit` from launching.
 
 One usage scenario is to run unit tests in the hook.
 
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+p4-changelist
+~~~~~~~~~~~~~
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+
+
 Rebase options
 ~~~~~~~~~~~~~~
 These options can be used to modify 'git p4 rebase' behavior.
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 50365f2914..67e78a8a05 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -515,6 +515,52 @@ The exit status determines whether git will use the data from the
 hook to limit its search.  On error, it will fall back to verifying
 all files and folders.
 
+p4-changelist
+~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+Run `git-p4 submit --help` for details.
+
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+Run `git-p4 submit --help` for details.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+Run `git-p4 submit --help` for details.
+
 p4-pre-submit
 ~~~~~~~~~~~~~
 
diff --git a/git-p4.py b/git-p4.py
index 61cfd1c1ae..fe633acf84 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1521,13 +1521,39 @@ def __init__(self):
                                      "work from a local git branch that is not master"),
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
+                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
+                                     help="Bypass p4-pre-submit and p4-changelist hooks"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
-    The `p4-pre-submit` hook is executed if it exists and is executable.
-    The hook takes no parameters and nothing from standard input. Exiting with
-    non-zero status from this script prevents `git-p4 submit` from launching.
-
-    One usage scenario is to run unit tests in the hook."""
+    The `p4-pre-submit` hook is executed if it exists and is executable. It
+    can be bypassed with the `--no-verify` command line option. The hook takes
+    no parameters and nothing from standard input. Exiting with a non-zero status
+    from this script prevents `git-p4 submit` from launching.
+
+    One usage scenario is to run unit tests in the hook.
+
+    The `p4-prepare-changelist` hook is executed right after preparing the default
+    changelist message and before the editor is started. It takes one parameter,
+    the name of the file that contains the changelist text. Exiting with a non-zero
+    status from the script will abort the process.
+
+    The purpose of the hook is to edit the message file in place, and it is not
+    supressed by the `--no-verify` option. This hook is called even if
+    `--prepare-p4-only` is set.
+
+    The `p4-changelist` hook is executed after the changelist message has been
+    edited by the user. It can be bypassed with the `--no-verify` option. It
+    takes a single parameter, the name of the file that holds the proposed
+    changelist text. Exiting with a non-zero status causes the command to abort.
+
+    The hook is allowed to edit the changelist file and can be used to normalize
+    the text into some project standard format. It can also be used to refuse the
+    Submit after inspect the message file.
+
+    The `p4-post-changelist` hook is invoked after the submit has successfully
+    occured in P4. It takes no parameters and is meant primarily for notification
+    and cannot affect the outcome of the git p4 submit action.
+    """
 
         self.usage += " [name of git branch to submit into perforce depot]"
         self.origin = ""
@@ -1545,6 +1571,7 @@ def __init__(self):
         self.exportLabels = False
         self.p4HasMoveCommand = p4_has_move_command()
         self.branch = None
+        self.no_verify = False
 
         if gitConfig('git-p4.largeFileSystem'):
             die("Large file system not supported for git-p4 submit command. Please remove it from config.")
@@ -1931,6 +1958,9 @@ def applyCommit(self, id):
         applyPatchCmd = patchcmd + "--check --apply -"
         patch_succeeded = True
 
+        if verbose:
+            print("TryPatch: %s" % tryPatchCmd)
+
         if os.system(tryPatchCmd) != 0:
             fixed_rcs_keywords = False
             patch_succeeded = False
@@ -1970,6 +2000,7 @@ def applyCommit(self, id):
                 print("Retrying the patch with RCS keywords cleaned up")
                 if os.system(tryPatchCmd) == 0:
                     patch_succeeded = True
+                    print("Patch succeesed this time")
 
         if not patch_succeeded:
             for f in editedFiles:
@@ -2030,66 +2061,73 @@ 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
-            # and let the user decide what to do next
-            #
-            print()
-            print("P4 workspace prepared for submission.")
-            print("To submit or revert, go to client workspace")
-            print("  " + self.clientPath)
-            print()
-            print("To submit, use \"p4 submit\" to write a new description,")
-            print("or \"p4 submit -i <%s\" to use the one prepared by" \
-                  " \"git p4\"." % fileName)
-            print("You can delete the file \"%s\" when finished." % fileName)
-
-            if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
-                print("To preserve change ownership by user %s, you must\n" \
-                      "do \"p4 change -f <change>\" after submitting and\n" \
-                      "edit the User field.")
-            if pureRenameCopy:
-                print("After submitting, renamed files must be re-synced.")
-                print("Invoke \"p4 sync -f\" on each of these files:")
-                for f in pureRenameCopy:
-                    print("  " + f)
-
-            print()
-            print("To revert the changes, use \"p4 revert ...\", and delete")
-            print("the submit template file \"%s\"" % fileName)
-            if filesToAdd:
-                print("Since the commit adds new files, they must be deleted:")
-                for f in filesToAdd:
-                    print("  " + f)
-            print()
-            return True
-
-        #
-        # Let the user edit the change description, then submit it.
-        #
         submitted = False
 
         try:
+            # Allow the hook to edit the changelist text before presenting it
+            # to the user.
+            if not gitRunHook("p4-prepare-changelist", [fileName]):
+                return False
+
+            if self.prepare_p4_only:
+                #
+                # Leave the p4 tree prepared, and the submit template around
+                # and let the user decide what to do next
+                #
+                submitted = True
+                print("")
+                print("P4 workspace prepared for submission.")
+                print("To submit or revert, go to client workspace")
+                print("  " + self.clientPath)
+                print("")
+                print("To submit, use \"p4 submit\" to write a new description,")
+                print("or \"p4 submit -i <%s\" to use the one prepared by" \
+                      " \"git p4\"." % fileName)
+                print("You can delete the file \"%s\" when finished." % fileName)
+
+                if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
+                    print("To preserve change ownership by user %s, you must\n" \
+                          "do \"p4 change -f <change>\" after submitting and\n" \
+                          "edit the User field.")
+                if pureRenameCopy:
+                    print("After submitting, renamed files must be re-synced.")
+                    print("Invoke \"p4 sync -f\" on each of these files:")
+                    for f in pureRenameCopy:
+                        print("  " + f)
+
+                print("")
+                print("To revert the changes, use \"p4 revert ...\", and delete")
+                print("the submit template file \"%s\"" % fileName)
+                if filesToAdd:
+                    print("Since the commit adds new files, they must be deleted:")
+                    for f in filesToAdd:
+                        print("  " + f)
+                print("")
+                sys.stdout.flush()
+                return True
+
             if self.edit_template(fileName):
+                if not self.no_verify:
+                    if not gitRunHook("p4-changelist", [fileName]):
+                        print("The p4-changelist hook failed.")
+                        sys.stdout.flush()
+                        return False
+
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
                 message = tmpFile.read()
                 tmpFile.close()
                 if self.isWindows:
                     message = message.replace("\r\n", "\n")
-                submitTemplate = message[:message.index(separatorLine)]
+                if message.find(separatorLine) <> -1:
+                    submitTemplate = message[:message.index(separatorLine)]
+                else:
+                    submitTemplate = message
+
+                if len(submitTemplate.strip()) == 0:
+                    print("Changelist is empty, aborting this changelist.")
+                    sys.stdout.flush()
+                    return False
 
                 if update_shelve:
                     p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)
@@ -2112,20 +2150,22 @@ def applyCommit(self, id):
 
                 submitted = True
 
+                gitRunHook("p4-post-changelist")
         finally:
-            # skip this patch
+            # Revert changes if we skip this patch
             if not submitted or self.shelve:
                 if self.shelve:
                     print ("Reverting shelved files.")
                 else:
                     print ("Submission cancelled, undoing p4 changes.")
+                sys.stdout.flush()
                 for f in editedFiles | filesToDelete:
                     p4_revert(f)
                 for f in filesToAdd:
                     p4_revert(f)
                     os.remove(f)
 
-        os.remove(fileName)
+                os.remove(fileName)
         return submitted
 
     # Export git tags as p4 labels. Create a p4 label and then tag
@@ -2349,13 +2389,12 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        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-submit")
-        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
-            sys.exit(1)
+        if not self.no_verify:
+            if not gitRunHook("p4-pre-submit"):
+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
+                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
+                    "however,\nthis will also skip the p4-changelist hook as well.")
+                sys.exit(1)
 
         #
         # Apply the commits, one at a time.  On failure, ask if should
-- 
gitgitgadget

Re: [PATCH v2 2/4] git-p4: create new method gitRunHook

[Junio C Hamano]

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

> diff --git a/git-p4.py b/git-p4.py
> index 7d8a5ee788..4e481b3b55 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -4125,6 +4125,35 @@ def printUsage(commands):
>      "unshelve" : P4Unshelve,
>  }
>  
> +def gitRunHook(cmd, param=[]):
> +    """Execute a hook if the hook exists."""
> +    if verbose:
> +        sys.stderr.write("Looking for hook: %s\n" % cmd)
> +        sys.stderr.flush()
> +
> +    hooks_path = gitConfig("core.hooksPath")
> +    if len(hooks_path) <= 0:
> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")

This assumes that the process when his function is called (by the
way, even though the title of the patch uses the word "method", this
is not a method but a function, no?), it is always at the top level
of the working tree.  Is that a good assumption?  I don't know the
code well, so "yes it is good because a very early thing we do is to
go up to the top" is a good answer.

> +    hook_file = os.path.join(hooks_path, cmd)
> +    if isinstance(param,basestring):
> +        param=[param]
> +
> +    if platform.system() == 'Windows':
> +        exepath = os.environ.get("EXEPATH")
> +        if exepath is None:
> +            exepath = ""
> +        shexe = os.path.join(exepath, "bin", "sh.exe")
> +        if os.path.isfile(shexe) \
> +            and os.path.isfile(hook_file) \
> +            and os.access(hook_file, os.X_OK) \
> +            and subprocess.call([shexe, hook_file] + param) != 0:
> +            return False
> +
> +    else:
> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file] + param) != 0:
> +            return False

Doesn't this mean that on Windows, a hook MUST be written as a shell
script, but on other platforms, a hook can be any executable?

I am not sure if it is necessary to penalize Windows users this way.
How do other parts of the system run hooks on Windows?  E.g. can
"pre-commit" hook be an executable Python script on Windows?

Even if it is needed to have different implementations (and possibly
reduced capabilities) for "we found this file is a hook, now run it
with these parameters" on different platform, the above looks a bit
inverted.  If the code in this function were

    if os.path.isfile(hook_file) and
       os.access(hook_file, os.X_OK) and
       run_hook_command(hook_file, param) != 0:
	return False
    else:
	return True

and a thin helper function whose only task is "now run it with these
parameters" is separately written, e.g.

    def run_hook_command(hook_file, params):
	if Windows:
		... do things in Windows specific way ...
	else:
		return subprocess.call([hook_file] + param)

That would have been 100% better, as it would have made it clear
that logically gitRunHook() does exactly the same thing on all
platforms (i.e. find where the hook is, normalize param, check if
the hook file is actually enabled, and finally execute the hook with
the param), while the details of how the "execute" part (and only
that part) works may be different.

> +    return True
>  
>  def main():
>      if len(sys.argv[1:]) == 0:

Re: [PATCH v2 4/4] git-p4: add p4 submit hooks

[Junio C Hamano]

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

> From: Ben Keene <seraphire@gmail.com>
>
> The git command "commit" supports a number of hooks that support
> changing the behavior of the commit command.  The git-p4.py program only
> has one existing hook, "p4-p4-pre-submit".  This command occurs early
> in the process.  There are no hooks in the process flow for modifying
> the P4 changelist text programmatically.

OK.  It is unclear to me how the line between 3/4 and 4/4 was
decided.  It makes sense that 2/4 was done as adding the mechanism
to run hooks.  

A natural progression after your 1/4 and 2/4 would have been to add
the "--[no-]verify" mechanism to disable any verification hook in
3/n, possibly rewrite the implementation of existing hooks using the
mechanism you added with 2/4 and 3/n in 4/n, and then either (1) add
and document all new hooks in 5/n, or (2) add and document one new
hook per one step in 5/n, 6/n, 7/n and 8/n (you are adding one in
your 3/4 and then three in your 4/4 here).  As a reviewer I do not
have strong preference either way (i.e. 4 final steps to add 4
hooks, or 1 final step to add 4 hooks), but 3/4 and 4/4 you have
here look harder to reason than necessary, at least to me.

Thanks.

Re: [PATCH v2 2/4] git-p4: create new method gitRunHook

[Ben Keene]

On 2/4/2020 3:40 PM, Junio C Hamano wrote:
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/git-p4.py b/git-p4.py
>> index 7d8a5ee788..4e481b3b55 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -4125,6 +4125,35 @@ def printUsage(commands):
>>       "unshelve" : P4Unshelve,
>>   }
>>   
>> +def gitRunHook(cmd, param=[]):
>> +    """Execute a hook if the hook exists."""
>> +    if verbose:
>> +        sys.stderr.write("Looking for hook: %s\n" % cmd)
>> +        sys.stderr.flush()
>> +
>> +    hooks_path = gitConfig("core.hooksPath")
>> +    if len(hooks_path) <= 0:
>> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
> This assumes that the process when his function is called (by the
> way, even though the title of the patch uses the word "method", this
> is not a method but a function, no?), it is always at the top level
> of the working tree.  Is that a good assumption?  I don't know the
> code well, so "yes it is good because a very early thing we do is to
> go up to the top" is a good answer.
I'm not sure what you mean by top level of the tree unless you mean
that it is not part of a class, but a "Free standing" function? And
yes, it returns a value so it should be called a function. I'll
correct that.  I chose to not put the function within a class so
that if other hooks should be added, it would not require a refactoring
of the code to use the function in other classes.
>> +    hook_file = os.path.join(hooks_path, cmd)
>> +    if isinstance(param,basestring):
>> +        param=[param]
>> +
>> +    if platform.system() == 'Windows':
>> +        exepath = os.environ.get("EXEPATH")
>> +        if exepath is None:
>> +            exepath = ""
>> +        shexe = os.path.join(exepath, "bin", "sh.exe")
>> +        if os.path.isfile(shexe) \
>> +            and os.path.isfile(hook_file) \
>> +            and os.access(hook_file, os.X_OK) \
>> +            and subprocess.call([shexe, hook_file] + param) != 0:
>> +            return False
>> +
>> +    else:
>> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file] + param) != 0:
>> +            return False
> Doesn't this mean that on Windows, a hook MUST be written as a shell
> script, but on other platforms, a hook can be any executable?
Good point.
>
> I am not sure if it is necessary to penalize Windows users this way.
> How do other parts of the system run hooks on Windows?  E.g. can
> "pre-commit" hook be an executable Python script on Windows?

Unfortunately, the original code for running the p4-pre-submit hook
was under-developed and there was no way to run other executable
files in the context of a hook. Nothing ran for me, which is what
prompted this change.  But to your point, the restrictions are
unnecessary. I googled around a little and found these two SO articles:

https://stackoverflow.com/questions/18277429/executing-git-hooks-on-windows

https://stackoverflow.com/questions/22074247/git-hook-under-windows

But I haven't found information on how Git for Windows handles
hooks directly.

>
> Even if it is needed to have different implementations (and possibly
> reduced capabilities) for "we found this file is a hook, now run it
> with these parameters" on different platform, the above looks a bit
> inverted.  If the code in this function were
>
>      if os.path.isfile(hook_file) and
>         os.access(hook_file, os.X_OK) and
>         run_hook_command(hook_file, param) != 0:
> 	return False
>      else:
> 	return True
>
> and a thin helper function whose only task is "now run it with these
> parameters" is separately written, e.g.
>
>      def run_hook_command(hook_file, params):
> 	if Windows:
> 		... do things in Windows specific way ...
> 	else:
> 		return subprocess.call([hook_file] + param)
>
> That would have been 100% better, as it would have made it clear
> that logically gitRunHook() does exactly the same thing on all
> platforms (i.e. find where the hook is, normalize param, check if
> the hook file is actually enabled, and finally execute the hook with
> the param), while the details of how the "execute" part (and only
> that part) works may be different.
I'm committing a new version of this change that will define 
run_hook_command.
>> +    return True
>>   
>>   def main():
>>       if len(sys.argv[1:]) == 0:

Re: [PATCH v2 2/4] git-p4: create new method gitRunHook

[Junio C Hamano]

Ben Keene <seraphire@gmail.com> writes:

>>> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>> This assumes that the process when his function is called (by the
>> way, even though the title of the patch uses the word "method", this
>> is not a method but a function, no?), it is always at the top level
>> of the working tree.  Is that a good assumption?  I don't know the
>> code well, so "yes it is good because a very early thing we do is to
>> go up to the top" is a good answer.
> I'm not sure what you mean by top level of the tree unless you mean
> that it is not part of a class, but a "Free standing" function?

No.  The discussion about function vs method was over immediately
after we left the parentheses ;-)

The "top level of the working tree" is the directory where the files
you see in "git ls-tree $commit^{tree}" would appear in.  In our
project, that is where the primary Makefile, COPYING, Documentation/,
etc. hangs from.

The code in your patch (quoted above) says that "When $GIT_DIR is
not set, '.git/hooks/' is the directory the hooks live in".  That is
true only when your process is at the top level of the working tree.
If you chdir'ed to a subdirectory (e.g. Documentation/ in our
project) and then let the quoted code run, hooks_path is set to
".git/hooks/", but from the point of view of the process running
inside "Documentation/" subdirectory, it should actually be
"../.git/hooks/", right?

> And
> yes, it returns a value so it should be called a function. I'll
> correct that.

This is an irrelevant tangent ;-) but yeah, it is a function, as
opposed to a method, because it is not attached to any class.  I did
not think Python differentiated functions that return a value and
ones that do not (e.g. Pascal call the latter "procedure").

> I chose to not put the function within a class so
> that if other hooks should be added, it would not require a refactoring
> of the code to use the function in other classes.

I think that is a sensible design decision to have a free-standing
function to execute hooks.

Thanks.

Re: [PATCH v2 2/4] git-p4: create new method gitRunHook

[Ben Keene]

On 2/5/2020 4:42 PM, Junio C Hamano wrote:
> Ben Keene <seraphire@gmail.com> writes:
>
>>>> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>>> This assumes that the process when his function is called (by the
>>> way, even though the title of the patch uses the word "method", this
>>> is not a method but a function, no?), it is always at the top level
>>> of the working tree.  Is that a good assumption?  I don't know the
>>> code well, so "yes it is good because a very early thing we do is to
>>> go up to the top" is a good answer.
>> I'm not sure what you mean by top level of the tree unless you mean
>> that it is not part of a class, but a "Free standing" function?
> No.  The discussion about function vs method was over immediately
> after we left the parentheses ;-)
>
> The "top level of the working tree" is the directory where the files
> you see in "git ls-tree $commit^{tree}" would appear in.  In our
> project, that is where the primary Makefile, COPYING, Documentation/,
> etc. hangs from.
>
> The code in your patch (quoted above) says that "When $GIT_DIR is
> not set, '.git/hooks/' is the directory the hooks live in".  That is
> true only when your process is at the top level of the working tree.
> If you chdir'ed to a subdirectory (e.g. Documentation/ in our
> project) and then let the quoted code run, hooks_path is set to
> ".git/hooks/", but from the point of view of the process running
> inside "Documentation/" subdirectory, it should actually be
> "../.git/hooks/", right?

Okay, NOW I get what you meant.  Thank you for the explanation!
The hook directory resolution is what was originally found in the
git-p4.py script.  I just tried to describe what it is doing
because I moved the code around.

As best as I understand the mechanics, and I haven't examined the
source code of git, this is just experimental, the GIT_DIR environment
value is set (at least in Git for Windows) when the program is
executed, so this always has GIT_DIR set, which may be why this
wasn't an issue in the past?

Does this prompt the need to search the hierarchy if we don't find
the directory?

>> And
>> yes, it returns a value so it should be called a function. I'll
>> correct that.
> This is an irrelevant tangent ;-) but yeah, it is a function, as
> opposed to a method, because it is not attached to any class.  I did
> not think Python differentiated functions that return a value and
> ones that do not (e.g. Pascal call the latter "procedure").
>
>> I chose to not put the function within a class so
>> that if other hooks should be added, it would not require a refactoring
>> of the code to use the function in other classes.
> I think that is a sensible design decision to have a free-standing
> function to execute hooks.
>
> Thanks.

[PATCH v3 0/5] git-p4: add hook p4-pre-edit-changelist

[Ben Keene via GitGitGadget]

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.

v1: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.

v2:Instead of a single hook, p4-pre-edit-changelist, follow the git
convention for hook names and add the trio of hooks that work together,
similar to git commit.

The hook names are: 

 * p4-prepare-changelist
 * p4-changelist
 * p4-post-changelist

The hooks should follow the same convention as git commit, so a new command
line option for the git-p4 submit function --no-verify should also be added.

v3:2-Feb-2020 This version reorganizes the commits to be easier to read. The
function for running the git hook has been rewritten to include suggestions.
This version no longer restricts the executable supported by windows for the
hook. It will first look for a file with the hook name without an extension
and if found, it will pass it to sh.exe (Git for Window's MINGW shell)
Otherwise it will take the file with the lowest alphabetical extension and
ask Windows to execute the program.

Ben Keene (5):
  git-p4: rewrite prompt to be Windows compatible
  git-p4: create new function run_git_hook
  git-p4: add --no-verify option
  git-p4: restructure code in submit
  git-p4: add p4 submit hooks

 Documentation/git-p4.txt   |  45 +++++++-
 Documentation/githooks.txt |  51 ++++++++-
 git-p4.py                  | 226 ++++++++++++++++++++++++++++---------
 3 files changed, 264 insertions(+), 58 deletions(-)


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

Range-diff vs v2:

 1:  585bdd51b2 = 1:  8881d76c46 git-p4: rewrite prompt to be Windows compatible
 2:  f1f9fdc542 ! 2:  85918a7edb git-p4: create new method gitRunHook
     @@ -1,22 +1,22 @@
      Author: Ben Keene <seraphire@gmail.com>
      
     -    git-p4: create new method gitRunHook
     +    git-p4: create new function run_git_hook
      
          This commit is in preparation of introducing new p4 submit hooks.
      
          The current code in the python script git-p4.py makes the assumption
     -    that the git hooks can be executed by subprocess.call() method. However,
     -    when git is run on Windows, this may not work as expected.
     +    that the git hooks can be executed by subprocess.call() function.
     +    However, when git is run on Windows, this may not work as expected.
      
     -    The subprocess.call() does not execute SH.EXE implictly under Windows,
     -    so the scripts may fail. In other words, the hooks do not execute under
     -    windows because the shell interpreter is not automatically loaded.
     +    The subprocess.call() does not cover all the use cases for properly
     +    executing the various types of executable files on Windows.
      
     -    Add a new function, gitRunHook, that takes 2 parameters:
     -    * the filename of an optionally registered git hook
     +    Prepare for remediation by adding a new function, run_git_hook, that
     +    takes 2 parameters:
     +    * the short filename of an optionally registered git hook
          * an optional list of parameters
      
     -    The gitRunHook function will honor the existing behavior seen in the
     +    The run_git_hook function will honor the existing behavior seen in the
          current code for executing the p4-pre-submit hook:
      
          * Hooks are looked for in core.hooksPath directory.
     @@ -28,15 +28,32 @@
            return True.
          * If the hook returns a non-zero exit code, the function returns False.
      
     -    Add new conditional behavior for Windows:
     -    * Check for an evironment variable 'EXEPATH' which should be set by
     -      git when git-p4.py is envoked.
     -    * If EXEPATH is None - treat it as an empty string.
     -    * If EXEPATH is set, look for sh.exe in the bin/ directory located
     -      in EXEPATH.
     -    * If EXEPATH is not set, attempt to resolve against "bin/sh.exe"
     -    * Add a new test for Windows that checks to see of sh.exe can be
     -      located. If not, return True.
     +    Add the following additional functionality if git-p4.py is run on
     +    Windows.
     +    * If hook file is not located without an extension, search for
     +      any file in the associated hook directory (from the list above) that
     +      has the same name but with an extension.
     +    * If the file is still not found, return True (the hook is missing)
     +
     +    Add a new function run_hook_command() that wraps the OS dependent
     +    functionality for actually running the subprocess.call() with OS
     +    dependent behavior:
     +
     +    If a hook file exists on Windows:
     +    * If there is no extension, set the launch executable to be SH.EXE
     +      - Look for SH.EXE under the environmental variable EXEPATH in the
     +        bin/ directory.
     +      - If %EXEPATH%/bin/sh.exe exists, use this as the actual executable.
     +      - If %EXEPATH%/bin/sh.exe does not exist, use sh.exe
     +      - Execute subprocess.call() without the shell (shell=False)
     +    * If there is an extension, execute subprocess.call() with teh shell
     +      (shell=True) and consider the file to be the executable.
     +
     +    The return value from run_hook_command() is the subprocess.call()
     +    return value.
     +
     +    These functions are added in this commit, but are only staged and not
     +    yet used.
      
          Signed-off-by: Ben Keene <seraphire@gmail.com>
      
     @@ -44,10 +61,18 @@
       --- a/git-p4.py
       +++ b/git-p4.py
      @@
     -     "unshelve" : P4Unshelve,
     - }
     + import zlib
     + import ctypes
     + import errno
     ++import glob
       
     -+def gitRunHook(cmd, param=[]):
     + # support basestring in python3
     + try:
     +@@
     +         if response in choices:
     +             return response
     + 
     ++def run_git_hook(cmd, param=[]):
      +    """Execute a hook if the hook exists."""
      +    if verbose:
      +        sys.stderr.write("Looking for hook: %s\n" % cmd)
     @@ -57,25 +82,65 @@
      +    if len(hooks_path) <= 0:
      +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
      +
     -+    hook_file = os.path.join(hooks_path, cmd)
     -+    if isinstance(param,basestring):
     ++    if not isinstance(param, list):
      +        param=[param]
      +
     ++    # resolve hook file name, OS depdenent
     ++    hook_file = os.path.join(hooks_path, cmd)
      +    if platform.system() == 'Windows':
     -+        exepath = os.environ.get("EXEPATH")
     -+        if exepath is None:
     -+            exepath = ""
     -+        shexe = os.path.join(exepath, "bin", "sh.exe")
     -+        if os.path.isfile(shexe) \
     -+            and os.path.isfile(hook_file) \
     -+            and os.access(hook_file, os.X_OK) \
     -+            and subprocess.call([shexe, hook_file] + param) != 0:
     -+            return False
     ++        if not os.path.isfile(hook_file):
     ++            # look for the file with an extension
     ++            files = glob.glob(hook_file + ".*")
     ++            if not files:
     ++                return True
     ++            files.sort()
     ++            hook_file = files[0]
     ++
     ++    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
     ++        return True
      +
     -+    else:
     -+        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file] + param) != 0:
     -+            return False
     -+    return True
     ++    return run_hook_command(hook_file, param) == 0
     ++
     ++def run_hook_command(cmd, param):
     ++    """Executes a git hook command
     ++       cmd = the command line file to be executed. This can be
     ++       a file that is run by OS association.
     ++
     ++       param = a list of parameters to pass to the cmd command
     ++
     ++       On windows, the extension is checked to see if it should
     ++       be run with the Git for Windows Bash shell.  If there
     ++       is no file extension, the file is deemed a bash shell
     ++       and will be handed off to sh.exe. Otherwise, Windows
     ++       will be called with the shell to handle the file assocation.
     ++
     ++       For non Windows operating systems, the file is called
     ++       as an executable.
     ++    """
     ++    cli = [cmd] + param
     ++    use_shell = False
     ++    if platform.system() == 'Windows':
     ++        (root,ext) = os.path.splitext(cmd)
     ++        if ext == "":
     ++            exe_path = os.environ.get("EXEPATH")
     ++            if exe_path is None:
     ++                exe_path = ""
     ++            else:
     ++                exe_path = os.path.join(exe_path, "bin")
     ++            cli = [os.path.join(exe_path, "SH.EXE")] + cli
     ++        else:
     ++            use_shell = True
     ++    return subprocess.call(cli, shell=use_shell)
     ++
     ++
     + def write_pipe(c, stdin):
     +     if verbose:
     +         sys.stderr.write('Writing pipe: %s\n' % str(c))
     +@@
     +     "unshelve" : P4Unshelve,
     + }
       
     +-
       def main():
           if len(sys.argv[1:]) == 0:
     +         printUsage(commands.keys())
 -:  ---------- > 3:  b538ad08b6 git-p4: add --no-verify option
 3:  1bdcdc4c18 ! 4:  662b864d70 git-p4: add hook p4-pre-edit-changelist
     @@ -1,23 +1,38 @@
      Author: Ben Keene <seraphire@gmail.com>
      
     -    git-p4: add hook p4-pre-edit-changelist
     +    git-p4: restructure code in submit
      
     -    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.
     +    In preparation for adding new hooks to the submit method of git-p4,
     +    restructure the applyCommit function in the P4Submit class.  Make the
     +    following changes:
      
     -    This hook will be called prior to checking for the flag
     -    "--prepare-p4-only".
     +    * Move all the code after the definition of submitted = False into the
     +      Try-Finally block. This ensures that any error that occurs will
     +      properly recover.  This is not necessary with the current code because
     +      none of it should throw an exception, however the next set of changes
     +      will introduce exceptional code.
      
     -    The hook is optional, if it does not exist, it will be skipped.
     +      Existing flow control can remain as defined - the if-block for
     +      prepare-p4-only sets the local variable "submitted" to True and exits
     +      the function. New early exits, leave submitted set to False so the
     +      Finally block will undo changes to the P4 workspace.
      
     -    The hook takes a single parameter, the filename of the temporary file
     -    that contains the P4 submit text.
     +    * Make the small usability change of adding an empty string to the
     +      print statements displayed to the user when the prepare-p4-only option
     +      is selected.  On Windows, the command print() may display a set of
     +      parentheses "()" to the user when the print() function is called with
     +      no parameters. By supplying an empty string, the intended blank line
     +      will print as expected.
      
     -    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.
     +    * Fix a small bug when the submittedTemplate is edited by the user
     +      and all content in the file is removed. The existing code will throw
     +      an exception if the separateLine is not found in the file. Change this
     +      code to test for the separator line using a find() test first and only
     +      split on the separator if it is found.
     +
     +    * Additionally, add the new behavior that if the changelist file has
     +      been completely emptied that the Submit action for this changelist
     +      will be aborted.
      
          Signed-off-by: Ben Keene <seraphire@gmail.com>
      
     @@ -28,17 +43,126 @@
               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")
     +-        if self.prepare_p4_only:
     +-            #
     +-            # Leave the p4 tree prepared, and the submit template around
     +-            # and let the user decide what to do next
     +-            #
     +-            print()
     +-            print("P4 workspace prepared for submission.")
     +-            print("To submit or revert, go to client workspace")
     +-            print("  " + self.clientPath)
     +-            print()
     +-            print("To submit, use \"p4 submit\" to write a new description,")
     +-            print("or \"p4 submit -i <%s\" to use the one prepared by" \
     +-                  " \"git p4\"." % fileName)
     +-            print("You can delete the file \"%s\" when finished." % fileName)
     +-
     +-            if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
     +-                print("To preserve change ownership by user %s, you must\n" \
     +-                      "do \"p4 change -f <change>\" after submitting and\n" \
     +-                      "edit the User field.")
     +-            if pureRenameCopy:
     +-                print("After submitting, renamed files must be re-synced.")
     +-                print("Invoke \"p4 sync -f\" on each of these files:")
     +-                for f in pureRenameCopy:
     +-                    print("  " + f)
     +-
     +-            print()
     +-            print("To revert the changes, use \"p4 revert ...\", and delete")
     +-            print("the submit template file \"%s\"" % fileName)
     +-            if filesToAdd:
     +-                print("Since the commit adds new files, they must be deleted:")
     +-                for f in filesToAdd:
     +-                    print("  " + f)
     +-            print()
     +-            return True
     +-
     +-        #
     +-        # Let the user edit the change description, then submit it.
     +-        #
     +         submitted = False
     + 
     +         try:
     ++
     ++            if self.prepare_p4_only:
     ++                #
     ++                # Leave the p4 tree prepared, and the submit template around
     ++                # and let the user decide what to do next
     ++                #
     ++                submitted = True
     ++                print("")
     ++                print("P4 workspace prepared for submission.")
     ++                print("To submit or revert, go to client workspace")
     ++                print("  " + self.clientPath)
     ++                print("")
     ++                print("To submit, use \"p4 submit\" to write a new description,")
     ++                print("or \"p4 submit -i <%s\" to use the one prepared by" \
     ++                      " \"git p4\"." % fileName)
     ++                print("You can delete the file \"%s\" when finished." % fileName)
      +
     -+        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.preserveUser and p4User and not self.p4UserIsMe(p4User):
     ++                    print("To preserve change ownership by user %s, you must\n" \
     ++                          "do \"p4 change -f <change>\" after submitting and\n" \
     ++                          "edit the User field.")
     ++                if pureRenameCopy:
     ++                    print("After submitting, renamed files must be re-synced.")
     ++                    print("Invoke \"p4 sync -f\" on each of these files:")
     ++                    for f in pureRenameCopy:
     ++                        print("  " + f)
     ++
     ++                print("")
     ++                print("To revert the changes, use \"p4 revert ...\", and delete")
     ++                print("the submit template file \"%s\"" % fileName)
     ++                if filesToAdd:
     ++                    print("Since the commit adds new files, they must be deleted:")
     ++                    for f in filesToAdd:
     ++                        print("  " + f)
     ++                print("")
     ++                sys.stdout.flush()
     ++                return True
     ++
     +             if self.edit_template(fileName):
     +                 # read the edited message and submit
     +                 tmpFile = open(fileName, "rb")
     +@@
     +                 tmpFile.close()
     +                 if self.isWindows:
     +                     message = message.replace("\r\n", "\n")
     +-                submitTemplate = message[:message.index(separatorLine)]
     ++                if message.find(separatorLine) <> -1:
     ++                    submitTemplate = message[:message.index(separatorLine)]
     ++                else:
     ++                    submitTemplate = message
      +
     -         if self.prepare_p4_only:
     -             #
     -             # Leave the p4 tree prepared, and the submit template around
     ++                if len(submitTemplate.strip()) == 0:
     ++                    print("Changelist is empty, aborting this changelist.")
     ++                    sys.stdout.flush()
     ++                    return False
     + 
     +                 if update_shelve:
     +                     p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)
     +@@
     +                 submitted = True
     + 
     +         finally:
     +-            # skip this patch
     ++            # Revert changes if we skip this patch
     +             if not submitted or self.shelve:
     +                 if self.shelve:
     +                     print ("Reverting shelved files.")
     +                 else:
     +                     print ("Submission cancelled, undoing p4 changes.")
     ++                sys.stdout.flush()
     +                 for f in editedFiles | filesToDelete:
     +                     p4_revert(f)
     +                 for f in filesToAdd:
     +                     p4_revert(f)
     +                     os.remove(f)
     + 
     +-        os.remove(fileName)
     ++            if not self.prepare_p4_only:
     ++                os.remove(fileName)
     +         return submitted
     + 
     +     # Export git tags as p4 labels. Create a p4 label and then tag
 4:  3d34dd042e ! 5:  e34c377066 git-p4: add p4 submit hooks
     @@ -4,19 +4,11 @@
      
          The git command "commit" supports a number of hooks that support
          changing the behavior of the commit command.  The git-p4.py program only
     -    has one existing hook, "p4-p4-pre-submit".  This command occurs early
     -    in the process.  There are no hooks in the process flow for modifying
     +    has one existing hook, "p4-pre-submit".  This command occurs early in
     +    the process.  There are no hooks in the process flow for modifying
          the P4 changelist text programmatically.
      
     -    Adds 3 new hooks and 1 new option to git-p4.py to the submit option.
     -
     -    The new command line option --no-verify:
     -
     -    Add a new command line option "--no-verify" to the Submit command of
     -    git-p4.py.  This option will function in the spirit of the existing
     -    --no-verify command line option found in git commit. It will cause the
     -    P4 Submit function to ignore the existing p4-pre-submit and the new
     -    p4-changelist hooks.
     +    Adds 3 new hooks to git-p4.py to the submit option.
      
          The new hooks are:
          * p4-prepare-changelist - Execute this hook after the changelist file
     @@ -34,62 +26,16 @@
            executed regardless of the --no-verify option.  It's return value will
            not be checked.
      
     -    Change the execution of the existing trigger p4-pre-submit to honor the
     -    --no-verify option. Before exiting on failure of this hook, display
     -    text to the user explaining which hook has failed and the impact
     -    of using the --no-verify option.
     -
     -    Move all the code after creating the submitTemplate text into the
     -    try-finally block in the applyCommit() method.  By restructuring the
     -    code, the P4 workspace is protected against otherwise unhandled
     -    exceptions and other early exits.
     -
          The calls to the new hooks: p4-prepare-changelist, p4-changelist,
          and p4-post-changelist should all be called inside the try-finally
          block.
      
     -    Existing flow control can remain as defined - the if-block for
     -    prepare-p4-only sets the local variable "submitted" to True and exits
     -    the function. All other early exits, leave submitted set to False so the
     -    Finally block will undo changes to the P4 workspace.
     -
     -    Make the small usability change of adding an empty string to the
     -    print statements displayed to the user when the prepare-p4-only option
     -    is selected.  On Windows, the command print() may display a set of
     -    parentheses () to the user when the print() function is called with no
     -    parameters. By supplying an empty string, the intended blank line will
     -    print as expected.
     -
     -    Fix a small bug when the submittedTemplate is edited by the user
     -    and all content in the file is removed. The existing code will throw
     -    an exception if the separateLine is not found in the file.  Change this
     -    code to test for the separator line using a find() test first and only
     -    split on the separator if it is found.
     -
     -    Additionally, add the new behavior that if the changelist file has been
     -    completely emptied that the Submit action for this changelist will be
     -    aborted.
     -
          Signed-off-by: Ben Keene <seraphire@gmail.com>
      
       diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
       --- a/Documentation/git-p4.txt
       +++ b/Documentation/git-p4.txt
      @@
     -     been submitted. Implies --disable-rebase. Can also be set with
     -     git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
     - 
     --Hook for submit
     --~~~~~~~~~~~~~~~
     -+Hooks for submit
     -+----------------
     -+
     -+p4-pre-submit
     -+~~~~~~~~~~~~~
     -+
     - The `p4-pre-submit` hook is executed if it exists and is executable.
     - The hook takes no parameters and nothing from standard input. Exiting with
     - non-zero status from this script prevents `git-p4 submit` from launching.
       
       One usage scenario is to run unit tests in the hook.
       
     @@ -128,10 +74,9 @@
      +git p4 submit action.
      +
      +
     -+
     + 
       Rebase options
       ~~~~~~~~~~~~~~
     - These options can be used to modify 'git p4 rebase' behavior.
      
       diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
       --- a/Documentation/githooks.txt
     @@ -194,24 +139,18 @@
       --- a/git-p4.py
       +++ b/git-p4.py
      @@
     -                                      "work from a local git branch that is not master"),
                       optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                            help="Skip Perforce sync of p4/master after submit or shelve"),
     -+                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
     +                 optparse.make_option("--no-verify", dest="no_verify", action="store_true",
     +-                                     help="Bypass p4-pre-submit"),
      +                                     help="Bypass p4-pre-submit and p4-changelist hooks"),
               ]
               self.description = """Submit changes from git to the perforce depot.\n
     --    The `p4-pre-submit` hook is executed if it exists and is executable.
     --    The hook takes no parameters and nothing from standard input. Exiting with
     --    non-zero status from this script prevents `git-p4 submit` from launching.
     --
     --    One usage scenario is to run unit tests in the hook."""
     -+    The `p4-pre-submit` hook is executed if it exists and is executable. It
     -+    can be bypassed with the `--no-verify` command line option. The hook takes
     -+    no parameters and nothing from standard input. Exiting with a non-zero status
     -+    from this script prevents `git-p4 submit` from launching.
     -+
     -+    One usage scenario is to run unit tests in the hook.
     +     The `p4-pre-submit` hook is executed if it exists and is executable. It
     +@@
     +     from this script prevents `git-p4 submit` from launching.
     + 
     +     One usage scenario is to run unit tests in the hook.
      +
      +    The `p4-prepare-changelist` hook is executed right after preparing the default
      +    changelist message and before the editor is started. It takes one parameter,
     @@ -234,137 +173,26 @@
      +    The `p4-post-changelist` hook is invoked after the submit has successfully
      +    occured in P4. It takes no parameters and is meant primarily for notification
      +    and cannot affect the outcome of the git p4 submit action.
     -+    """
     +     """
       
               self.usage += " [name of git branch to submit into perforce depot]"
     -         self.origin = ""
      @@
     -         self.exportLabels = False
     -         self.p4HasMoveCommand = p4_has_move_command()
     -         self.branch = None
     -+        self.no_verify = False
     - 
     -         if gitConfig('git-p4.largeFileSystem'):
     -             die("Large file system not supported for git-p4 submit command. Please remove it from config.")
     -@@
     -         applyPatchCmd = patchcmd + "--check --apply -"
     -         patch_succeeded = True
     - 
     -+        if verbose:
     -+            print("TryPatch: %s" % tryPatchCmd)
     -+
     -         if os.system(tryPatchCmd) != 0:
     -             fixed_rcs_keywords = False
     -             patch_succeeded = False
     -@@
     -                 print("Retrying the patch with RCS keywords cleaned up")
     -                 if os.system(tryPatchCmd) == 0:
     -                     patch_succeeded = True
     -+                    print("Patch succeesed this time")
     - 
     -         if not patch_succeeded:
     -             for f in editedFiles:
     -@@
     -         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
     --            # and let the user decide what to do next
     --            #
     --            print()
     --            print("P4 workspace prepared for submission.")
     --            print("To submit or revert, go to client workspace")
     --            print("  " + self.clientPath)
     --            print()
     --            print("To submit, use \"p4 submit\" to write a new description,")
     --            print("or \"p4 submit -i <%s\" to use the one prepared by" \
     --                  " \"git p4\"." % fileName)
     --            print("You can delete the file \"%s\" when finished." % fileName)
     --
     --            if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
     --                print("To preserve change ownership by user %s, you must\n" \
     --                      "do \"p4 change -f <change>\" after submitting and\n" \
     --                      "edit the User field.")
     --            if pureRenameCopy:
     --                print("After submitting, renamed files must be re-synced.")
     --                print("Invoke \"p4 sync -f\" on each of these files:")
     --                for f in pureRenameCopy:
     --                    print("  " + f)
     --
     --            print()
     --            print("To revert the changes, use \"p4 revert ...\", and delete")
     --            print("the submit template file \"%s\"" % fileName)
     --            if filesToAdd:
     --                print("Since the commit adds new files, they must be deleted:")
     --                for f in filesToAdd:
     --                    print("  " + f)
     --            print()
     --            return True
     --
     --        #
     --        # Let the user edit the change description, then submit it.
     --        #
               submitted = False
       
               try:
      +            # Allow the hook to edit the changelist text before presenting it
      +            # to the user.
     -+            if not gitRunHook("p4-prepare-changelist", [fileName]):
     ++            if not run_git_hook("p4-prepare-changelist", [fileName]):
      +                return False
     -+
     -+            if self.prepare_p4_only:
     -+                #
     -+                # Leave the p4 tree prepared, and the submit template around
     -+                # and let the user decide what to do next
     -+                #
     -+                submitted = True
     -+                print("")
     -+                print("P4 workspace prepared for submission.")
     -+                print("To submit or revert, go to client workspace")
     -+                print("  " + self.clientPath)
     -+                print("")
     -+                print("To submit, use \"p4 submit\" to write a new description,")
     -+                print("or \"p4 submit -i <%s\" to use the one prepared by" \
     -+                      " \"git p4\"." % fileName)
     -+                print("You can delete the file \"%s\" when finished." % fileName)
     -+
     -+                if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
     -+                    print("To preserve change ownership by user %s, you must\n" \
     -+                          "do \"p4 change -f <change>\" after submitting and\n" \
     -+                          "edit the User field.")
     -+                if pureRenameCopy:
     -+                    print("After submitting, renamed files must be re-synced.")
     -+                    print("Invoke \"p4 sync -f\" on each of these files:")
     -+                    for f in pureRenameCopy:
     -+                        print("  " + f)
     -+
     -+                print("")
     -+                print("To revert the changes, use \"p4 revert ...\", and delete")
     -+                print("the submit template file \"%s\"" % fileName)
     -+                if filesToAdd:
     -+                    print("Since the commit adds new files, they must be deleted:")
     -+                    for f in filesToAdd:
     -+                        print("  " + f)
     -+                print("")
     -+                sys.stdout.flush()
     -+                return True
     -+
     + 
     +             if self.prepare_p4_only:
     +                 #
     +@@
     +                 return True
     + 
                   if self.edit_template(fileName):
      +                if not self.no_verify:
     -+                    if not gitRunHook("p4-changelist", [fileName]):
     ++                    if not run_git_hook("p4-changelist", [fileName]):
      +                        print("The p4-changelist hook failed.")
      +                        sys.stdout.flush()
      +                        return False
     @@ -372,64 +200,11 @@
                       # read the edited message and submit
                       tmpFile = open(fileName, "rb")
                       message = tmpFile.read()
     -                 tmpFile.close()
     -                 if self.isWindows:
     -                     message = message.replace("\r\n", "\n")
     --                submitTemplate = message[:message.index(separatorLine)]
     -+                if message.find(separatorLine) <> -1:
     -+                    submitTemplate = message[:message.index(separatorLine)]
     -+                else:
     -+                    submitTemplate = message
     -+
     -+                if len(submitTemplate.strip()) == 0:
     -+                    print("Changelist is empty, aborting this changelist.")
     -+                    sys.stdout.flush()
     -+                    return False
     - 
     -                 if update_shelve:
     -                     p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)
      @@
       
                       submitted = True
       
     -+                gitRunHook("p4-post-changelist")
     ++                run_git_hook("p4-post-changelist")
               finally:
     --            # skip this patch
     -+            # Revert changes if we skip this patch
     +             # Revert changes if we skip this patch
                   if not submitted or self.shelve:
     -                 if self.shelve:
     -                     print ("Reverting shelved files.")
     -                 else:
     -                     print ("Submission cancelled, undoing p4 changes.")
     -+                sys.stdout.flush()
     -                 for f in editedFiles | filesToDelete:
     -                     p4_revert(f)
     -                 for f in filesToAdd:
     -                     p4_revert(f)
     -                     os.remove(f)
     - 
     --        os.remove(fileName)
     -+                os.remove(fileName)
     -         return submitted
     - 
     -     # Export git tags as p4 labels. Create a p4 label and then tag
     -@@
     -             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
     -                      (len(commits), num_shelves))
     - 
     --        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-submit")
     --        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
     --            sys.exit(1)
     -+        if not self.no_verify:
     -+            if not gitRunHook("p4-pre-submit"):
     -+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
     -+                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
     -+                    "however,\nthis will also skip the p4-changelist hook as well.")
     -+                sys.exit(1)
     - 
     -         #
     -         # Apply the commits, one at a time.  On failure, ask if should

-- 
gitgitgadget

[PATCH v3 1/5] git-p4: rewrite prompt to be Windows compatible

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The existing function prompt(prompt_text) does not work correctly when
run on Windows 10 bash terminal when launched from the sourcetree
GUI application. The stdout is not flushed properly so the prompt text
is not displayed to the user until the next flush of stdout, which is
quite confusing.

Change this method by:
* Adding flush to stderr, stdout, and stdin
* Use readline from sys.stdin instead of raw_input.

The existing strip().lower() are retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 40d9e7c594..7d8a5ee788 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -175,7 +175,11 @@ def prompt(prompt_text):
     """
     choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))
     while True:
-        response = raw_input(prompt_text).strip().lower()
+        sys.stderr.flush()
+        sys.stdout.write(prompt_text)
+        sys.stdout.flush()
+        sys.stdin.flush()
+        response=sys.stdin.readline().strip().lower()
         if not response:
             continue
         response = response[0]
-- 
gitgitgadget

[PATCH v3 2/5] git-p4: create new function run_git_hook

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

This commit is in preparation of introducing new p4 submit hooks.

The current code in the python script git-p4.py makes the assumption
that the git hooks can be executed by subprocess.call() function.
However, when git is run on Windows, this may not work as expected.

The subprocess.call() does not cover all the use cases for properly
executing the various types of executable files on Windows.

Prepare for remediation by adding a new function, run_git_hook, that
takes 2 parameters:
* the short filename of an optionally registered git hook
* an optional list of parameters

The run_git_hook function will honor the existing behavior seen in the
current code for executing the p4-pre-submit hook:

* Hooks are looked for in core.hooksPath directory.
* If core.hooksPath is not set, then the current .git/hooks directory
  is checked.
* If the hook does not exist, the function returns True.
* If the hook file is not accessible, the function returns True.
* If the hook returns a zero exit code when executed, the function
  return True.
* If the hook returns a non-zero exit code, the function returns False.

Add the following additional functionality if git-p4.py is run on
Windows.
* If hook file is not located without an extension, search for
  any file in the associated hook directory (from the list above) that
  has the same name but with an extension.
* If the file is still not found, return True (the hook is missing)

Add a new function run_hook_command() that wraps the OS dependent
functionality for actually running the subprocess.call() with OS
dependent behavior:

If a hook file exists on Windows:
* If there is no extension, set the launch executable to be SH.EXE
  - Look for SH.EXE under the environmental variable EXEPATH in the
    bin/ directory.
  - If %EXEPATH%/bin/sh.exe exists, use this as the actual executable.
  - If %EXEPATH%/bin/sh.exe does not exist, use sh.exe
  - Execute subprocess.call() without the shell (shell=False)
* If there is an extension, execute subprocess.call() with teh shell
  (shell=True) and consider the file to be the executable.

The return value from run_hook_command() is the subprocess.call()
return value.

These functions are added in this commit, but are only staged and not
yet used.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 7d8a5ee788..d4c39f112b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -26,6 +26,7 @@
 import zlib
 import ctypes
 import errno
+import glob
 
 # support basestring in python3
 try:
@@ -186,6 +187,67 @@ def prompt(prompt_text):
         if response in choices:
             return response
 
+def run_git_hook(cmd, param=[]):
+    """Execute a hook if the hook exists."""
+    if verbose:
+        sys.stderr.write("Looking for hook: %s\n" % cmd)
+        sys.stderr.flush()
+
+    hooks_path = gitConfig("core.hooksPath")
+    if len(hooks_path) <= 0:
+        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
+
+    if not isinstance(param, list):
+        param=[param]
+
+    # resolve hook file name, OS depdenent
+    hook_file = os.path.join(hooks_path, cmd)
+    if platform.system() == 'Windows':
+        if not os.path.isfile(hook_file):
+            # look for the file with an extension
+            files = glob.glob(hook_file + ".*")
+            if not files:
+                return True
+            files.sort()
+            hook_file = files[0]
+
+    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
+        return True
+
+    return run_hook_command(hook_file, param) == 0
+
+def run_hook_command(cmd, param):
+    """Executes a git hook command
+       cmd = the command line file to be executed. This can be
+       a file that is run by OS association.
+
+       param = a list of parameters to pass to the cmd command
+
+       On windows, the extension is checked to see if it should
+       be run with the Git for Windows Bash shell.  If there
+       is no file extension, the file is deemed a bash shell
+       and will be handed off to sh.exe. Otherwise, Windows
+       will be called with the shell to handle the file assocation.
+
+       For non Windows operating systems, the file is called
+       as an executable.
+    """
+    cli = [cmd] + param
+    use_shell = False
+    if platform.system() == 'Windows':
+        (root,ext) = os.path.splitext(cmd)
+        if ext == "":
+            exe_path = os.environ.get("EXEPATH")
+            if exe_path is None:
+                exe_path = ""
+            else:
+                exe_path = os.path.join(exe_path, "bin")
+            cli = [os.path.join(exe_path, "SH.EXE")] + cli
+        else:
+            use_shell = True
+    return subprocess.call(cli, shell=use_shell)
+
+
 def write_pipe(c, stdin):
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))
@@ -4125,7 +4187,6 @@ def printUsage(commands):
     "unshelve" : P4Unshelve,
 }
 
-
 def main():
     if len(sys.argv[1:]) == 0:
         printUsage(commands.keys())
-- 
gitgitgadget

[PATCH v3 3/5] git-p4: add --no-verify option

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

Add new command line option --no-verify:

Add a new command line option "--no-verify" to the Submit command of
git-p4.py.  This option will function in the spirit of the existing
--no-verify command line option found in git commit. It will cause the
P4 Submit function to ignore the existing p4-pre-submit.

Change the execution of the existing trigger p4-pre-submit to honor the
--no-verify option. Before exiting on failure of this hook, display
text to the user explaining which hook has failed and the impact
of using the --no-verify option.

Change the call of the p4-pre-submit hook to use the new run_git_hook
function. This is in preparation of additional hooks to be added.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   | 10 ++++++++--
 Documentation/githooks.txt |  5 ++++-
 git-p4.py                  | 30 +++++++++++++++++++-----------
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3494a1db3e..362b50eb21 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -374,14 +374,20 @@ These options can be used to modify 'git p4 submit' behavior.
     been submitted. Implies --disable-rebase. Can also be set with
     git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
 
-Hook for submit
-~~~~~~~~~~~~~~~
+Hooks for submit
+----------------
+
+p4-pre-submit
+~~~~~~~~~~~~~
+
 The `p4-pre-submit` hook is executed if it exists and is executable.
 The hook takes no parameters and nothing from standard input. Exiting with
 non-zero status from this script prevents `git-p4 submit` from launching.
+It can be bypassed with the `--no-verify` command line option.
 
 One usage scenario is to run unit tests in the hook.
 
+
 Rebase options
 ~~~~~~~~~~~~~~
 These options can be used to modify 'git p4 rebase' behavior.
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 50365f2914..8cf6b08b55 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -520,7 +520,10 @@ p4-pre-submit
 
 This hook is invoked by `git-p4 submit`. It takes no parameters and nothing
 from standard input. Exiting with non-zero status from this script prevent
-`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
+`git-p4 submit` from launching. It can be bypassed with the `--no-verify`
+command line option. Run `git-p4 submit --help` for details.
+
+
 
 post-index-change
 ~~~~~~~~~~~~~~~~~
diff --git a/git-p4.py b/git-p4.py
index d4c39f112b..b377484464 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1583,13 +1583,17 @@ def __init__(self):
                                      "work from a local git branch that is not master"),
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
+                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
+                                     help="Bypass p4-pre-submit"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
-    The `p4-pre-submit` hook is executed if it exists and is executable.
-    The hook takes no parameters and nothing from standard input. Exiting with
-    non-zero status from this script prevents `git-p4 submit` from launching.
+    The `p4-pre-submit` hook is executed if it exists and is executable. It
+    can be bypassed with the `--no-verify` command line option. The hook takes
+    no parameters and nothing from standard input. Exiting with a non-zero status
+    from this script prevents `git-p4 submit` from launching.
 
-    One usage scenario is to run unit tests in the hook."""
+    One usage scenario is to run unit tests in the hook.
+    """
 
         self.usage += " [name of git branch to submit into perforce depot]"
         self.origin = ""
@@ -1607,6 +1611,7 @@ def __init__(self):
         self.exportLabels = False
         self.p4HasMoveCommand = p4_has_move_command()
         self.branch = None
+        self.no_verify = False
 
         if gitConfig('git-p4.largeFileSystem'):
             die("Large file system not supported for git-p4 submit command. Please remove it from config.")
@@ -1993,6 +1998,9 @@ def applyCommit(self, id):
         applyPatchCmd = patchcmd + "--check --apply -"
         patch_succeeded = True
 
+        if verbose:
+            print("TryPatch: %s" % tryPatchCmd)
+
         if os.system(tryPatchCmd) != 0:
             fixed_rcs_keywords = False
             patch_succeeded = False
@@ -2032,6 +2040,7 @@ def applyCommit(self, id):
                 print("Retrying the patch with RCS keywords cleaned up")
                 if os.system(tryPatchCmd) == 0:
                     patch_succeeded = True
+                    print("Patch succeesed this time")
 
         if not patch_succeeded:
             for f in editedFiles:
@@ -2400,13 +2409,12 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        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-submit")
-        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
-            sys.exit(1)
+        if not self.no_verify:
+            if not run_git_hook("p4-pre-submit"):
+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
+                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
+                    "however,\nthis will also skip the p4-changelist hook as well.")
+                sys.exit(1)
 
         #
         # Apply the commits, one at a time.  On failure, ask if should
-- 
gitgitgadget

[PATCH v3 4/5] git-p4: restructure code in submit

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

In preparation for adding new hooks to the submit method of git-p4,
restructure the applyCommit function in the P4Submit class.  Make the
following changes:

* Move all the code after the definition of submitted = False into the
  Try-Finally block. This ensures that any error that occurs will
  properly recover.  This is not necessary with the current code because
  none of it should throw an exception, however the next set of changes
  will introduce exceptional code.

  Existing flow control can remain as defined - the if-block for
  prepare-p4-only sets the local variable "submitted" to True and exits
  the function. New early exits, leave submitted set to False so the
  Finally block will undo changes to the P4 workspace.

* Make the small usability change of adding an empty string to the
  print statements displayed to the user when the prepare-p4-only option
  is selected.  On Windows, the command print() may display a set of
  parentheses "()" to the user when the print() function is called with
  no parameters. By supplying an empty string, the intended blank line
  will print as expected.

* Fix a small bug when the submittedTemplate is edited by the user
  and all content in the file is removed. The existing code will throw
  an exception if the separateLine is not found in the file. Change this
  code to test for the separator line using a find() test first and only
  split on the separator if it is found.

* Additionally, add the new behavior that if the changelist file has
  been completely emptied that the Submit action for this changelist
  will be aborted.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 92 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index b377484464..1d15ea7464 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2101,47 +2101,47 @@ def applyCommit(self, id):
         tmpFile.write(submitTemplate)
         tmpFile.close()
 
-        if self.prepare_p4_only:
-            #
-            # Leave the p4 tree prepared, and the submit template around
-            # and let the user decide what to do next
-            #
-            print()
-            print("P4 workspace prepared for submission.")
-            print("To submit or revert, go to client workspace")
-            print("  " + self.clientPath)
-            print()
-            print("To submit, use \"p4 submit\" to write a new description,")
-            print("or \"p4 submit -i <%s\" to use the one prepared by" \
-                  " \"git p4\"." % fileName)
-            print("You can delete the file \"%s\" when finished." % fileName)
-
-            if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
-                print("To preserve change ownership by user %s, you must\n" \
-                      "do \"p4 change -f <change>\" after submitting and\n" \
-                      "edit the User field.")
-            if pureRenameCopy:
-                print("After submitting, renamed files must be re-synced.")
-                print("Invoke \"p4 sync -f\" on each of these files:")
-                for f in pureRenameCopy:
-                    print("  " + f)
-
-            print()
-            print("To revert the changes, use \"p4 revert ...\", and delete")
-            print("the submit template file \"%s\"" % fileName)
-            if filesToAdd:
-                print("Since the commit adds new files, they must be deleted:")
-                for f in filesToAdd:
-                    print("  " + f)
-            print()
-            return True
-
-        #
-        # Let the user edit the change description, then submit it.
-        #
         submitted = False
 
         try:
+
+            if self.prepare_p4_only:
+                #
+                # Leave the p4 tree prepared, and the submit template around
+                # and let the user decide what to do next
+                #
+                submitted = True
+                print("")
+                print("P4 workspace prepared for submission.")
+                print("To submit or revert, go to client workspace")
+                print("  " + self.clientPath)
+                print("")
+                print("To submit, use \"p4 submit\" to write a new description,")
+                print("or \"p4 submit -i <%s\" to use the one prepared by" \
+                      " \"git p4\"." % fileName)
+                print("You can delete the file \"%s\" when finished." % fileName)
+
+                if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
+                    print("To preserve change ownership by user %s, you must\n" \
+                          "do \"p4 change -f <change>\" after submitting and\n" \
+                          "edit the User field.")
+                if pureRenameCopy:
+                    print("After submitting, renamed files must be re-synced.")
+                    print("Invoke \"p4 sync -f\" on each of these files:")
+                    for f in pureRenameCopy:
+                        print("  " + f)
+
+                print("")
+                print("To revert the changes, use \"p4 revert ...\", and delete")
+                print("the submit template file \"%s\"" % fileName)
+                if filesToAdd:
+                    print("Since the commit adds new files, they must be deleted:")
+                    for f in filesToAdd:
+                        print("  " + f)
+                print("")
+                sys.stdout.flush()
+                return True
+
             if self.edit_template(fileName):
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
@@ -2149,7 +2149,15 @@ def applyCommit(self, id):
                 tmpFile.close()
                 if self.isWindows:
                     message = message.replace("\r\n", "\n")
-                submitTemplate = message[:message.index(separatorLine)]
+                if message.find(separatorLine) <> -1:
+                    submitTemplate = message[:message.index(separatorLine)]
+                else:
+                    submitTemplate = message
+
+                if len(submitTemplate.strip()) == 0:
+                    print("Changelist is empty, aborting this changelist.")
+                    sys.stdout.flush()
+                    return False
 
                 if update_shelve:
                     p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)
@@ -2173,19 +2181,21 @@ def applyCommit(self, id):
                 submitted = True
 
         finally:
-            # skip this patch
+            # Revert changes if we skip this patch
             if not submitted or self.shelve:
                 if self.shelve:
                     print ("Reverting shelved files.")
                 else:
                     print ("Submission cancelled, undoing p4 changes.")
+                sys.stdout.flush()
                 for f in editedFiles | filesToDelete:
                     p4_revert(f)
                 for f in filesToAdd:
                     p4_revert(f)
                     os.remove(f)
 
-        os.remove(fileName)
+            if not self.prepare_p4_only:
+                os.remove(fileName)
         return submitted
 
     # Export git tags as p4 labels. Create a p4 label and then tag
-- 
gitgitgadget

[PATCH v3 5/5] git-p4: add p4 submit hooks

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The git command "commit" supports a number of hooks that support
changing the behavior of the commit command.  The git-p4.py program only
has one existing hook, "p4-pre-submit".  This command occurs early in
the process.  There are no hooks in the process flow for modifying
the P4 changelist text programmatically.

Adds 3 new hooks to git-p4.py to the submit option.

The new hooks are:
* p4-prepare-changelist - Execute this hook after the changelist file
  has been created. The hook will be executed even if the
  --prepare-p4-only option is set.  This hook ignores the --no-verify
  option in keeping with the existing behavior of git commit.

* p4-changelist - Execute this hook after the user has edited the
  changelist. Do not execute this hook if the user has selected the
  --prepare-p4-only option. This hook will honor the --no-verify,
  following the conventions of git commit.

* p4-post-changelist - Execute this hook after the P4 submission process
  has completed successfully. This hook takes no parameters and is
  executed regardless of the --no-verify option.  It's return value will
  not be checked.

The calls to the new hooks: p4-prepare-changelist, p4-changelist,
and p4-post-changelist should all be called inside the try-finally
block.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   | 35 +++++++++++++++++++++++++++++
 Documentation/githooks.txt | 46 ++++++++++++++++++++++++++++++++++++++
 git-p4.py                  | 35 ++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 362b50eb21..dab9609013 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -387,6 +387,41 @@ It can be bypassed with the `--no-verify` command line option.
 
 One usage scenario is to run unit tests in the hook.
 
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+p4-changelist
+~~~~~~~~~~~~~
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+
 
 Rebase options
 ~~~~~~~~~~~~~~
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8cf6b08b55..8aa9c2176c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -515,6 +515,52 @@ The exit status determines whether git will use the data from the
 hook to limit its search.  On error, it will fall back to verifying
 all files and folders.
 
+p4-changelist
+~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+Run `git-p4 submit --help` for details.
+
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+Run `git-p4 submit --help` for details.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+Run `git-p4 submit --help` for details.
+
 p4-pre-submit
 ~~~~~~~~~~~~~
 
diff --git a/git-p4.py b/git-p4.py
index 1d15ea7464..c912576094 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1584,7 +1584,7 @@ def __init__(self):
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
                 optparse.make_option("--no-verify", dest="no_verify", action="store_true",
-                                     help="Bypass p4-pre-submit"),
+                                     help="Bypass p4-pre-submit and p4-changelist hooks"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
     The `p4-pre-submit` hook is executed if it exists and is executable. It
@@ -1593,6 +1593,28 @@ def __init__(self):
     from this script prevents `git-p4 submit` from launching.
 
     One usage scenario is to run unit tests in the hook.
+
+    The `p4-prepare-changelist` hook is executed right after preparing the default
+    changelist message and before the editor is started. It takes one parameter,
+    the name of the file that contains the changelist text. Exiting with a non-zero
+    status from the script will abort the process.
+
+    The purpose of the hook is to edit the message file in place, and it is not
+    supressed by the `--no-verify` option. This hook is called even if
+    `--prepare-p4-only` is set.
+
+    The `p4-changelist` hook is executed after the changelist message has been
+    edited by the user. It can be bypassed with the `--no-verify` option. It
+    takes a single parameter, the name of the file that holds the proposed
+    changelist text. Exiting with a non-zero status causes the command to abort.
+
+    The hook is allowed to edit the changelist file and can be used to normalize
+    the text into some project standard format. It can also be used to refuse the
+    Submit after inspect the message file.
+
+    The `p4-post-changelist` hook is invoked after the submit has successfully
+    occured in P4. It takes no parameters and is meant primarily for notification
+    and cannot affect the outcome of the git p4 submit action.
     """
 
         self.usage += " [name of git branch to submit into perforce depot]"
@@ -2104,6 +2126,10 @@ def applyCommit(self, id):
         submitted = False
 
         try:
+            # Allow the hook to edit the changelist text before presenting it
+            # to the user.
+            if not run_git_hook("p4-prepare-changelist", [fileName]):
+                return False
 
             if self.prepare_p4_only:
                 #
@@ -2143,6 +2169,12 @@ def applyCommit(self, id):
                 return True
 
             if self.edit_template(fileName):
+                if not self.no_verify:
+                    if not run_git_hook("p4-changelist", [fileName]):
+                        print("The p4-changelist hook failed.")
+                        sys.stdout.flush()
+                        return False
+
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
                 message = tmpFile.read()
@@ -2180,6 +2212,7 @@ def applyCommit(self, id):
 
                 submitted = True
 
+                run_git_hook("p4-post-changelist")
         finally:
             # Revert changes if we skip this patch
             if not submitted or self.shelve:
-- 
gitgitgadget

Re: [PATCH v2 2/4] git-p4: create new method gitRunHook

[Junio C Hamano]

Ben Keene <seraphire@gmail.com> writes:

> On 2/5/2020 4:42 PM, Junio C Hamano wrote:
>> Ben Keene <seraphire@gmail.com> writes:
>>
>>>>> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>>>> This assumes that the process when his function is called (by the
>>>> way, even though the title of the patch uses the word "method", this
>>>> is not a method but a function, no?), it is always at the top level
>>>> of the working tree.  Is that a good assumption?  I don't know the
>>>> code well, so "yes it is good because a very early thing we do is to
>>>> go up to the top" is a good answer.
>>> ...
> As best as I understand the mechanics, and I haven't examined the
> source code of git, this is just experimental,...

As I already said that I do not know the code well, it is useless
for you to also speculate.  One of us must read the code before
speaking further ;-)

I just scanned "def main()", and it seems that it always goes to the
top-level of the working tree by doing chdir(cdup), where cdup is
learned from "git rev-parse --show-cdup", i.e. "tell me how to chdir
up to the top-level of the working tree".

I am assuming that nobody runs "git p4" in a bare repository, so
under that assumption, I think it would be safe to say that we can
assume we are always at the top.  Also, GIT_DIR is exported from
there, so it probably is a good idea to make sure that the run-hook
helper just uses os.environ.get("GIT_DIR") and barfs if the environ
is not set (i.e. there is something funny going on) without
pretending that it is prepared to deal with such a case, which is
what the "[, default]" parameter to .get method is doing.

I.e.

	hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
    
> Does this prompt the need to search the hierarchy if we don't find
> the directory?

No, we just saw that it is done early in "def main()".  It is done
by "rev-parse --git-dir" and "rev-parse --show-cdup".

Re: [PATCH v3 1/5] git-p4: rewrite prompt to be Windows compatible

[Junio C Hamano]

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

> From: Ben Keene <seraphire@gmail.com>
>
> The existing function prompt(prompt_text) does not work correctly when
> run on Windows 10 bash terminal when launched from the sourcetree
> GUI application. The stdout is not flushed properly so the prompt text
> is not displayed to the user until the next flush of stdout, which is
> quite confusing.

Is that the bug in raw_input(prompt_text) used in the source, or is
that the bug in your environment (whatever "the sourcetree GUI
application" is)?  I cannot quite tell if this is butchering code
that is perfectly working well for other people just to cope with a
broken invoker that is what really needs fixing, or if it is working
around a bug in raw_input().  If the former, the change is not what
we want, and if the latter, the change should go to Python upstream,
so either way, I am not sure if we want this patch without further
information.

Anybody on a similar platform have opinion on this?  I am OK as long
as this change does *not* break the program in an environment that
it is working fine, but that is not even clear.

> Change this method by:
> * Adding flush to stderr, stdout, and stdin
> * Use readline from sys.stdin instead of raw_input.
>
> The existing strip().lower() are retained.
>
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  git-p4.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 40d9e7c594..7d8a5ee788 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -175,7 +175,11 @@ def prompt(prompt_text):
>      """
>      choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))
>      while True:
> -        response = raw_input(prompt_text).strip().lower()
> +        sys.stderr.flush()
> +        sys.stdout.write(prompt_text)
> +        sys.stdout.flush()

raw_input() is getting replace with input() in another series to
bring us to Python3 compatible world, but because you are getting
rid of its use, as long as the resulting code works with both
Python2 and Python3, we are happy ;-)

> +        sys.stdin.flush()

What does it even mean to flush the input stream here?  At least in
C, it is meaningless and causes an undefined behaviour.

> +        response=sys.stdin.readline().strip().lower()
>          if not response:
>              continue
>          response = response[0]

Thanks.

Re: [PATCH v3 3/5] git-p4: add --no-verify option

[Junio C Hamano]

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

> From: Ben Keene <seraphire@gmail.com>
>
> Add new command line option --no-verify:
>
> Add a new command line option "--no-verify" to the Submit command of
> git-p4.py.  This option will function in the spirit of the existing
> --no-verify command line option found in git commit. It will cause the
> P4 Submit function to ignore the existing p4-pre-submit.
>
> Change the execution of the existing trigger p4-pre-submit to honor the
> --no-verify option. Before exiting on failure of this hook, display
> text to the user explaining which hook has failed and the impact
> of using the --no-verify option.
>
> Change the call of the p4-pre-submit hook to use the new run_git_hook
> function. This is in preparation of additional hooks to be added.
>
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  Documentation/git-p4.txt   | 10 ++++++++--
>  Documentation/githooks.txt |  5 ++++-
>  git-p4.py                  | 30 +++++++++++++++++++-----------
>  3 files changed, 31 insertions(+), 14 deletions(-)

Nicely done.  If your strategy is to "add a feature and use it in
the same patch as the feature is added", which is what is done for
the new "no-verify" feature that is applied to existing p4-pre-submit
hook, then the code that runs p4-pre-submit in the original should
be changed to use run_git_hook() in the previous step, which added
the new run_git_hook() feature.

I see new print() that is not protected with "if verbose:"; is it
debugging cruft added during development, or is it a useful addition
for end-users to see under --verbose mode?  I suspect it is the
latter.

Thanks.

> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 3494a1db3e..362b50eb21 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -374,14 +374,20 @@ These options can be used to modify 'git p4 submit' behavior.
>      been submitted. Implies --disable-rebase. Can also be set with
>      git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
>  
> -Hook for submit
> -~~~~~~~~~~~~~~~
> +Hooks for submit
> +----------------
> +
> +p4-pre-submit
> +~~~~~~~~~~~~~
> +
>  The `p4-pre-submit` hook is executed if it exists and is executable.
>  The hook takes no parameters and nothing from standard input. Exiting with
>  non-zero status from this script prevents `git-p4 submit` from launching.
> +It can be bypassed with the `--no-verify` command line option.
>  
>  One usage scenario is to run unit tests in the hook.
>  
> +
>  Rebase options
>  ~~~~~~~~~~~~~~
>  These options can be used to modify 'git p4 rebase' behavior.
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 50365f2914..8cf6b08b55 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -520,7 +520,10 @@ p4-pre-submit
>  
>  This hook is invoked by `git-p4 submit`. It takes no parameters and nothing
>  from standard input. Exiting with non-zero status from this script prevent
> -`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
> +`git-p4 submit` from launching. It can be bypassed with the `--no-verify`
> +command line option. Run `git-p4 submit --help` for details.
> +
> +
>  
>  post-index-change
>  ~~~~~~~~~~~~~~~~~
> diff --git a/git-p4.py b/git-p4.py
> index d4c39f112b..b377484464 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1583,13 +1583,17 @@ def __init__(self):
>                                       "work from a local git branch that is not master"),
>                  optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
>                                       help="Skip Perforce sync of p4/master after submit or shelve"),
> +                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
> +                                     help="Bypass p4-pre-submit"),
>          ]
>          self.description = """Submit changes from git to the perforce depot.\n
> -    The `p4-pre-submit` hook is executed if it exists and is executable.
> -    The hook takes no parameters and nothing from standard input. Exiting with
> -    non-zero status from this script prevents `git-p4 submit` from launching.
> +    The `p4-pre-submit` hook is executed if it exists and is executable. It
> +    can be bypassed with the `--no-verify` command line option. The hook takes
> +    no parameters and nothing from standard input. Exiting with a non-zero status
> +    from this script prevents `git-p4 submit` from launching.
>  
> -    One usage scenario is to run unit tests in the hook."""
> +    One usage scenario is to run unit tests in the hook.
> +    """
>  
>          self.usage += " [name of git branch to submit into perforce depot]"
>          self.origin = ""
> @@ -1607,6 +1611,7 @@ def __init__(self):
>          self.exportLabels = False
>          self.p4HasMoveCommand = p4_has_move_command()
>          self.branch = None
> +        self.no_verify = False
>  
>          if gitConfig('git-p4.largeFileSystem'):
>              die("Large file system not supported for git-p4 submit command. Please remove it from config.")
> @@ -1993,6 +1998,9 @@ def applyCommit(self, id):
>          applyPatchCmd = patchcmd + "--check --apply -"
>          patch_succeeded = True
>  
> +        if verbose:
> +            print("TryPatch: %s" % tryPatchCmd)
> +
>          if os.system(tryPatchCmd) != 0:
>              fixed_rcs_keywords = False
>              patch_succeeded = False
> @@ -2032,6 +2040,7 @@ def applyCommit(self, id):
>                  print("Retrying the patch with RCS keywords cleaned up")
>                  if os.system(tryPatchCmd) == 0:
>                      patch_succeeded = True
> +                    print("Patch succeesed this time")
>  
>          if not patch_succeeded:
>              for f in editedFiles:
> @@ -2400,13 +2409,12 @@ def run(self, args):
>              sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
>                       (len(commits), num_shelves))
>  
> -        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-submit")
> -        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
> -            sys.exit(1)
> +        if not self.no_verify:
> +            if not run_git_hook("p4-pre-submit"):
> +                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
> +                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
> +                    "however,\nthis will also skip the p4-changelist hook as well.")
> +                sys.exit(1)
>  
>          #
>          # Apply the commits, one at a time.  On failure, ask if should

Re: [PATCH v3 2/5] git-p4: create new function run_git_hook

[Junio C Hamano]

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

> +def run_git_hook(cmd, param=[]):
> +    """Execute a hook if the hook exists."""
> +    if verbose:
> +        sys.stderr.write("Looking for hook: %s\n" % cmd)
> +        sys.stderr.flush()
> +
> +    hooks_path = gitConfig("core.hooksPath")
> +    if len(hooks_path) <= 0:
> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")

Using the .get() with default is misleading, I think (see an earlier reply).

> +    if not isinstance(param, list):
> +        param=[param]
> +
> +    # resolve hook file name, OS depdenent
> +    hook_file = os.path.join(hooks_path, cmd)
> +    if platform.system() == 'Windows':
> +        if not os.path.isfile(hook_file):
> +            # look for the file with an extension
> +            files = glob.glob(hook_file + ".*")
> +            if not files:
> +                return True
> +            files.sort()
> +            hook_file = files[0]
> +
> +    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
> +        return True
> +
> +    return run_hook_command(hook_file, param) == 0
> +
> +def run_hook_command(cmd, param):
> +    """Executes a git hook command
> +       cmd = the command line file to be executed. This can be
> +       a file that is run by OS association.
> +
> +       param = a list of parameters to pass to the cmd command
> +
> +       On windows, the extension is checked to see if it should
> +       be run with the Git for Windows Bash shell.  If there
> +       is no file extension, the file is deemed a bash shell
> +       and will be handed off to sh.exe. Otherwise, Windows
> +       will be called with the shell to handle the file assocation.
> +
> +       For non Windows operating systems, the file is called
> +       as an executable.
> +    """
> +    cli = [cmd] + param
> +    use_shell = False
> +    if platform.system() == 'Windows':
> +        (root,ext) = os.path.splitext(cmd)
> +        if ext == "":
> +            exe_path = os.environ.get("EXEPATH")
> +            if exe_path is None:
> +                exe_path = ""
> +            else:
> +                exe_path = os.path.join(exe_path, "bin")
> +            cli = [os.path.join(exe_path, "SH.EXE")] + cli
> +        else:
> +            use_shell = True

Please ask somebody familiar with Windows to review this "if
Windows" and the other one in run_git_hook().

> +    return subprocess.call(cli, shell=use_shell)
> +
> +
>  def write_pipe(c, stdin):
>      if verbose:
>          sys.stderr.write('Writing pipe: %s\n' % str(c))
> @@ -4125,7 +4187,6 @@ def printUsage(commands):
>      "unshelve" : P4Unshelve,
>  }
>  
> -
>  def main():
>      if len(sys.argv[1:]) == 0:
>          printUsage(commands.keys())

Re: [PATCH v3 1/5] git-p4: rewrite prompt to be Windows compatible

[Ben Keene]

On 2/6/2020 2:28 PM, Junio C Hamano wrote:
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Ben Keene <seraphire@gmail.com>
>>
>> The existing function prompt(prompt_text) does not work correctly when
>> run on Windows 10 bash terminal when launched from the sourcetree
>> GUI application. The stdout is not flushed properly so the prompt text
>> is not displayed to the user until the next flush of stdout, which is
>> quite confusing.
> Is that the bug in raw_input(prompt_text) used in the source, or is
> that the bug in your environment (whatever "the sourcetree GUI
> application" is)?  I cannot quite tell if this is butchering code
> that is perfectly working well for other people just to cope with a
> broken invoker that is what really needs fixing, or if it is working
> around a bug in raw_input().  If the former, the change is not what
> we want, and if the latter, the change should go to Python upstream,
> so either way, I am not sure if we want this patch without further
> information.
Sourcetree is a graphical UI for GIT maintained by Atlassian. It
invokes the git-bash.exe in the git for windows installation as
the shell interface. In this context, the raw_input() function
failed to work for me.  I do not know if I am unique in using
git-p4.py on a windows platform, but from what I could find online,
raw_input() does not flush stdout. (See
https://bugs.python.org/issue526382) From what I saw in response
to this problem, everyone suggested reading from stdin instead of
using raw_input. Though I can understand the hesitation for this
sort of change.
>
> Anybody on a similar platform have opinion on this?  I am OK as long
> as this change does *not* break the program in an environment that
> it is working fine, but that is not even clear.
>
>> Change this method by:
>> * Adding flush to stderr, stdout, and stdin
>> * Use readline from sys.stdin instead of raw_input.
>>
>> The existing strip().lower() are retained.
>>
>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> ---
>>   git-p4.py | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 40d9e7c594..7d8a5ee788 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -175,7 +175,11 @@ def prompt(prompt_text):
>>       """
>>       choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))
>>       while True:
>> -        response = raw_input(prompt_text).strip().lower()
>> +        sys.stderr.flush()
>> +        sys.stdout.write(prompt_text)
>> +        sys.stdout.flush()
> raw_input() is getting replace with input() in another series to
> bring us to Python3 compatible world, but because you are getting
> rid of its use, as long as the resulting code works with both
> Python2 and Python3, we are happy ;-)
>
>> +        sys.stdin.flush()
> What does it even mean to flush the input stream here?  At least in
> C, it is meaningless and causes an undefined behaviour.
The intent, was to clear out the input buffer before reading the
input, however, in a non-interactive mode, this would be undesirable.

I will remove the stdin.flush().


>
>> +        response=sys.stdin.readline().strip().lower()
>>           if not response:
>>               continue
>>           response = response[0]
> Thanks.

Re: [PATCH v3 3/5] git-p4: add --no-verify option

[Ben Keene]

On 2/6/2020 2:42 PM, Junio C Hamano wrote:
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Ben Keene <seraphire@gmail.com>
>>
>> Add new command line option --no-verify:
>>
>> Add a new command line option "--no-verify" to the Submit command of
>> git-p4.py.  This option will function in the spirit of the existing
>> --no-verify command line option found in git commit. It will cause the
>> P4 Submit function to ignore the existing p4-pre-submit.
>>
>> Change the execution of the existing trigger p4-pre-submit to honor the
>> --no-verify option. Before exiting on failure of this hook, display
>> text to the user explaining which hook has failed and the impact
>> of using the --no-verify option.
>>
>> Change the call of the p4-pre-submit hook to use the new run_git_hook
>> function. This is in preparation of additional hooks to be added.
>>
>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> ---
>>   Documentation/git-p4.txt   | 10 ++++++++--
>>   Documentation/githooks.txt |  5 ++++-
>>   git-p4.py                  | 30 +++++++++++++++++++-----------
>>   3 files changed, 31 insertions(+), 14 deletions(-)
> Nicely done.  If your strategy is to "add a feature and use it in
> the same patch as the feature is added", which is what is done for
> the new "no-verify" feature that is applied to existing p4-pre-submit
> hook, then the code that runs p4-pre-submit in the original should
> be changed to use run_git_hook() in the previous step, which added
> the new run_git_hook() feature.
Thank you and and I'll resubmit the commits with these additional
suggestions.
>
> I see new print() that is not protected with "if verbose:"; is it
> debugging cruft added during development, or is it a useful addition
> for end-users to see under --verbose mode?  I suspect it is the
> latter.
The print statement has been changed to:
print("Patch succeesed this time with RCS keywords cleaned")
and will be submitted as a separate commit so that it can be discussed
on its own. However, the rationale for it is that the current flow
reports to the user that the patch has failed and that it will attempt
to re-run the patch after cleaning up the RCS keywords. Since the
program told the user that it failed, I felt they should also be told
of the success at the same level of verbosity.
>
> Thanks.
>
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index 3494a1db3e..362b50eb21 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -374,14 +374,20 @@ These options can be used to modify 'git p4 submit' behavior.
>>       been submitted. Implies --disable-rebase. Can also be set with
>>       git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
>>   
>> -Hook for submit
>> -~~~~~~~~~~~~~~~
>> +Hooks for submit
>> +----------------
>> +
>> +p4-pre-submit
>> +~~~~~~~~~~~~~
>> +
>>   The `p4-pre-submit` hook is executed if it exists and is executable.
>>   The hook takes no parameters and nothing from standard input. Exiting with
>>   non-zero status from this script prevents `git-p4 submit` from launching.
>> +It can be bypassed with the `--no-verify` command line option.
>>   
>>   One usage scenario is to run unit tests in the hook.
>>   
>> +
>>   Rebase options
>>   ~~~~~~~~~~~~~~
>>   These options can be used to modify 'git p4 rebase' behavior.
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index 50365f2914..8cf6b08b55 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -520,7 +520,10 @@ p4-pre-submit
>>   
>>   This hook is invoked by `git-p4 submit`. It takes no parameters and nothing
>>   from standard input. Exiting with non-zero status from this script prevent
>> -`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
>> +`git-p4 submit` from launching. It can be bypassed with the `--no-verify`
>> +command line option. Run `git-p4 submit --help` for details.
>> +
>> +
>>   
>>   post-index-change
>>   ~~~~~~~~~~~~~~~~~
>> diff --git a/git-p4.py b/git-p4.py
>> index d4c39f112b..b377484464 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1583,13 +1583,17 @@ def __init__(self):
>>                                        "work from a local git branch that is not master"),
>>                   optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
>>                                        help="Skip Perforce sync of p4/master after submit or shelve"),
>> +                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
>> +                                     help="Bypass p4-pre-submit"),
>>           ]
>>           self.description = """Submit changes from git to the perforce depot.\n
>> -    The `p4-pre-submit` hook is executed if it exists and is executable.
>> -    The hook takes no parameters and nothing from standard input. Exiting with
>> -    non-zero status from this script prevents `git-p4 submit` from launching.
>> +    The `p4-pre-submit` hook is executed if it exists and is executable. It
>> +    can be bypassed with the `--no-verify` command line option. The hook takes
>> +    no parameters and nothing from standard input. Exiting with a non-zero status
>> +    from this script prevents `git-p4 submit` from launching.
>>   
>> -    One usage scenario is to run unit tests in the hook."""
>> +    One usage scenario is to run unit tests in the hook.
>> +    """
>>   
>>           self.usage += " [name of git branch to submit into perforce depot]"
>>           self.origin = ""
>> @@ -1607,6 +1611,7 @@ def __init__(self):
>>           self.exportLabels = False
>>           self.p4HasMoveCommand = p4_has_move_command()
>>           self.branch = None
>> +        self.no_verify = False
>>   
>>           if gitConfig('git-p4.largeFileSystem'):
>>               die("Large file system not supported for git-p4 submit command. Please remove it from config.")
>> @@ -1993,6 +1998,9 @@ def applyCommit(self, id):
>>           applyPatchCmd = patchcmd + "--check --apply -"
>>           patch_succeeded = True
>>   
>> +        if verbose:
>> +            print("TryPatch: %s" % tryPatchCmd)
>> +
>>           if os.system(tryPatchCmd) != 0:
>>               fixed_rcs_keywords = False
>>               patch_succeeded = False
>> @@ -2032,6 +2040,7 @@ def applyCommit(self, id):
>>                   print("Retrying the patch with RCS keywords cleaned up")
>>                   if os.system(tryPatchCmd) == 0:
>>                       patch_succeeded = True
>> +                    print("Patch succeesed this time")
>>   
>>           if not patch_succeeded:
>>               for f in editedFiles:
>> @@ -2400,13 +2409,12 @@ def run(self, args):
>>               sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
>>                        (len(commits), num_shelves))
>>   
>> -        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-submit")
>> -        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
>> -            sys.exit(1)
>> +        if not self.no_verify:
>> +            if not run_git_hook("p4-pre-submit"):
>> +                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
>> +                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
>> +                    "however,\nthis will also skip the p4-changelist hook as well.")
>> +                sys.exit(1)
>>   
>>           #
>>           # Apply the commits, one at a time.  On failure, ask if should

Re: [PATCH v3 2/5] git-p4: create new function run_git_hook

[Ben Keene]

On 2/6/2020 2:42 PM, Junio C Hamano wrote:
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +def run_git_hook(cmd, param=[]):
>> +    """Execute a hook if the hook exists."""
>> +    if verbose:
>> +        sys.stderr.write("Looking for hook: %s\n" % cmd)
>> +        sys.stderr.flush()
>> +
>> +    hooks_path = gitConfig("core.hooksPath")
>> +    if len(hooks_path) <= 0:
>> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
> Using the .get() with default is misleading, I think (see an earlier reply).
Looking back through the original discussion, I see that it was decided that
the application uses "rev-parse --git-dir" and "rev-parse --show-cdup" 
to find
the top directory of the git repo. At least for testing, being able to 
resolve
the .git directory directly is useful.  I will modify the code to throw an
exception if neither GIT_DIR or .git resolve correctly.
>
>> +    if not isinstance(param, list):
>> +        param=[param]
>> +
>> +    # resolve hook file name, OS depdenent
>> +    hook_file = os.path.join(hooks_path, cmd)
>> +    if platform.system() == 'Windows':
>> +        if not os.path.isfile(hook_file):
>> +            # look for the file with an extension
>> +            files = glob.glob(hook_file + ".*")
>> +            if not files:
>> +                return True
>> +            files.sort()
>> +            hook_file = files[0]
>> +
>> +    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
>> +        return True
>> +
>> +    return run_hook_command(hook_file, param) == 0
>> +
>> +def run_hook_command(cmd, param):
>> +    """Executes a git hook command
>> +       cmd = the command line file to be executed. This can be
>> +       a file that is run by OS association.
>> +
>> +       param = a list of parameters to pass to the cmd command
>> +
>> +       On windows, the extension is checked to see if it should
>> +       be run with the Git for Windows Bash shell.  If there
>> +       is no file extension, the file is deemed a bash shell
>> +       and will be handed off to sh.exe. Otherwise, Windows
>> +       will be called with the shell to handle the file assocation.
>> +
>> +       For non Windows operating systems, the file is called
>> +       as an executable.
>> +    """
>> +    cli = [cmd] + param
>> +    use_shell = False
>> +    if platform.system() == 'Windows':
>> +        (root,ext) = os.path.splitext(cmd)
>> +        if ext == "":
>> +            exe_path = os.environ.get("EXEPATH")
>> +            if exe_path is None:
>> +                exe_path = ""
>> +            else:
>> +                exe_path = os.path.join(exe_path, "bin")
>> +            cli = [os.path.join(exe_path, "SH.EXE")] + cli
>> +        else:
>> +            use_shell = True
> Please ask somebody familiar with Windows to review this "if
> Windows" and the other one in run_git_hook().
>
>> +    return subprocess.call(cli, shell=use_shell)
>> +
>> +
>>   def write_pipe(c, stdin):
>>       if verbose:
>>           sys.stderr.write('Writing pipe: %s\n' % str(c))
>> @@ -4125,7 +4187,6 @@ def printUsage(commands):
>>       "unshelve" : P4Unshelve,
>>   }
>>   
>> -
>>   def main():
>>       if len(sys.argv[1:]) == 0:
>>           printUsage(commands.keys())

[PATCH v4 0/6] git-p4: add hooks for p4-changelist

[Ben Keene via GitGitGadget]

REQUEST FOR HELP - SEE END

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.

v1: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.

v2:Instead of a single hook, p4-pre-edit-changelist, follow the git
convention for hook names and add the trio of hooks that work together,
similar to git commit.

The hook names are: 

 * p4-prepare-changelist
 * p4-changelist
 * p4-post-changelist

The hooks should follow the same convention as git commit, so a new command
line option for the git-p4 submit function --no-verify should also be added.

v3:2-Feb-2020 This version reorganizes the commits to be easier to read. The
function for running the git hook has been rewritten to include suggestions.
This version no longer restricts the executable supported by windows for the
hook. It will first look for a file with the hook name without an extension
and if found, it will pass it to sh.exe (Git for Window's MINGW shell)
Otherwise it will take the file with the lowest alphabetical extension and
ask Windows to execute the program.

v3:10-Feb-2020 Integrated the suggested changes from the mailing list into
this release. The changes are:

 * Restructure the commits to pull the p4-pre-submit hook changes into the
   commit for adding git_run_hook.
 * Update the git_run_hook to include additional tests for GIT_DIR being set
   if getting the environment returns no value.
 * Updated the windows file resolution to ensure that the script to be run
   is NOT the .SAMPLE version of the hooks.
 * Split the RCS keyword cleanup message into its own commit.

Request for helpThere are 2 areas that I am requesting help with:

 1. Validating code - if there are any experts with python and Windows, I
    would appreciate a review of the changes that I have presented here.
    
    
 2. Git documentation - On my latest commit, I am getting an error with
    `test-documentation.sh . Searching for a fix, I see this thread: 
    https://patchwork.kernel.org/patch/11136547/ that points to an update
    for the documentation checking, but this is out of scope for the work I
    can put to this patch. 
    
    

The actual error can be seen here: 
https://dev.azure.com/git/git/_build/results?buildId=1705&view=logs&j=bbfa2fa6-a202-51ab-3559-6013e9e5f686&t=c5c726a2-e78b-50d9-5d91-f31f918b4814&l=2688
[https://dev.azure.com/git/git/_build/results?buildId=1705&view=logs&j=bbfa2fa6-a202-51ab-3559-6013e9e5f686&t=c5c726a2-e78b-50d9-5d91-f31f918b4814&l=2688]

Thank you in advance!

Ben Keene (6):
  git-p4: rewrite prompt to be Windows compatible
  git-p4: create new function run_git_hook
  git-p4: add --no-verify option
  git-p4: restructure code in submit
  git-p4: add p4 submit hooks
  git-4: add RCS keyword status message

 Documentation/git-p4.txt   |  45 ++++++-
 Documentation/githooks.txt |  51 +++++++-
 git-p4.py                  | 233 ++++++++++++++++++++++++++++---------
 3 files changed, 272 insertions(+), 57 deletions(-)


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

Range-diff vs v3:

 1:  8881d76c46 ! 1:  71c51ccfb0 git-p4: rewrite prompt to be Windows compatible
     @@ -27,7 +27,6 @@
      +        sys.stderr.flush()
      +        sys.stdout.write(prompt_text)
      +        sys.stdout.flush()
     -+        sys.stdin.flush()
      +        response=sys.stdin.readline().strip().lower()
               if not response:
                   continue
 2:  85918a7edb ! 2:  596b18e5e5 git-p4: create new function run_git_hook
     @@ -80,7 +80,7 @@
      +
      +    hooks_path = gitConfig("core.hooksPath")
      +    if len(hooks_path) <= 0:
     -+        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
     ++        hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
      +
      +    if not isinstance(param, list):
      +        param=[param]
     @@ -94,7 +94,13 @@
      +            if not files:
      +                return True
      +            files.sort()
     -+            hook_file = files[0]
     ++            hook_file = files.pop()
     ++            while hook_file.upper().endswith(".SAMPLE"):
     ++                # The file is a sample hook. We don't want it
     ++                if len(files) > 0:
     ++                    hook_file = files.pop()
     ++                else:
     ++                    return True
      +
      +    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
      +        return True
     @@ -136,6 +142,28 @@
       def write_pipe(c, stdin):
           if verbose:
               sys.stderr.write('Writing pipe: %s\n' % str(c))
     +@@
     +             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
     +                      (len(commits), num_shelves))
     + 
     +-        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-submit")
     +-        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
     ++        try:
     ++            if not run_git_hook("p4-pre-submit"):
     ++                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
     ++                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
     ++                    "however,\nthis will also skip the p4-changelist hook as well.")
     ++                sys.exit(1)
     ++        except Exception as e:
     ++            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
     ++                "with the error '{0}'".format(e.message) )
     +             sys.exit(1)
     + 
     +         #
      @@
           "unshelve" : P4Unshelve,
       }
 3:  b538ad08b6 ! 3:  57a6166ed1 git-p4: add --no-verify option
     @@ -95,41 +95,3 @@
       
               if gitConfig('git-p4.largeFileSystem'):
                   die("Large file system not supported for git-p4 submit command. Please remove it from config.")
     -@@
     -         applyPatchCmd = patchcmd + "--check --apply -"
     -         patch_succeeded = True
     - 
     -+        if verbose:
     -+            print("TryPatch: %s" % tryPatchCmd)
     -+
     -         if os.system(tryPatchCmd) != 0:
     -             fixed_rcs_keywords = False
     -             patch_succeeded = False
     -@@
     -                 print("Retrying the patch with RCS keywords cleaned up")
     -                 if os.system(tryPatchCmd) == 0:
     -                     patch_succeeded = True
     -+                    print("Patch succeesed this time")
     - 
     -         if not patch_succeeded:
     -             for f in editedFiles:
     -@@
     -             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
     -                      (len(commits), num_shelves))
     - 
     --        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-submit")
     --        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
     --            sys.exit(1)
     -+        if not self.no_verify:
     -+            if not run_git_hook("p4-pre-submit"):
     -+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
     -+                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
     -+                    "however,\nthis will also skip the p4-changelist hook as well.")
     -+                sys.exit(1)
     - 
     -         #
     -         # Apply the commits, one at a time.  On failure, ask if should
 4:  662b864d70 = 4:  d17e032767 git-p4: restructure code in submit
 5:  e34c377066 = 5:  ceb2b0aff6 git-p4: add p4 submit hooks
 -:  ---------- > 6:  b92e017cb1 git-4: add RCS keyword status message

-- 
gitgitgadget

[PATCH v4 1/6] git-p4: rewrite prompt to be Windows compatible

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The existing function prompt(prompt_text) does not work correctly when
run on Windows 10 bash terminal when launched from the sourcetree
GUI application. The stdout is not flushed properly so the prompt text
is not displayed to the user until the next flush of stdout, which is
quite confusing.

Change this method by:
* Adding flush to stderr, stdout, and stdin
* Use readline from sys.stdin instead of raw_input.

The existing strip().lower() are retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 40d9e7c594..65b6d4dca0 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -175,7 +175,10 @@ def prompt(prompt_text):
     """
     choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))
     while True:
-        response = raw_input(prompt_text).strip().lower()
+        sys.stderr.flush()
+        sys.stdout.write(prompt_text)
+        sys.stdout.flush()
+        response=sys.stdin.readline().strip().lower()
         if not response:
             continue
         response = response[0]
-- 
gitgitgadget

[PATCH v4 2/6] git-p4: create new function run_git_hook

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

This commit is in preparation of introducing new p4 submit hooks.

The current code in the python script git-p4.py makes the assumption
that the git hooks can be executed by subprocess.call() function.
However, when git is run on Windows, this may not work as expected.

The subprocess.call() does not cover all the use cases for properly
executing the various types of executable files on Windows.

Prepare for remediation by adding a new function, run_git_hook, that
takes 2 parameters:
* the short filename of an optionally registered git hook
* an optional list of parameters

The run_git_hook function will honor the existing behavior seen in the
current code for executing the p4-pre-submit hook:

* Hooks are looked for in core.hooksPath directory.
* If core.hooksPath is not set, then the current .git/hooks directory
  is checked.
* If the hook does not exist, the function returns True.
* If the hook file is not accessible, the function returns True.
* If the hook returns a zero exit code when executed, the function
  return True.
* If the hook returns a non-zero exit code, the function returns False.

Add the following additional functionality if git-p4.py is run on
Windows.
* If hook file is not located without an extension, search for
  any file in the associated hook directory (from the list above) that
  has the same name but with an extension.
* If the file is still not found, return True (the hook is missing)

Add a new function run_hook_command() that wraps the OS dependent
functionality for actually running the subprocess.call() with OS
dependent behavior:

If a hook file exists on Windows:
* If there is no extension, set the launch executable to be SH.EXE
  - Look for SH.EXE under the environmental variable EXEPATH in the
    bin/ directory.
  - If %EXEPATH%/bin/sh.exe exists, use this as the actual executable.
  - If %EXEPATH%/bin/sh.exe does not exist, use sh.exe
  - Execute subprocess.call() without the shell (shell=False)
* If there is an extension, execute subprocess.call() with teh shell
  (shell=True) and consider the file to be the executable.

The return value from run_hook_command() is the subprocess.call()
return value.

These functions are added in this commit, but are only staged and not
yet used.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 65b6d4dca0..b1c86678fc 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -26,6 +26,7 @@
 import zlib
 import ctypes
 import errno
+import glob
 
 # support basestring in python3
 try:
@@ -185,6 +186,73 @@ def prompt(prompt_text):
         if response in choices:
             return response
 
+def run_git_hook(cmd, param=[]):
+    """Execute a hook if the hook exists."""
+    if verbose:
+        sys.stderr.write("Looking for hook: %s\n" % cmd)
+        sys.stderr.flush()
+
+    hooks_path = gitConfig("core.hooksPath")
+    if len(hooks_path) <= 0:
+        hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
+
+    if not isinstance(param, list):
+        param=[param]
+
+    # resolve hook file name, OS depdenent
+    hook_file = os.path.join(hooks_path, cmd)
+    if platform.system() == 'Windows':
+        if not os.path.isfile(hook_file):
+            # look for the file with an extension
+            files = glob.glob(hook_file + ".*")
+            if not files:
+                return True
+            files.sort()
+            hook_file = files.pop()
+            while hook_file.upper().endswith(".SAMPLE"):
+                # The file is a sample hook. We don't want it
+                if len(files) > 0:
+                    hook_file = files.pop()
+                else:
+                    return True
+
+    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
+        return True
+
+    return run_hook_command(hook_file, param) == 0
+
+def run_hook_command(cmd, param):
+    """Executes a git hook command
+       cmd = the command line file to be executed. This can be
+       a file that is run by OS association.
+
+       param = a list of parameters to pass to the cmd command
+
+       On windows, the extension is checked to see if it should
+       be run with the Git for Windows Bash shell.  If there
+       is no file extension, the file is deemed a bash shell
+       and will be handed off to sh.exe. Otherwise, Windows
+       will be called with the shell to handle the file assocation.
+
+       For non Windows operating systems, the file is called
+       as an executable.
+    """
+    cli = [cmd] + param
+    use_shell = False
+    if platform.system() == 'Windows':
+        (root,ext) = os.path.splitext(cmd)
+        if ext == "":
+            exe_path = os.environ.get("EXEPATH")
+            if exe_path is None:
+                exe_path = ""
+            else:
+                exe_path = os.path.join(exe_path, "bin")
+            cli = [os.path.join(exe_path, "SH.EXE")] + cli
+        else:
+            use_shell = True
+    return subprocess.call(cli, shell=use_shell)
+
+
 def write_pipe(c, stdin):
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))
@@ -2337,12 +2405,15 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        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-submit")
-        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
+        try:
+            if not run_git_hook("p4-pre-submit"):
+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
+                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
+                    "however,\nthis will also skip the p4-changelist hook as well.")
+                sys.exit(1)
+        except Exception as e:
+            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
+                "with the error '{0}'".format(e.message) )
             sys.exit(1)
 
         #
@@ -4124,7 +4195,6 @@ def printUsage(commands):
     "unshelve" : P4Unshelve,
 }
 
-
 def main():
     if len(sys.argv[1:]) == 0:
         printUsage(commands.keys())
-- 
gitgitgadget

[PATCH v4 3/6] git-p4: add --no-verify option

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

Add new command line option --no-verify:

Add a new command line option "--no-verify" to the Submit command of
git-p4.py.  This option will function in the spirit of the existing
--no-verify command line option found in git commit. It will cause the
P4 Submit function to ignore the existing p4-pre-submit.

Change the execution of the existing trigger p4-pre-submit to honor the
--no-verify option. Before exiting on failure of this hook, display
text to the user explaining which hook has failed and the impact
of using the --no-verify option.

Change the call of the p4-pre-submit hook to use the new run_git_hook
function. This is in preparation of additional hooks to be added.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   | 10 ++++++++--
 Documentation/githooks.txt |  5 ++++-
 git-p4.py                  | 13 +++++++++----
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3494a1db3e..362b50eb21 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -374,14 +374,20 @@ These options can be used to modify 'git p4 submit' behavior.
     been submitted. Implies --disable-rebase. Can also be set with
     git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
 
-Hook for submit
-~~~~~~~~~~~~~~~
+Hooks for submit
+----------------
+
+p4-pre-submit
+~~~~~~~~~~~~~
+
 The `p4-pre-submit` hook is executed if it exists and is executable.
 The hook takes no parameters and nothing from standard input. Exiting with
 non-zero status from this script prevents `git-p4 submit` from launching.
+It can be bypassed with the `--no-verify` command line option.
 
 One usage scenario is to run unit tests in the hook.
 
+
 Rebase options
 ~~~~~~~~~~~~~~
 These options can be used to modify 'git p4 rebase' behavior.
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 50365f2914..8cf6b08b55 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -520,7 +520,10 @@ p4-pre-submit
 
 This hook is invoked by `git-p4 submit`. It takes no parameters and nothing
 from standard input. Exiting with non-zero status from this script prevent
-`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
+`git-p4 submit` from launching. It can be bypassed with the `--no-verify`
+command line option. Run `git-p4 submit --help` for details.
+
+
 
 post-index-change
 ~~~~~~~~~~~~~~~~~
diff --git a/git-p4.py b/git-p4.py
index b1c86678fc..58551bcb09 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1588,13 +1588,17 @@ def __init__(self):
                                      "work from a local git branch that is not master"),
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
+                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
+                                     help="Bypass p4-pre-submit"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
-    The `p4-pre-submit` hook is executed if it exists and is executable.
-    The hook takes no parameters and nothing from standard input. Exiting with
-    non-zero status from this script prevents `git-p4 submit` from launching.
+    The `p4-pre-submit` hook is executed if it exists and is executable. It
+    can be bypassed with the `--no-verify` command line option. The hook takes
+    no parameters and nothing from standard input. Exiting with a non-zero status
+    from this script prevents `git-p4 submit` from launching.
 
-    One usage scenario is to run unit tests in the hook."""
+    One usage scenario is to run unit tests in the hook.
+    """
 
         self.usage += " [name of git branch to submit into perforce depot]"
         self.origin = ""
@@ -1612,6 +1616,7 @@ def __init__(self):
         self.exportLabels = False
         self.p4HasMoveCommand = p4_has_move_command()
         self.branch = None
+        self.no_verify = False
 
         if gitConfig('git-p4.largeFileSystem'):
             die("Large file system not supported for git-p4 submit command. Please remove it from config.")
-- 
gitgitgadget

[PATCH v4 4/6] git-p4: restructure code in submit

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

In preparation for adding new hooks to the submit method of git-p4,
restructure the applyCommit function in the P4Submit class.  Make the
following changes:

* Move all the code after the definition of submitted = False into the
  Try-Finally block. This ensures that any error that occurs will
  properly recover.  This is not necessary with the current code because
  none of it should throw an exception, however the next set of changes
  will introduce exceptional code.

  Existing flow control can remain as defined - the if-block for
  prepare-p4-only sets the local variable "submitted" to True and exits
  the function. New early exits, leave submitted set to False so the
  Finally block will undo changes to the P4 workspace.

* Make the small usability change of adding an empty string to the
  print statements displayed to the user when the prepare-p4-only option
  is selected.  On Windows, the command print() may display a set of
  parentheses "()" to the user when the print() function is called with
  no parameters. By supplying an empty string, the intended blank line
  will print as expected.

* Fix a small bug when the submittedTemplate is edited by the user
  and all content in the file is removed. The existing code will throw
  an exception if the separateLine is not found in the file. Change this
  code to test for the separator line using a find() test first and only
  split on the separator if it is found.

* Additionally, add the new behavior that if the changelist file has
  been completely emptied that the Submit action for this changelist
  will be aborted.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 92 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 58551bcb09..0224f2e032 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2102,47 +2102,47 @@ def applyCommit(self, id):
         tmpFile.write(submitTemplate)
         tmpFile.close()
 
-        if self.prepare_p4_only:
-            #
-            # Leave the p4 tree prepared, and the submit template around
-            # and let the user decide what to do next
-            #
-            print()
-            print("P4 workspace prepared for submission.")
-            print("To submit or revert, go to client workspace")
-            print("  " + self.clientPath)
-            print()
-            print("To submit, use \"p4 submit\" to write a new description,")
-            print("or \"p4 submit -i <%s\" to use the one prepared by" \
-                  " \"git p4\"." % fileName)
-            print("You can delete the file \"%s\" when finished." % fileName)
-
-            if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
-                print("To preserve change ownership by user %s, you must\n" \
-                      "do \"p4 change -f <change>\" after submitting and\n" \
-                      "edit the User field.")
-            if pureRenameCopy:
-                print("After submitting, renamed files must be re-synced.")
-                print("Invoke \"p4 sync -f\" on each of these files:")
-                for f in pureRenameCopy:
-                    print("  " + f)
-
-            print()
-            print("To revert the changes, use \"p4 revert ...\", and delete")
-            print("the submit template file \"%s\"" % fileName)
-            if filesToAdd:
-                print("Since the commit adds new files, they must be deleted:")
-                for f in filesToAdd:
-                    print("  " + f)
-            print()
-            return True
-
-        #
-        # Let the user edit the change description, then submit it.
-        #
         submitted = False
 
         try:
+
+            if self.prepare_p4_only:
+                #
+                # Leave the p4 tree prepared, and the submit template around
+                # and let the user decide what to do next
+                #
+                submitted = True
+                print("")
+                print("P4 workspace prepared for submission.")
+                print("To submit or revert, go to client workspace")
+                print("  " + self.clientPath)
+                print("")
+                print("To submit, use \"p4 submit\" to write a new description,")
+                print("or \"p4 submit -i <%s\" to use the one prepared by" \
+                      " \"git p4\"." % fileName)
+                print("You can delete the file \"%s\" when finished." % fileName)
+
+                if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
+                    print("To preserve change ownership by user %s, you must\n" \
+                          "do \"p4 change -f <change>\" after submitting and\n" \
+                          "edit the User field.")
+                if pureRenameCopy:
+                    print("After submitting, renamed files must be re-synced.")
+                    print("Invoke \"p4 sync -f\" on each of these files:")
+                    for f in pureRenameCopy:
+                        print("  " + f)
+
+                print("")
+                print("To revert the changes, use \"p4 revert ...\", and delete")
+                print("the submit template file \"%s\"" % fileName)
+                if filesToAdd:
+                    print("Since the commit adds new files, they must be deleted:")
+                    for f in filesToAdd:
+                        print("  " + f)
+                print("")
+                sys.stdout.flush()
+                return True
+
             if self.edit_template(fileName):
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
@@ -2150,7 +2150,15 @@ def applyCommit(self, id):
                 tmpFile.close()
                 if self.isWindows:
                     message = message.replace("\r\n", "\n")
-                submitTemplate = message[:message.index(separatorLine)]
+                if message.find(separatorLine) <> -1:
+                    submitTemplate = message[:message.index(separatorLine)]
+                else:
+                    submitTemplate = message
+
+                if len(submitTemplate.strip()) == 0:
+                    print("Changelist is empty, aborting this changelist.")
+                    sys.stdout.flush()
+                    return False
 
                 if update_shelve:
                     p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)
@@ -2174,19 +2182,21 @@ def applyCommit(self, id):
                 submitted = True
 
         finally:
-            # skip this patch
+            # Revert changes if we skip this patch
             if not submitted or self.shelve:
                 if self.shelve:
                     print ("Reverting shelved files.")
                 else:
                     print ("Submission cancelled, undoing p4 changes.")
+                sys.stdout.flush()
                 for f in editedFiles | filesToDelete:
                     p4_revert(f)
                 for f in filesToAdd:
                     p4_revert(f)
                     os.remove(f)
 
-        os.remove(fileName)
+            if not self.prepare_p4_only:
+                os.remove(fileName)
         return submitted
 
     # Export git tags as p4 labels. Create a p4 label and then tag
-- 
gitgitgadget

[PATCH v4 5/6] git-p4: add p4 submit hooks

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The git command "commit" supports a number of hooks that support
changing the behavior of the commit command.  The git-p4.py program only
has one existing hook, "p4-pre-submit".  This command occurs early in
the process.  There are no hooks in the process flow for modifying
the P4 changelist text programmatically.

Adds 3 new hooks to git-p4.py to the submit option.

The new hooks are:
* p4-prepare-changelist - Execute this hook after the changelist file
  has been created. The hook will be executed even if the
  --prepare-p4-only option is set.  This hook ignores the --no-verify
  option in keeping with the existing behavior of git commit.

* p4-changelist - Execute this hook after the user has edited the
  changelist. Do not execute this hook if the user has selected the
  --prepare-p4-only option. This hook will honor the --no-verify,
  following the conventions of git commit.

* p4-post-changelist - Execute this hook after the P4 submission process
  has completed successfully. This hook takes no parameters and is
  executed regardless of the --no-verify option.  It's return value will
  not be checked.

The calls to the new hooks: p4-prepare-changelist, p4-changelist,
and p4-post-changelist should all be called inside the try-finally
block.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   | 35 +++++++++++++++++++++++++++++
 Documentation/githooks.txt | 46 ++++++++++++++++++++++++++++++++++++++
 git-p4.py                  | 35 ++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 362b50eb21..dab9609013 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -387,6 +387,41 @@ It can be bypassed with the `--no-verify` command line option.
 
 One usage scenario is to run unit tests in the hook.
 
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+p4-changelist
+~~~~~~~~~~~~~
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+
 
 Rebase options
 ~~~~~~~~~~~~~~
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8cf6b08b55..8aa9c2176c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -515,6 +515,52 @@ The exit status determines whether git will use the data from the
 hook to limit its search.  On error, it will fall back to verifying
 all files and folders.
 
+p4-changelist
+~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+Run `git-p4 submit --help` for details.
+
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+Run `git-p4 submit --help` for details.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+Run `git-p4 submit --help` for details.
+
 p4-pre-submit
 ~~~~~~~~~~~~~
 
diff --git a/git-p4.py b/git-p4.py
index 0224f2e032..4b77879679 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1589,7 +1589,7 @@ def __init__(self):
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
                 optparse.make_option("--no-verify", dest="no_verify", action="store_true",
-                                     help="Bypass p4-pre-submit"),
+                                     help="Bypass p4-pre-submit and p4-changelist hooks"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
     The `p4-pre-submit` hook is executed if it exists and is executable. It
@@ -1598,6 +1598,28 @@ def __init__(self):
     from this script prevents `git-p4 submit` from launching.
 
     One usage scenario is to run unit tests in the hook.
+
+    The `p4-prepare-changelist` hook is executed right after preparing the default
+    changelist message and before the editor is started. It takes one parameter,
+    the name of the file that contains the changelist text. Exiting with a non-zero
+    status from the script will abort the process.
+
+    The purpose of the hook is to edit the message file in place, and it is not
+    supressed by the `--no-verify` option. This hook is called even if
+    `--prepare-p4-only` is set.
+
+    The `p4-changelist` hook is executed after the changelist message has been
+    edited by the user. It can be bypassed with the `--no-verify` option. It
+    takes a single parameter, the name of the file that holds the proposed
+    changelist text. Exiting with a non-zero status causes the command to abort.
+
+    The hook is allowed to edit the changelist file and can be used to normalize
+    the text into some project standard format. It can also be used to refuse the
+    Submit after inspect the message file.
+
+    The `p4-post-changelist` hook is invoked after the submit has successfully
+    occured in P4. It takes no parameters and is meant primarily for notification
+    and cannot affect the outcome of the git p4 submit action.
     """
 
         self.usage += " [name of git branch to submit into perforce depot]"
@@ -2105,6 +2127,10 @@ def applyCommit(self, id):
         submitted = False
 
         try:
+            # Allow the hook to edit the changelist text before presenting it
+            # to the user.
+            if not run_git_hook("p4-prepare-changelist", [fileName]):
+                return False
 
             if self.prepare_p4_only:
                 #
@@ -2144,6 +2170,12 @@ def applyCommit(self, id):
                 return True
 
             if self.edit_template(fileName):
+                if not self.no_verify:
+                    if not run_git_hook("p4-changelist", [fileName]):
+                        print("The p4-changelist hook failed.")
+                        sys.stdout.flush()
+                        return False
+
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
                 message = tmpFile.read()
@@ -2181,6 +2213,7 @@ def applyCommit(self, id):
 
                 submitted = True
 
+                run_git_hook("p4-post-changelist")
         finally:
             # Revert changes if we skip this patch
             if not submitted or self.shelve:
-- 
gitgitgadget

[PATCH v4 6/6] git-4: add RCS keyword status message

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

During the p4 submit process, git-p4 will attempt to apply a patch
to the files found in the p4 workspace. However, it P4 uses RCS
keyword expansion, this patch may fail.

When the patch fails, the user is alerted to the failure and that git-p4
will attempt to clear the expanded text from the files and re-apply
the patch. The current version of git-p4 does not tell the user the
result of applying the patch after the RCS expansion has been removed
which can be confusing.

Add a new print statement after the git patch has been successfully
applied when the RCS keywords have been cleansed.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 4b77879679..1c9c5270b2 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2025,6 +2025,9 @@ def applyCommit(self, id):
         applyPatchCmd = patchcmd + "--check --apply -"
         patch_succeeded = True
 
+        if verbose:
+            print("TryPatch: %s" % tryPatchCmd)
+
         if os.system(tryPatchCmd) != 0:
             fixed_rcs_keywords = False
             patch_succeeded = False
@@ -2064,6 +2067,7 @@ def applyCommit(self, id):
                 print("Retrying the patch with RCS keywords cleaned up")
                 if os.system(tryPatchCmd) == 0:
                     patch_succeeded = True
+                    print("Patch succeesed this time with RCS keywords cleaned")
 
         if not patch_succeeded:
             for f in editedFiles:
-- 
gitgitgadget

Re: [PATCH v4 2/6] git-p4: create new function run_git_hook

[Junio C Hamano]

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

> @@ -2337,12 +2405,15 @@ def run(self, args):
>              sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
>                       (len(commits), num_shelves))
>  
> -        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-submit")
> -        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
> +        try:
> +            if not run_git_hook("p4-pre-submit"):
> +                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
> +                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
> +                    "however,\nthis will also skip the p4-changelist hook as well.")
> +                sys.exit(1)
> +        except Exception as e:
> +            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
> +                "with the error '{0}'".format(e.message) )
>              sys.exit(1)

If I am reading the patches correctly, we do not have the
"--no-verify" option at this point in the series.  Shouldn't this
step limit itself to replace the removed lines we see above
with just a singlie liner:

	if not run_git_hook("p4-pre-submit"):
		sys.exit(1)

instead?

Also, af the end of these 6-patch series, I do not see a change to
guard this part of the code with "if not self.no_verify" like the
previous round did.

[PATCH v5 0/7] git-p4: add hooks for p4-changelist

[Ben Keene via GitGitGadget]

Request for help - if there are any experts with python and Windows, I would
appreciate a review of the changes that I have presented here.


----------------------------------------------------------------------------

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.

v1: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.

v2:Instead of a single hook, p4-pre-edit-changelist, follow the git
convention for hook names and add the trio of hooks that work together,
similar to git commit.

The hook names are: 

 * p4-prepare-changelist
 * p4-changelist
 * p4-post-changelist

The hooks should follow the same convention as git commit, so a new command
line option for the git-p4 submit function --no-verify should also be added.

v3:2-Feb-2020 This version reorganizes the commits to be easier to read. The
function for running the git hook has been rewritten to include suggestions.
This version no longer restricts the executable supported by windows for the
hook. It will first look for a file with the hook name without an extension
and if found, it will pass it to sh.exe (Git for Window's MINGW shell)
Otherwise it will take the file with the lowest alphabetical extension and
ask Windows to execute the program.

v3:10-Feb-2020 Integrated the suggested changes from the mailing list into
this release. The changes are:

 * Restructure the commits to pull the p4-pre-submit hook changes into the
   commit for adding git_run_hook.
 * Update the git_run_hook to include additional tests for GIT_DIR being set
   if getting the environment returns no value.
 * Updated the windows file resolution to ensure that the script to be run
   is NOT the .SAMPLE version of the hooks.
 * Split the RCS keyword cleanup message into its own commit.

Thank you in advance!

Ben Keene (7):
  git-p4: rewrite prompt to be Windows compatible
  git-p4: create new function run_git_hook
  git-p4: add p4-pre-submit exit text
  git-p4: add --no-verify option
  git-p4: restructure code in submit
  git-p4: add p4 submit hooks
  git-p4: add RCS keyword status message

 Documentation/git-p4.txt   |  45 ++++++-
 Documentation/githooks.txt |  51 +++++++-
 git-p4.py                  | 236 ++++++++++++++++++++++++++++---------
 3 files changed, 274 insertions(+), 58 deletions(-)


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

Range-diff vs v4:

 1:  71c51ccfb0 = 1:  71c51ccfb0 git-p4: rewrite prompt to be Windows compatible
 2:  596b18e5e5 ! 2:  a2342f7161 git-p4: create new function run_git_hook
     @@ -152,15 +152,7 @@
      -
      -        hook_file = os.path.join(hooks_path, "p4-pre-submit")
      -        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
     -+        try:
     -+            if not run_git_hook("p4-pre-submit"):
     -+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
     -+                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
     -+                    "however,\nthis will also skip the p4-changelist hook as well.")
     -+                sys.exit(1)
     -+        except Exception as e:
     -+            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
     -+                "with the error '{0}'".format(e.message) )
     ++        if not run_git_hook("p4-pre-submit"):
                   sys.exit(1)
       
               #
 -:  ---------- > 3:  5f2b47bf6a git-p4: add p4-pre-submit exit text
 3:  57a6166ed1 ! 4:  05b8c7ad67 git-p4: add --no-verify option
     @@ -95,3 +95,30 @@
       
               if gitConfig('git-p4.largeFileSystem'):
                   die("Large file system not supported for git-p4 submit command. Please remove it from config.")
     +@@
     +             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
     +                      (len(commits), num_shelves))
     + 
     +-        try:
     +-            if not run_git_hook("p4-pre-submit"):
     +-                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
     +-                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
     +-                    "however,\nthis will also skip the p4-changelist hook as well.")
     ++        if not self.no_verify:
     ++            try:
     ++                if not run_git_hook("p4-pre-submit"):
     ++                    print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
     ++                        "this pre-submission check by adding\nthe command line option '--no-verify', " \
     ++                        "however,\nthis will also skip the p4-changelist hook as well.")
     ++                    sys.exit(1)
     ++            except Exception as e:
     ++                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
     ++                    "with the error '{0}'".format(e.message) )
     +                 sys.exit(1)
     +-        except Exception as e:
     +-            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
     +-                "with the error '{0}'".format(e.message) )
     +-            sys.exit(1)
     + 
     +         #
     +         # Apply the commits, one at a time.  On failure, ask if should
 4:  d17e032767 = 5:  5299f61e4e git-p4: restructure code in submit
 5:  ceb2b0aff6 = 6:  7a6d9fafce git-p4: add p4 submit hooks
 6:  b92e017cb1 ! 7:  c0aca43185 git-4: add RCS keyword status message
     @@ -1,15 +1,15 @@
      Author: Ben Keene <seraphire@gmail.com>
      
     -    git-4: add RCS keyword status message
     +    git-p4: add RCS keyword status message
      
          During the p4 submit process, git-p4 will attempt to apply a patch
     -    to the files found in the p4 workspace. However, it P4 uses RCS
     +    to the files found in the p4 workspace. However, if P4 uses RCS
          keyword expansion, this patch may fail.
      
          When the patch fails, the user is alerted to the failure and that git-p4
          will attempt to clear the expanded text from the files and re-apply
          the patch. The current version of git-p4 does not tell the user the
     -    result of applying the patch after the RCS expansion has been removed
     +    result of the re-apply attempt after the RCS expansion has been removed
          which can be confusing.
      
          Add a new print statement after the git patch has been successfully

-- 
gitgitgadget

[PATCH v5 1/7] git-p4: rewrite prompt to be Windows compatible

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The existing function prompt(prompt_text) does not work correctly when
run on Windows 10 bash terminal when launched from the sourcetree
GUI application. The stdout is not flushed properly so the prompt text
is not displayed to the user until the next flush of stdout, which is
quite confusing.

Change this method by:
* Adding flush to stderr, stdout, and stdin
* Use readline from sys.stdin instead of raw_input.

The existing strip().lower() are retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 40d9e7c594..65b6d4dca0 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -175,7 +175,10 @@ def prompt(prompt_text):
     """
     choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))
     while True:
-        response = raw_input(prompt_text).strip().lower()
+        sys.stderr.flush()
+        sys.stdout.write(prompt_text)
+        sys.stdout.flush()
+        response=sys.stdin.readline().strip().lower()
         if not response:
             continue
         response = response[0]
-- 
gitgitgadget

[PATCH v5 2/7] git-p4: create new function run_git_hook

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

This commit is in preparation of introducing new p4 submit hooks.

The current code in the python script git-p4.py makes the assumption
that the git hooks can be executed by subprocess.call() function.
However, when git is run on Windows, this may not work as expected.

The subprocess.call() does not cover all the use cases for properly
executing the various types of executable files on Windows.

Prepare for remediation by adding a new function, run_git_hook, that
takes 2 parameters:
* the short filename of an optionally registered git hook
* an optional list of parameters

The run_git_hook function will honor the existing behavior seen in the
current code for executing the p4-pre-submit hook:

* Hooks are looked for in core.hooksPath directory.
* If core.hooksPath is not set, then the current .git/hooks directory
  is checked.
* If the hook does not exist, the function returns True.
* If the hook file is not accessible, the function returns True.
* If the hook returns a zero exit code when executed, the function
  return True.
* If the hook returns a non-zero exit code, the function returns False.

Add the following additional functionality if git-p4.py is run on
Windows.
* If hook file is not located without an extension, search for
  any file in the associated hook directory (from the list above) that
  has the same name but with an extension.
* If the file is still not found, return True (the hook is missing)

Add a new function run_hook_command() that wraps the OS dependent
functionality for actually running the subprocess.call() with OS
dependent behavior:

If a hook file exists on Windows:
* If there is no extension, set the launch executable to be SH.EXE
  - Look for SH.EXE under the environmental variable EXEPATH in the
    bin/ directory.
  - If %EXEPATH%/bin/sh.exe exists, use this as the actual executable.
  - If %EXEPATH%/bin/sh.exe does not exist, use sh.exe
  - Execute subprocess.call() without the shell (shell=False)
* If there is an extension, execute subprocess.call() with teh shell
  (shell=True) and consider the file to be the executable.

The return value from run_hook_command() is the subprocess.call()
return value.

These functions are added in this commit, but are only staged and not
yet used.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 65b6d4dca0..4eccea3fa5 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -26,6 +26,7 @@
 import zlib
 import ctypes
 import errno
+import glob
 
 # support basestring in python3
 try:
@@ -185,6 +186,73 @@ def prompt(prompt_text):
         if response in choices:
             return response
 
+def run_git_hook(cmd, param=[]):
+    """Execute a hook if the hook exists."""
+    if verbose:
+        sys.stderr.write("Looking for hook: %s\n" % cmd)
+        sys.stderr.flush()
+
+    hooks_path = gitConfig("core.hooksPath")
+    if len(hooks_path) <= 0:
+        hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
+
+    if not isinstance(param, list):
+        param=[param]
+
+    # resolve hook file name, OS depdenent
+    hook_file = os.path.join(hooks_path, cmd)
+    if platform.system() == 'Windows':
+        if not os.path.isfile(hook_file):
+            # look for the file with an extension
+            files = glob.glob(hook_file + ".*")
+            if not files:
+                return True
+            files.sort()
+            hook_file = files.pop()
+            while hook_file.upper().endswith(".SAMPLE"):
+                # The file is a sample hook. We don't want it
+                if len(files) > 0:
+                    hook_file = files.pop()
+                else:
+                    return True
+
+    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
+        return True
+
+    return run_hook_command(hook_file, param) == 0
+
+def run_hook_command(cmd, param):
+    """Executes a git hook command
+       cmd = the command line file to be executed. This can be
+       a file that is run by OS association.
+
+       param = a list of parameters to pass to the cmd command
+
+       On windows, the extension is checked to see if it should
+       be run with the Git for Windows Bash shell.  If there
+       is no file extension, the file is deemed a bash shell
+       and will be handed off to sh.exe. Otherwise, Windows
+       will be called with the shell to handle the file assocation.
+
+       For non Windows operating systems, the file is called
+       as an executable.
+    """
+    cli = [cmd] + param
+    use_shell = False
+    if platform.system() == 'Windows':
+        (root,ext) = os.path.splitext(cmd)
+        if ext == "":
+            exe_path = os.environ.get("EXEPATH")
+            if exe_path is None:
+                exe_path = ""
+            else:
+                exe_path = os.path.join(exe_path, "bin")
+            cli = [os.path.join(exe_path, "SH.EXE")] + cli
+        else:
+            use_shell = True
+    return subprocess.call(cli, shell=use_shell)
+
+
 def write_pipe(c, stdin):
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))
@@ -2337,12 +2405,7 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        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-submit")
-        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
+        if not run_git_hook("p4-pre-submit"):
             sys.exit(1)
 
         #
@@ -4124,7 +4187,6 @@ def printUsage(commands):
     "unshelve" : P4Unshelve,
 }
 
-
 def main():
     if len(sys.argv[1:]) == 0:
         printUsage(commands.keys())
-- 
gitgitgadget

[PATCH v5 3/7] git-p4: add p4-pre-submit exit text

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

When the p4-pre-submit exits with a non-zero exit code, the application
will abort the process with no additional information presented to the
user. This can be confusing for new users as it may not be clear that
the p4-pre-submit action caused the error.

Add text to explain why the program aborted the submit action.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 4eccea3fa5..b1c86678fc 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2405,7 +2405,15 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        if not run_git_hook("p4-pre-submit"):
+        try:
+            if not run_git_hook("p4-pre-submit"):
+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
+                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
+                    "however,\nthis will also skip the p4-changelist hook as well.")
+                sys.exit(1)
+        except Exception as e:
+            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
+                "with the error '{0}'".format(e.message) )
             sys.exit(1)
 
         #
-- 
gitgitgadget

[PATCH v5 4/7] git-p4: add --no-verify option

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

Add new command line option --no-verify:

Add a new command line option "--no-verify" to the Submit command of
git-p4.py.  This option will function in the spirit of the existing
--no-verify command line option found in git commit. It will cause the
P4 Submit function to ignore the existing p4-pre-submit.

Change the execution of the existing trigger p4-pre-submit to honor the
--no-verify option. Before exiting on failure of this hook, display
text to the user explaining which hook has failed and the impact
of using the --no-verify option.

Change the call of the p4-pre-submit hook to use the new run_git_hook
function. This is in preparation of additional hooks to be added.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   | 10 ++++++++--
 Documentation/githooks.txt |  5 ++++-
 git-p4.py                  | 32 +++++++++++++++++++-------------
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3494a1db3e..362b50eb21 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -374,14 +374,20 @@ These options can be used to modify 'git p4 submit' behavior.
     been submitted. Implies --disable-rebase. Can also be set with
     git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
 
-Hook for submit
-~~~~~~~~~~~~~~~
+Hooks for submit
+----------------
+
+p4-pre-submit
+~~~~~~~~~~~~~
+
 The `p4-pre-submit` hook is executed if it exists and is executable.
 The hook takes no parameters and nothing from standard input. Exiting with
 non-zero status from this script prevents `git-p4 submit` from launching.
+It can be bypassed with the `--no-verify` command line option.
 
 One usage scenario is to run unit tests in the hook.
 
+
 Rebase options
 ~~~~~~~~~~~~~~
 These options can be used to modify 'git p4 rebase' behavior.
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 50365f2914..8cf6b08b55 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -520,7 +520,10 @@ p4-pre-submit
 
 This hook is invoked by `git-p4 submit`. It takes no parameters and nothing
 from standard input. Exiting with non-zero status from this script prevent
-`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
+`git-p4 submit` from launching. It can be bypassed with the `--no-verify`
+command line option. Run `git-p4 submit --help` for details.
+
+
 
 post-index-change
 ~~~~~~~~~~~~~~~~~
diff --git a/git-p4.py b/git-p4.py
index b1c86678fc..c969d9235b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1588,13 +1588,17 @@ def __init__(self):
                                      "work from a local git branch that is not master"),
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
+                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
+                                     help="Bypass p4-pre-submit"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
-    The `p4-pre-submit` hook is executed if it exists and is executable.
-    The hook takes no parameters and nothing from standard input. Exiting with
-    non-zero status from this script prevents `git-p4 submit` from launching.
+    The `p4-pre-submit` hook is executed if it exists and is executable. It
+    can be bypassed with the `--no-verify` command line option. The hook takes
+    no parameters and nothing from standard input. Exiting with a non-zero status
+    from this script prevents `git-p4 submit` from launching.
 
-    One usage scenario is to run unit tests in the hook."""
+    One usage scenario is to run unit tests in the hook.
+    """
 
         self.usage += " [name of git branch to submit into perforce depot]"
         self.origin = ""
@@ -1612,6 +1616,7 @@ def __init__(self):
         self.exportLabels = False
         self.p4HasMoveCommand = p4_has_move_command()
         self.branch = None
+        self.no_verify = False
 
         if gitConfig('git-p4.largeFileSystem'):
             die("Large file system not supported for git-p4 submit command. Please remove it from config.")
@@ -2405,16 +2410,17 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        try:
-            if not run_git_hook("p4-pre-submit"):
-                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
-                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
-                    "however,\nthis will also skip the p4-changelist hook as well.")
+        if not self.no_verify:
+            try:
+                if not run_git_hook("p4-pre-submit"):
+                    print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
+                        "this pre-submission check by adding\nthe command line option '--no-verify', " \
+                        "however,\nthis will also skip the p4-changelist hook as well.")
+                    sys.exit(1)
+            except Exception as e:
+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
+                    "with the error '{0}'".format(e.message) )
                 sys.exit(1)
-        except Exception as e:
-            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
-                "with the error '{0}'".format(e.message) )
-            sys.exit(1)
 
         #
         # Apply the commits, one at a time.  On failure, ask if should
-- 
gitgitgadget

[PATCH v5 5/7] git-p4: restructure code in submit

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

In preparation for adding new hooks to the submit method of git-p4,
restructure the applyCommit function in the P4Submit class.  Make the
following changes:

* Move all the code after the definition of submitted = False into the
  Try-Finally block. This ensures that any error that occurs will
  properly recover.  This is not necessary with the current code because
  none of it should throw an exception, however the next set of changes
  will introduce exceptional code.

  Existing flow control can remain as defined - the if-block for
  prepare-p4-only sets the local variable "submitted" to True and exits
  the function. New early exits, leave submitted set to False so the
  Finally block will undo changes to the P4 workspace.

* Make the small usability change of adding an empty string to the
  print statements displayed to the user when the prepare-p4-only option
  is selected.  On Windows, the command print() may display a set of
  parentheses "()" to the user when the print() function is called with
  no parameters. By supplying an empty string, the intended blank line
  will print as expected.

* Fix a small bug when the submittedTemplate is edited by the user
  and all content in the file is removed. The existing code will throw
  an exception if the separateLine is not found in the file. Change this
  code to test for the separator line using a find() test first and only
  split on the separator if it is found.

* Additionally, add the new behavior that if the changelist file has
  been completely emptied that the Submit action for this changelist
  will be aborted.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 92 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index c969d9235b..b6da7bcb02 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2102,47 +2102,47 @@ def applyCommit(self, id):
         tmpFile.write(submitTemplate)
         tmpFile.close()
 
-        if self.prepare_p4_only:
-            #
-            # Leave the p4 tree prepared, and the submit template around
-            # and let the user decide what to do next
-            #
-            print()
-            print("P4 workspace prepared for submission.")
-            print("To submit or revert, go to client workspace")
-            print("  " + self.clientPath)
-            print()
-            print("To submit, use \"p4 submit\" to write a new description,")
-            print("or \"p4 submit -i <%s\" to use the one prepared by" \
-                  " \"git p4\"." % fileName)
-            print("You can delete the file \"%s\" when finished." % fileName)
-
-            if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
-                print("To preserve change ownership by user %s, you must\n" \
-                      "do \"p4 change -f <change>\" after submitting and\n" \
-                      "edit the User field.")
-            if pureRenameCopy:
-                print("After submitting, renamed files must be re-synced.")
-                print("Invoke \"p4 sync -f\" on each of these files:")
-                for f in pureRenameCopy:
-                    print("  " + f)
-
-            print()
-            print("To revert the changes, use \"p4 revert ...\", and delete")
-            print("the submit template file \"%s\"" % fileName)
-            if filesToAdd:
-                print("Since the commit adds new files, they must be deleted:")
-                for f in filesToAdd:
-                    print("  " + f)
-            print()
-            return True
-
-        #
-        # Let the user edit the change description, then submit it.
-        #
         submitted = False
 
         try:
+
+            if self.prepare_p4_only:
+                #
+                # Leave the p4 tree prepared, and the submit template around
+                # and let the user decide what to do next
+                #
+                submitted = True
+                print("")
+                print("P4 workspace prepared for submission.")
+                print("To submit or revert, go to client workspace")
+                print("  " + self.clientPath)
+                print("")
+                print("To submit, use \"p4 submit\" to write a new description,")
+                print("or \"p4 submit -i <%s\" to use the one prepared by" \
+                      " \"git p4\"." % fileName)
+                print("You can delete the file \"%s\" when finished." % fileName)
+
+                if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
+                    print("To preserve change ownership by user %s, you must\n" \
+                          "do \"p4 change -f <change>\" after submitting and\n" \
+                          "edit the User field.")
+                if pureRenameCopy:
+                    print("After submitting, renamed files must be re-synced.")
+                    print("Invoke \"p4 sync -f\" on each of these files:")
+                    for f in pureRenameCopy:
+                        print("  " + f)
+
+                print("")
+                print("To revert the changes, use \"p4 revert ...\", and delete")
+                print("the submit template file \"%s\"" % fileName)
+                if filesToAdd:
+                    print("Since the commit adds new files, they must be deleted:")
+                    for f in filesToAdd:
+                        print("  " + f)
+                print("")
+                sys.stdout.flush()
+                return True
+
             if self.edit_template(fileName):
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
@@ -2150,7 +2150,15 @@ def applyCommit(self, id):
                 tmpFile.close()
                 if self.isWindows:
                     message = message.replace("\r\n", "\n")
-                submitTemplate = message[:message.index(separatorLine)]
+                if message.find(separatorLine) <> -1:
+                    submitTemplate = message[:message.index(separatorLine)]
+                else:
+                    submitTemplate = message
+
+                if len(submitTemplate.strip()) == 0:
+                    print("Changelist is empty, aborting this changelist.")
+                    sys.stdout.flush()
+                    return False
 
                 if update_shelve:
                     p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)
@@ -2174,19 +2182,21 @@ def applyCommit(self, id):
                 submitted = True
 
         finally:
-            # skip this patch
+            # Revert changes if we skip this patch
             if not submitted or self.shelve:
                 if self.shelve:
                     print ("Reverting shelved files.")
                 else:
                     print ("Submission cancelled, undoing p4 changes.")
+                sys.stdout.flush()
                 for f in editedFiles | filesToDelete:
                     p4_revert(f)
                 for f in filesToAdd:
                     p4_revert(f)
                     os.remove(f)
 
-        os.remove(fileName)
+            if not self.prepare_p4_only:
+                os.remove(fileName)
         return submitted
 
     # Export git tags as p4 labels. Create a p4 label and then tag
-- 
gitgitgadget

[PATCH v5 6/7] git-p4: add p4 submit hooks

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The git command "commit" supports a number of hooks that support
changing the behavior of the commit command.  The git-p4.py program only
has one existing hook, "p4-pre-submit".  This command occurs early in
the process.  There are no hooks in the process flow for modifying
the P4 changelist text programmatically.

Adds 3 new hooks to git-p4.py to the submit option.

The new hooks are:
* p4-prepare-changelist - Execute this hook after the changelist file
  has been created. The hook will be executed even if the
  --prepare-p4-only option is set.  This hook ignores the --no-verify
  option in keeping with the existing behavior of git commit.

* p4-changelist - Execute this hook after the user has edited the
  changelist. Do not execute this hook if the user has selected the
  --prepare-p4-only option. This hook will honor the --no-verify,
  following the conventions of git commit.

* p4-post-changelist - Execute this hook after the P4 submission process
  has completed successfully. This hook takes no parameters and is
  executed regardless of the --no-verify option.  It's return value will
  not be checked.

The calls to the new hooks: p4-prepare-changelist, p4-changelist,
and p4-post-changelist should all be called inside the try-finally
block.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   | 35 +++++++++++++++++++++++++++++
 Documentation/githooks.txt | 46 ++++++++++++++++++++++++++++++++++++++
 git-p4.py                  | 35 ++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 362b50eb21..dab9609013 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -387,6 +387,41 @@ It can be bypassed with the `--no-verify` command line option.
 
 One usage scenario is to run unit tests in the hook.
 
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+p4-changelist
+~~~~~~~~~~~~~
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+
 
 Rebase options
 ~~~~~~~~~~~~~~
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8cf6b08b55..8aa9c2176c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -515,6 +515,52 @@ The exit status determines whether git will use the data from the
 hook to limit its search.  On error, it will fall back to verifying
 all files and folders.
 
+p4-changelist
+~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+Run `git-p4 submit --help` for details.
+
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+Run `git-p4 submit --help` for details.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+Run `git-p4 submit --help` for details.
+
 p4-pre-submit
 ~~~~~~~~~~~~~
 
diff --git a/git-p4.py b/git-p4.py
index b6da7bcb02..f14c71c97d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1589,7 +1589,7 @@ def __init__(self):
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
                 optparse.make_option("--no-verify", dest="no_verify", action="store_true",
-                                     help="Bypass p4-pre-submit"),
+                                     help="Bypass p4-pre-submit and p4-changelist hooks"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
     The `p4-pre-submit` hook is executed if it exists and is executable. It
@@ -1598,6 +1598,28 @@ def __init__(self):
     from this script prevents `git-p4 submit` from launching.
 
     One usage scenario is to run unit tests in the hook.
+
+    The `p4-prepare-changelist` hook is executed right after preparing the default
+    changelist message and before the editor is started. It takes one parameter,
+    the name of the file that contains the changelist text. Exiting with a non-zero
+    status from the script will abort the process.
+
+    The purpose of the hook is to edit the message file in place, and it is not
+    supressed by the `--no-verify` option. This hook is called even if
+    `--prepare-p4-only` is set.
+
+    The `p4-changelist` hook is executed after the changelist message has been
+    edited by the user. It can be bypassed with the `--no-verify` option. It
+    takes a single parameter, the name of the file that holds the proposed
+    changelist text. Exiting with a non-zero status causes the command to abort.
+
+    The hook is allowed to edit the changelist file and can be used to normalize
+    the text into some project standard format. It can also be used to refuse the
+    Submit after inspect the message file.
+
+    The `p4-post-changelist` hook is invoked after the submit has successfully
+    occured in P4. It takes no parameters and is meant primarily for notification
+    and cannot affect the outcome of the git p4 submit action.
     """
 
         self.usage += " [name of git branch to submit into perforce depot]"
@@ -2105,6 +2127,10 @@ def applyCommit(self, id):
         submitted = False
 
         try:
+            # Allow the hook to edit the changelist text before presenting it
+            # to the user.
+            if not run_git_hook("p4-prepare-changelist", [fileName]):
+                return False
 
             if self.prepare_p4_only:
                 #
@@ -2144,6 +2170,12 @@ def applyCommit(self, id):
                 return True
 
             if self.edit_template(fileName):
+                if not self.no_verify:
+                    if not run_git_hook("p4-changelist", [fileName]):
+                        print("The p4-changelist hook failed.")
+                        sys.stdout.flush()
+                        return False
+
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
                 message = tmpFile.read()
@@ -2181,6 +2213,7 @@ def applyCommit(self, id):
 
                 submitted = True
 
+                run_git_hook("p4-post-changelist")
         finally:
             # Revert changes if we skip this patch
             if not submitted or self.shelve:
-- 
gitgitgadget

[PATCH v5 7/7] git-p4: add RCS keyword status message

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

During the p4 submit process, git-p4 will attempt to apply a patch
to the files found in the p4 workspace. However, if P4 uses RCS
keyword expansion, this patch may fail.

When the patch fails, the user is alerted to the failure and that git-p4
will attempt to clear the expanded text from the files and re-apply
the patch. The current version of git-p4 does not tell the user the
result of the re-apply attempt after the RCS expansion has been removed
which can be confusing.

Add a new print statement after the git patch has been successfully
applied when the RCS keywords have been cleansed.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index f14c71c97d..f1fb2222a4 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2025,6 +2025,9 @@ def applyCommit(self, id):
         applyPatchCmd = patchcmd + "--check --apply -"
         patch_succeeded = True
 
+        if verbose:
+            print("TryPatch: %s" % tryPatchCmd)
+
         if os.system(tryPatchCmd) != 0:
             fixed_rcs_keywords = False
             patch_succeeded = False
@@ -2064,6 +2067,7 @@ def applyCommit(self, id):
                 print("Retrying the patch with RCS keywords cleaned up")
                 if os.system(tryPatchCmd) == 0:
                     patch_succeeded = True
+                    print("Patch succeesed this time with RCS keywords cleaned")
 
         if not patch_succeeded:
             for f in editedFiles:
-- 
gitgitgadget

[PATCH v6 0/7] git-p4: add hooks for p4-changelist

[Ben Keene via GitGitGadget]

Request for help - if there are any experts with python and Windows, I would
appreciate a review of the changes that I have presented here.


----------------------------------------------------------------------------

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.

v1: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.

v2:Instead of a single hook, p4-pre-edit-changelist, follow the git
convention for hook names and add the trio of hooks that work together,
similar to git commit.

The hook names are: 

 * p4-prepare-changelist
 * p4-changelist
 * p4-post-changelist

The hooks should follow the same convention as git commit, so a new command
line option for the git-p4 submit function --no-verify should also be added.

v3:2-Feb-2020 This version reorganizes the commits to be easier to read. The
function for running the git hook has been rewritten to include suggestions.
This version no longer restricts the executable supported by windows for the
hook. It will first look for a file with the hook name without an extension
and if found, it will pass it to sh.exe (Git for Window's MINGW shell)
Otherwise it will take the file with the lowest alphabetical extension and
ask Windows to execute the program.

v5:10-Feb-2020 Integrated the suggested changes from the mailing list into
this release. The changes are:

 * Restructure the commits to pull the p4-pre-submit hook changes into the
   commit for adding git_run_hook.
 * Update the git_run_hook to include additional tests for GIT_DIR being set
   if getting the environment returns no value.
 * Updated the windows file resolution to ensure that the script to be run
   is NOT the .SAMPLE version of the hooks.
 * Split the RCS keyword cleanup message into its own commit.

v6:
14-Feb-2020 Update commit for git-p4: restructure code in submit to be
python 3 compatible:

> This change comes from 'git-p4: restructure code in submit' in
> 'bk/p4-pre-edit-changelist' which introduced the use of the <>operator. In
> Python 2, this is valid but in Python 3, it was removed.
>
> We can simply replace the <> with != which is the new way of writing "not
> equals".

Ben Keene (7):
  git-p4: rewrite prompt to be Windows compatible
  git-p4: create new function run_git_hook
  git-p4: add p4-pre-submit exit text
  git-p4: add --no-verify option
  git-p4: restructure code in submit
  git-p4: add p4 submit hooks
  git-p4: add RCS keyword status message

 Documentation/git-p4.txt   |  45 ++++++-
 Documentation/githooks.txt |  51 +++++++-
 git-p4.py                  | 236 ++++++++++++++++++++++++++++---------
 3 files changed, 274 insertions(+), 58 deletions(-)


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

Range-diff vs v5:

 1:  71c51ccfb0c = 1:  013b27760a0 git-p4: rewrite prompt to be Windows compatible
 2:  a2342f71619 = 2:  72c3d642a97 git-p4: create new function run_git_hook
 3:  5f2b47bf6aa = 3:  ae1a4614ee4 git-p4: add p4-pre-submit exit text
 4:  05b8c7ad673 = 4:  cf4de224f8a git-p4: add --no-verify option
 5:  5299f61e4e1 ! 5:  22ac4384bd5 git-p4: restructure code in submit
     @@ -130,7 +130,7 @@
                       if self.isWindows:
                           message = message.replace("\r\n", "\n")
      -                submitTemplate = message[:message.index(separatorLine)]
     -+                if message.find(separatorLine) <> -1:
     ++                if message.find(separatorLine) != -1:
      +                    submitTemplate = message[:message.index(separatorLine)]
      +                else:
      +                    submitTemplate = message
 6:  7a6d9fafcea = 6:  a240b292a17 git-p4: add p4 submit hooks
 7:  c0aca431858 = 7:  bdf07d8f7aa git-p4: add RCS keyword status message

-- 
gitgitgadget

[PATCH v6 1/7] git-p4: rewrite prompt to be Windows compatible

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The existing function prompt(prompt_text) does not work correctly when
run on Windows 10 bash terminal when launched from the sourcetree
GUI application. The stdout is not flushed properly so the prompt text
is not displayed to the user until the next flush of stdout, which is
quite confusing.

Change this method by:
* Adding flush to stderr, stdout, and stdin
* Use readline from sys.stdin instead of raw_input.

The existing strip().lower() are retained.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 40d9e7c594e..65b6d4dca08 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -175,7 +175,10 @@ def prompt(prompt_text):
     """
     choices = set(m.group(1) for m in re.finditer(r"\[(.)\]", prompt_text))
     while True:
-        response = raw_input(prompt_text).strip().lower()
+        sys.stderr.flush()
+        sys.stdout.write(prompt_text)
+        sys.stdout.flush()
+        response=sys.stdin.readline().strip().lower()
         if not response:
             continue
         response = response[0]
-- 
gitgitgadget

[PATCH v6 2/7] git-p4: create new function run_git_hook

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

This commit is in preparation of introducing new p4 submit hooks.

The current code in the python script git-p4.py makes the assumption
that the git hooks can be executed by subprocess.call() function.
However, when git is run on Windows, this may not work as expected.

The subprocess.call() does not cover all the use cases for properly
executing the various types of executable files on Windows.

Prepare for remediation by adding a new function, run_git_hook, that
takes 2 parameters:
* the short filename of an optionally registered git hook
* an optional list of parameters

The run_git_hook function will honor the existing behavior seen in the
current code for executing the p4-pre-submit hook:

* Hooks are looked for in core.hooksPath directory.
* If core.hooksPath is not set, then the current .git/hooks directory
  is checked.
* If the hook does not exist, the function returns True.
* If the hook file is not accessible, the function returns True.
* If the hook returns a zero exit code when executed, the function
  return True.
* If the hook returns a non-zero exit code, the function returns False.

Add the following additional functionality if git-p4.py is run on
Windows.
* If hook file is not located without an extension, search for
  any file in the associated hook directory (from the list above) that
  has the same name but with an extension.
* If the file is still not found, return True (the hook is missing)

Add a new function run_hook_command() that wraps the OS dependent
functionality for actually running the subprocess.call() with OS
dependent behavior:

If a hook file exists on Windows:
* If there is no extension, set the launch executable to be SH.EXE
  - Look for SH.EXE under the environmental variable EXEPATH in the
    bin/ directory.
  - If %EXEPATH%/bin/sh.exe exists, use this as the actual executable.
  - If %EXEPATH%/bin/sh.exe does not exist, use sh.exe
  - Execute subprocess.call() without the shell (shell=False)
* If there is an extension, execute subprocess.call() with teh shell
  (shell=True) and consider the file to be the executable.

The return value from run_hook_command() is the subprocess.call()
return value.

These functions are added in this commit, but are only staged and not
yet used.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 65b6d4dca08..4eccea3fa58 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -26,6 +26,7 @@
 import zlib
 import ctypes
 import errno
+import glob
 
 # support basestring in python3
 try:
@@ -185,6 +186,73 @@ def prompt(prompt_text):
         if response in choices:
             return response
 
+def run_git_hook(cmd, param=[]):
+    """Execute a hook if the hook exists."""
+    if verbose:
+        sys.stderr.write("Looking for hook: %s\n" % cmd)
+        sys.stderr.flush()
+
+    hooks_path = gitConfig("core.hooksPath")
+    if len(hooks_path) <= 0:
+        hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
+
+    if not isinstance(param, list):
+        param=[param]
+
+    # resolve hook file name, OS depdenent
+    hook_file = os.path.join(hooks_path, cmd)
+    if platform.system() == 'Windows':
+        if not os.path.isfile(hook_file):
+            # look for the file with an extension
+            files = glob.glob(hook_file + ".*")
+            if not files:
+                return True
+            files.sort()
+            hook_file = files.pop()
+            while hook_file.upper().endswith(".SAMPLE"):
+                # The file is a sample hook. We don't want it
+                if len(files) > 0:
+                    hook_file = files.pop()
+                else:
+                    return True
+
+    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
+        return True
+
+    return run_hook_command(hook_file, param) == 0
+
+def run_hook_command(cmd, param):
+    """Executes a git hook command
+       cmd = the command line file to be executed. This can be
+       a file that is run by OS association.
+
+       param = a list of parameters to pass to the cmd command
+
+       On windows, the extension is checked to see if it should
+       be run with the Git for Windows Bash shell.  If there
+       is no file extension, the file is deemed a bash shell
+       and will be handed off to sh.exe. Otherwise, Windows
+       will be called with the shell to handle the file assocation.
+
+       For non Windows operating systems, the file is called
+       as an executable.
+    """
+    cli = [cmd] + param
+    use_shell = False
+    if platform.system() == 'Windows':
+        (root,ext) = os.path.splitext(cmd)
+        if ext == "":
+            exe_path = os.environ.get("EXEPATH")
+            if exe_path is None:
+                exe_path = ""
+            else:
+                exe_path = os.path.join(exe_path, "bin")
+            cli = [os.path.join(exe_path, "SH.EXE")] + cli
+        else:
+            use_shell = True
+    return subprocess.call(cli, shell=use_shell)
+
+
 def write_pipe(c, stdin):
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))
@@ -2337,12 +2405,7 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        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-submit")
-        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
+        if not run_git_hook("p4-pre-submit"):
             sys.exit(1)
 
         #
@@ -4124,7 +4187,6 @@ def printUsage(commands):
     "unshelve" : P4Unshelve,
 }
 
-
 def main():
     if len(sys.argv[1:]) == 0:
         printUsage(commands.keys())
-- 
gitgitgadget

[PATCH v6 3/7] git-p4: add p4-pre-submit exit text

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

When the p4-pre-submit exits with a non-zero exit code, the application
will abort the process with no additional information presented to the
user. This can be confusing for new users as it may not be clear that
the p4-pre-submit action caused the error.

Add text to explain why the program aborted the submit action.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 4eccea3fa58..b1c86678fc6 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2405,7 +2405,15 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        if not run_git_hook("p4-pre-submit"):
+        try:
+            if not run_git_hook("p4-pre-submit"):
+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
+                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
+                    "however,\nthis will also skip the p4-changelist hook as well.")
+                sys.exit(1)
+        except Exception as e:
+            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
+                "with the error '{0}'".format(e.message) )
             sys.exit(1)
 
         #
-- 
gitgitgadget

[PATCH v6 4/7] git-p4: add --no-verify option

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

Add new command line option --no-verify:

Add a new command line option "--no-verify" to the Submit command of
git-p4.py.  This option will function in the spirit of the existing
--no-verify command line option found in git commit. It will cause the
P4 Submit function to ignore the existing p4-pre-submit.

Change the execution of the existing trigger p4-pre-submit to honor the
--no-verify option. Before exiting on failure of this hook, display
text to the user explaining which hook has failed and the impact
of using the --no-verify option.

Change the call of the p4-pre-submit hook to use the new run_git_hook
function. This is in preparation of additional hooks to be added.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   | 10 ++++++++--
 Documentation/githooks.txt |  5 ++++-
 git-p4.py                  | 32 +++++++++++++++++++-------------
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3494a1db3eb..362b50eb212 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -374,14 +374,20 @@ These options can be used to modify 'git p4 submit' behavior.
     been submitted. Implies --disable-rebase. Can also be set with
     git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
 
-Hook for submit
-~~~~~~~~~~~~~~~
+Hooks for submit
+----------------
+
+p4-pre-submit
+~~~~~~~~~~~~~
+
 The `p4-pre-submit` hook is executed if it exists and is executable.
 The hook takes no parameters and nothing from standard input. Exiting with
 non-zero status from this script prevents `git-p4 submit` from launching.
+It can be bypassed with the `--no-verify` command line option.
 
 One usage scenario is to run unit tests in the hook.
 
+
 Rebase options
 ~~~~~~~~~~~~~~
 These options can be used to modify 'git p4 rebase' behavior.
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 50365f2914e..8cf6b08b55b 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -520,7 +520,10 @@ p4-pre-submit
 
 This hook is invoked by `git-p4 submit`. It takes no parameters and nothing
 from standard input. Exiting with non-zero status from this script prevent
-`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
+`git-p4 submit` from launching. It can be bypassed with the `--no-verify`
+command line option. Run `git-p4 submit --help` for details.
+
+
 
 post-index-change
 ~~~~~~~~~~~~~~~~~
diff --git a/git-p4.py b/git-p4.py
index b1c86678fc6..c969d9235b1 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1588,13 +1588,17 @@ def __init__(self):
                                      "work from a local git branch that is not master"),
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
+                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
+                                     help="Bypass p4-pre-submit"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
-    The `p4-pre-submit` hook is executed if it exists and is executable.
-    The hook takes no parameters and nothing from standard input. Exiting with
-    non-zero status from this script prevents `git-p4 submit` from launching.
+    The `p4-pre-submit` hook is executed if it exists and is executable. It
+    can be bypassed with the `--no-verify` command line option. The hook takes
+    no parameters and nothing from standard input. Exiting with a non-zero status
+    from this script prevents `git-p4 submit` from launching.
 
-    One usage scenario is to run unit tests in the hook."""
+    One usage scenario is to run unit tests in the hook.
+    """
 
         self.usage += " [name of git branch to submit into perforce depot]"
         self.origin = ""
@@ -1612,6 +1616,7 @@ def __init__(self):
         self.exportLabels = False
         self.p4HasMoveCommand = p4_has_move_command()
         self.branch = None
+        self.no_verify = False
 
         if gitConfig('git-p4.largeFileSystem'):
             die("Large file system not supported for git-p4 submit command. Please remove it from config.")
@@ -2405,16 +2410,17 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
-        try:
-            if not run_git_hook("p4-pre-submit"):
-                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
-                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
-                    "however,\nthis will also skip the p4-changelist hook as well.")
+        if not self.no_verify:
+            try:
+                if not run_git_hook("p4-pre-submit"):
+                    print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
+                        "this pre-submission check by adding\nthe command line option '--no-verify', " \
+                        "however,\nthis will also skip the p4-changelist hook as well.")
+                    sys.exit(1)
+            except Exception as e:
+                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
+                    "with the error '{0}'".format(e.message) )
                 sys.exit(1)
-        except Exception as e:
-            print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nThe hook failed "\
-                "with the error '{0}'".format(e.message) )
-            sys.exit(1)
 
         #
         # Apply the commits, one at a time.  On failure, ask if should
-- 
gitgitgadget

[PATCH v6 5/7] git-p4: restructure code in submit

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

In preparation for adding new hooks to the submit method of git-p4,
restructure the applyCommit function in the P4Submit class.  Make the
following changes:

* Move all the code after the definition of submitted = False into the
  Try-Finally block. This ensures that any error that occurs will
  properly recover.  This is not necessary with the current code because
  none of it should throw an exception, however the next set of changes
  will introduce exceptional code.

  Existing flow control can remain as defined - the if-block for
  prepare-p4-only sets the local variable "submitted" to True and exits
  the function. New early exits, leave submitted set to False so the
  Finally block will undo changes to the P4 workspace.

* Make the small usability change of adding an empty string to the
  print statements displayed to the user when the prepare-p4-only option
  is selected.  On Windows, the command print() may display a set of
  parentheses "()" to the user when the print() function is called with
  no parameters. By supplying an empty string, the intended blank line
  will print as expected.

* Fix a small bug when the submittedTemplate is edited by the user
  and all content in the file is removed. The existing code will throw
  an exception if the separateLine is not found in the file. Change this
  code to test for the separator line using a find() test first and only
  split on the separator if it is found.

* Additionally, add the new behavior that if the changelist file has
  been completely emptied that the Submit action for this changelist
  will be aborted.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 92 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index c969d9235b1..4ae6aa30f06 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2102,47 +2102,47 @@ def applyCommit(self, id):
         tmpFile.write(submitTemplate)
         tmpFile.close()
 
-        if self.prepare_p4_only:
-            #
-            # Leave the p4 tree prepared, and the submit template around
-            # and let the user decide what to do next
-            #
-            print()
-            print("P4 workspace prepared for submission.")
-            print("To submit or revert, go to client workspace")
-            print("  " + self.clientPath)
-            print()
-            print("To submit, use \"p4 submit\" to write a new description,")
-            print("or \"p4 submit -i <%s\" to use the one prepared by" \
-                  " \"git p4\"." % fileName)
-            print("You can delete the file \"%s\" when finished." % fileName)
-
-            if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
-                print("To preserve change ownership by user %s, you must\n" \
-                      "do \"p4 change -f <change>\" after submitting and\n" \
-                      "edit the User field.")
-            if pureRenameCopy:
-                print("After submitting, renamed files must be re-synced.")
-                print("Invoke \"p4 sync -f\" on each of these files:")
-                for f in pureRenameCopy:
-                    print("  " + f)
-
-            print()
-            print("To revert the changes, use \"p4 revert ...\", and delete")
-            print("the submit template file \"%s\"" % fileName)
-            if filesToAdd:
-                print("Since the commit adds new files, they must be deleted:")
-                for f in filesToAdd:
-                    print("  " + f)
-            print()
-            return True
-
-        #
-        # Let the user edit the change description, then submit it.
-        #
         submitted = False
 
         try:
+
+            if self.prepare_p4_only:
+                #
+                # Leave the p4 tree prepared, and the submit template around
+                # and let the user decide what to do next
+                #
+                submitted = True
+                print("")
+                print("P4 workspace prepared for submission.")
+                print("To submit or revert, go to client workspace")
+                print("  " + self.clientPath)
+                print("")
+                print("To submit, use \"p4 submit\" to write a new description,")
+                print("or \"p4 submit -i <%s\" to use the one prepared by" \
+                      " \"git p4\"." % fileName)
+                print("You can delete the file \"%s\" when finished." % fileName)
+
+                if self.preserveUser and p4User and not self.p4UserIsMe(p4User):
+                    print("To preserve change ownership by user %s, you must\n" \
+                          "do \"p4 change -f <change>\" after submitting and\n" \
+                          "edit the User field.")
+                if pureRenameCopy:
+                    print("After submitting, renamed files must be re-synced.")
+                    print("Invoke \"p4 sync -f\" on each of these files:")
+                    for f in pureRenameCopy:
+                        print("  " + f)
+
+                print("")
+                print("To revert the changes, use \"p4 revert ...\", and delete")
+                print("the submit template file \"%s\"" % fileName)
+                if filesToAdd:
+                    print("Since the commit adds new files, they must be deleted:")
+                    for f in filesToAdd:
+                        print("  " + f)
+                print("")
+                sys.stdout.flush()
+                return True
+
             if self.edit_template(fileName):
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
@@ -2150,7 +2150,15 @@ def applyCommit(self, id):
                 tmpFile.close()
                 if self.isWindows:
                     message = message.replace("\r\n", "\n")
-                submitTemplate = message[:message.index(separatorLine)]
+                if message.find(separatorLine) != -1:
+                    submitTemplate = message[:message.index(separatorLine)]
+                else:
+                    submitTemplate = message
+
+                if len(submitTemplate.strip()) == 0:
+                    print("Changelist is empty, aborting this changelist.")
+                    sys.stdout.flush()
+                    return False
 
                 if update_shelve:
                     p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)
@@ -2174,19 +2182,21 @@ def applyCommit(self, id):
                 submitted = True
 
         finally:
-            # skip this patch
+            # Revert changes if we skip this patch
             if not submitted or self.shelve:
                 if self.shelve:
                     print ("Reverting shelved files.")
                 else:
                     print ("Submission cancelled, undoing p4 changes.")
+                sys.stdout.flush()
                 for f in editedFiles | filesToDelete:
                     p4_revert(f)
                 for f in filesToAdd:
                     p4_revert(f)
                     os.remove(f)
 
-        os.remove(fileName)
+            if not self.prepare_p4_only:
+                os.remove(fileName)
         return submitted
 
     # Export git tags as p4 labels. Create a p4 label and then tag
-- 
gitgitgadget

[PATCH v6 6/7] git-p4: add p4 submit hooks

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

The git command "commit" supports a number of hooks that support
changing the behavior of the commit command.  The git-p4.py program only
has one existing hook, "p4-pre-submit".  This command occurs early in
the process.  There are no hooks in the process flow for modifying
the P4 changelist text programmatically.

Adds 3 new hooks to git-p4.py to the submit option.

The new hooks are:
* p4-prepare-changelist - Execute this hook after the changelist file
  has been created. The hook will be executed even if the
  --prepare-p4-only option is set.  This hook ignores the --no-verify
  option in keeping with the existing behavior of git commit.

* p4-changelist - Execute this hook after the user has edited the
  changelist. Do not execute this hook if the user has selected the
  --prepare-p4-only option. This hook will honor the --no-verify,
  following the conventions of git commit.

* p4-post-changelist - Execute this hook after the P4 submission process
  has completed successfully. This hook takes no parameters and is
  executed regardless of the --no-verify option.  It's return value will
  not be checked.

The calls to the new hooks: p4-prepare-changelist, p4-changelist,
and p4-post-changelist should all be called inside the try-finally
block.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 Documentation/git-p4.txt   | 35 +++++++++++++++++++++++++++++
 Documentation/githooks.txt | 46 ++++++++++++++++++++++++++++++++++++++
 git-p4.py                  | 35 ++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 362b50eb212..dab9609013f 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -387,6 +387,41 @@ It can be bypassed with the `--no-verify` command line option.
 
 One usage scenario is to run unit tests in the hook.
 
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+p4-changelist
+~~~~~~~~~~~~~
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+
 
 Rebase options
 ~~~~~~~~~~~~~~
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8cf6b08b55b..8aa9c2176c6 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -515,6 +515,52 @@ The exit status determines whether git will use the data from the
 hook to limit its search.  On error, it will fall back to verifying
 all files and folders.
 
+p4-changelist
+~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-changelist` hook is executed after the changelist
+message has been edited by the user. It can be bypassed with the
+`--no-verify` option. It takes a single parameter, the name
+of the file that holds the proposed changelist text. Exiting
+with a non-zero status causes the command to abort.
+
+The hook is allowed to edit the changelist file and can be used
+to normalize the text into some project standard format. It can
+also be used to refuse the Submit after inspect the message file.
+
+Run `git-p4 submit --help` for details.
+
+p4-prepare-changelist
+~~~~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-prepare-changelist` hook is executed right after preparing
+the default changelist message and before the editor is started.
+It takes one parameter, the name of the file that contains the
+changelist text. Exiting with a non-zero status from the script
+will abort the process.
+
+The purpose of the hook is to edit the message file in place,
+and it is not supressed by the `--no-verify` option. This hook
+is called even if `--prepare-p4-only` is set.
+
+Run `git-p4 submit --help` for details.
+
+p4-post-changelist
+~~~~~~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`.
+
+The `p4-post-changelist` hook is invoked after the submit has
+successfully occured in P4. It takes no parameters and is meant
+primarily for notification and cannot affect the outcome of the
+git p4 submit action.
+
+Run `git-p4 submit --help` for details.
+
 p4-pre-submit
 ~~~~~~~~~~~~~
 
diff --git a/git-p4.py b/git-p4.py
index 4ae6aa30f06..8761c70e1c6 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1589,7 +1589,7 @@ def __init__(self):
                 optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
                                      help="Skip Perforce sync of p4/master after submit or shelve"),
                 optparse.make_option("--no-verify", dest="no_verify", action="store_true",
-                                     help="Bypass p4-pre-submit"),
+                                     help="Bypass p4-pre-submit and p4-changelist hooks"),
         ]
         self.description = """Submit changes from git to the perforce depot.\n
     The `p4-pre-submit` hook is executed if it exists and is executable. It
@@ -1598,6 +1598,28 @@ def __init__(self):
     from this script prevents `git-p4 submit` from launching.
 
     One usage scenario is to run unit tests in the hook.
+
+    The `p4-prepare-changelist` hook is executed right after preparing the default
+    changelist message and before the editor is started. It takes one parameter,
+    the name of the file that contains the changelist text. Exiting with a non-zero
+    status from the script will abort the process.
+
+    The purpose of the hook is to edit the message file in place, and it is not
+    supressed by the `--no-verify` option. This hook is called even if
+    `--prepare-p4-only` is set.
+
+    The `p4-changelist` hook is executed after the changelist message has been
+    edited by the user. It can be bypassed with the `--no-verify` option. It
+    takes a single parameter, the name of the file that holds the proposed
+    changelist text. Exiting with a non-zero status causes the command to abort.
+
+    The hook is allowed to edit the changelist file and can be used to normalize
+    the text into some project standard format. It can also be used to refuse the
+    Submit after inspect the message file.
+
+    The `p4-post-changelist` hook is invoked after the submit has successfully
+    occured in P4. It takes no parameters and is meant primarily for notification
+    and cannot affect the outcome of the git p4 submit action.
     """
 
         self.usage += " [name of git branch to submit into perforce depot]"
@@ -2105,6 +2127,10 @@ def applyCommit(self, id):
         submitted = False
 
         try:
+            # Allow the hook to edit the changelist text before presenting it
+            # to the user.
+            if not run_git_hook("p4-prepare-changelist", [fileName]):
+                return False
 
             if self.prepare_p4_only:
                 #
@@ -2144,6 +2170,12 @@ def applyCommit(self, id):
                 return True
 
             if self.edit_template(fileName):
+                if not self.no_verify:
+                    if not run_git_hook("p4-changelist", [fileName]):
+                        print("The p4-changelist hook failed.")
+                        sys.stdout.flush()
+                        return False
+
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
                 message = tmpFile.read()
@@ -2181,6 +2213,7 @@ def applyCommit(self, id):
 
                 submitted = True
 
+                run_git_hook("p4-post-changelist")
         finally:
             # Revert changes if we skip this patch
             if not submitted or self.shelve:
-- 
gitgitgadget

[PATCH v6 7/7] git-p4: add RCS keyword status message

[Ben Keene via GitGitGadget]

From: Ben Keene <seraphire@gmail.com>

During the p4 submit process, git-p4 will attempt to apply a patch
to the files found in the p4 workspace. However, if P4 uses RCS
keyword expansion, this patch may fail.

When the patch fails, the user is alerted to the failure and that git-p4
will attempt to clear the expanded text from the files and re-apply
the patch. The current version of git-p4 does not tell the user the
result of the re-apply attempt after the RCS expansion has been removed
which can be confusing.

Add a new print statement after the git patch has been successfully
applied when the RCS keywords have been cleansed.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 8761c70e1c6..258b9b98b93 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2025,6 +2025,9 @@ def applyCommit(self, id):
         applyPatchCmd = patchcmd + "--check --apply -"
         patch_succeeded = True
 
+        if verbose:
+            print("TryPatch: %s" % tryPatchCmd)
+
         if os.system(tryPatchCmd) != 0:
             fixed_rcs_keywords = False
             patch_succeeded = False
@@ -2064,6 +2067,7 @@ def applyCommit(self, id):
                 print("Retrying the patch with RCS keywords cleaned up")
                 if os.system(tryPatchCmd) == 0:
                     patch_succeeded = True
+                    print("Patch succeesed this time with RCS keywords cleaned")
 
         if not patch_succeeded:
             for f in editedFiles:
-- 
gitgitgadget