threads / patch / 35634

patchgit-p4: Do not include diff in spec file when just preparing p4

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

## tl;dr

13 messages between Jan 10, 2014 and Jun 11, 2014. Diffs are folded; open one to read it.

replies: 12people: 2as markdown or json

Maxime Coste· Jan 10, 2014, 18:18 UTC · lore

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

Show changes to git-p4.py +39 −31

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

Pete Wyckoff· Jan 12, 2014, 22:29 UTC · re: Maxime Coste · lore

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

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

Maxime Coste· Jan 13, 2014, 12:10 UTC · re: Pete Wyckoff · lore

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

Hello,

On Sun, Jan 12, 2014 at 05:29:46PM -0500, Pete Wyckoff wrote:

Show 7 quoted lines

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

Pete Wyckoff· Jan 14, 2014, 00:06 UTC · re: Maxime Coste · lore

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

frrrwww@gmail.com wrote on Mon, 13 Jan 2014 12:10 +0000:

Show 24 quoted lines

> 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

Maxime Coste· May 24, 2014, 01:39 UTC · re: Pete Wyckoff · lore

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

Show changes to 2 files +28 −24

git-p4.py, t/t9807-git-p4-submit.sh

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

Maxime Coste· May 24, 2014, 01:44 UTC · re: Maxime Coste · lore

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

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.

Pete Wyckoff· May 24, 2014, 13:52 UTC · re: Maxime Coste · lore

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

frrrwww@gmail.com wrote on Sat, 24 May 2014 02:39 +0100:

Show 13 quoted lines

> 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

Maxime Coste· May 24, 2014, 17:40 UTC · re: Pete Wyckoff · lore

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

Show changes to 2 files +28 −24

git-p4.py, t/t9807-git-p4-submit.sh

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

Maxime Coste· Jun 10, 2014, 12:14 UTC · re: Maxime Coste · lore

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

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

Show changes to git-p4.py +6 −6

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

Pete Wyckoff· Jun 10, 2014, 22:39 UTC · re: Maxime Coste · lore

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

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

Show 56 quoted 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
> 
>

Maxime Coste· Jun 11, 2014, 13:06 UTC · re: Pete Wyckoff · lore

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

On Tue, Jun 10, 2014 at 06:39:58PM -0400, Pete Wyckoff wrote:

Show 6 quoted lines

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

Show 5 quoted lines

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

Pete Wyckoff· Jun 11, 2014, 13:36 UTC · re: Maxime Coste · lore

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

frrrwww@gmail.com wrote on Wed, 11 Jun 2014 14:06 +0100:

Show 11 quoted lines

> 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

Maxime Coste· Jun 11, 2014, 13:09 UTC · re: Pete Wyckoff · lore

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

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

Show changes to git-p4.py +6 −6

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