[v3,6/7] git-submodule.sh: improve variables readability

Commit Message

Roy Eldar Dec. 10, 2024, 6:44 p.m. UTC

When git-submodule.sh parses various options and switches, it sets some
variables to values; the variables in turn affect the options given to
git-submodule--helper.

Currently, variables which correspond to switches have boolean values
(for example, whenever "--force" is passed, force=1), while variables
which correspond to options which take arguments have string values that
sometimes contain the option name and sometimes only the option value.

Set all of the variables to strings which contain the option name (e.g.
force="--force" rather than force=1); this has a couple of advantages:
it improves consistency, readability and debuggability.

Suggested-by: Junio C Hamano <[e-mail]>
Signed-off-by: Roy Eldar <[e-mail]>
---
 git-submodule.sh | 213 +++++++++++++++++++++--------------------------
 1 file changed, 95 insertions(+), 118 deletions(-)

Comments

Junio C Hamano Dec. 11, 2024, 12:14 a.m. UTC | #1

Roy Eldar <[e-mail]> writes:

> +		-b* | --branch*)
> +			branch="$1"

Once another option like "--branches-only" will be caught by this
case arm.  Do not omit "=" in the pattern.

> @@ -142,14 +142,14 @@ cmd_add()
>  	fi
>  
>  	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add \
> -		${quiet:+--quiet} \
> -		${force:+--force} \
> -		${progress:+"--progress"} \
> -		${branch:+--branch "$branch"} \
> -		${reference_path:+--reference "$reference_path"} \
> +		$quiet \
> +		$force \
> +		$progress \
> +		${branch:+"$branch"} \
> +		${reference:+"$reference"} \
>  		${ref_format:+"$ref_format"} \
> -		${dissociate:+--dissociate} \
> -		${custom_name:+--name "$custom_name"} \
> +		$dissociate \
> +		${custom_name:+"$custom_name"} \
>  		${depth:+"$depth"} \
>  		-- \
>  		"$@"

Looking good.

Đoàn Trần Công Danh Dec. 11, 2024, 1:56 a.m. UTC | #2

On 2024-12-10 20:44:41+0200, Roy Eldar <[e-mail]> wrote:
>  			;;
>  		--reference)
>  			case "$2" in '') usage ;; esac
> -			reference_path=$2
> +			reference="--reference=$2"
>  			shift
>  			;;
>  		--reference=*)
> -			reference_path="${1#--reference=}"
> +			reference="$1"

--reference takes a path to some repository,
(see also git-clone --reference),
thus it can have any characters, including but not limit to whitespace.
I think we need to discard this hunk!

Junio C Hamano Dec. 11, 2024, 6:09 a.m. UTC | #3

Đoàn Trần Công Danh <[e-mail]> writes:

>>  		--reference=*)
>> -			reference_path="${1#--reference=}"
>> +			reference="$1"
>
> --reference takes a path to some repository,
> (see also git-clone --reference),
> thus it can have any characters, including but not limit to whitespace.
> I think we need to discard this hunk!

I didn't double check the code that uses the variable, but as long
as the code that uses $reference writs it correctly, e.g.

    git subcmd ${reference:+"$reference"} ...

it should correctly pass what it received in the above assignment
from the user in $1 just fine, even if its value can contain any
arbitrary byte, I would think.

Roy Eldar Dec. 11, 2024, 6:21 a.m. UTC | #4

Junio C Hamano <[e-mail]> wrote:

> Roy Eldar <[e-mail]> writes:
>
> > +             -b* | --branch*)
> > +                     branch="$1"
>
> Once another option like "--branches-only" will be caught by this
> case arm.  Do not omit "=" in the pattern.

Thanks. I noticed that I did so in a couple of other places as well, so
I'll fix that and send a v4 patch series.

Patch

diff --git a/git-submodule.sh b/git-submodule.sh
index ee86e4de46..2da4d55d64 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -52,6 +52,10 @@  single_branch=
 jobs=
 recommend_shallow=
 filter=
+deinit_all=
+default=
+summary_limit=
+for_status=
 
 #
 # Add a new submodule to the working tree, .gitmodules and the index
@@ -63,37 +67,33 @@  filter=
 cmd_add()
 {
 	# parse $args after "submodule ... add".
-	reference_path=
 	while test $# -ne 0
 	do
 		case "$1" in
 		-b | --branch)
 			case "$2" in '') usage ;; esac
-			branch=$2
+			branch="--branch=$2"
 			shift
 			;;
-		-b*)
-			branch="${1#-b}"
-			;;
-		--branch=*)
-			branch="${1#--branch=}"
+		-b* | --branch*)
+			branch="$1"
 			;;
 		-f | --force)
 			force=$1
 			;;
 		-q|--quiet)
-			quiet=1
+			quiet=$1
 			;;
 		--progress)
-			progress=1
+			progress=$1
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac
-			reference_path=$2
+			reference="--reference=$2"
 			shift
 			;;
 		--reference=*)
-			reference_path="${1#--reference=}"
+			reference="$1"
 			;;
 		--ref-format)
 			case "$2" in '') usage ;; esac
@@ -104,15 +104,15 @@  cmd_add()
 			ref_format="$1"
 			;;
 		--dissociate)
-			dissociate=1
+			dissociate=$1
 			;;
 		--name)
 			case "$2" in '') usage ;; esac
-			custom_name=$2
+			custom_name="--name=$2"
 			shift
 			;;
 		--name=*)
-			custom_name="${1#--name=}"
+			custom_name="$1"
 			;;
 		--depth)
 			case "$2" in '') usage ;; esac
@@ -120,7 +120,7 @@  cmd_add()
 			shift
 			;;
 		--depth=*)
-			depth=$1
+			depth="$1"
 			;;
 		--)
 			shift
@@ -142,14 +142,14 @@  cmd_add()
 	fi
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add \
-		${quiet:+--quiet} \
-		${force:+--force} \
-		${progress:+"--progress"} \
-		${branch:+--branch "$branch"} \
-		${reference_path:+--reference "$reference_path"} \
+		$quiet \
+		$force \
+		$progress \
+		${branch:+"$branch"} \
+		${reference:+"$reference"} \
 		${ref_format:+"$ref_format"} \
-		${dissociate:+--dissociate} \
-		${custom_name:+--name "$custom_name"} \
+		$dissociate \
+		${custom_name:+"$custom_name"} \
 		${depth:+"$depth"} \
 		-- \
 		"$@"
@@ -168,10 +168,10 @@  cmd_foreach()
 	do
 		case "$1" in
 		-q|--quiet)
-			quiet=1
+			quiet=$1
 			;;
 		--recursive)
-			recursive=1
+			recursive=$1
 			;;
 		-*)
 			usage
@@ -184,8 +184,8 @@  cmd_foreach()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach \
-		${quiet:+--quiet} \
-		${recursive:+--recursive} \
+		$quiet \
+		$recursive \
 		-- \
 		"$@"
 }
@@ -202,7 +202,7 @@  cmd_init()
 	do
 		case "$1" in
 		-q|--quiet)
-			quiet=1
+			quiet=$1
 			;;
 		--)
 			shift
@@ -219,7 +219,7 @@  cmd_init()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init \
-		${quiet:+--quiet} \
+		$quiet \
 		-- \
 		"$@"
 }
@@ -230,7 +230,6 @@  cmd_init()
 cmd_deinit()
 {
 	# parse $args after "submodule ... deinit".
-	deinit_all=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -238,10 +237,10 @@  cmd_deinit()
 			force=$1
 			;;
 		-q|--quiet)
-			quiet=1
+			quiet=$1
 			;;
 		--all)
-			deinit_all=t
+			deinit_all=$1
 			;;
 		--)
 			shift
@@ -258,9 +257,9 @@  cmd_deinit()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit \
-		${quiet:+--quiet} \
-		${force:+--force} \
-		${deinit_all:+--all} \
+		$quiet \
+		$force \
+		$deinit_all \
 		-- \
 		"$@"
 }
@@ -277,31 +276,31 @@  cmd_update()
 	do
 		case "$1" in
 		-q|--quiet)
-			quiet=1
+			quiet=$1
 			;;
 		-v|--verbose)
-			quiet=0
+			quiet=
 			;;
 		--progress)
-			progress=1
+			progress=$1
 			;;
 		-i|--init)
-			init=1
+			init=$1
 			;;
 		--require-init)
-			require_init=1
+			require_init=$1
 			;;
 		--remote)
-			remote=1
+			remote=$1
 			;;
 		-N|--no-fetch)
-			nofetch=1
+			nofetch=$1
 			;;
 		-f|--force)
 			force=$1
 			;;
 		-r|--rebase)
-			rebase=1
+			rebase=$1
 			;;
 		--ref-format)
 			case "$2" in '') usage ;; esac
@@ -320,22 +319,19 @@  cmd_update()
 			reference="$1"
 			;;
 		--dissociate)
-			dissociate=1
+			dissociate=$1
 			;;
 		-m|--merge)
-			merge=1
+			merge=$1
 			;;
 		--recursive)
-			recursive=1
+			recursive=$1
 			;;
 		--checkout)
-			checkout=1
-			;;
-		--recommend-shallow)
-			recommend_shallow="--recommend-shallow"
+			checkout=$1
 			;;
-		--no-recommend-shallow)
-			recommend_shallow="--no-recommend-shallow"
+		--recommend-shallow|--no-recommend-shallow)
+			recommend_shallow=$1
 			;;
 		--depth)
 			case "$2" in '') usage ;; esac
@@ -343,24 +339,18 @@  cmd_update()
 			shift
 			;;
 		--depth=*)
-			depth=$1
+			depth="$1"
 			;;
 		-j|--jobs)
 			case "$2" in '') usage ;; esac
 			jobs="--jobs=$2"
 			shift
 			;;
-		-j*)
-			jobs="--jobs=${1#-j}"
-			;;
-		--jobs=*)
-			jobs=$1
+		-j*|--jobs*)
+			jobs="$1"
 			;;
-		--single-branch)
-			single_branch="--single-branch"
-			;;
-		--no-single-branch)
-			single_branch="--no-single-branch"
+		--single-branch|--no-single-branch)
+			single_branch=$1
 			;;
 		--filter)
 			case "$2" in '') usage ;; esac
@@ -385,22 +375,21 @@  cmd_update()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
-		${quiet:+--quiet} \
-		${force:+--force} \
-		${progress:+"--progress"} \
-		${remote:+--remote} \
-		${recursive:+--recursive} \
-		${init:+--init} \
-		${nofetch:+--no-fetch} \
-		${rebase:+--rebase} \
-		${merge:+--merge} \
-		${checkout:+--checkout} \
+		$quiet \
+		$force \
+		$progress \
+		$remote \
+		$recursive \
+		$init \
+		$nofetch \
+		$rebase \
+		$merge \
+		$checkout \
 		${ref_format:+"$ref_format"} \
 		${reference:+"$reference"} \
-		${dissociate:+"--dissociate"} \
+		$dissociate \
 		${depth:+"$depth"} \
-		${require_init:+--require-init} \
-		${dissociate:+"--dissociate"} \
+		$require_init \
 		$single_branch \
 		$recommend_shallow \
 		$jobs \
@@ -415,9 +404,6 @@  cmd_update()
 # $@ = requested path
 #
 cmd_set_branch() {
-	default=
-	branch=
-
 	# parse $args after "submodule ... set-branch".
 	while test $# -ne 0
 	do
@@ -426,18 +412,15 @@  cmd_set_branch() {
 			# we don't do anything with this but we need to accept it
 			;;
 		-d|--default)
-			default=1
+			default=$1
 			;;
 		-b|--branch)
 			case "$2" in '') usage ;; esac
