# [PATCH] git-submodule.sh: avoid "test -a/-o " 4 messages from 2014-06-10 to 2014-06-10. Participants: Elia Pinto, Junio C Hamano, Eric Sunshine. Thread: https://gitlist.dev/t/36882 ## Elia Pinto, 2014-06-10 16:43 Subject: [PATCH] git-submodule.sh: avoid "test -a/-o " Message-ID: <1402418594-1377-1-git-send-email-gitter.spiros@gmail.com> URL: https://gitlist.dev/e/1402418594-1377-1-git-send-email-gitter.spiros%40gmail.com ``` The construct is error-prone; "test" being built-in in most modern shells, the reason to avoid "test && test " spawning one extra process by using a single "test -a " no longer exists. Signed-off-by: Elia Pinto --- This is the fifth revision. Change based on Junio bugfix and better rewrite of the case condition http://permalink.gmane.org/gmane.comp.version-control.git/251198 I dropped also the echo -> printf replacement for doing it in another patch. Pass all the t/*submodule* tests. Finally ! :=) Thank you all very much and sorry for the mess. git-submodule.sh | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index e146b83..e128a4a 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -393,7 +393,7 @@ cmd_add() sed -e 's|/$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g') fi - if test -z "$repo" -o -z "$sm_path"; then + if test -z "$repo" || test -z "$sm_path"; then usage fi @@ -450,7 +450,7 @@ Use -f if you really want to add it." >&2 # perhaps the path exists and is already a git repo, else clone it if test -e "$sm_path" then - if test -d "$sm_path"/.git -o -f "$sm_path"/.git + if test -d "$sm_path"/.git || test -f "$sm_path"/.git then eval_gettextln "Adding existing repo at '\$sm_path' to the index" else @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?")" continue fi - if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git + if ! test -d "$sm_path"/.git && test -f "$sm_path"/.git then module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit cloned_modules="$cloned_modules;$name" @@ -857,11 +857,11 @@ Maybe you want to use 'update --init'?")" die "$(eval_gettext "Unable to find current ${remote_name}/${branch} revision in submodule path '\$sm_path'")" fi - if test "$subsha1" != "$sha1" -o -n "$force" + if test "$subsha1" != "$sha1" || test -n "$force" then subforce=$force # If we don't already have a -f flag and the submodule has never been checked out - if test -z "$subsha1" -a -z "$force" + if test -z "$subsha1" && test -z "$force" then subforce="-f" fi @@ -1031,7 +1031,7 @@ cmd_summary() { then head=$rev test $# = 0 || shift - elif test -z "$1" -o "$1" = "HEAD" + elif test -z "$1" || test "$1" = "HEAD" then # before the first commit: compare with an empty tree head=$(git hash-object -w -t tree --stdin module) - test $status = D -o $status = T && echo "$sm_path" && continue + if test "$status" = D || test "$status" = T + then + echo "$sm_path" && + continue + fi # Respect the ignore setting for --for-status. if test -n "$for_status" then name=$(module_name "$sm_path") ignore_config=$(get_submodule_config "$name" ignore none) - test $status != A -a $ignore_config = all && continue + test $status != A && test $ignore_config = all && continue fi # Also show added or modified modules which are checked out GIT_DIR="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 && @@ -1122,7 +1126,7 @@ cmd_summary() { *) errmsg= total_commits=$( - if test $mod_src = 160000 -a $mod_dst = 160000 + if test $mod_src = 160000 && test $mod_dst = 160000 then range="$sha1_src...$sha1_dst" elif test $mod_src = 160000 @@ -1159,7 +1163,7 @@ cmd_summary() { # i.e. deleted or changed to blob test $mod_dst = 160000 && echo "$errmsg" else - if test $mod_src = 160000 -a $mod_dst = 160000 + if test $mod_src = 160000 && test $mod_dst = 160000 then limit= test $summary_limit -gt 0 && limit="-$summary_limit" @@ -1230,7 +1234,11 @@ cmd_status() say "U$sha1 $displaypath" continue fi - if test -z "$url" || ! test -d "$sm_path"/.git -o -f "$sm_path"/.git + if test -z "$url" || + { + ! test -d "$sm_path"/.git && + ! test -f "$sm_path"/.git + } then say "-$sha1 $displaypath" continue; @@ -1399,7 +1407,7 @@ then fi # "--cached" is accepted only by "status" and "summary" -if test -n "$cached" && test "$command" != status -a "$command" != summary +if test -n "$cached" && test "$command" != status && test "$command" != summary then usage fi -- 1.7.10.4 ``` ## Junio C Hamano, 2014-06-10 17:02 Subject: Re: [PATCH] git-submodule.sh: avoid "test -a/-o " Message-ID: URL: https://gitlist.dev/e/xmqqegywaeb5.fsf%40gitster.dls.corp.google.com In-Reply-To: <1402418594-1377-1-git-send-email-gitter.spiros@gmail.com> ``` Elia Pinto writes: > @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?")" > continue > fi > > - if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git > + if ! test -d "$sm_path"/.git && test -f "$sm_path"/.git Hmmmm. Is the above correct? $ if ! false && true; then echo true; else echo false; fi true In other words, "! cmd1 && cmd2" parses not as "! (cmd1 && cmd2)" but as "(! cmd1) && cmd2". It almost makes me wonder if the code may become simpler if we did it the way in the attached. That is, "if $sm_path/.git is there (whether as an embedded repository, or a file pointing to a repository elsewhere), then grab its HEAD, otherwise $sm_path is a submodule that hasn't been run 'submodule init' on, so run the whole nine yards starting from module_clone". git-submodule.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index e146b83..018f1bb 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -832,15 +832,15 @@ Maybe you want to use 'update --init'?")" continue fi - if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git + if test -e "$sm_path/.git" then - module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit - cloned_modules="$cloned_modules;$name" - subsha1= - else subsha1=$(clear_local_git_env; cd "$sm_path" && git rev-parse --verify HEAD) || die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")" + else + module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit + cloned_modules="$cloned_modules;$name" + subsha1= fi if test -n "$remote" ``` ## Junio C Hamano, 2014-06-10 17:03 Subject: Re: [PATCH] git-submodule.sh: avoid "test -a/-o " Message-ID: URL: https://gitlist.dev/e/xmqqd2egae9f.fsf%40gitster.dls.corp.google.com In-Reply-To: <1402418594-1377-1-git-send-email-gitter.spiros@gmail.com> ``` Elia Pinto writes: > @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?")" > continue > fi > > - if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git > + if ! test -d "$sm_path"/.git && test -f "$sm_path"/.git Hmmmm. Is the above correct? $ if ! false && true; then echo true; else echo false; fi true In other words, "! cmd1 && cmd2" parses not as "! (cmd1 && cmd2)" but as "(! cmd1) && cmd2". It almost makes me wonder if the code may become simpler if we did it the way in the attached. That is, "if $sm_path/.git is there (whether as an embedded repository, or a file pointing to a repository elsewhere), then grab its HEAD, otherwise $sm_path is a submodule that hasn't been run 'submodule init' on, so run the whole nine yards starting from module_clone". Such a rewrite is not within the scope of this series, so if ! test -d "$sm_path/.git" && ! test -f "$sm_path/.git" may be the closest to the original intent, I would think. git-submodule.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index e146b83..018f1bb 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -832,15 +832,15 @@ Maybe you want to use 'update --init'?")" continue fi - if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git + if test -e "$sm_path/.git" then - module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit - cloned_modules="$cloned_modules;$name" - subsha1= - else subsha1=$(clear_local_git_env; cd "$sm_path" && git rev-parse --verify HEAD) || die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")" + else + module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit + cloned_modules="$cloned_modules;$name" + subsha1= fi if test -n "$remote" ``` ## Eric Sunshine, 2014-06-10 19:47 Subject: Re: [PATCH] git-submodule.sh: avoid "test -a/-o " Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTTOCe0-U_bMbzo6fp3PFsN%2B8eJHi9iTxoE9d3UHyRMhg%40mail.gmail.com In-Reply-To: <1402418594-1377-1-git-send-email-gitter.spiros@gmail.com> ``` On Tue, Jun 10, 2014 at 12:43 PM, Elia Pinto wrote: > The construct is error-prone; "test" being built-in in most modern > shells, the reason to avoid "test && test " spawning > one extra process by using a single "test -a " no > longer exists. > > Signed-off-by: Elia Pinto > --- > This is the fifth revision. > > Change based on Junio bugfix and better rewrite of the case condition > http://permalink.gmane.org/gmane.comp.version-control.git/251198 > > I dropped also the echo -> printf replacement for doing > it in another patch. > > Pass all the t/*submodule* tests. Finally ! :=) > > Thank you all very much and sorry for the mess. > > git-submodule.sh | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/git-submodule.sh b/git-submodule.sh > index e146b83..e128a4a 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > while read mod_src mod_dst sha1_src sha1_dst status sm_path > do > # Always show modules deleted or type-changed (blob<->module) > - test $status = D -o $status = T && echo "$sm_path" && continue > + if test "$status" = D || test "$status" = T > + then > + echo "$sm_path" && Unnecessary &&. > + continue > + fi > # Respect the ignore setting for --for-status. > if test -n "$for_status" > then > name=$(module_name "$sm_path") > ignore_config=$(get_submodule_config "$name" ignore none) > - test $status != A -a $ignore_config = all && continue > + test $status != A && test $ignore_config = all && continue > fi > # Also show added or modified modules which are checked out > GIT_DIR="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 && ```