[PATCH] - git submodule subcommand parsing modified.

[PATCH] - git submodule subcommand parsing modified.

[imyousuf]

From: Imran M Yousuf <imyousuf@smartitengineering.com>

- manual page of git-submodule and usage mentioned in git-subcommand.sh
were not same, thus synchronized them. In doing so also had to change the
way the subcommands were parsed.

- Previous version did not allow commands such as
	git-submodule add init update
as the command parser incorrectly made subcommand names reserve.
Thus refusing them to be used as parameters to subcommands. As a result it
was impossible to add a submodule whose (symbolic) name is "init" and that
resides at path "update" was refused. For more details the following case
can be considered -

mkdir g; mkdir f; cd g/
touch g.txt; echo "sample text for g.txt" >> ./g.txt; git-init;
git-add g.txt; git-commit -a -m "First commit on g"
cd ../f/; ln -s ../g/ init
git-init; git-submodule add init update;
git-commit -a -m "With module update"
mkdir ../test; cd ../test
git-clone ../f/; cd f
git-submodule init update; git-submodule update update
cd ../..; rm -rf ./f/ ./test/ ./g/

This patch fixes this issue and allows it as well.

- Status currently is implemented to show list only but later
implementation might change and list and status could coexists. Thus
status module is introduced. The module is also used to parse its
arguments

- Subcommands will also parse their own commands; thus enabling command
specific arguments to be passed after the command. For example,
	git-submodule -q add -b master module_a
	git-submodule -q status -c
It is to be noted that -q or --quiet is specified before the subcommand
since it is for the submodule command in general rather than the
subcommand. It is mention worthy that backward compatibility exists and
thus commands like git submodule --cached status will also work as expected

- Subcommands that currently do not take any arguments (init and update)
has a case which is introduced just to ensure that no argument is
deliberately sent as the first argument and also to serve the purpose of
providing a future extension point for its arguments.

- Though ther was short and long version for quiet (-q or --quiet and
branch (-b or --branch) but there was no short version for cached. Thus
it is now introduced (-c or --cached).

- Added 3 specific messages for usage error related to branch and cached

- Simplified subcommand action invocation by simply invoking the action if
all conditions are fulfilled. Excepting for parsing command line arguments
case statements are avoided and instead more direct if statement is
introduced.
---
 git-submodule.sh |  158 +++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 114 insertions(+), 44 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ad9fe62..22e7e5f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -4,18 +4,23 @@
 #
 # Copyright (c) 2007 Lars Hjemli
 
-USAGE='[--quiet] [--cached] [add <repo> [-b branch]|status|init|update] [--] [<path>...]'
+# Synopsis of this commands are as follows
+# git-submodule [-q|--quiet] add [-b|--branch branch] <repository> [<path>]
+# git-submodule [-q|--quiet] [status] [-c|--cached] [--] [<path>...]
+# git-submodule [-q|--quiet] init [--] [<path>...]
+# git-submodule [-q|--quiet] update [--] [<path>...]
+USAGE='[-q|--quiet] [[[add [-b|--branch branch] <repo>]|[[[status [-c|--cached]]|init|update] [--]]]  [<path>...]]'
 OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
 
+MODULES_LIST='modules_list'
+
 add=
 branch=
-init=
-update=
-status=
 quiet=
 cached=
+command=
 
 #
 # print stuff on stdout unless -q was specified
@@ -114,6 +119,17 @@ module_clone()
 	die "Clone of '$url' into submodule path '$path' failed"
 }
 
+# Parses the branch name and exits if not present
+parse_branch_name()
+{
+	branch="$1"; 
+	if test -z "$branch"
+	then
+		echo Branch name must me specified	
+		usage
+	fi
+}
+
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #
@@ -123,6 +139,16 @@ module_clone()
 #
 module_add()
 {
+	case "$1" in
+		-b|--branch)
+			shift
+			parse_branch_name "$@" &&
+			shift
+			;;
+		-*)
+			usage
+			;;
+	esac
 	repo=$1
 	path=$2
 
@@ -176,6 +202,14 @@ module_add()
 #
 modules_init()
 {
+	# Added here to ensure that no argument is passed to be treated as
+	# parameter to the sub command. This will be used to parse any 
+	# to the subcommand
+	case "$1" in
+		-*)
+			usage
+			;;
+	esac
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
 	do
@@ -209,6 +243,14 @@ modules_init()
 #
 modules_update()
 {
+	# Added here to ensure that no argument is passed to be treated as
+	# parameter to the sub command. This will be used to parse any 
+	# to the subcommand
+	case "$1" in
+		-*)
+			usage
+			;;
+	esac
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
 	do
