git-p4.skipSubmitEdit

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Quoted 13 lines · L. A. Linden Levy<alevy<at> mobitv.com> writes:

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Hi Loren,

This option was already included in a recent commit. The name that was used is "skipSubmitEditCheck". Please make sure you are using the most recent version of the script.

But don't let this discourage you from submitting patches. Just makesure you clone git's repository and apply your patch over "maint" or "master" branches. For more details on how to submit patches you can read Documentation/SubmittingPatches.

Vitor

--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 16 lines · Vitor Antunes

Quoted 13 lines · L. A. Linden Levy<alevy<at> mobitv.com> writes:

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Quoted 5 lines

Hi Loren,

This option was already included in a recent commit. The name that was used is "skipSubmitEditCheck". Please make sure you are using the most recent version of the script.

I put that option in - glad it's of use!

That option actually just skips the check of 'did the user edit the file'. git-p4 will still go ahead and bring up the file in the editor first.

I get around this myself by setting EDITOR=/bin/true. That works for me because I'm only using it in a script.

But it's possible that an additional option would actually be useful.

Quoted 12 lines

But don't let this discourage you from submitting patches. Just makesure you clone git's repository and apply your patch over "maint" or "master" branches. For more details on how to submit patches you can read Documentation/SubmittingPatches.

Vitor

--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I noticed that it only skipped the edit check. That is why I added the skipSubmitEdit option. If they are both true then it never opens the editor and never checks for an edit. Probably they should just be one option. I think it should probably also be a command line option to skip the editor.

- Alex

Quoted 48 lines · Luke Diamand

Quoted 16 lines · Vitor Antunes

Quoted 13 lines · L. A. Linden Levy<alevy<at> mobitv.com> writes:

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Quoted 5 lines

Hi Loren,

This option was already included in a recent commit. The name that was used is "skipSubmitEditCheck". Please make sure you are using the most recent version of the script.

I put that option in - glad it's of use!

That option actually just skips the check of 'did the user edit the file'. git-p4 will still go ahead and bring up the file in the editor first.

I get around this myself by setting EDITOR=/bin/true. That works for me because I'm only using it in a script.

But it's possible that an additional option would actually be useful.

Quoted 12 lines

But don't let this discourage you from submitting patches. Just makesure you clone git's repository and apply your patch over "maint" or "master" branches. For more details on how to submit patches you can read Documentation/SubmittingPatches.

Vitor

--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Below are the patch and environment results:

$ git config -l
...
user.name=Loren A. Linden Levy
git-p4.skipsubmitedit=true
git-p4.skipsubmiteditcheck=true
...

$ git format-patch origin/master --stdout

From 16c4344de0047cbaf3381eca590a3e59b0d0a25c Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy"<[email protected]>
Date: Thu, 8 Sep 2011 13:37:22 -0700
Subject: [PATCH] changed git-p4

---
  contrib/fast-import/git-p4 |   15 ++++++++++-----
  1 files changed, 10 insertions(+), 5 deletions(-)

contrib/fast-import/git-p4 +10 -5

index 2f7b270..a438b3e 100755
@@ -935,18 +935,23 @@ class P4Submit(Command, P4UserMap):
              tmpFile.write(submitTemplate + separatorLine + diff +
 newdiff)
              tmpFile.close()
              mtime = os.stat(fileName).st_mtime
-            if os.environ.has_key("P4EDITOR"):
-                editor = os.environ.get("P4EDITOR")
+            if gitConfig("git-p4.skipSubmitEdit") == "true":
+                pass
              else:
-                editor = read_pipe("git var GIT_EDITOR").strip()
-            system(editor + " " + fileName)
-
+                if os.environ.has_key("P4EDITOR"):
+                    editor = os.environ.get("P4EDITOR")
+                else:
+                    editor = read_pipe("git var GIT_EDITOR").strip()
+
+                    system(editor + " " + fileName)
+
              if gitConfig("git-p4.skipSubmitEditCheck") == "true":
                  checkModTime = False
              else:
                  checkModTime = True
 
              response = "y"
