Re: [PATCH] bisect: fix replay of CRLF logs

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 7, 2020, 22:13 UTC
Message-ID: <CAPig+cTbvmGPjR85RupL4DZjzCwjeHxBqnbPg1+8Gd-Xmzf7CA@mail.gmail.com>
In-Reply-To: <pull.629.git.1588886980377.gitgitgadget@gmail.com>

On Thu, May 7, 2020 at 5:29 PM Christopher Warrington via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 20 quoted lines

> Sometimes bisect logs have CRLF newlines. (E.g., if they've been edited
> on a Windows machine and their LF-only nature wasn't preserved.)
> Previously, such log files would cause odd failures deep in the guts of
> git bisect, like "?? what are you talking about?" or "couldn't get the
> oid of the rev '...?'" (notice the trailing ?) as each line's CR ends up
> part of the final value read from the log.
>
> This commit fixes that by stripping CRs from the log before further
> processing.
> [...]
> Signed-off-by: Christopher Warrington <chwarr@microsoft.com>
> ---
> diff --git a/git-bisect.sh b/git-bisect.sh
> @@ -209,7 +209,11 @@ bisect_replay () {
> -       while read git bisect command rev
> +
> +       # We remove any CR in the input to handle bisect log files that have
> +       # CRLF line endings. The assumption is that CR within bisect
> +       # commands also don't matter.
> +       tr -d '\r' <"$file" | while read git bisect command rev

Due to portability concerns, I worry about using '\r' here. Indeed this would be its first use in this codebase. On the other hand, '\015' is heavily used (at least in the tests), so that would likely be a safer alternative.

Show 6 quoted lines

> @@ -231,7 +235,9 @@ bisect_replay () {
> -       done <"$file"
> +       done
> +
> +       get_terms
>         bisect_auto_next

Why the new get_terms() invocation? Is that leftover debugging gunk?

> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> @@ -792,6 +792,13 @@ test_expect_success 'bisect replay with old and new' '
> +test_expect_success 'bisect replay with CRLF log' '
> +       awk 1 "ORS=\\r\\n" <log_to_replay.txt >log_to_replay_crlf.txt &&

This would be the first use of awk's ORS in this codebase, which may invite portability problems. In this codebase, the more typical way to do this is via a combination of 'sed' and 'tr', however, even better would be to take advantage of append_cr() from t/test-lib-functions.sh:

    appenc_cr <log_to_replay.txt >log_to_replay_crlf.txt &&

> +       git bisect replay log_to_replay_crlf.txt >bisect_result_crlf &&
> +       grep "$HASH2 is the first new commit" bisect_result_crlf &&
> +       git bisect reset
> +'