@@ -293,36 +335,69 @@ modules_list()
 	done
 }
 
+# Delgates to modules_list after parsing its arguments
+modules_status()
+{
+	case "$1" in
+		-c|--cached)
+			shift
+			cached=1
+			;;
+		-*)
+			usage
+			;;
+	esac
+	"$MODULES_LIST" "$@"
+}
+
+# If there is '--' as the first argument simply ignores it and thus shifts
+check_for_terminator()
+{
+	if test -n "$1" && test "$1" = "--"
+	then
+		shift
+	fi
+}
+
+
+
+# Command synopsis clearly shows that all arguments after
+# subcommand are arguments to the command itself. Thus
+# there lies no command that has configuration argument
+# after the mention of the subcommand. Thus once the
+# subcommand is found and the separator ('--') is ignored
+# rest can be safely sent the subcommand action
+# It is to be noted that pre-subcommand arguments are parsed
+# just to have backward compatibility.
 while test $# != 0
 do
 	case "$1" in
 	add)
 		add=1
+		command="module_$1"
+		shift
+		break
 		;;
-	init)
-		init=1
-		;;
-	update)
-		update=1
-		;;
-	status)
-		status=1
+	init|update|status)
+		command="modules_$1"
+		shift
+		check_for_terminator "$1"
+		break
 		;;
 	-q|--quiet)
 		quiet=1
 		;;
 	-b|--branch)
-		case "$2" in
-		'')
-			usage
-			;;
-		esac
-		branch="$2"; shift
+		shift
+		parse_branch_name "$@"
 		;;
-	--cached)
+	-c|--cached)
 		cached=1
 		;;
 	--)
+		# It is shifted so that it is not passed
+		# as an argument to the default subcommand
+		shift
 		break
 		;;
 	-*)
@@ -335,30 +410,25 @@ do
 	shift
 done
 
-case "$add,$branch" in
-1,*)
-	;;
-,)
-	;;
-,*)
+# Throws usage error if branch is not used with add command
+if test -n "$branch" &&
+   test -z "$add"
+then
+	echo Branch can not be specified without add subcommand
 	usage
-	;;
-esac
-
-case "$add,$init,$update,$status,$cached" in
-1,,,,)
-	module_add "$@"
-	;;
-,1,,,)
-	modules_init "$@"
-	;;
-,,1,,)
-	modules_update "$@"
-	;;
-,,,*,*)
-	modules_list "$@"
-	;;
-*)
+fi
+
+# If no command is specified then default command
+# is - git submodule status
+test -z "$command" && command="modules_status"
+
+# Throws usage if --cached is used by other than status, init or update
+# that is used with add command
+if test -n "$cached" &&
+   test "$command" != "modules_status"
+then
+	echo Cached can only be used with the status subcommand
 	usage
-	;;
-esac
+fi
+
+"$command" "$@"
-- 
1.5.3.7

Re: [PATCH] - git submodule subcommand parsing modified.

[Junio C Hamano]

imyousuf@gmail.com writes:

> From: Imran M Yousuf <imyousuf@smartitengineering.com>
>
> - manual page of git-submodule and usage mentioned in git-subcommand.sh
> were not same, thus synchronized them. In doing so also had to change the
> way the subcommands were parsed.
>
> - Previous version did not allow commands such as
> 	git-submodule add init update
> as the command parser incorrectly made subcommand names reserve.
> Thus refusing them to be used as parameters to subcommands. As a result it
> was impossible to add a submodule whose (symbolic) name is "init" and that
> resides at path "update" was refused. For more details the following case
> can be considered -
>
> mkdir g; mkdir f; cd g/
> touch g.txt; echo "sample text for g.txt" >> ./g.txt; git-init;
> git-add g.txt; git-commit -a -m "First commit on g"
> cd ../f/; ln -s ../g/ init
> git-init; git-submodule add init update;
> git-commit -a -m "With module update"
> mkdir ../test; cd ../test
> git-clone ../f/; cd f
> git-submodule init update; git-submodule update update
> cd ../..; rm -rf ./f/ ./test/ ./g/

 - I'd drop everything after "For more details...".  If you feel
   that the part before "For more details" cannot be understood
   without that thick and solid sample script, perhaps that
   description needs to be rewritten to make it easier to
   understand (personally I do not see it as hard to understand,
   modulo grammatical errors).

> This patch fixes this issue and allows it as well.

 - "it" in this sentence can easily be mistaken as referring to
   "this issue".  I'd suggest dropping "and allows...".

