[PATCH] git-p4: Do not include diff in spec file when just preparing p4

[PATCH] git-p4: Do not include diff in spec file when just preparing p4

[Maxime Coste]

The diff information render the spec file unusable as is by p4,
do not include it when run with --prepare-p4-only so that the
given file can be directly passed to p4.
---
 git-p4.py | 70 +++++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 5ea8bb8..7c65340 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1397,38 +1397,14 @@ class P4Submit(Command, P4UserMap):
             submitTemplate += "######## Use option --preserve-user to modify authorship.\n"
             submitTemplate += "######## Variable git-p4.skipUserNameCheck hides this message.\n"
 
-        separatorLine = "######## everything below this line is just the diff #######\n"
-
-        # diff
-        if os.environ.has_key("P4DIFF"):
-            del(os.environ["P4DIFF"])
-        diff = ""
-        for editedFile in editedFiles:
-            diff += p4_read_pipe(['diff', '-du',
-                                  wildcard_encode(editedFile)])
-
-        # new file diff
-        newdiff = ""
-        for newFile in filesToAdd:
-            newdiff += "==== new file ====\n"
-            newdiff += "--- /dev/null\n"
-            newdiff += "+++ %s\n" % newFile
-            f = open(newFile, "r")
-            for line in f.readlines():
-                newdiff += "+" + line
-            f.close()
-
-        # change description file: submitTemplate, separatorLine, diff, newdiff
-        (handle, fileName) = tempfile.mkstemp()
-        tmpFile = os.fdopen(handle, "w+")
-        if self.isWindows:
-            submitTemplate = submitTemplate.replace("\n", "\r\n")
-            separatorLine = separatorLine.replace("\n", "\r\n")
-            newdiff = newdiff.replace("\n", "\r\n")
-        tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
-        tmpFile.close()
-
         if self.prepare_p4_only:
+            (handle, fileName) = tempfile.mkstemp()
+            tmpFile = os.fdopen(handle, "w+")
+            if self.isWindows:
+                submitTemplate = submitTemplate.replace("\n", "\r\n")
+            tmpFile.write(submitTemplate)
+            tmpFile.close()
+
             #
             # Leave the p4 tree prepared, and the submit template around
             # and let the user decide what to do next
@@ -1463,6 +1439,38 @@ class P4Submit(Command, P4UserMap):
             print
             return True
 
+        else:
+            separatorLine = "######## everything below this line is just the diff #######\n"
+
+            # diff
+            if os.environ.has_key("P4DIFF"):
+                del(os.environ["P4DIFF"])
+            diff = ""
+            for editedFile in editedFiles:
+                diff += p4_read_pipe(['diff', '-du',
+                                      wildcard_encode(editedFile)])
+
+            # new file diff
+            newdiff = ""
+            for newFile in filesToAdd:
+                newdiff += "==== new file ====\n"
+                newdiff += "--- /dev/null\n"
+                newdiff += "+++ %s\n" % newFile
+                f = open(newFile, "r")
+                for line in f.readlines():
+                    newdiff += "+" + line
+                f.close()
+
+            # change description file: submitTemplate, separatorLine, diff, newdiff
+            (handle, fileName) = tempfile.mkstemp()
+            tmpFile = os.fdopen(handle, "w+")
+            if self.isWindows:
+                submitTemplate = submitTemplate.replace("\n", "\r\n")
+                separatorLine = separatorLine.replace("\n", "\r\n")
+                newdiff = newdiff.replace("\n", "\r\n")
+            tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
+            tmpFile.close()
+
         #
         # Let the user edit the change description, then submit it.
         #
-- 
1.8.5.2

Re: [PATCH] git-p4: Do not include diff in spec file when just preparing p4

[Pete Wyckoff]

frrrwww@gmail.com wrote on Fri, 10 Jan 2014 18:18 +0000:
> The diff information render the spec file unusable as is by p4,
> do not include it when run with --prepare-p4-only so that the
> given file can be directly passed to p4.

Thanks for the patch, but I'm curious how you'd like this to
work.  I never use the option myself.

As it is, --prepare-p4-only generates a file in /tmp/ that has
exactly the contents you'd see in the editor during "git p4
submit".  It includes the diff of the change, presumably to help
with writing the description.

Now you can't actually feed this file directly to "p4 submit"
without deleting the diff.  That's the part you don't like?

		-- Pete

Re: [PATCH] git-p4: Do not include diff in spec file when just preparing p4

