RE: [EXTERNAL] Re: [PATCH 2/3] bisect: remove CR characters from revision in replay

On 2020-05-20 at 10:09-07:00, Carlo Marcelo Arenas Belón wrote:

My initial attempt to handle CRLF logs was shaped like this:

	tr -d '\r' <"$file" | while read ...

This introduces a subshell, so there were concerns about propagating variables and exits. So, Peff also suggested preprocessing to a file. Around the same time Junio tried using IFS, and that was simpler.

It would be pretty easy to tr -d '\r' >"$GIT_DIR/BISECT_REPLAY_LOG" (or some other name) and then read from that.

Two things I'm not sure of with such an approach:

1. Is $GIT_DIR the right place to put this? If not, is there a helper to
   create a temporary file? mktemp may not be available everywhere. I only
   see git-mergetool.sh using it and only behind the mergetool.writeToTemp
   variable.

2. How and when does this temp file need to be cleaned up? Looking at using
   something like trap "rm -f \"$temp_file\"" 0 will conflict with the traps
   that git-bisect.sh's bisect_start installs, and bisect_replay calls
   bisect_start. I could introduce a helper like
   maybe_unlink_temp_replay_log and invoke that from a trap installed in
   both bisect_replay and bisect_start.

   I looked at whether bisect.c's bisect_clean_state() is the right place to
   add such clean up: it does not look like the right place. It would result
   in the temp file getting deleted while it was being read. (git-bisect.sh
   -> bisect_replay()'s loop -> bisect_start() -> bisect--helper.c's
   bisect_start() -> bisect.c's bisect_clean_state()) Alas, not all file
   systems are OK with that happening.

Any guidance?

Here's a sketch that leaves cleanup on non-successful paths unaddressed.

bisect_replay () {
	file="$1"
	test "$#" -eq 1 || die "$(gettext "No logfile given")"
	test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
	git bisect--helper --bisect-reset || exit

	scrubbed_file="$GIT_DIR/BISECT_CLEANED_LOG"
	tr -d '\r' <"$file" >"scrubbed_file" || die "badness"

	while read git bisect command rev
	do ... done <"$scrubbed_file"

	rm -f "$scrubbed_file"
	bisect_auto_next

-- Christopher Warrington <chwarr@microsoft.com> Microsoft Corp.