> - Status currently is implemented to show list only but later
> implementation might change and list and status could coexists. Thus
> status module is introduced. The module is also used to parse its
> arguments

 - That is probably better in a separate patch, if the purpose
   of the patch is about straightening out the command parser to
   fix existing issues.  Generally it is a good idea to have
   fixes and enhancement as separate patches.

> - Subcommands will also parse their own commands; thus enabling command
> specific arguments to be passed after the command. For example,
> 	git-submodule -q add -b master module_a
> 	git-submodule -q status -c
> It is to be noted that -q or --quiet is specified before the subcommand
> since it is for the submodule command in general rather than the
> subcommand. It is mention worthy that backward compatibility exists and
> thus commands like git submodule --cached status will also work as expected

 - I have a mild distaste against commands that expect the users
   to intimately know what option is command wide and what
   option is subcommand specific.  IOW it is not very nice to
   accept "git submodule -q status" and reject "git submodule
   status -q".

> - Subcommands that currently do not take any arguments (init and update)
> has a case which is introduced just to ensure that no argument is
> deliberately sent as the first argument and also to serve the purpose of
> providing a future extension point for its arguments.

 - As far as I understand what they do, they both do take
   paths arguments to name specific modules, so the above
   sentence is bogus.  Maybe you meant rejecting non-existent
   options, and I'd agree with the intent if that is the case
   (but your implementation around -- is bogus).

> - Though ther was short and long version for quiet (-q or --quiet and
> branch (-b or --branch) but there was no short version for cached. Thus
> it is now introduced (-c or --cached).
>
> - Added 3 specific messages for usage error related to branch and cached
>
> - Simplified subcommand action invocation by simply invoking the action if
> all conditions are fulfilled. Excepting for parsing command line arguments
> case statements are avoided and instead more direct if statement is
> introduced.
> ---

 - Lacks sign-off.

 - The message felt very hard to read.  Perhaps it is just that
   these unindented sentences in bullet-list, together with the
   two displayed command line that are not separated from the
   rest of the text with a blank line, hurts the eyes.

 - Perhaps the patch tries to do too many things.  For example,
   introduction of -c does not have to belong here.  nor
   "status" which currently is the same as "list".  Maybe that
   is why the description needs to talk about too many things,
   which in turn could be the reason why I find the above
   message very hard to read.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index ad9fe62..22e7e5f 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -4,18 +4,23 @@
>  #
>  # Copyright (c) 2007 Lars Hjemli
>  
> -USAGE='[--quiet] [--cached] [add <repo> [-b branch]|status|init|update] [--] [<path>...]'
> +# Synopsis of this commands are as follows
> +# git-submodule [-q|--quiet] add [-b|--branch branch] <repository> [<path>]
> +# git-submodule [-q|--quiet] [status] [-c|--cached] [--] [<path>...]
> +# git-submodule [-q|--quiet] init [--] [<path>...]
> +# git-submodule [-q|--quiet] update [--] [<path>...]
> +USAGE='[-q|--quiet] [[[add [-b|--branch branch] <repo>]|[[[status [-c|--cached]]|init|update] [--]]]  [<path>...]]'
>  OPTIONS_SPEC=
>  . git-sh-setup
>  require_work_tree
>  
> +MODULES_LIST='modules_list'
> +

 - What's the purpose of this variable?  If you are planning to
   change the default command by changing the value of this
   variable, then the variable is not about MODULES_LIST, but
   about DEFAULT_COMMAND or something, and should be named as
   such.

>  add=
>  branch=
> -init=
> -update=
> -status=
>  quiet=
>  cached=
> +command=
>  
>  #
>  # print stuff on stdout unless -q was specified
> @@ -114,6 +119,17 @@ module_clone()
>  	die "Clone of '$url' into submodule path '$path' failed"
>  }
>  
> +# Parses the branch name and exits if not present
> +parse_branch_name()
> +{
> +	branch="$1"; 
> +	if test -z "$branch"
> +	then
> +		echo Branch name must me specified	
> +		usage
> +	fi

 - Shouldn't the message go to stderr?

 - If we use "usage", that already tells the user -b is followed
   by branch.  Is the extra message needed in the first place?
   If this patch is about "fixing", perhaps you should not be
   mixing this kind of unnecessary changes in it.

> +}
> +
>  #
>  # Add a new submodule to the working tree, .gitmodules and the index
>  #
> @@ -123,6 +139,16 @@ module_clone()
>  #
>  module_add()
>  {
> +	case "$1" in
> +		-b|--branch)
> +			shift
> +			parse_branch_name "$@" &&
> +			shift
> +			;;
> +		-*)
> +			usage
> +			;;
> +	esac

 - Style.  Please align case arms with "case/esac", like other
   scripts (and the original version of this script) do.  I.e.

	case "$1" in
        -b|--branch)
                ...
	esac

   This comment applies to other parts of the patch as well.

