Re: [PATCH 1/3] submodule: clean up subsititions in script

From: Junio C Hamano <gitster@pobox.com>
Date: May 2, 2018, 05:59 UTC
Message-ID: <xmqqy3h2vbqj.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180502005528.19740-2-kcghost@gmail.com>

Casey Fitzpatrick <kcghost@gmail.com> writes:

> 'recommend_shallow' and 'jobs' variables do not need quotes (they never contain
> spaces) and do not require any additional prefix, therefore remove the
> unnecessary subsitition.

The resulting patch is good, but "they never contain spaces" is not a very good rationale. The real reason is that (1) we use them only to hold a single token value (or leave them empty) in the current code, and (2) if the feature they represent is enhanced in the future to make them multi-token options (e.g. we may allow $jobs to contain, in addition to "--jobs=2", "--jobs 2" for whatever reason later), it is likely that we would want these multi-tokens split at $IFS (e.g. "--jobs" and "2" get passed as separate option, not a single "--jobs 2" string).

> 'progress' is a boolean value. Treat it like the other boolean values in the
> script by using a substitution.

This is OK.

Show 12 quoted lines

>  	git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
> -		${progress:+"$progress"} \
> +		${progress:+"--progress"} \
>  		${wt_prefix:+--prefix "$wt_prefix"} \
>  		${prefix:+--recursive-prefix "$prefix"} \
>  		${update:+--update "$update"} \
>  		${reference:+"$reference"} \
>  		${depth:+--depth "$depth"} \
> -		${recommend_shallow:+"$recommend_shallow"} \
> -		${jobs:+$jobs} \
> +		$recommend_shallow \
> +		$jobs \