+
              if checkModTime and (os.stat(fileName).st_mtime<= mtime):
                  response = "x"

                  while response != "y" and response != "n":

Quoted 12 lines · L. A. Linden Levy

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Sorry - I've not had a chance to look at this before now. But a couple of comments:

- Is there a line wrap problem in the patch? It doesn't seem to want
to apply for me.

- needs an update to the documentation in git-p4.txt

- is there any way to eliminate the slightly ugly
"if/pass/else/do-stuff" construct?

- I'd think if you turned off the editor completely then there's no
point doing the submit-edit-check.

- You probably need to follow the instructions in
Documentation/SubmittingPatches so that Junio will pay attention to the patch.

Regards!
Luke

Quoted 56 lines

Below are the patch and environment results:

$ git config -l
...
user.name=Loren A. Linden Levy
git-p4.skipsubmitedit=true
git-p4.skipsubmiteditcheck=true
...

$ git format-patch origin/master --stdout

From 16c4344de0047cbaf3381eca590a3e59b0d0a25c Mon Sep 17 00:00:00 2001
From: "Loren A. Linden Levy"<[email protected]>
Date: Thu, 8 Sep 2011 13:37:22 -0700
Subject: [PATCH] changed git-p4

---
  contrib/fast-import/git-p4 |   15 ++++++++++-----
  1 files changed, 10 insertions(+), 5 deletions(-)

contrib/fast-import/git-p4 +10 -5

index 2f7b270..a438b3e 100755
@@ -935,18 +935,23 @@ class P4Submit(Command, P4UserMap):
              tmpFile.write(submitTemplate + separatorLine + diff +
 newdiff)
              tmpFile.close()
              mtime = os.stat(fileName).st_mtime
-            if os.environ.has_key("P4EDITOR"):
-                editor = os.environ.get("P4EDITOR")
+            if gitConfig("git-p4.skipSubmitEdit") == "true":
+                pass
              else:
-                editor = read_pipe("git var GIT_EDITOR").strip()
-            system(editor + " " + fileName)
-
+                if os.environ.has_key("P4EDITOR"):
+                    editor = os.environ.get("P4EDITOR")
+                else:
+                    editor = read_pipe("git var GIT_EDITOR").strip()
+
+                    system(editor + " " + fileName)
+
              if gitConfig("git-p4.skipSubmitEditCheck") == "true":
                  checkModTime = False
              else:
                  checkModTime = True
 
              response = "y"
+
              if checkModTime and (os.stat(fileName).st_mtime<= mtime):
                  response = "x"

                  while response != "y" and response != "n":

I agree with almost all your points. I have answered them each in-line below.

Quoted 15 lines · Luke Diamand

Quoted 12 lines · L. A. Linden Levy

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Quoted 5 lines

Sorry - I've not had a chance to look at this before now. But a couple of comments:

- Is there a line wrap problem in the patch? It doesn't seem to want
to apply for me.

Probably. Below are the results from following the patch submission instructions.

[email protected] wrote on Mon, 12 Sep 2011 10:12 -0700:

Quoted 19 lines

I agree with almost all your points. I have answered them each in-line below.

Quoted 15 lines · Luke Diamand

Quoted 12 lines · L. A. Linden Levy

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Quoted 8 lines

Quoted 5 lines

Sorry - I've not had a chance to look at this before now. But a couple of comments:

- Is there a line wrap problem in the patch? It doesn't seem to want
to apply for me.

Probably. Below are the results from following the patch submission instructions.

Sorry I sat on this for a month. It is a good idea. Your patches were good in content, but they didn't apply well due to being line-wrapped and having one duplicate.

Reading the code, though, I decided that the whole skipSubmitEdit* checking was a bit convoluted even before you got to it. So I moved it all to a separate function. And added a unit test.

