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

[Eric Sunshine <sunshine@sunshineco.com>]

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

> @@ -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
> +'