git-p4: problem with commit 97a21ca50ef8

git-p4: problem with commit 97a21ca50ef8

[Michael Wookey]

[ please CC me as I am not subscribed to the list ]

Hi,

Commit 97a21ca50ef893a171a50c863fe21a924935fd2a "git-p4: stop ignoring
apple filetype" isn't correct. Without knowing too much about how
git-p4 works, it appears that the "apple" filetype includes the
resource fork, and the "p4 print" that is used to obtain the content
from the perforce server doesn't take this into account, or maybe some
post processing of the file needs to be done to include the data, but
not the resource fork, before inclusion into the git repo.

With the above commit, a binary blob that literally contains the
resource fork and data is included within the git repo. Of course,
without the above commit, the intended file was never included in the
git repo at all. Perhaps the resource fork issue was a known problem
by the original git-p4 author.

A sample file that that demonstrates what the above commit produces is
here (use curl/wget):

  http://dl.dropbox.com/u/1006983/sample_image_fail.png

This is literally a binary blob with about 110 KiB of resource fork
plus the PNG data. The same image, minus about 110 KiB of resource
fork is here:

  http://dl.dropbox.com/u/1006983/sample_image_correct.png

I'm happy to test patches as we have a perforce repository with files
of the "apple" filetype.

Thanks

Re: git-p4: problem with commit 97a21ca50ef8

[Pete Wyckoff]

michaelwookey@gmail.com wrote on Tue, 01 Nov 2011 10:11 +1100:
> [ please CC me as I am not subscribed to the list ]
> 
> Hi,
> 
> Commit 97a21ca50ef893a171a50c863fe21a924935fd2a "git-p4: stop ignoring
> apple filetype" isn't correct. Without knowing too much about how
> git-p4 works, it appears that the "apple" filetype includes the
> resource fork, and the "p4 print" that is used to obtain the content
> from the perforce server doesn't take this into account, or maybe some
> post processing of the file needs to be done to include the data, but
> not the resource fork, before inclusion into the git repo.
> 
> With the above commit, a binary blob that literally contains the
> resource fork and data is included within the git repo. Of course,
> without the above commit, the intended file was never included in the
> git repo at all. Perhaps the resource fork issue was a known problem
> by the original git-p4 author.
> 
> A sample file that that demonstrates what the above commit produces is
> here (use curl/wget):
> 
>   http://dl.dropbox.com/u/1006983/sample_image_fail.png
> 
> This is literally a binary blob with about 110 KiB of resource fork
> plus the PNG data. The same image, minus about 110 KiB of resource
> fork is here:
> 
>   http://dl.dropbox.com/u/1006983/sample_image_correct.png
> 
> I'm happy to test patches as we have a perforce repository with files
> of the "apple" filetype.

Thanks so much for taking the time to find this and to narrow it
down.

I found icnsutils that shows the fail.png file has a bunch of
icons glued onto the front of the correct image file.

We can certainly revert this commit, but first I'd like to
understand what the right behavior should be.

I managed to include an apple filetype in a repo from a linux box
by hacking:

    $ cp /tmp/sample_image_fail.png fail.png
    $ p4 add -t apple fail.png 
    $ p4 submit -dfail-apple
    Submitting change 2.
    Locking 1 files ...
    add //depot/fail.png#1
    Unable to read AppleDouble Header.
    open for read: /home/pw/src/perforce/cli/%fail.png: No such
    file or directory
    Submit aborted -- fix problems then use 'p4 submit -c 2'.
    Some file(s) could not be transferred from client.

Hrm.  Fake it by copying your example apple file to what it asks
for:

    $ cp fail.png %fail.png
    $ p4 submit -c 2
    Submitting change 2.
    add //depot/fail.png#1
    Change 2 submitted.

(But later p4 sync -f destroy both files.)

Voila:

    $ p4 fstat //depot/fail.png
    ... depotFile //depot/fail.png
    ... clientFile /home/pw/src/perforce/cli/fail.png
    ... isMapped 
    ... headAction add
    ... headType apple
    ... headTime 1320111844
    ... headRev 1
    ... headChange 2
    ... headModTime 1320111842
    ... haveRev 1

And git-p4 checks it out intact:

    $ git p4 clone //depot
    [..]
    $ sha1sum depot/fail.png /tmp/sample_image_fail.png 
    93d175ad906147f4d75296bd2adb6d706f798c64  depot/fail.png
    93d175ad906147f4d75296bd2adb6d706f798c64  /tmp/sample_image_fail.png