-			branch=$2
+			branch="--branch=$2"
 			shift
 			;;
-		-b*)
-			branch="${1#-b}"
-			;;
-		--branch=*)
-			branch="${1#--branch=}"
+		-b*|--branch=*)
+			branch="$1"
 			;;
 		--)
 			shift
@@ -454,9 +437,9 @@  cmd_set_branch() {
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch \
-		${quiet:+--quiet} \
-		${branch:+--branch "$branch"} \
-		${default:+--default} \
+		$quiet \
+		${branch:+"$branch"} \
+		$default \
 		-- \
 		"$@"
 }
@@ -472,7 +455,7 @@  cmd_set_url() {
 	do
 		case "$1" in
 		-q|--quiet)
-			quiet=1
+			quiet=$1
 			;;
 		--)
 			shift
@@ -489,7 +472,7 @@  cmd_set_url() {
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url \
-		${quiet:+--quiet} \
+		$quiet \
 		-- \
 		"$@"
 }
@@ -503,32 +486,26 @@  cmd_set_url() {
 # $@ = [commit (default 'HEAD'),] requested paths (default all)
 #
 cmd_summary() {
-	summary_limit=-1
-	for_status=
-
 	# parse $args after "submodule ... summary".
 	while test $# -ne 0
 	do
 		case "$1" in
 		--cached)
-			cached=1
+			cached=$1
 			;;
 		--files)
-			files="$1"
+			files=$1
 			;;
 		--for-status)
-			for_status="$1"
+			for_status=$1
 			;;
 		-n|--summary-limit)
 			case "$2" in '') usage ;; esac
-			summary_limit="$2"
+			summary_limit="--summary-limit=$2"
 			shift
 			;;
-		-n*)
-			summary_limit="${1#-n}"
-			;;
-		--summary-limit=*)
-			summary_limit="${1#--summary-limit=}"
+		-n*|--summary-limit=*)
+			summary_limit="$1"
 			;;
 		--)
 			shift
@@ -545,10 +522,10 @@  cmd_summary() {
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary \
-		${files:+--files} \
-		${cached:+--cached} \
-		${for_status:+--for-status} \
-		${summary_limit:+-n "$summary_limit"} \
+		$files \
+		$cached \
+		$for_status \
+		${summary_limit:+"$summary_limit"} \
 		-- \
 		"$@"
 }
@@ -569,13 +546,13 @@  cmd_status()
 	do
 		case "$1" in
 		-q|--quiet)
-			quiet=1
+			quiet=$1
 			;;
 		--cached)
-			cached=1
+			cached=$1
 			;;
 		--recursive)
-			recursive=1
+			recursive=$1
 			;;
 		--)
 			shift
@@ -592,9 +569,9 @@  cmd_status()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status \
-		${quiet:+--quiet} \
-		${cached:+--cached} \
-		${recursive:+--recursive} \
+		$quiet \
+		$cached \
+		$recursive \
 		-- \
 		"$@"
 }
@@ -611,11 +588,11 @@  cmd_sync()
 	do
 		case "$1" in
 		-q|--quiet)
-			quiet=1
+			quiet=$1
 			shift
 			;;
 		--recursive)
-			recursive=1
+			recursive=$1
 			shift
 			;;
 		--)
@@ -632,8 +609,8 @@  cmd_sync()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync \
-		${quiet:+--quiet} \
-		${recursive:+--recursive} \
+		$quiet \
+		$recursive \
 		-- \
 		"$@"
 }
@@ -656,10 +633,10 @@  do
 		command=$1
 		;;
 	-q|--quiet)
-		quiet=1
+		quiet=$1
 		;;
 	--cached)
-		cached=1
+		cached=$1
 		;;
 	--)
 		break