Tell me if you think this is okay instead. If I got a bit too wordy in the doc, please help with that too.

		-- Pete

--- 8< ---

Subject: [PATCH] git-p4: introduce skipSubmitEdit

Add a configuration variable to skip invoking the editor in the submit path.

The existing variable skipSubmitEditCheck continues to make sure that the submit template was indeed modified by the editor; but, it is not considered is skipSubmitEdit is true.

Reported-by Loren A. Linden Levy <[email protected]>

Signed-off-by Pete Wyckoff <[email protected]>

---
 contrib/fast-import/git-p4     |   60 +++++++++++++++++++----------
 contrib/fast-import/git-p4.txt |   19 ++++++++-
 t/t9804-skip-submit-edit.sh    |   82 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 24 deletions(-)
 create mode 100755 t/t9804-skip-submit-edit.sh

contrib/fast-import/git-p4 +39 -21

index f885d70..abd6778 100755
@@ -847,6 +847,39 @@ class P4Submit(Command, P4UserMap):
 
         return template
 
+    def edit_template(self, template_file):
+        """Invoke the editor to let the user change the submission
+           message.  Return true if okay to continue with the submit."""
+
+        # if configured to skip the editing part, just submit
+        if gitConfig("git-p4.skipSubmitEdit") == "true":
+            return True
+
+        # look at the modification time, to check later if the user saved
+        # the file
+        mtime = os.stat(template_file).st_mtime
+
+        # invoke the editor
+        if os.environ.has_key("P4EDITOR"):
+            editor = os.environ.get("P4EDITOR")
+        else:
+            editor = read_pipe("git var GIT_EDITOR").strip()
+        system(editor + " " + template_file)
+
+        # If the file was not saved, prompt to see if this patch should
+        # be skipped.  But skip this verification step if configured so.
+        if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+            print "return true for skipSubmitEditCheck"
+            return True
+
+        if os.stat(template_file).st_mtime <= mtime:
+            while True:
+                response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
+                if response == 'y':
+                    return True
+                if response == 'n':
+                    return False
+
     def applyCommit(self, id):
         print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
 

@@ -1001,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
 
             separatorLine = "######## everything below this line is just the diff #######\n"
 
-            [handle, fileName] = tempfile.mkstemp()
+            (handle, fileName) = tempfile.mkstemp()
             tmpFile = os.fdopen(handle, "w+")
             if self.isWindows:
                 submitTemplate = submitTemplate.replace("\n", "\r\n")

@@ -1009,25 +1042,9 @@ class P4Submit(Command, P4UserMap):
                 newdiff = newdiff.replace("\n", "\r\n")
             tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
             tmpFile.close()
-            mtime = os.stat(fileName).st_mtime
-            if os.environ.has_key("P4EDITOR"):
-                editor = os.environ.get("P4EDITOR")
-            else:
-                editor = read_pipe("git var GIT_EDITOR").strip()
-            system(editor + " " + fileName)
 
-            if gitConfig("git-p4.skipSubmitEditCheck") == "true":
-                checkModTime = False
-            else:
-                checkModTime = True
-
-            response = "y"
-            if checkModTime and (os.stat(fileName).st_mtime <= mtime):
-                response = "x"
-                while response != "y" and response != "n":
-                    response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
-
-            if response == "y":
+            if self.edit_template(fileName):
+                # read the edited message and submit
                 tmpFile = open(fileName, "rb")
                 message = tmpFile.read()
                 tmpFile.close()

@@ -1039,11 +1056,12 @@ class P4Submit(Command, P4UserMap):
                 if self.preserveUser:
                     if p4User:
                         # Get last changelist number. Cannot easily get it from
-                        # the submit command output as the output is unmarshalled.
+                        # the submit command output as the output is
+                        # unmarshalled.
                         changelist = self.lastP4Changelist()
                         self.modifyChangelistUser(changelist, p4User)
