Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2022, 23:20 UTC
Message-ID: <xmqqley7yd6e.fsf@gitster.g>
In-Reply-To: <patch-1.4-75c8f7a719c-20220218T205753Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 19 quoted lines

> Change our ASAN_OPTIONS and LSAN_OPTIONS to set defaults for those
> variables, rather than punting out entirely if we already have them in
> the environment.
>
> We do want to take any user-provided settings over our own, but we can
> do do that by prepending our defaults to the variable. The
> libsanitizer options parsing has "last option wins" semantics.
>
> It's now possible to do e.g.:
>
>     LSAN_OPTIONS=report_objects=1 ./t0006-date.sh
>
> And not have the "report_objects=1" setting overwrite our sensible
> default of "abort_on_error=1", but by prepending to the list we ensure
> that:
>
>     LSAN_OPTIONS=report_objects=1:abort_on_error=0 ./t0006-date.sh
>
> Will take the desired "abort_on_error=0" over our default.

Makes sense.

> +# Prepend a string to a VAR using an arbitrary ":" delimiter, not
> +# adding the delimiter if VAR is empty. I.e. a generalized:
> +#
> +#	VAR=$1${VAR:+$VAR}

This reads: "Begin with the first parameter, and if VAR is set to a non-empty string, append $VAR immediately after it without any delimiter". I would understand if it were

    VAR=$1${VAR:+":$VAR"}

(or without the first colon, allowing an empty string as a valid member of colon-separated list).

Show 5 quoted lines

> +# Usage (using ":" as a delimiter):
> +#
> +#	prepend_var VAR : $1
> +prepend_var () {
> +	eval "$1=$3\${$1:+$2\$$1}"

This one is correct; the above sample, when passed ":" and "VAR" to $1 and $2, will specialize into the above example.

> +}
> +
> +# If [AL]SAN is in effect we want to abort so that we notice problems.
> +prepend_var XSAN_OPTIONS : abort_on_error=1

XSAN_OPTIONS stands for "options that are common to all ?SAN", I guess.

Show 8 quoted lines

>  # If we were built with ASAN, it may complain about leaks
>  # of program-lifetime variables. Disable it by default to lower
>  # the noise level. This needs to happen at the start of the script,
>  # before we even do our "did we build git yet" check (since we don't
>  # want that one to complain to stderr).
> -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
> +prepend_var ASAN_OPTIONS : $XSAN_OPTIONS
> +prepend_var ASAN_OPTIONS : detect_leaks=0

This makes me wonder if you want to generalize prepend_var even further to notice when "$3" is an empty string. It already avoids spending an extra colon (and introducing an empty element in the colon delimited list) by paying attention to the current value of $1, so it would make sense to do the same for the incoming value.

IOW, the current prepend_var implementation relies on $XSAN_OPTIONS and detect_leaks=0 both being non-empty string.

Show 6 quoted lines

>  export ASAN_OPTIONS
>  
> -# If LSAN is in effect we _do_ want leak checking, but we still
> -# want to abort so that we notice the problems.
> -: ${LSAN_OPTIONS=abort_on_error=1}
> +prepend_var LSAN_OPTIONS : $XSAN_OPTIONS

But other than that, I think this step makes quite a lot of sense.

>  export LSAN_OPTIONS
>  
>  if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS