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

[Andrew Oakley <andrew@adoakley.name>]

Thanks for doing this.  I've been meaning to write some similar code
for years and never quite got around to it.  So maybe my opinion
shouldn't be worth much :/.

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.

>      * 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?

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

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

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.