Re: [PATCH v2] [RFC] git-p4: improve encoding handling to support inconsistent encodings

From: Tao Klerks <tao@klerks.biz>
Date: Apr 14, 2022, 09:57 UTC
Message-ID: <CAPMMpoiXNKNnARhJ2n+nzOj==-27YA68OvMmUyYnSaoLbfE4xw@mail.gmail.com>
In-Reply-To: <20220413214109.48097ac1@ado-tr.dyn.home.arpa>

On Wed, Apr 13, 2022 at 10:41 PM Andrew Oakley <andrew@adoakley.name> wrote:

Show 24 quoted lines

>
> On Wed, 13 Apr 2022 06:24:29 +0000
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> wrote:
> > Make the changelist-description- and user-fullname-handling code
> > python-runtime-agnostic, introducing three "strategies" selectable via
> > config:
> > - 'legacy', behaving as previously under python2,
> > - 'strict', behaving as previously under python3, and
> > - 'fallback', favoring utf-8 but supporting a secondary encoding when
> > utf-8 decoding fails, and finally replacing remaining unmappable
> > bytes.
>
> I was thinking about making the config option be a list of encodings to
> try.  So the options you've given map something like this:
> - "legacy" -> "raw"
> - "strict" -> "utf8"
> - "fallback" -> "utf8 cp1252" (or whatever is configured)
>
> This doesn't handle the case of using a replacement character, but in
> reality I suspect that fallback encoding will always be a legacy 8bit
> codec anyway.
>
> I think what you've proposed is fine too, I'm not sure what would end
> up being easier to understand.

I'm not sure I understand the proposal... Specifically, I don't understand how "raw" would work in that scheme.

In "my" current scheme, there is a big difference between "legacy" and the other two strategies: the legacy strategy reads "raw", but also *writes* "raw".

The other schemes read whatever encoding, and then write utf-8. In the case of strict, that actually works out the same as "raw", as long as the input bytes were valid utf-8 (and otherwise nothing happens anyway). In the case of fallback, that's a completely different behavior to legacy's read-raw write-raw.

Is your proposal to independently specify the read encodings *and* the write encoding, as separate parameters? That was actually my original approach, but it turned out to, in my opinion, be harder to understand (and implement :) )

Show 12 quoted lines

>
> >      * Does it make sense to make "fallback" the default decoding
> > strategy in python3? This is definitely a change in behavior, but I
> > believe for the better; failing with "we defaulted to strict, but you
> > can run again with this other option if you want it to work" seems
> > unkind, only making sense if we thought fallback to cp1252 would be
> > wrong in a substantial proportion of cases...
>
> The only issue I can see with changing the default is that it might
> lead to a surprising loss of data for someone migrating to git.  Maybe
> print a warning the first time git-p4 encounters something that can't be
> decoded as UTF-8, but then continue with the fallback to cp1252?

Honestly, I'm not sure how much a warning does. In my perforce repo, for example, any new warnings during the import would get drowned out by the mac metadata ignoring warnings.

I understand and share the data loss concern.

As I just answered Ævar, I *think* I'd like to address the data loss concern by escaping all x80+ bytes if something cannot be interpreted even using the fallback encoding. In a commit message there could also be a suffix explaining what happened, although I suspect that's pointless complexity. The advantage of this approach is that it makes it *possible* to reconstruct the original bytestream precisely, but without creating badly-encoded git commit messages that need to be skirted around.

Show 9 quoted lines

>
> >      * Is it OK to duplicate the bulk of the testing code across
> >        t9835-git-p4-metadata-encoding-python2.sh and
> >        t9836-git-p4-metadata-encoding-python3.sh?
> >      * Is it OK to explicitly call "git-p4.py" in tests, rather than
> > the build output "git-p4", in order to be able to select the python
> >        runtime on a per-test basis? Is there a better approach?
>
> I tried to find a nicer way to do this and failed.

OK thx.

Show 7 quoted lines

>
> >      * Is the naming of the strategies appropriate? Should the default
> >        python2 strategy be called something less opinionated, like
> >        "passthrough"?
>
> I think that "passthrough" or "raw" would be more descriptive names.
>

OK thx, I'll take "passthrough" as it feels slightly less positive than "raw", for some reason that I can't put my finger on :)

> The changes to git-p4 itself look good to me.  I think that dealing
> with bytes more and strings less will be good going forward.

Thx for your feedback!