>  	repo=$1
>  	path=$2
>  
> @@ -176,6 +202,14 @@ module_add()
>  #
>  modules_init()
>  {
> +	# Added here to ensure that no argument is passed to be treated as
> +	# parameter to the sub command. This will be used to parse any 
> +	# to the subcommand
> +	case "$1" in
> +		-*)
> +			usage
> +			;;
> +	esac

 - If I understand correctly, "git submodule init" takes paths
   to submodules as arguments.  Are you disallowing paths that
   begin with '-', even though the body of the command (ls-files
   piped to while loop) is written in such a way that is
   perfectly capable of handling such a path?

>  	git ls-files --stage -- "$@" | grep -e '^160000 ' |
>  	while read mode sha1 stage path
>  	do
> @@ -209,6 +243,14 @@ modules_init()
>  #
>  modules_update()
>  {
> +	# Added here to ensure that no argument is passed to be treated as
> +	# parameter to the sub command. This will be used to parse any 
> +	# to the subcommand
> +	case "$1" in
> +		-*)
> +			usage
> +			;;
> +	esac

 - The same comment as modules_init() above applies here.

>  	git ls-files --stage -- "$@" | grep -e '^160000 ' |
>  	while read mode sha1 stage path
>  	do
> @@ -293,36 +335,69 @@ modules_list()
>  	done
>  }
>  
> +# Delgates to modules_list after parsing its arguments

 - That's "delegates", but typically when we write a sentence
   without the subject like this in the comment, we use the
   imperative mood, so "Delegate to modules_list after ..."
   would be more appropriate.

> +modules_status()
> +{
> +	case "$1" in
> +		-c|--cached)
> +			shift
> +			cached=1
> +			;;
> +		-*)
> +			usage
> +			;;
> +	esac
> +	"$MODULES_LIST" "$@"
> +}

 - The same comment as modules_init() above applies here.

> +# If there is '--' as the first argument simply ignores it and thus shifts
> +check_for_terminator()
> +{
> +	if test -n "$1" && test "$1" = "--"
> +	then
> +		shift
> +	fi
> +}

 - That 'test -n "$1" && test "$1" = "--"' feels stupid; if "$1"
   is equal to "--", it certainly will be -n (i.e. not empty).
   Perhaps 'test $# -ge 1 && test "$1" = "--"' or even just
   'test "$1" = "--"'.

 - I do not think the 'shift' does anything useful.  It does not
   shift the positional parameters of the caller of this shell
   function, and would be a noop from the caller's point of
   view.  It only shifts the positional parameters inside this
   function (study e.g. "2.9.5 Function Definition Command",
   http://www.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html).

> +# Command synopsis clearly shows that all arguments after
> +# subcommand are arguments to the command itself. Thus
> +# there lies no command that has configuration argument
> +# after the mention of the subcommand. Thus once the
> +# subcommand is found and the separator ('--') is ignored
> +# rest can be safely sent the subcommand action

 - That's a valid justification but I think it is enough to say
   what it does (i.e. "Arguments after the subcommand name are
   given to the subcommand").  How you arrived to that design
   decision (i.e. your justification based on the synopsis) does
   not belong here.  It could however be part of the commit log
   message.

 - I do not agree with the (attempted) stripping of -- you talk
   about here (that is done in the loop below).

> +# It is to be noted that pre-subcommand arguments are parsed
> +# just to have backward compatibility.

 - Because we might want to deprecate and remove forms like "-b
   branch add" later, this is a good comment to have here.  It
   makes it clear that these oddballs are purely for backward
   compatibility.

>  while test $# != 0
>  do
>  	case "$1" in
>  	add)
>  		add=1
> +		command="module_$1"
> +		shift
> +		break
>  		;;
> -	init)
> -		init=1
> -		;;
> -	update)
> -		update=1
> -		;;
> -	status)
> -		status=1
> +	init|update|status)
> +		command="modules_$1"
> +		shift
> +		check_for_terminator "$1"
> +		break
>  		;;

 - Aside from my earlier comment that the code would become
   simpler if you consistently renamed the shell functions to
   module_$foo (or cmd_$foo) so that the dispatcher can follow a
   simple rule "subcommand $foo is handled by shell function
   cmd_$foo", which you seem to have ignored, and also aside
   from that your check_for_terminator does not do what you seem
   to have intended (see above), I think this handling of -- is
   wrong.  By stripping -- here, you are making the following
   two behave exactly the same:

   $ git submodule update -- $other_args
   $ git submodule update    $other_args

   The whole point of -- is so that you can tell the command
   that the argument at the beginning of $other_args that
   happens to begin with a dash is _not_ an option but is a
   literal path (or whatever).  Think of the case in which you
   had '-foo' and 'bar' in place of $other_args above.  The
   first one tells the command to update two modules ('-foo' and
   'bar'), the second one tells the command to update 'bar'
   module but the update operation needs to be done with -foo
   option (whatever that option means to 'update' command).

   By checking and shifting -- out, you are making it impossible
   for the implementation of the command (i.e. your
   "modules_update") to tell which case it is dealing with.

