git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] git-p4: preserve utf8 BOM when importing from p4 to git
@ 2022-04-04  5:50 Tao Klerks via GitGitGadget
  0 siblings, 0 replies; 7+ messages in thread
From: Tao Klerks via GitGitGadget @ 2022-04-04  5:50 UTC (permalink / raw)
  To: git; +Cc: Tao Klerks, Tao Klerks

From: Tao Klerks <tao@klerks.biz>

Perforce has a file type "utf8" which represents a text file with
explicit BOM. utf8-encoded files *without* BOM are stored as
regular file type "text". The "utf8" file type behaves like text
in all but one important way: it is stored, internally, without
the leading 3 BOM bytes.

git-p4 has historically imported utf8-with-BOM files (files stored,
in Perforce, as type "utf8") the same way as regular text files -
losing the BOM in the process.

Under most circumstances this issue has little functional impact,
as most systems consider the BOM to be optional and redundant, but
this *is* a correctness failure, and can have lead to practical
issues for example when BOMs are explicitly included in test files,
for example in a file encoding test suite.

Fix the handling of utf8-with-BOM files when importing changes from
p4 to git, and introduce a test that checks it is working correctly.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    git-p4: preserve utf8 BOM when importing from p4 to git
    
    I manually tested these changes with python2 and python3 - I don't know
    whether there is a more rigorous approach possible than changing the
    system default python and rerunning the "t98xx" tests, but that did work
    (and initially highlighted an issue, now fixed).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1203%2FTaoK%2Fgit-p4-utf8-bom-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1203/TaoK/git-p4-utf8-bom-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1203

 git-p4.py                  | 10 ++++++++++
 t/t9802-git-p4-filetype.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index a9b1f904410..6d932e7ed76 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2885,6 +2885,16 @@ class P4Sync(Command, P4UserMap):
             print("\nIgnoring apple filetype file %s" % file['depotFile'])
             return
 
+        if type_base == "utf8":
+            # The type utf8 explicitly means utf8 *with BOM*. These are
+            # streamed just like regular text files, however, without
+            # the BOM in the stream.
+            # Therefore, to accurately import these files into git, we
+            # need to explicitly re-add the BOM before writing.
+            # 'contents' is a set of bytes in this case, so create the
+            # BOM prefix as a b'' literal.
+            contents = [b'\xef\xbb\xbf' + contents[0]] + contents[1:]
+
         # Note that we do not try to de-mangle keywords on utf16 files,
         # even though in theory somebody may want that.
         regexp = p4_keywords_regexp_for_type(type_base, type_mods)
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 19073c6e9f8..2a6ee2a4678 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -333,4 +333,38 @@ test_expect_success SYMLINKS 'empty symlink target' '
 	)
 '
 
+test_expect_success SYMLINKS 'utf-8 with and without BOM in text file' '
+	(
+		cd "$cli" &&
+
+		# some utf8 content
+		echo some tǣxt >utf8-nobom-test &&
+
+		# same utf8 content as before but with bom
+		echo some tǣxt | sed '\''s/^/\xef\xbb\xbf/'\'' >utf8-bom-test &&
+
+		# bom only
+		dd bs=1 count=3 if=utf8-bom-test of=utf8-bom-empty-test &&
+
+		p4 add utf8-nobom-test utf8-bom-test utf8-bom-empty-test &&
+		p4 submit -d "add utf8 test files"
+	) &&
+	test_when_finished cleanup_git &&
+
+	git p4 clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		git checkout refs/remotes/p4/master &&
+
+		echo some tǣxt >utf8-nobom-check &&
+		test_cmp utf8-nobom-check utf8-nobom-test &&
+
+		echo some tǣxt | sed '\''s/^/\xef\xbb\xbf/'\'' >utf8-bom-check &&
+		test_cmp utf8-bom-check utf8-bom-test &&
+
+		dd bs=1 count=3 if=utf8-bom-check of=utf8-bom-empty-check &&
+		test_cmp utf8-bom-empty-check utf8-bom-empty-test
+	)
+'
+
 test_done

base-commit: 4b6846d9dcd391164b72bd70e8a0c0e09776afe3
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH] git-p4: preserve utf8 BOM when importing from p4 to git
@ 2022-12-14  6:10 Tzadik Vanderhoof
  2022-12-14  8:29 ` Junio C Hamano
  2022-12-14 18:24 ` Tao Klerks
  0 siblings, 2 replies; 7+ messages in thread
From: Tzadik Vanderhoof @ 2022-12-14  6:10 UTC (permalink / raw)
  To: Git List
  Cc: Tao Klerks, Luke Diamand, Junio C Hamano, Eric Sunshine,
	Ævar Arnfjörð Bjarmason, Dorgon Chang,
	Joachim Kuebart, Daniel Levin, Johannes Schindelin, Ben Keene,
	Andrew Oakley

From: Tao Klerks <tao@klerks.biz>
> Perforce has a file type "utf8" which represents a text file with
> explicit BOM. utf8-encoded files *without* BOM are stored as
> regular file type "text". The "utf8" file type behaves like text
> in all but one important way: it is stored, internally, without
> the leading 3 BOM bytes.
>
> git-p4 has historically imported utf8-with-BOM files (files stored,
> in Perforce, as type "utf8") the same way as regular text files -
> losing the BOM in the process.
>
> Under most circumstances this issue has little functional impact,
> as most systems consider the BOM to be optional and redundant, but
> this *is* a correctness failure, and can have lead to practical
> issues for example when BOMs are explicitly included in test files,
> for example in a file encoding test suite.
>
> Fix the handling of utf8-with-BOM files when importing changes from
> p4 to git, and introduce a test that checks it is working correctly

I don't really understand the problem that this patch addresses, but I
feel it was incorrect to modify the behavior of git-p4 in this way.
This change has made git-p4 behave differently than the native
Perforce tools.

Perforce already has a "variable" (setting) to control this behavior.
If P4CHARSET is set to "utf8-bom", the BOM will be added to files in
the local workspace of type "utf8".  If P4CHARSET is set to "utf8",
the BOM will NOT be stored in the local workspace file, even if its
type is "utf8"!

Whatever the problem was that motivated this change, it should have
been solved using the Perforce variable P4CHARSET, not by modifying
the behavior of git-p4.  This new behavior has made it impossible for
me to submit changes to files of type "utf8"!  Any attempt fails with
"patch does not apply" and the erroneously added BOM is the cause.

I propose rolling back the patch that introduced this behavior, and if
that is not possible, I would like to submit a patch that adds a new
setting in the "git-p4" section that will enable users to opt out of
this new behavior (for example a boolean setting "git-p4.noUtf8Bom").

-- 
Tzadik

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-12-22  8:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-04  5:50 [PATCH] git-p4: preserve utf8 BOM when importing from p4 to git Tao Klerks via GitGitGadget
  -- strict thread matches above, loose matches on Subject: below --
2022-12-14  6:10 Tzadik Vanderhoof
2022-12-14  8:29 ` Junio C Hamano
2022-12-14 18:24 ` Tao Klerks
2022-12-14 23:11   ` Junio C Hamano
2022-12-19  9:09     ` Tao Klerks
2022-12-22  8:26       ` Tao Klerks

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).