git-p4: Add hook p4-pre-pedit-changelist

From: Ben Keene <[email protected]>

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.

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

index 40d9e7c594..1f8c7383df 100755
@@ -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

[jc] asking for help from those who made non-trivial changes to "git p4" in the past 18 months or so for reviewing.

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

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

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

Ack.

Luke

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.

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.

Thanks for summarizing your questions, below are my thoughts.

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.

I agree with your suggestion.

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.

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

I'll make a new submission soon.

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

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

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

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

Thanks.

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

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 <[email protected]>

- 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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 40d9e7c594..7d8a5ee788 100755
@@ -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

From: Ben Keene <[email protected]>

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.

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

index 7d8a5ee788..4e481b3b55 100755
@@ -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

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.

Good point.

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.

I'm committing a new version of this change that will define run_hook_command.

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?

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 think that is a sensible design decision to have a free-standing function to execute hooks.

Thanks.

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?

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

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

From: Ben Keene <[email protected]>

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.

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

index 4e481b3b55..61cfd1c1ae 100755
@@ -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

From: Ben Keene <[email protected]>

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.

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

index 3494a1db3e..f17c1d0561 100644
@@ -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.

gitgitgadget

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.

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 <[email protected]>

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

@@ -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 <[email protected]>

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

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

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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 40d9e7c594..7d8a5ee788 100755
@@ -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

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.

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.

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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

index 7d8a5ee788..d4c39f112b 100755
@@ -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

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.

From: Ben Keene <[email protected]>

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.

---
 Documentation/git-p4.txt   | 10 ++++++++--
 Documentation/githooks.txt |  5 ++++-
 git-p4.py                  | 30 +++++++++++++++++++-----------
 3 files changed, 31 insertions(+), 14 deletions(-)

index 3494a1db3e..362b50eb21 100644
@@ -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.

index 50365f2914..8cf6b08b55 100644
@@ -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
 ~~~~~~~~~~~~~~~~~

index d4c39f112b..b377484464 100755
@@ -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

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.

Thank you and and I'll resubmit the commits with these additional suggestions.

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.

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 92 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

index b377484464..1d15ea7464 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 Documentation/git-p4.txt   | 35 +++++++++++++++++++++++++++++
 Documentation/githooks.txt | 46 ++++++++++++++++++++++++++++++++++++++
 git-p4.py                  | 35 ++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

index 362b50eb21..dab9609013 100644
@@ -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
 ~~~~~~~~~~~~~~

index 1d15ea7464..c912576094 100755
@@ -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

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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 40d9e7c594..65b6d4dca0 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 7 deletions(-)

index 65b6d4dca0..b1c86678fc 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 Documentation/git-p4.txt   | 10 ++++++++--
 Documentation/githooks.txt |  5 ++++-
 git-p4.py                  | 13 +++++++++----
 3 files changed, 21 insertions(+), 7 deletions(-)

index 3494a1db3e..362b50eb21 100644
@@ -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.

index 50365f2914..8cf6b08b55 100644
@@ -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
 ~~~~~~~~~~~~~~~~~

index b1c86678fc..58551bcb09 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 92 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

index 58551bcb09..0224f2e032 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 Documentation/git-p4.txt   | 35 +++++++++++++++++++++++++++++
 Documentation/githooks.txt | 46 ++++++++++++++++++++++++++++++++++++++
 git-p4.py                  | 35 ++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

index 362b50eb21..dab9609013 100644
@@ -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
 ~~~~~~~~~~~~~~

index 0224f2e032..4b77879679 100755
@@ -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

From: Ben Keene <[email protected]>

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.

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

index 4b77879679..1c9c5270b2 100755
@@ -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

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 <[email protected]>

- 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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 40d9e7c594..65b6d4dca0 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 7 deletions(-)

index 65b6d4dca0..4eccea3fa5 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index 4eccea3fa5..b1c86678fc 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 Documentation/git-p4.txt   | 10 ++++++++--
 Documentation/githooks.txt |  5 ++++-
 git-p4.py                  | 32 +++++++++++++++++++-------------
 3 files changed, 31 insertions(+), 16 deletions(-)

index 3494a1db3e..362b50eb21 100644
@@ -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.

index 50365f2914..8cf6b08b55 100644
@@ -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
 ~~~~~~~~~~~~~~~~~

index b1c86678fc..c969d9235b 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 92 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

index c969d9235b..b6da7bcb02 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 Documentation/git-p4.txt   | 35 +++++++++++++++++++++++++++++
 Documentation/githooks.txt | 46 ++++++++++++++++++++++++++++++++++++++
 git-p4.py                  | 35 ++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

index 362b50eb21..dab9609013 100644
@@ -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
 ~~~~~~~~~~~~~~

index b6da7bcb02..f14c71c97d 100755
@@ -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

From: Ben Keene <[email protected]>

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.

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

index f14c71c97d..f1fb2222a4 100755
@@ -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

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:

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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 40d9e7c594e..65b6d4dca08 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 7 deletions(-)

index 65b6d4dca08..4eccea3fa58 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index 4eccea3fa58..b1c86678fc6 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 Documentation/git-p4.txt   | 10 ++++++++--
 Documentation/githooks.txt |  5 ++++-
 git-p4.py                  | 32 +++++++++++++++++++-------------
 3 files changed, 31 insertions(+), 16 deletions(-)

index 3494a1db3eb..362b50eb212 100644
@@ -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.

index 50365f2914e..8cf6b08b55b 100644
@@ -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
 ~~~~~~~~~~~~~~~~~

index b1c86678fc6..c969d9235b1 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 git-p4.py | 92 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

index c969d9235b1..4ae6aa30f06 100755
@@ -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

From: Ben Keene <[email protected]>

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.

---
 Documentation/git-p4.txt   | 35 +++++++++++++++++++++++++++++
 Documentation/githooks.txt | 46 ++++++++++++++++++++++++++++++++++++++
 git-p4.py                  | 35 ++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

index 362b50eb212..dab9609013f 100644
@@ -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
 ~~~~~~~~~~~~~~

index 4ae6aa30f06..8761c70e1c6 100755
@@ -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

From: Ben Keene <[email protected]>

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.

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

index 8761c70e1c6..258b9b98b93 100755
@@ -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