>  	-q|--quiet)
>  		quiet=1
>  		;;
>  	-b|--branch)
> -		case "$2" in
> -		'')
> -			usage
> -			;;
> -		esac
> -		branch="$2"; shift
> +		shift
> +		parse_branch_name "$@"
>  		;;
> -	--cached)
> +	-c|--cached)
>  		cached=1
>  		;;
>  	--)
> +		# It is shifted so that it is not passed
> +		# as an argument to the default subcommand
> +		shift
>  		break
>  		;;
>  	-*)
> @@ -335,30 +410,25 @@ do
>  	shift
>  done
>  
> -case "$add,$branch" in
> -1,*)
> -	;;
> -,)
> -	;;
> -,*)
> +# Throws usage error if branch is not used with add command
> +if test -n "$branch" &&
> +   test -z "$add"
> +then
> +	echo Branch can not be specified without add subcommand
>  	usage
> -	;;
> -esac
> -
> -case "$add,$init,$update,$status,$cached" in
> -1,,,,)
> -	module_add "$@"
> -	;;
> -,1,,,)
> -	modules_init "$@"
> -	;;
> -,,1,,)
> -	modules_update "$@"
> -	;;
> -,,,*,*)
> -	modules_list "$@"
> -	;;
> -*)
> +fi
> +
> +# If no command is specified then default command
> +# is - git submodule status
> +test -z "$command" && command="modules_status"
> +
> +# Throws usage if --cached is used by other than status, init or update
> +# that is used with add command
> +if test -n "$cached" &&
> +   test "$command" != "modules_status"
> +then
> +	echo Cached can only be used with the status subcommand
>  	usage
> -	;;
> -esac
> +fi
> +
> +"$command" "$@"
> -- 
> 1.5.3.7

[PATCH 1/3] git-submodule: rename shell functions for consistency

[Junio C Hamano]

This renames the shell functions used in git-submodule that
implement top-level subcommands.  The rule is that the
subcommand $foo is implemented by cmd_$foo function.

A noteworthy change is that modules_list() is now known as
cmd_status().  There is no "submodule list" command.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * We could probably do something like this.  This first part is
   about making the command dispatcher maintainable.

   Note that I haven't seriously tested this series.  This and
   the next one are primarily to illustrate what I think the fix
   you are trying should look like.

 git-submodule.sh |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ad9fe62..3c104e3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -86,9 +86,9 @@ module_name()
 #
 # Clone a submodule
 #
-# Prior to calling, modules_update checks that a possibly existing
+# Prior to calling, cmd_update checks that a possibly existing
 # path is not a git repository.
-# Likewise, module_add checks that path does not exist at all,
+# Likewise, cmd_add checks that path does not exist at all,
 # since it is the location of a new submodule.
 #
 module_clone()
@@ -121,7 +121,7 @@ module_clone()
 #
 # optional branch is stored in global branch variable
 #
-module_add()
+cmd_add()
 {
 	repo=$1
 	path=$2
@@ -174,7 +174,7 @@ module_add()
 #
 # $@ = requested paths (default to all)
 #
-modules_init()
+cmd_init()
 {
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
@@ -207,7 +207,7 @@ modules_init()
 #
 # $@ = requested paths (default to all)
 #
-modules_update()
+cmd_update()
 {
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
@@ -266,7 +266,7 @@ set_name_rev () {
 #
 # $@ = requested paths (default to all)
 #
-modules_list()
+cmd_status()
 {
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
@@ -347,16 +347,16 @@ esac
 
 case "$add,$init,$update,$status,$cached" in
 1,,,,)
-	module_add "$@"
+	cmd_add "$@"
 	;;
 ,1,,,)
-	modules_init "$@"
+	cmd_init "$@"
 	;;
 ,,1,,)
-	modules_update "$@"
+	cmd_update "$@"
 	;;
 ,,,*,*)
-	modules_list "$@"
+	cmd_status "$@"
 	;;
 *)
 	usage