Which is what I thought an apple-filetype user would want.
Reverting the patch causes _no_ file to be created.  Is
this better?  Maybe the single-blob file, since it no longer
appears in AppleDouble format, is just as useless as no file?

The other option is to use "p4 print" without the -G, which
seems to retrieve only the data fork, and leave that in the repo.
Of course, if you change it, and submit it, it makes a mess.

Would it be good if git-p4 understood how to identify and create
AppleDouble files on Mac?  If yes, eventually, we can revert the
commit and explain how this feature doesn't quite work yet.
Even if no, it seems like we should revert and complain that
this apple support is broken.

		-- Pete

Re: git-p4: problem with commit 97a21ca50ef8

[Michael Wookey]

> Would it be good if git-p4 understood how to identify and create
> AppleDouble files on Mac?  If yes, eventually, we can revert the
> commit and explain how this feature doesn't quite work yet.
> Even if no, it seems like we should revert and complain that
> this apple support is broken.

I've used git-p4 for many years, and have always had to work around
the limitation of the "apple" filetype and the resulting lack of files
added to the git repo.

Of course, I'd love to have git-p4 work seamlessly for this scenario.
Even Perforce have a KB article on the limitation of the "apple"
filetype with git-p4:

  http://kb.perforce.com/article/1417/git-p4

At least with 97a21ca50ef8 reverted, there is a warning that files
will be missing. The current behaviour results in a git repo with
unusable files without any warning whatsoever. I think having unusable
files, and without warnings, is worse as there is no indication that
there is a problem with files in the working tree.

Re: git-p4: problem with commit 97a21ca50ef8

[Vitor Antunes]

Michael Wookey <michaelwookey <at> gmail.com> writes:
> Of course, I'd love to have git-p4 work seamlessly for this scenario.
> Even Perforce have a KB article on the limitation of the "apple"
> filetype with git-p4:
> 
>   http://kb.perforce.com/article/1417/git-p4
> 
"""
Step 2: Download Git-p4

Recommended version is ermshiperete’s branch, which is available from:

https://github.com/ermshiperete/git-p4

Note: Omit the “git-p4.py25” file, which is an older version that is no longer
needed.
Avoid Kernel.org’s Version of Git-p4

Git’s main source at http://git-scm.com/download and
http://www.kernel.org/pub/software/scm/git/ contains an older version of Git-p4
with limitations that ermshiperete’s branch avoids.
"""

I can almost guess _who_ wrote this KB ;)

But this is really frustrating. Why can't people just cooperate to make sure the
version in the main branch is the latest?


Vitor

Re: git-p4: problem with commit 97a21ca50ef8

[Michael Wookey]

On 3 November 2011 01:43, Vitor Antunes <vitor.hda@gmail.com> wrote:
> Michael Wookey <michaelwookey <at> gmail.com> writes:
>> Of course, I'd love to have git-p4 work seamlessly for this scenario.
>> Even Perforce have a KB article on the limitation of the "apple"
>> filetype with git-p4:
>>
>>   http://kb.perforce.com/article/1417/git-p4
>>
> """
> Step 2: Download Git-p4
>
> Recommended version is ermshiperete’s branch, which is available from:
>
> https://github.com/ermshiperete/git-p4
>
> Note: Omit the “git-p4.py25” file, which is an older version that is no longer
> needed.
> Avoid Kernel.org’s Version of Git-p4
>
> Git’s main source at http://git-scm.com/download and
> http://www.kernel.org/pub/software/scm/git/ contains an older version of Git-p4
> with limitations that ermshiperete’s branch avoids.
> """
>
> I can almost guess _who_ wrote this KB ;)
>
> But this is really frustrating. Why can't people just cooperate to make sure the
> version in the main branch is the latest?

I tried your suggested version of git-p4 (at rev 630fb678c46c) and
unfortunately, the perforce repository fails to import. Firstly, there
was a problem with importing UTF-16 encoded files, secondly the
"apple" filetype files are still skipped.

Re: git-p4: problem with commit 97a21ca50ef8

[Vitor Antunes]

Hi Michael,

On Wed, Nov 2, 2011 at 10:42 PM, Michael Wookey <michaelwookey@gmail.com> wrote:
> I tried your suggested version of git-p4 (at rev 630fb678c46c) and
> unfortunately, the perforce repository fails to import. Firstly, there
> was a problem with importing UTF-16 encoded files, secondly the
> "apple" filetype files are still skipped.

I had no intention of directing you to try that version. Sorry for
misleading you on this.