[Maxime Coste]

Hello,

On Sun, Jan 12, 2014 at 05:29:46PM -0500, Pete Wyckoff wrote:
> Thanks for the patch, but I'm curious how you'd like this to
> work.  I never use the option myself.
> 
> As it is, --prepare-p4-only generates a file in /tmp/ that has
> exactly the contents you'd see in the editor during "git p4
> submit".  It includes the diff of the change, presumably to help
> with writing the description.

Yes, I believe it makes sense to display the diff in this case, as we
can remove it later programmatically.
 
> Now you can't actually feed this file directly to "p4 submit"
> without deleting the diff.  That's the part you don't like?

Yes, I do not use that for submitting, but for shelving. I can run
git p4 submit --prepare-p4-only followed by p4 shelve -i < /tmp/...
and perforce will shelve the corresponding change.

Removing the diff could be done externally, however git-p4 itself
tells the user it can submit using the generated file, which is
not the case if we keep the diff in it.

Cheers,

Maxime Coste.

Re: [PATCH] git-p4: Do not include diff in spec file when just preparing p4

[Pete Wyckoff]

frrrwww@gmail.com wrote on Mon, 13 Jan 2014 12:10 +0000:
> Hello,
> 
> On Sun, Jan 12, 2014 at 05:29:46PM -0500, Pete Wyckoff wrote:
> > Thanks for the patch, but I'm curious how you'd like this to
> > work.  I never use the option myself.
> > 
> > As it is, --prepare-p4-only generates a file in /tmp/ that has
> > exactly the contents you'd see in the editor during "git p4
> > submit".  It includes the diff of the change, presumably to help
> > with writing the description.
> 
> Yes, I believe it makes sense to display the diff in this case, as we
> can remove it later programmatically.
>  
> > Now you can't actually feed this file directly to "p4 submit"
> > without deleting the diff.  That's the part you don't like?
> 
> Yes, I do not use that for submitting, but for shelving. I can run
> git p4 submit --prepare-p4-only followed by p4 shelve -i < /tmp/...
> and perforce will shelve the corresponding change.
> 
> Removing the diff could be done externally, however git-p4 itself
> tells the user it can submit using the generated file, which is
> not the case if we keep the diff in it.

I'm convinced.  That explanation makes sense, thanks.

It would be nice to do a few more things with this patch.  Here's
some ideas, sorted in priority order.

    1.  Put slightly more text into the commit message, possibly
    from your email above.

    2.  Refactor out that big chunk of code instead of just
    moving it.  Selectively call it only if not prepare_p4_only.

    3.  Modify the t9807 test 'submit --prepare-p4-only' to make
    sure the diff isn't there.

    4.  Documentation update?  Probably not necessary.

Let me know if you're interested in doing any of this.

		-- Pete

[PATCH] git-p4: Do not include diff in spec file when just preparing p4

[Maxime Coste]

The diff information render the spec file unusable as is by p4,
do not include it when run with --prepare-p4-only so that the
given file can be directly passed to p4.

With --prepare-p4-only, git-p4 already tells the user it can use
p4 submit with the generated spec file. This fails because of the
diff being present in the file. Not including the diff fixes that.

Without --prepare-p4-only, keeping the diff makes sense for a
quick review of the patch before submitting it. And does not cause
problems with p4 as we remove it programmatically.

Signed-off-by: Maxime Coste <frrrwww@gmail.com>
---
 git-p4.py                | 49 +++++++++++++++++++++++++-----------------------
 t/t9807-git-p4-submit.sh |  3 ++-
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 773cafc..7bb0f73 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1238,6 +1238,28 @@ class P4Submit(Command, P4UserMap):
             if response == 'n':
                 return False
 
+    def get_diff_description(self, editedFiles):
+        # diff
+        if os.environ.has_key("P4DIFF"):
+            del(os.environ["P4DIFF"])
+        diff = ""
+        for editedFile in editedFiles:
+            diff += p4_read_pipe(['diff', '-du',
+                                  wildcard_encode(editedFile)])
+
+        # new file diff
+        newdiff = ""
+        for newFile in filesToAdd:
+            newdiff += "==== new file ====\n"
+            newdiff += "--- /dev/null\n"
+            newdiff += "+++ %s\n" % newFile
+            f = open(newFile, "r")
+            for line in f.readlines():
+                newdiff += "+" + line
+            f.close()
+
+        return diff + newdiff
+
     def applyCommit(self, id):
         """Apply one commit, return True if it succeeded."""
 