-- 
1.5.4.rc3.11.g4e67

Re: [PATCH 1/3] git-submodule: rename shell functions for consistency

[Imran M Yousuf]

Thanks Junio for showing how it should be done. Due to some
pre-scheduled appointment I was unavailable yesterday evening and thus
was neither able to reply nor resubmit the changes.

On Jan 15, 2008 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This renames the shell functions used in git-submodule that
> implement top-level subcommands.  The rule is that the
> subcommand $foo is implemented by cmd_$foo function.
>
> A noteworthy change is that modules_list() is now known as
> cmd_status().  There is no "submodule list" command.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * We could probably do something like this.  This first part is
>    about making the command dispatcher maintainable.
>
>    Note that I haven't seriously tested this series.  This and
>    the next one are primarily to illustrate what I think the fix
>    you are trying should look like.
>
>  git-submodule.sh |   20 ++++++++++----------
>  1 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index ad9fe62..3c104e3 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -86,9 +86,9 @@ module_name()
>  #
>  # Clone a submodule
>  #
> -# Prior to calling, modules_update checks that a possibly existing
> +# Prior to calling, cmd_update checks that a possibly existing
>  # path is not a git repository.
> -# Likewise, module_add checks that path does not exist at all,
> +# Likewise, cmd_add checks that path does not exist at all,
>  # since it is the location of a new submodule.
>  #
>  module_clone()
> @@ -121,7 +121,7 @@ module_clone()
>  #
>  # optional branch is stored in global branch variable
>  #
> -module_add()
> +cmd_add()

After reading your reply I was about to suggest renaming module to cmd
but you have done it before I could propose or submit the patch.

>  {
>         repo=$1
>         path=$2
> @@ -174,7 +174,7 @@ module_add()
>  #
>  # $@ = requested paths (default to all)
>  #
> -modules_init()
> +cmd_init()
>  {
>         git ls-files --stage -- "$@" | grep -e '^160000 ' |
>         while read mode sha1 stage path
> @@ -207,7 +207,7 @@ modules_init()
>  #
>  # $@ = requested paths (default to all)
>  #
> -modules_update()
> +cmd_update()
>  {
>         git ls-files --stage -- "$@" | grep -e '^160000 ' |
>         while read mode sha1 stage path
> @@ -266,7 +266,7 @@ set_name_rev () {
>  #
>  # $@ = requested paths (default to all)
>  #
> -modules_list()
> +cmd_status()
>  {
>         git ls-files --stage -- "$@" | grep -e '^160000 ' |
>         while read mode sha1 stage path
> @@ -347,16 +347,16 @@ esac
>
>  case "$add,$init,$update,$status,$cached" in
>  1,,,,)
> -       module_add "$@"
> +       cmd_add "$@"
>         ;;
>  ,1,,,)
> -       modules_init "$@"
> +       cmd_init "$@"
>         ;;
>  ,,1,,)
> -       modules_update "$@"
> +       cmd_update "$@"
>         ;;
>  ,,,*,*)
> -       modules_list "$@"
> +       cmd_status "$@"
>         ;;
>  *)
>         usage
> --
> 1.5.4.rc3.11.g4e67
>
>



-- 
Imran M Yousuf

Re: [PATCH 1/3] git-submodule: rename shell functions for consistency

[Junio C Hamano]

"Imran M Yousuf" <imyousuf@gmail.com> writes:

> Thanks Junio for showing how it should be done. Due to some
> pre-scheduled appointment I was unavailable yesterday evening and thus
> was neither able to reply nor resubmit the changes.

Well, I did not show how it _should_ be done.  That series was
merely an illustration of how I _think_ it should look like.  I
did not test it, I do not know if it introduced new bugs, and
most importantly I do not know if it fulfills what you intended
to achieve with your patch.

In other words, I just tried to turn the table around.  Instead
of me and others commenting on your patch saying "I do not like
this" piecemeal, now you have something you can comment on.  You
can say the whole range of things from "I tested this and it is
what I want", "I like the general concept but I found this and
that bug and here is a fix", to "This is much worse than what I
proposed and here is why."

[PATCH 2/3] git-submodule: fix subcommand parser

[Junio C Hamano]

The subcommand parser of "git submodule" made its subcommand
names reserved words.  As a consequence, a command like this:

    $ git submodule add init update

which is meant to add a submodule called 'init' at path 'update'
was misinterpreted as a request to invoke more than one mutually
incompatible subcommands and incorrectly rejected.

This patch fixes the issue by stopping the subcommand parsing at
the first subcommand word, to allow the sample command line
above to work as expected.