I just found it interesting that P4's KB contains an article that
directs users to another version which isn't this one.

-- 
Vitor Antunes

Re: git-p4: problem with commit 97a21ca50ef8

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Thu, 03 Nov 2011 11:04 +0000:
> Hi Michael,
> 
> On Wed, Nov 2, 2011 at 10:42 PM, Michael Wookey <michaelwookey@gmail.com> wrote:
> > I tried your suggested version of git-p4 (at rev 630fb678c46c) and
> > unfortunately, the perforce repository fails to import. Firstly, there
> > was a problem with importing UTF-16 encoded files, secondly the
> > "apple" filetype files are still skipped.
> 
> I had no intention of directing you to try that version. Sorry for
> misleading you on this.
> 
> I just found it interesting that P4's KB contains an article that
> directs users to another version which isn't this one.

We're making contact offline with perforce folk and other git-p4
folk.  I'll update with the results.

I've not run the kb version, but for git's git-p4, Utf-16 was
fixed only recently (55aa571).  I'm going to revert the apple
filetype issue (97a21ca) that Michael found soon, hopefully
before v1.7.8 goes out.

		-- Pete

[PATCH] git-p4: ignore apple filetype

[Pete Wyckoff]

Revert 97a21ca (git-p4: stop ignoring apple filetype, 2011-10-16)
and add a test case.

Reported-by: Michael Wookey <michaelwookey@gmail.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---

This is mostly a revert, but the test moves down a bit to be near
a similar clause for utf16.  Adding a big comment and test case
hopefully keeps this code in place in the future.

Michael: if you're willing to test this, I'd appreciate it.  In
fact, running all the git-p4 unit tests on Mac would be great
if you have a p4d:

    mac$ ( cd t ; make t98* )

 contrib/fast-import/git-p4 |   13 +++++++++++++
 t/t9802-git-p4-filetype.sh |   31 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index f885d70..b975d67 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1318,6 +1318,19 @@ class P4Sync(Command, P4UserMap):
             text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
             contents = [ text ]
 
+        if type_base == "apple":
+            # Apple filetype files will be streamed as a concatenation of
+            # its appledouble header and the contents.  This is useless
+            # on both macs and non-macs.  If using "print -q -o xx", it
+            # will create "xx" with the data, and "%xx" with the header.
+            # This is also not very useful.
+            #
+            # Ideally, someday, this script can learn how to generate
+            # appledouble files directly and import those to git, but
+            # non-mac machines can never find a use for apple filetype.
+            print "\nIgnoring apple filetype file %s" % file['depotFile']
+            return
+
         # Perhaps windows wants unicode, utf16 newlines translated too;
         # but this is not doing it.
         if self.isWindows and type_base == "text":
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 3b358ef..992bb8c 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -101,6 +101,37 @@ test_expect_success 'keyword file test' '
 	)
 '
 
+build_gendouble() {
+	cat >gendouble.py <<-\EOF
+	import sys
+	import struct
+	import array
+
+	s = array.array("c", '\0' * 26)
+	struct.pack_into(">L", s,  0, 0x00051607)  # AppleDouble
+	struct.pack_into(">L", s,  4, 0x00020000)  # version 2
+	s.tofile(sys.stdout)
+	EOF
+}
+
+test_expect_success 'ignore apple' '
+	test_when_finished rm -f gendouble.py &&
+	build_gendouble &&
+	(
+		cd "$cli" &&
+		test-genrandom apple 1024 >double.png &&
+		"$PYTHON_PATH" "$TRASH_DIRECTORY/gendouble.py" >%double.png &&
+		p4 add -t apple double.png &&
+		p4 submit -d appledouble
+	) &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		test ! -f double.png
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.7.7.345.g88d3c

Re: [PATCH] git-p4: ignore apple filetype

[Michael Wookey]

> This is mostly a revert, but the test moves down a bit to be near
> a similar clause for utf16.  Adding a big comment and test case
> hopefully keeps this code in place in the future.
>
> Michael: if you're willing to test this, I'd appreciate it.  In
> fact, running all the git-p4 unit tests on Mac would be great
> if you have a p4d:
>
>    mac$ ( cd t ; make t98* )

I tested this and the warnings about the "apple" filetype do indeed
appear. I have also run the test suite and all git-p4 tests pass on
Mac OS X (10.7.2).

Thanks again.

Re: [PATCH] git-p4: ignore apple filetype

[Junio C Hamano]

Thanks, both. Will include the patch in 1.7.8-rc1.