@@ -1398,34 +1420,15 @@ class P4Submit(Command, P4UserMap):
             submitTemplate += "######## Variable git-p4.skipUserNameCheck hides this message.\n"
 
         separatorLine = "######## everything below this line is just the diff #######\n"
+        if not self.prepare_p4_only:
+            submitTemplate += separatorLine
+            submitTemplate += self.get_diff_description(editedFiles)
 
-        # diff
-        if os.environ.has_key("P4DIFF"):
-            del(os.environ["P4DIFF"])
-        diff = ""
-        for editedFile in editedFiles:
-            diff += p4_read_pipe(['diff', '-du',
-                                  wildcard_encode(editedFile)])
-
-        # new file diff
-        newdiff = ""
-        for newFile in filesToAdd:
-            newdiff += "==== new file ====\n"
-            newdiff += "--- /dev/null\n"
-            newdiff += "+++ %s\n" % newFile
-            f = open(newFile, "r")
-            for line in f.readlines():
-                newdiff += "+" + line
-            f.close()
-
-        # change description file: submitTemplate, separatorLine, diff, newdiff
         (handle, fileName) = tempfile.mkstemp()
         tmpFile = os.fdopen(handle, "w+")
         if self.isWindows:
             submitTemplate = submitTemplate.replace("\n", "\r\n")
-            separatorLine = separatorLine.replace("\n", "\r\n")
-            newdiff = newdiff.replace("\n", "\r\n")
-        tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
+        tmpFile.write(submitTemplate)
         tmpFile.close()
 
         if self.prepare_p4_only:
diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index 4caf36e..7fab2ed 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -403,7 +403,8 @@ test_expect_success 'submit --prepare-p4-only' '
 		git commit -m "prep only add" &&
 		git p4 submit --prepare-p4-only >out &&
 		test_i18ngrep "prepared for submission" out &&
-		test_i18ngrep "must be deleted" out
+		test_i18ngrep "must be deleted" out &&
+		! test_i18ngrep "everything below this line is just the diff" out
 	) &&
 	(
 		cd "$cli" &&
-- 
1.9.3

Re: [PATCH] git-p4: Do not include diff in spec file when just preparing p4

[Maxime Coste]

Hello

Sorry for the delay, I hope that version is more acceptable.

I updated the test case as well, but did not manage to get the actual p4
tests to work here (I have p4 and p4d installed, they start but all the
other tests seems to fail). Still the change is straightforward.

Cheers,

Maxime Coste.

Re: [PATCH] git-p4: Do not include diff in spec file when just preparing p4

[Pete Wyckoff]

frrrwww@gmail.com wrote on Sat, 24 May 2014 02:39 +0100:
> The diff information render the spec file unusable as is by p4,
> do not include it when run with --prepare-p4-only so that the
> given file can be directly passed to p4.
> 
> With --prepare-p4-only, git-p4 already tells the user it can use
> p4 submit with the generated spec file. This fails because of the
> diff being present in the file. Not including the diff fixes that.
> 
> Without --prepare-p4-only, keeping the diff makes sense for a
> quick review of the patch before submitting it. And does not cause
> problems with p4 as we remove it programmatically.
> 
> Signed-off-by: Maxime Coste <frrrwww@gmail.com>

Hi Maxime.  This looks really good.  Even the Windows section
is fine; thanks for paying attention there too.

I'm not particularly worried about having a new test for this.
Your tweak to the existing 9807 is fine.  Unless of course you
have one ready to go.

Acked-by: Pete Wyckoff <pw@padd.com>

You might add my ack and send it directly to Junio + CC the list.
It'll be a nice improvement for the next available release.

		-- Pete

[PATCH] git-p4: Do not include diff in spec file when just preparing p4

[Maxime Coste]

The diff information render the spec file unusable as is by p4,
do not include it when run with --prepare-p4-only so that the
given file can be directly passed to p4.

With --prepare-p4-only, git-p4 already tells the user it can use
p4 submit with the generated spec file. This fails because of the
diff being present in the file. Not including the diff fixes that.

Without --prepare-p4-only, keeping the diff makes sense for a
quick review of the patch before submitting it. And does not cause
problems with p4 as we remove it programmatically.

Signed-off-by: Maxime Coste <frrrwww@gmail.com>
Acked-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                | 49 +++++++++++++++++++++++++-----------------------
 t/t9807-git-p4-submit.sh |  3 ++-
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 773cafc..7bb0f73 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1238,6 +1238,28 @@ class P4Submit(Command, P4UserMap):
             if response == 'n':
                 return False
 
+    def get_diff_description(self, editedFiles):
+        # diff
+        if os.environ.has_key("P4DIFF"):
+            del(os.environ["P4DIFF"])
+        diff = ""
+        for editedFile in editedFiles:
+            diff += p4_read_pipe(['diff', '-du',
+                                  wildcard_encode(editedFile)])
+
+        # new file diff
+        newdiff = ""
+        for newFile in filesToAdd:
+            newdiff += "==== new file ====\n"
+            newdiff += "--- /dev/null\n"
+            newdiff += "+++ %s\n" % newFile
+            f = open(newFile, "r")
+            for line in f.readlines():
+                newdiff += "+" + line
+            f.close()
+
+        return diff + newdiff
+
     def applyCommit(self, id):
         """Apply one commit, return True if it succeeded."""
 
@@ -1398,34 +1420,15 @@ class P4Submit(Command, P4UserMap):
             submitTemplate += "######## Variable git-p4.skipUserNameCheck hides this message.\n"
 
         separatorLine = "######## everything below this line is just the diff #######\n"
+        if not self.prepare_p4_only:
+            submitTemplate += separatorLine
+            submitTemplate += self.get_diff_description(editedFiles)
 
-        # diff
-        if os.environ.has_key("P4DIFF"):
-            del(os.environ["P4DIFF"])
-        diff = ""
-        for editedFile in editedFiles:
-            diff += p4_read_pipe(['diff', '-du',
-                                  wildcard_encode(editedFile)])
-
-        # new file diff
-        newdiff = ""
-        for newFile in filesToAdd:
-            newdiff += "==== new file ====\n"
-            newdiff += "--- /dev/null\n"
-            newdiff += "+++ %s\n" % newFile
-            f = open(newFile, "r")
-            for line in f.readlines():
-                newdiff += "+" + line
-            f.close()
-
-        # change description file: submitTemplate, separatorLine, diff, newdiff
         (handle, fileName) = tempfile.mkstemp()
         tmpFile = os.fdopen(handle, "w+")
         if self.isWindows:
             submitTemplate = submitTemplate.replace("\n", "\r\n")
-            separatorLine = separatorLine.replace("\n", "\r\n")
-            newdiff = newdiff.replace("\n", "\r\n")
-        tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
+        tmpFile.write(submitTemplate)
         tmpFile.close()
 
         if self.prepare_p4_only:
diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index 4caf36e..7fab2ed 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -403,7 +403,8 @@ test_expect_success 'submit --prepare-p4-only' '
 		git commit -m "prep only add" &&
 		git p4 submit --prepare-p4-only >out &&
 		test_i18ngrep "prepared for submission" out &&
-		test_i18ngrep "must be deleted" out
+		test_i18ngrep "must be deleted" out &&
+		! test_i18ngrep "everything below this line is just the diff" out
 	) &&
 	(
 		cd "$cli" &&
-- 
1.9.3

[PATCH] Fix git-p4 submit in non --prepare-p4-only mode

[Maxime Coste]

b4073bb387ef303c9ac3c044f46d6a8ae6e190f0 broke git p4 submit, here
is a proper fix, including proper handling for windows end of lines.

Signed-off-by: Maxime Coste <frrrwww@gmail.com>
---
 git-p4.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 7bb0f73..ff132b2 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1238,7 +1238,7 @@ class P4Submit(Command, P4UserMap):
             if response == 'n':
                 return False
 
-    def get_diff_description(self, editedFiles):
+    def get_diff_description(self, editedFiles, filesToAdd):
         # diff
         if os.environ.has_key("P4DIFF"):
             del(os.environ["P4DIFF"])
@@ -1258,7 +1258,7 @@ class P4Submit(Command, P4UserMap):
                 newdiff += "+" + line
             f.close()
 
-        return diff + newdiff
+        return (diff + newdiff).replace('\r\n', '\n')
 
     def applyCommit(self, id):
         """Apply one commit, return True if it succeeded."""
@@ -1422,10 +1422,10 @@ class P4Submit(Command, P4UserMap):
         separatorLine = "######## everything below this line is just the diff #######\n"
         if not self.prepare_p4_only:
             submitTemplate += separatorLine
-            submitTemplate += self.get_diff_description(editedFiles)
+            submitTemplate += self.get_diff_description(editedFiles, filesToAdd)
 
         (handle, fileName) = tempfile.mkstemp()
-        tmpFile = os.fdopen(handle, "w+")
+        tmpFile = os.fdopen(handle, "w+b")
         if self.isWindows:
             submitTemplate = submitTemplate.replace("\n", "\r\n")
         tmpFile.write(submitTemplate)
@@ -1475,9 +1475,9 @@ class P4Submit(Command, P4UserMap):
             tmpFile = open(fileName, "rb")
             message = tmpFile.read()
             tmpFile.close()
-            submitTemplate = message[:message.index(separatorLine)]
             if self.isWindows:
-                submitTemplate = submitTemplate.replace("\r\n", "\n")
+                message = message.replace("\r\n", "\n")
+            submitTemplate = message[:message.index(separatorLine)]
             p4_write_pipe(['submit', '-i'], submitTemplate)
 
             if self.preserveUser:
-- 
2.0.0

Re: [PATCH] Fix git-p4 submit in non --prepare-p4-only mode

[Pete Wyckoff]

frrrwww@gmail.com wrote on Tue, 10 Jun 2014 13:14 +0100:
> b4073bb387ef303c9ac3c044f46d6a8ae6e190f0 broke git p4 submit, here
> is a proper fix, including proper handling for windows end of lines.

I guess we don't have test coverage for these cases?  Is this
something that should get put into a maintenance release, quickly?

The fix looks good.  It's surprising that none of the tests
managed to add a file and trigger the failure.

I'll ack this again, as it looks okay, but hope you ran all the
unit tests successfully on your machine.

		-- Pete

> Signed-off-by: Maxime Coste <frrrwww@gmail.com>
> ---
>  git-p4.py | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index 7bb0f73..ff132b2 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1238,7 +1238,7 @@ class P4Submit(Command, P4UserMap):
>              if response == 'n':
>                  return False
>  
> -    def get_diff_description(self, editedFiles):
> +    def get_diff_description(self, editedFiles, filesToAdd):
>          # diff
>          if os.environ.has_key("P4DIFF"):
>              del(os.environ["P4DIFF"])
> @@ -1258,7 +1258,7 @@ class P4Submit(Command, P4UserMap):
>                  newdiff += "+" + line
>              f.close()
>  
> -        return diff + newdiff
> +        return (diff + newdiff).replace('\r\n', '\n')
>  
>      def applyCommit(self, id):
>          """Apply one commit, return True if it succeeded."""
> @@ -1422,10 +1422,10 @@ class P4Submit(Command, P4UserMap):
>          separatorLine = "######## everything below this line is just the diff #######\n"
>          if not self.prepare_p4_only:
>              submitTemplate += separatorLine
> -            submitTemplate += self.get_diff_description(editedFiles)
> +            submitTemplate += self.get_diff_description(editedFiles, filesToAdd)
>  
>          (handle, fileName) = tempfile.mkstemp()
> -        tmpFile = os.fdopen(handle, "w+")
> +        tmpFile = os.fdopen(handle, "w+b")
>          if self.isWindows:
>              submitTemplate = submitTemplate.replace("\n", "\r\n")
>          tmpFile.write(submitTemplate)
> @@ -1475,9 +1475,9 @@ class P4Submit(Command, P4UserMap):
>              tmpFile = open(fileName, "rb")
>              message = tmpFile.read()
>              tmpFile.close()
> -            submitTemplate = message[:message.index(separatorLine)]
>              if self.isWindows:
> -                submitTemplate = submitTemplate.replace("\r\n", "\n")
> +                message = message.replace("\r\n", "\n")
> +            submitTemplate = message[:message.index(separatorLine)]
>              p4_write_pipe(['submit', '-i'], submitTemplate)
>  
>              if self.preserveUser:
> -- 
> 2.0.0
> 
> 

Re: [PATCH] Fix git-p4 submit in non --prepare-p4-only mode

[Maxime Coste]

On Tue, Jun 10, 2014 at 06:39:58PM -0400, Pete Wyckoff wrote:
> frrrwww@gmail.com wrote on Tue, 10 Jun 2014 13:14 +0100:
> > b4073bb387ef303c9ac3c044f46d6a8ae6e190f0 broke git p4 submit, here
> > is a proper fix, including proper handling for windows end of lines.
> 
> I guess we don't have test coverage for these cases?  Is this
> something that should get put into a maintenance release, quickly?

We have test cases for that, however we need to create a link to git-p4.py
named git-p4 in order for them to work. I did not run the first patch through
the tests (see my previous email) because of that. Sorry about that.

> The fix looks good.  It's surprising that none of the tests
> managed to add a file and trigger the failure.
> 
> I'll ack this again, as it looks okay, but hope you ran all the
> unit tests successfully on your machine.

It works, only one test fail (detect copy), but this test already fails
with my two patches reverted.

This should be applied soon (or alternatively
b4073bb387ef303c9ac3c044f46d6a8ae6e190f0 should be reverted) in master,
as in the current state git p4 submit will fail most of the time.

I'll send that with your ack to Junio.

Cheers,

Maxime Coste.

[PATCH] Fix git-p4 submit in non --prepare-p4-only mode

[Maxime Coste]

b4073bb387ef303c9ac3c044f46d6a8ae6e190f0 broke git p4 submit, here
is a proper fix, including proper handling for windows end of lines.

Signed-off-by: Maxime Coste <frrrwww@gmail.com>
Acked-by: Pete Wyckoff <pw@padd.com> 
---
 git-p4.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 7bb0f73..ff132b2 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1238,7 +1238,7 @@ class P4Submit(Command, P4UserMap):
             if response == 'n':
                 return False
 
-    def get_diff_description(self, editedFiles):
+    def get_diff_description(self, editedFiles, filesToAdd):
         # diff
         if os.environ.has_key("P4DIFF"):
             del(os.environ["P4DIFF"])
@@ -1258,7 +1258,7 @@ class P4Submit(Command, P4UserMap):
                 newdiff += "+" + line
             f.close()
 
-        return diff + newdiff
+        return (diff + newdiff).replace('\r\n', '\n')
 
     def applyCommit(self, id):
         """Apply one commit, return True if it succeeded."""
@@ -1422,10 +1422,10 @@ class P4Submit(Command, P4UserMap):
         separatorLine = "######## everything below this line is just the diff #######\n"
         if not self.prepare_p4_only:
             submitTemplate += separatorLine
-            submitTemplate += self.get_diff_description(editedFiles)
+            submitTemplate += self.get_diff_description(editedFiles, filesToAdd)
 
         (handle, fileName) = tempfile.mkstemp()
-        tmpFile = os.fdopen(handle, "w+")
+        tmpFile = os.fdopen(handle, "w+b")
         if self.isWindows:
             submitTemplate = submitTemplate.replace("\n", "\r\n")
         tmpFile.write(submitTemplate)
@@ -1475,9 +1475,9 @@ class P4Submit(Command, P4UserMap):
             tmpFile = open(fileName, "rb")
             message = tmpFile.read()
             tmpFile.close()
-            submitTemplate = message[:message.index(separatorLine)]
             if self.isWindows:
-                submitTemplate = submitTemplate.replace("\r\n", "\n")
+                message = message.replace("\r\n", "\n")
+            submitTemplate = message[:message.index(separatorLine)]
             p4_write_pipe(['submit', '-i'], submitTemplate)
 
             if self.preserveUser:
-- 
2.0.0

Re: [PATCH] Fix git-p4 submit in non --prepare-p4-only mode

[Pete Wyckoff]

frrrwww@gmail.com wrote on Wed, 11 Jun 2014 14:06 +0100:
> On Tue, Jun 10, 2014 at 06:39:58PM -0400, Pete Wyckoff wrote:
> > frrrwww@gmail.com wrote on Tue, 10 Jun 2014 13:14 +0100:
> > > b4073bb387ef303c9ac3c044f46d6a8ae6e190f0 broke git p4 submit, here
> > > is a proper fix, including proper handling for windows end of lines.
> > 
> > I guess we don't have test coverage for these cases?  Is this
> > something that should get put into a maintenance release, quickly?
> 
> We have test cases for that, however we need to create a link to git-p4.py
> named git-p4 in order for them to work. I did not run the first patch through
> the tests (see my previous email) because of that. Sorry about that.

The secret is to "build" the code before running tests, just like
when working on .c files.  I tend to do something like:

    make git-p4 && (cd t ; make T="$(echo t98*)") ; pkill p4d

Thanks for catching the problem quickly and fixing it.

		-- Pete