-
             else:
+                # skip this patch
                 for f in editedFiles:
                     p4_revert(f)
                 for f in filesToAdd:

contrib/fast-import/git-p4.txt +16 -3

index 52003ae..5044a12 100644
@@ -202,11 +202,24 @@ able to find the relevant client.  This client spec will be used to
 both filter the files cloned by git and set the directory layout as
 specified in the client (this implies --keep-path style semantics).
 
-git-p4.skipSubmitModTimeCheck
+git-p4.skipSubmitEdit
 
-  git config [--global] git-p4.skipSubmitModTimeCheck false
+  git config [--global] git-p4.skipSubmitEdit false
 
-If true, submit will not check if the p4 change template has been modified.
+Normally, git-p4 invokes an editor after each commit is applied so
+that you can make changes to the submit message.  Setting this
+variable to true will skip the editing step, submitting the change as is.
+
+git-p4.skipSubmitEditCheck
+
+  git config [--global] git-p4.skipSubmitEditCheck false
+
+After the editor is invoked, git-p4 normally makes sure you saved the
+change description, as an indication that you did indeed read it over
+and edit it.  You can quit without saving to abort the submit (or skip
+this change and continue).  Setting this variable to true will cause
+git-p4 not to check if you saved the change description.  This variable
+only matters if git-p4.skipSubmitEdit has not been set to true.
 
 git-p4.preserveUser
 

t/t9804-skip-submit-edit.sh new file +82 -0

new file mode 100755
index 0000000..734ccf2
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='git-p4 skipSubmitEdit config variables'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "change 1"
+	)
+'
+
+# this works because EDITOR is set to :
+test_expect_success 'no config, unedited, say yes' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		echo line >>file1 &&
+		git commit -a -m "change 2" &&
+		echo y | "$GITP4" submit &&
+		p4 changes //depot/... >wc &&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'no config, unedited, say no' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		echo line >>file1 &&
+		git commit -a -m "change 3 (not really)" &&
+		printf "bad response\nn\n" | "$GITP4" submit
+		p4 changes //depot/... >wc &&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'skipSubmitEdit' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		# will fail if editor is even invoked
+		git config core.editor /bin/false &&
+		echo line >>file1 &&
+		git commit -a -m "change 3" &&
+		"$GITP4" submit &&
+		p4 changes //depot/... >wc &&
+		test_line_count = 3 wc
+	)
+'
+
+test_expect_success 'skipSubmitEditCheck' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
+		echo line >>file1 &&
+		git commit -a -m "change 4" &&
+		"$GITP4" submit &&
+		p4 changes //depot/... >wc &&
+		test_line_count = 4 wc
+	)
+'
+
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done

This is just fine for me. Like I said initially I did not approach the problem from an engineering perspective, I just changed it to prevent the patch questions so I could commit faster. Looking forward to seeing the update in the repo.

[email protected] wrote on Mon, 12 Sep 2011 10:12 -0700:

Quoted 19 lines

I agree with almost all your points. I have answered them each in-line below.

Quoted 15 lines · Luke Diamand

Quoted 12 lines · L. A. Linden Levy

Hi All,

I have been using git-p4 for a while and it has allowed me to completely change the way I develop and still be able to use perforce which my company has for its main VCS. One thing that was driving me nuts was that "git p4 submit" cycles through all of my individual commits and asks me if I want to change them. The way I develop I often am checking in 20 to 50 different small commits each with a descriptive git comment. I felt like I was doing double duty by having emacs open on every commit into perforce. So I modified git-p4 to have an option to skip the editor. This option coupled with git-p4.skipSubmitEditCheck will make the submission non-interactive for "git p4 submit".

Quoted 8 lines

Quoted 5 lines

Sorry - I've not had a chance to look at this before now. But a couple of comments:

- Is there a line wrap problem in the patch? It doesn't seem to want
to apply for me.