It also introduces the usual -- option disambiguator, so that a
submodule at path '-foo' can be updated with

    $ git submodule update -- -foo

without triggering an "unrecognized option -foo" error.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And this is the second part to fix the real issue.

 git-submodule.sh |  157 ++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 116 insertions(+), 41 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 3c104e3..a6aaf40 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,11 +9,8 @@ OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
 
-add=
+command=
 branch=
-init=
-update=
-status=
 quiet=
 cached=
 
@@ -123,6 +120,32 @@ module_clone()
 #
 cmd_add()
 {
+	# parse $args after "submodule ... add".
+	while test $# -ne 0
+	do
+		case "$1" in
+		-b | --branch)
+			case "$2" in '') usage ;; esac
+			branch=$2
+			shift
+			;;
+		-q|--quiet)
+			quiet=1
+			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			usage
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
 	repo=$1
 	path=$2
 
@@ -176,6 +199,27 @@ cmd_add()
 #
 cmd_init()
 {
+	# parse $args after "submodule ... init".
+	while test $# -ne 0
+	do
+		case "$1" in
+		-q|--quiet)
+			quiet=1
+			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			usage
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
 	do
@@ -209,6 +253,27 @@ cmd_init()
 #
 cmd_update()
 {
+	# parse $args after "submodule ... update".
+	while test $# -ne 0
+	do
+		case "$1" in
+		-q|--quiet)
+			quiet=1
+			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			usage
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
 	do
@@ -268,6 +333,30 @@ set_name_rev () {
 #
 cmd_status()
 {
+	# parse $args after "submodule ... status".
+	while test $# -ne 0
+	do
+		case "$1" in
+		-q|--quiet)
+			quiet=1
+			;;
+		--cached)
+			cached=1
+			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			usage
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
 	do
@@ -293,20 +382,17 @@ cmd_status()
 	done
 }
 
-while test $# != 0
+# This loop parses the command line arguments to find the
+# subcommand name to dispatch.  Parsing of the subcommand specific
+# options are primarily done by the subcommand implementations.
+# Subcommand specific options such as --branch and --cached are
+# parsed here as well, for backward compatibility.
+
+while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add)
-		add=1
-		;;
-	init)
-		init=1
-		;;
-	update)
-		update=1
-		;;
-	status)
-		status=1
+	add | init | update | status)
+		command=$1
 		;;
 	-q|--quiet)
 		quiet=1
@@ -335,30 +421,19 @@ do
 	shift
 done
 
-case "$add,$branch" in
-1,*)
-	;;
-,)
-	;;
-,*)
+# No command word defaults to "status"
+test -n "$command" || command=status
+
+# "-b branch" is accepted only by "add"
+if test -n "$branch" && test "$command" != add
+then
 	usage
-	;;
-esac
-
-case "$add,$init,$update,$status,$cached" in
-1,,,,)
-	cmd_add "$@"
-	;;
-,1,,,)
-	cmd_init "$@"
-	;;
-,,1,,)
-	cmd_update "$@"
-	;;
-,,,*,*)
-	cmd_status "$@"
-	;;
-*)
+fi
+
+# "--cached" is accepted only by "status"
+if test -n "$cached" && test "$command" != status
+then
 	usage
-	;;
-esac
+fi
+
+"cmd_$command" "$@"
-- 
1.5.4.rc3.11.g4e67

[PATCH 3/3] git-submodule: add test for the subcommand parser fix

[Junio C Hamano]

This modifies the existing t7400 test to use 'init' as the
pathname that a submodule is bound to.  Without the earlier
subcommand parser fix, this fails.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7400-submodule-basic.sh |   56 ++++++++++++++++++++++----------------------
 1 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 4fe3a41..2ef85a8 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -13,11 +13,11 @@ subcommands of git-submodule.
 
 #
 # Test setup:
-#  -create a repository in directory lib
+#  -create a repository in directory init
 #  -add a couple of files
-#  -add directory lib to 'superproject', this creates a DIRLINK entry
+#  -add directory init to 'superproject', this creates a DIRLINK entry
 #  -add a couple of regular files to enable testing of submodule filtering
-#  -mv lib subrepo
+#  -mv init subrepo
 #  -add an entry to .gitmodules for submodule 'example'
 #
 test_expect_success 'Prepare submodule testing' '
@@ -25,8 +25,8 @@ test_expect_success 'Prepare submodule testing' '
 	git-add t &&
 	git-commit -m "initial commit" &&
 	git branch initial HEAD &&
