Re: [PATCH v3] add git-p4.fallbackEncoding config setting, to prevent git-p4 from crashing on non UTF-8 changeset descriptions

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 22, 2021, 16:17 UTC
Message-ID: <CAPig+cQE0oHHY89D6fLyymduY3=zSe8y246cz1P2MjTZhrMHNQ@mail.gmail.com>
In-Reply-To: <20210422155047.3unltvv3mh5uq7wp@tb-raspi4>

On Thu, Apr 22, 2021 at 11:51 AM Torsten Bögershausen <tboegi@web.de> wrote:

Show 7 quoted lines

> On Wed, Apr 21, 2021 at 10:05:04PM -0700, Tzadik Vanderhoof wrote:
> > +if test_have_prereq !MINGW,!CYGWIN; then
> > +     skip_all='This system is not subject to encoding failures in "git p4 clone"'
> > +     test_done
> > +fi
>
> Out of curiosity: Why are Windows versions (MINGW, CYGWIN) excluded ?

The answer to this question is probably worthy of recording as an in-code comment just above this conditional so that people coming upon this test script in the future don't have to ask the same question (which is especially important if the author is no longer reachable). If an in-code comment is overkill, then it would probably be a good idea for the commit message to explain the reason.

Show 13 quoted lines

> > +test_expect_success 'clone succeeds with git-p4.fallbackEncoding set to "cp1252"' '
> > +     git config --global git-p4.fallbackEncoding cp1252 &&
> > +     test_when_finished cleanup_git &&
> > +     (
> > +             git p4 clone --dest="$git" //depot@all &&
> > +             cd "$git" &&
> > +             git log --oneline >log &&
> > +             desc=$(head -1 log | awk '\''{print $2}'\'') && [ "$desc" = "documentación" ]
>
> Style nit:
> See Documentation/CodingGuidelines: - We prefer "test" over "[ ... ]".
>
> desc=$(head -1 log | awk '\''{print $2}'\'') && test "$desc" = "documentación"

Style also suggests splitting the line after the &&.

We normally want to avoid using bare single-quotes inside the body of the test since the body itself is a single-quoted string. These single-quotes make it harder for a reader to reason about what is going on; especially with the $2 in there, one has to spend extra cycles wondering if $2 is correctly expanded when the test runs or when it is first defined. So, an easier-to-understand rewrite might be:

    desc=$(head -1 log | awk ''{print \$2}'') &&
    test "$desc" = "documentación"

Many existing tests in this project use `cut` for word-plucking, so an alternative would be:

    desc=$(head -1 log | cut -d" " -f2) &&