Probably. Below are the results from following the patch submission instructions.

Sorry I sat on this for a month. It is a good idea. Your patches were good in content, but they didn't apply well due to being line-wrapped and having one duplicate.

Reading the code, though, I decided that the whole skipSubmitEdit* checking was a bit convoluted even before you got to it. So I moved it all to a separate function. And added a unit test.

Tell me if you think this is okay instead. If I got a bit too wordy in the doc, please help with that too.

		-- Pete

--- 8< ---

Subject: [PATCH] git-p4: introduce skipSubmitEdit

Add a configuration variable to skip invoking the editor in the submit path.

The existing variable skipSubmitEditCheck continues to make sure that the submit template was indeed modified by the editor; but, it is not considered is skipSubmitEdit is true.

Reported-by Loren A. Linden Levy<[email protected]>

Signed-off-by Pete Wyckoff<[email protected]>

---
  contrib/fast-import/git-p4     |   60 +++++++++++++++++++----------
  contrib/fast-import/git-p4.txt |   19 ++++++++-
  t/t9804-skip-submit-edit.sh    |   82 ++++++++++++++++++++++++++++++++++++++++
  3 files changed, 137 insertions(+), 24 deletions(-)

create mode 100755 t/t9804-skip-submit-edit.sh

contrib/fast-import/git-p4 +8 -0

index f885d70..abd6778 100755
@@ -847,6 +847,39 @@ class P4Submit(Command, P4UserMap):
 
          return template
 
+    def edit_template(self, template_file):
+        """Invoke the editor to let the user change the submission
+           message.  Return true if okay to continue with the submit."""
+
+        # if configured to skip the editing part, just submit
+        if gitConfig("git-p4.skipSubmitEdit") == "true":
+            return True
+

contrib/fast-import/git-p4 +16 -21

+            return True
+
+        if os.stat(template_file).st_mtime<= mtime:
+            while True:
+                response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
+                if response == 'y':
+                    return True
+                if response == 'n':
+                    return False
+
      def applyCommit(self, id):
          print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
 

@@ -1001,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
 
              separatorLine = "######## everything below this line is just the diff #######\n"
 
-            [handle, fileName] = tempfile.mkstemp()
+            (handle, fileName) = tempfile.mkstemp()
              tmpFile = os.fdopen(handle, "w+")
              if self.isWindows:
                  submitTemplate = submitTemplate.replace("\n", "\r\n")

@@ -1009,25 +1042,9 @@ class P4Submit(Command, P4UserMap):
                  newdiff = newdiff.replace("\n", "\r\n")
              tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
              tmpFile.close()
-            mtime = os.stat(fileName).st_mtime
-            if os.environ.has_key("P4EDITOR"):
-                editor = os.environ.get("P4EDITOR")
-            else:
-                editor = read_pipe("git var GIT_EDITOR").strip()
-            system(editor + " " + fileName)
 
-            if gitConfig("git-p4.skipSubmitEditCheck") == "true":
-                checkModTime = False
-            else:
-                checkModTime = True
-
-            response = "y"
-            if checkModTime and (os.stat(fileName).st_mtime<= mtime):
-                response = "x"
-                while response != "y" and response != "n":
-                    response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
-
-            if response == "y":
+            if self.edit_template(fileName):
+                # read the edited message and submit
                  tmpFile = open(fileName, "rb")
                  message = tmpFile.read()
                  tmpFile.close()

@@ -1039,11 +1056,12 @@ class P4Submit(Command, P4UserMap):
                  if self.preserveUser:
                      if p4User:
                          # Get last changelist number. Cannot easily get it from
-                        # the submit command output as the output is unmarshalled.
+                        # the submit command output as the output is
+                        # unmarshalled.
                          changelist = self.lastP4Changelist()
                          self.modifyChangelistUser(changelist, p4User)
-
              else:
+                # skip this patch
                  for f in editedFiles:
                      p4_revert(f)
                  for f in filesToAdd:

contrib/fast-import/git-p4.txt +16 -3

index 52003ae..5044a12 100644
@@ -202,11 +202,24 @@ able to find the relevant client.  This client spec will be used to
  both filter the files cloned by git and set the directory layout as
  specified in the client (this implies --keep-path style semantics).
 
-git-p4.skipSubmitModTimeCheck
+git-p4.skipSubmitEdit
 
-  git config [--global] git-p4.skipSubmitModTimeCheck false
+  git config [--global] git-p4.skipSubmitEdit false
 
-If true, submit will not check if the p4 change template has been modified.
+Normally, git-p4 invokes an editor after each commit is applied so
+that you can make changes to the submit message.  Setting this
+variable to true will skip the editing step, submitting the change as is.
+
+git-p4.skipSubmitEditCheck
+
+  git config [--global] git-p4.skipSubmitEditCheck false
+
+After the editor is invoked, git-p4 normally makes sure you saved the
+change description, as an indication that you did indeed read it over
+and edit it.  You can quit without saving to abort the submit (or skip
+this change and continue).  Setting this variable to true will cause
+git-p4 not to check if you saved the change description.  This variable
+only matters if git-p4.skipSubmitEdit has not been set to true.
 
  git-p4.preserveUser
 

t/t9804-skip-submit-edit.sh new file +82 -0