-	mkdir lib &&
-	cd lib &&
+	mkdir init &&
+	cd init &&
 	git init &&
 	echo a >a &&
 	git add a &&
@@ -41,10 +41,10 @@ test_expect_success 'Prepare submodule testing' '
 	cd .. &&
 	echo a >a &&
 	echo z >z &&
-	git add a lib z &&
+	git add a init z &&
 	git-commit -m "super commit 1" &&
-	mv lib .subrepo &&
-	GIT_CONFIG=.gitmodules git config submodule.example.url git://example.com/lib.git
+	mv init .subrepo &&
+	GIT_CONFIG=.gitmodules git config submodule.example.url git://example.com/init.git
 '
 
 test_expect_success 'status should fail for unmapped paths' '
@@ -52,7 +52,7 @@ test_expect_success 'status should fail for unmapped paths' '
 	then
 		echo "[OOPS] submodule status succeeded"
 		false
-	elif ! GIT_CONFIG=.gitmodules git config submodule.example.path lib
+	elif ! GIT_CONFIG=.gitmodules git config submodule.example.path init
 	then
 		echo "[OOPS] git config failed to update .gitmodules"
 		false
@@ -71,7 +71,7 @@ test_expect_success 'status should initially be "missing"' '
 test_expect_success 'init should register submodule url in .git/config' '
 	git-submodule init &&
 	url=$(git config submodule.example.url) &&
-	if test "$url" != "git://example.com/lib.git"
+	if test "$url" != "git://example.com/init.git"
 	then
 		echo "[OOPS] init succeeded but submodule url is wrong"
 		false
@@ -83,41 +83,41 @@ test_expect_success 'init should register submodule url in .git/config' '
 '
 
 test_expect_success 'update should fail when path is used by a file' '
-	echo "hello" >lib &&
+	echo "hello" >init &&
 	if git-submodule update
 	then
 		echo "[OOPS] update should have failed"
 		false
-	elif test "$(cat lib)" != "hello"
+	elif test "$(cat init)" != "hello"
 	then
-		echo "[OOPS] update failed but lib file was molested"
+		echo "[OOPS] update failed but init file was molested"
 		false
 	else
-		rm lib
+		rm init
 	fi
 '
 
 test_expect_success 'update should fail when path is used by a nonempty directory' '
-	mkdir lib &&
-	echo "hello" >lib/a &&
+	mkdir init &&
+	echo "hello" >init/a &&
 	if git-submodule update
 	then
 		echo "[OOPS] update should have failed"
 		false
-	elif test "$(cat lib/a)" != "hello"
+	elif test "$(cat init/a)" != "hello"
 	then
-		echo "[OOPS] update failed but lib/a was molested"
+		echo "[OOPS] update failed but init/a was molested"
 		false
 	else
-		rm lib/a
+		rm init/a
 	fi
 '
 
 test_expect_success 'update should work when path is an empty dir' '
-	rm -rf lib &&
-	mkdir lib &&
+	rm -rf init &&
+	mkdir init &&
 	git-submodule update &&
-	head=$(cd lib && git rev-parse HEAD) &&
+	head=$(cd init && git rev-parse HEAD) &&
 	if test -z "$head"
 	then
 		echo "[OOPS] Failed to obtain submodule head"
@@ -134,7 +134,7 @@ test_expect_success 'status should be "up-to-date" after update' '
 '
 
 test_expect_success 'status should be "modified" after submodule commit' '
-	cd lib &&
+	cd init &&
 	echo b >b &&
 	git add b &&
 	git-commit -m "submodule commit 2" &&
@@ -157,8 +157,8 @@ test_expect_success 'git diff should report the SHA1 of the new submodule commit
 '
 
 test_expect_success 'update should checkout rev1' '
-	git-submodule update &&
-	head=$(cd lib && git rev-parse HEAD) &&
+	git-submodule update init &&
+	head=$(cd init && git rev-parse HEAD) &&
 	if test -z "$head"
 	then
 		echo "[OOPS] submodule git rev-parse returned nothing"
@@ -182,13 +182,13 @@ test_expect_success 'checkout superproject with subproject already present' '
 test_expect_success 'apply submodule diff' '
 	git branch second &&
 	(
-		cd lib &&
+		cd init &&
 		echo s >s &&
 		git add s &&
 		git commit -m "change subproject"
 	) &&
-	git update-index --add lib &&
-	git-commit -m "change lib" &&
+	git update-index --add init &&
+	git-commit -m "change init" &&
 	git-format-patch -1 --stdout >P.diff &&
 	git checkout second &&
 	git apply --index P.diff &&
-- 
1.5.4.rc3.11.g4e67