new file mode 100755
index 0000000..734ccf2
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='git-p4 skipSubmitEdit config variables'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'init depot' '
+	(
+		cd "$cli"&&
+		echo file1>file1&&
+		p4 add file1&&
+		p4 submit -d "change 1"
+	)
+'
+
+# this works because EDITOR is set to :
+test_expect_success 'no config, unedited, say yes' '
+	"$GITP4" clone --dest="$git" //depot&&
+	test_when_finished cleanup_git&&
+	(
+		cd "$git"&&
+		echo line>>file1&&
+		git commit -a -m "change 2"&&
+		echo y | "$GITP4" submit&&
+		p4 changes //depot/...>wc&&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'no config, unedited, say no' '
+	"$GITP4" clone --dest="$git" //depot&&
+	test_when_finished cleanup_git&&
+	(
+		cd "$git"&&
+		echo line>>file1&&
+		git commit -a -m "change 3 (not really)"&&
+		printf "bad response\nn\n" | "$GITP4" submit
+		p4 changes //depot/...>wc&&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'skipSubmitEdit' '
+	"$GITP4" clone --dest="$git" //depot&&
+	test_when_finished cleanup_git&&
+	(
+		cd "$git"&&
+		git config git-p4.skipSubmitEdit true&&
+		# will fail if editor is even invoked
+		git config core.editor /bin/false&&
+		echo line>>file1&&
+		git commit -a -m "change 3"&&
+		"$GITP4" submit&&
+		p4 changes //depot/...>wc&&
+		test_line_count = 3 wc
+	)
+'
+
+test_expect_success 'skipSubmitEditCheck' '
+	"$GITP4" clone --dest="$git" //depot&&
+	test_when_finished cleanup_git&&
+	(
+		cd "$git"&&
+		git config git-p4.skipSubmitEditCheck true&&
+		echo line>>file1&&
+		git commit -a -m "change 4"&&
+		"$GITP4" submit&&
+		p4 changes //depot/...>wc&&
+		test_line_count = 4 wc
+	)
+'
+
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done

Looks good, one minor nit (see below) and a comment.

contrib/fast-import/git-p4 class P4Submit(Command, P4UserMap): · line 867

Quoted 6 lines

contrib/fast-import/git-p4 +6 -0

+        # invoke the editor
+        if os.environ.has_key("P4EDITOR"):
+            editor = os.environ.get("P4EDITOR")
+        else:
+            editor = read_pipe("git var GIT_EDITOR").strip()
+        system(editor + " " + template_file)

This is where we should really check the return code. However, doing so seems to break lots of the existing tests so it's not as easy as it looks.

contrib/fast-import/git-p4 class P4Submit(Command, P4UserMap): · line 872

Quoted 5 lines

contrib/fast-import/git-p4 +5 -0

+
+        # If the file was not saved, prompt to see if this patch should
+        # be skipped.  But skip this verification step if configured so.
+        if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+            print "return true for skipSubmitEditCheck"

You print a helpful/annoying(?) message here, but not further up at skipSubmitEdit?

[email protected] wrote on Tue, 18 Oct 2011 18:53 +0100:

Quoted 1 line

Looks good, one minor nit (see below) and a comment.

[..]

Quoted 10 lines

Quoted 6 lines

+        # invoke the editor
+        if os.environ.has_key("P4EDITOR"):
+            editor = os.environ.get("P4EDITOR")
+        else:
+            editor = read_pipe("git var GIT_EDITOR").strip()
+        system(editor + " " + template_file)

This is where we should really check the return code. However, doing so seems to break lots of the existing tests so it's not as easy as it looks.

Indeed.  I'll not fix that now, but agree it should be.

Quoted 8 lines

Quoted 5 lines

+
+        # If the file was not saved, prompt to see if this patch should
+        # be skipped.  But skip this verification step if configured so.
+        if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+            print "return true for skipSubmitEditCheck"

You print a helpful/annoying(?) message here, but not further up at skipSubmitEdit?

Aargh.  Leaked debug code.  Thanks for noticing.  I got rid of it.

               -- Pete
--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

All,

It appears that this change has introduced a bug that causes submit to fail every time if you do not skip the submit edit.

From what I can tell, this is because the new edit_template method
does not return True at the end.

Quoted 8 lines · Michael Horowitz

All,

It appears that this change has introduced a bug that causes submit to fail every time if you do not skip the submit edit.

 From what I can tell, this is because the new edit_template method does not return True at the end.

Could you say exactly what you're doing?

I've just tried it myself and it seems to work fine:

git-p4 clone
git commit -m 'a change'
git-p4 submit
<quit from editor, with/without modifying it>

And I couldn't see any paths in edit_template that returned without a value of True, except the one where the user decides to bail out.

This is with Pete's skipSubmitEdit change.

Thanks!
Luke

Actually, it is the opposite.  Bailout works fine, it is when I ":wq" in Vi for example, that it fails to submit and reverts all my changes.

       if os.stat(template_file).st_mtime <= mtime:
           while True:
               response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
               if response == 'y':
                   return True
               if response == 'n':
                   return False
       # I think this else needs to be added here, so when the file has been modified since it was opened in the editor, it will properly submit the change.
       else:
           return True

Mike

Quoted 27 lines · Luke Diamand

Quoted 8 lines · Michael Horowitz

All,

It appears that this change has introduced a bug that causes submit to fail every time if you do not skip the submit edit.

 From what I can tell, this is because the new edit_template method does not return True at the end.

Could you say exactly what you're doing?

I've just tried it myself and it seems to work fine:

git-p4 clone
git commit -m 'a change'
git-p4 submit
<quit from editor, with/without modifying it>

And I couldn't see any paths in edit_template that returned without a value of True, except the one where the user decides to bail out.

This is with Pete's skipSubmitEdit change.

Thanks!
Luke

Oh, and in the case where you do edit the template, it doesn't give you an error or anything, it looks like it succeeded, but you'll notice the change never got submitted to Perforce. If you look carefully though, you can see it reverting each of your edited files in the P4 tree.

Quoted 7 lines

Quoted 7 lines · Michael Horowitz

Quoted 6 lines

It appears that this change has introduced a bug that causes submit to fail every time if you do not skip the submit edit.

 From what I can tell, this is because the new edit_template method does not return True at the end.