[PATCH] - Updated usage and simplified sub-command action invocation

[PATCH] - Updated usage and simplified sub-command action invocation

[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". Thus not satisfying the following case -

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.

- Added 2 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.

Signed-off-by: Imran M Yousuf <imyousuf@smartitengineering.com>
---
 git-submodule.sh |   97 ++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 65 insertions(+), 32 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ad9fe62..5d5e41d 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 [--quiet] [-b branch] add <repository> [<path>]
+# git-submodule [--quiet] [--cached] [status] [--] [<path>...]
+# git-submodule [--quiet] init [--] [<path>...]
+# git-submodule [--quiet] update [--] [<path>...]
+USAGE='[--quiet] [[[[-b branch] add <repo>]|[[[[--cached] status]|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
@@ -293,20 +298,47 @@ modules_list()
 	done
 }
 
+# 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
 while test $# != 0
 do
 	case "$1" in
 	add)
 		add=1
+		command="module_$1"
+		shift
+		break
 		;;
 	init)
-		init=1
+		command="modules_$1"
+		shift
+		check_for_terminator "$1"
+		break
 		;;
 	update)
-		update=1
+		command="modules_$1"
+		shift
+		check_for_terminator "$1"
+		break
 		;;
 	status)
-		status=1
+		command="$MODULES_LIST"
+		shift
+		check_for_terminator "$1";
+		break
 		;;
 	-q|--quiet)
 		quiet=1
@@ -323,6 +355,9 @@ do
 		cached=1
 		;;
 	--)
+		# It is shifted so that it is not passed
+		# as an argument to the default subcommand
+		shift
 		break
 		;;
 	-*)
@@ -335,30 +370,28 @@ 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 than default command
+# is - git submodule status
+if test -z "$command"
+then
+	command="$MODULES_LIST"
+fi
+
+# 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_LIST"
+then
+	echo Cached can only be used with the status subcommand
 	usage
-	;;
-esac
+fi
+
+"$command" "$@"
-- 
1.5.3.7

Re: [PATCH] - Updated usage and simplified sub-command action invocation

[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". Thus not satisfying the following case -
>
> 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 find this too verbose with too little information.

If I am reading you correctly, what you meant was that the way
command parser was structured made subcommand names such as
"init" and "update" reserved words, and it was impossible to use
them as arguments to commands.

You could have said something like this instead:

	The command parser incorrectly made subcommand names to
	git-submodule reserved, refusing them to be used as
	parameters to subcommands.  For example,

        	$ git submodule add init update

	to add a submodule whose (symbolic) name is "init" and
	that resides at path "update" was refused.

That would have been much cleaner and easier on the reader than
having to decipher what the 20+ command shell script sequence
was doing.

I do agree that the breakage is worth fixing, though.

> +# Synopsis of this commands are as follows
> +# git-submodule [--quiet] [-b branch] add <repository> [<path>]
> +# git-submodule [--quiet] [--cached] [status] [--] [<path>...]
> +# git-submodule [--quiet] init [--] [<path>...]
> +# git-submodule [--quiet] update [--] [<path>...]

I somehow feel that syntactically the original implementation
that allowed subcommand specific options to come before the
subcommand name was a mistake.  It may be easier for users that
both "-b branch add" and "add -b branch" are accepted, but I
have to wonder if it would really hurt if we made "-b branch
add" a syntax error.

So how about reorganizing the top-level option parser like this:

        while :
        do
                case $# in 0) break ;; esac
                case "$1" in
                add | status | init | update)
                        # we have found subcommand.
                        command="$1"
                        shift
                        break ;;
                --)
                        # end of parameters
                        shift
                        break ;;
                --quiet)
                        quiet=1
                        ;;
                -*)
                        die "unknown option $1"
                esac
                shift
        done
        test -n "$command" || command=$default_command
        module_$command "$@"

And then make individual command implementations responsible for 
parsing their own options (and perhaps the common ones, to allow
"git submodule add --quiet", but that is optional), like:

        module_add () {
                while :
                do
                        case $# in 0) break ;; esac
                        case "$1" in
                        --cached)
                                cached=1
                                ;;
                        -b | --branch)
                                shift
                                branch="$1"
                                test -n "$branch" ||
                                die "no branch name after -b?"
                                ;;
                        --)
                                shift
                                break
                                ;;
                        --quiet)
                                quiet=1
                                ;;
                        -*)
                                die "unknown option $1"
                        esac
                        shift
                done
                repo=$1
                path=$2
                ...
        }

In the above illustration I did not bother eliminating cut&paste
duplication, but there may be a better way to share the piece to
parse common options across subcommands option parsers and the
toplevel one.

Re: [PATCH] - Updated usage and simplified sub-command action invocation

[Imran M Yousuf]

On Jan 10, 2008 12:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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". Thus not satisfying the following case -
> >
> > 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 find this too verbose with too little information.
>
> If I am reading you correctly, what you meant was that the way
> command parser was structured made subcommand names such as
> "init" and "update" reserved words, and it was impossible to use
> them as arguments to commands.
>
> You could have said something like this instead:
>
>         The command parser incorrectly made subcommand names to
>         git-submodule reserved, refusing them to be used as
>         parameters to subcommands.  For example,
>
>                 $ git submodule add init update
>
>         to add a submodule whose (symbolic) name is "init" and
>         that resides at path "update" was refused.
>

I agree that your comment is better than, I will change it accordingly
when resubmitting it.

> That would have been much cleaner and easier on the reader than
> having to decipher what the 20+ command shell script sequence
> was doing.
>
> I do agree that the breakage is worth fixing, though.
>
> > +# Synopsis of this commands are as follows
> > +# git-submodule [--quiet] [-b branch] add <repository> [<path>]
> > +# git-submodule [--quiet] [--cached] [status] [--] [<path>...]
> > +# git-submodule [--quiet] init [--] [<path>...]
> > +# git-submodule [--quiet] update [--] [<path>...]
>
> I somehow feel that syntactically the original implementation
> that allowed subcommand specific options to come before the
> subcommand name was a mistake.  It may be easier for users that
> both "-b branch add" and "add -b branch" are accepted, but I
> have to wonder if it would really hurt if we made "-b branch
> add" a syntax error.
>

I will recode it to have all options except for --quiet (which is
inverse of -v or --verbose) be mentioned after the subcommand.

> So how about reorganizing the top-level option parser like this:
>
>         while :
>         do
>                 case $# in 0) break ;; esac
>                 case "$1" in
>                 add | status | init | update)
>                         # we have found subcommand.
>                         command="$1"
>                         shift
>                         break ;;
>                 --)
>                         # end of parameters
>                         shift
>                         break ;;
>                 --quiet)
>                         quiet=1
>                         ;;
>                 -*)
>                         die "unknown option $1"
>                 esac
>                 shift
>         done
>         test -n "$command" || command=$default_command
>         module_$command "$@"
>

Actually module_$command is not possible because only add's module is
module_add rest are modules_$command. Thus I would require another if
else and that was the original reason for not using it. Instead I
should have (and will) used -

       case "$1" in
       add)
               add=1
               command="module_$1"
               shift
               break
               ;;
       init|update|status)
               init=1
               command="modules_$1"
               shift
               check_for_terminator "$1"
               break
               ;;

> And then make individual command implementations responsible for
> parsing their own options (and perhaps the common ones, to allow
> "git submodule add --quiet", but that is optional), like:
>
>         module_add () {
>                 while :
>                 do
>                         case $# in 0) break ;; esac
>                         case "$1" in
>                         --cached)
>                                 cached=1
>                                 ;;
>                         -b | --branch)
>                                 shift
>                                 branch="$1"
>                                 test -n "$branch" ||
>                                 die "no branch name after -b?"
>                                 ;;
>                         --)
>                                 shift
>                                 break
>                                 ;;
>                         --quiet)
>                                 quiet=1
>                                 ;;
>                         -*)
>                                 die "unknown option $1"
>                         esac
>                         shift
>                 done
>                 repo=$1
>                 path=$2
>                 ...
>         }
>
> In the above illustration I did not bother eliminating cut&paste
> duplication, but there may be a better way to share the piece to
> parse common options across subcommands option parsers and the
> toplevel one.
>

As add subcommand does not support --cached it should be considered in
-*, just mentioning for your FYI, I got the point of module parsing
their own arguments and I am in agreement.

>

I will make the necessary changes and resubmit the patch tomorrow.

Best regards,

-- 
Imran M Yousuf

Re: [PATCH] - Updated usage and simplified sub-command action invocation

[Junio C Hamano]

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

> On Jan 10, 2008 12:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> I somehow feel that syntactically the original implementation
>> that allowed subcommand specific options to come before the
>> subcommand name was a mistake.  It may be easier for users that
>> both "-b branch add" and "add -b branch" are accepted, but I
>> have to wonder if it would really hurt if we made "-b branch
>> add" a syntax error.
>
> I will recode it to have all options except for --quiet (which is
> inverse of -v or --verbose) be mentioned after the subcommand.

Just a word of caution when dealing with me.

Unlike Linus, I am not always right.  And other people on the
list who are here longer already know this. I am reasonably sure
that some of them will disagree with me on design issues like
this one; I mildly suspect that this forbidding "-b branch add"
might be met with resistance from existing users.

You do not have to agree with me on every little detail I
mention.  If you feel a design issue might be contentious, it
could turn out to be a better use of your time to keep the code
as it is while waiting to see if other people would offer better
alternatives.

> Actually module_$command is not possible because only add's module is
> module_add rest are modules_$command....

Is there a fundamental reason why you cannot rename them to be
more consistent?

Re: [PATCH] - Updated usage and simplified sub-command action invocation

[Imran M Yousuf]

On Jan 10, 2008 1:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Imran M Yousuf" <imyousuf@gmail.com> writes:
>
> > On Jan 10, 2008 12:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > ...
> >> I somehow feel that syntactically the original implementation
> >> that allowed subcommand specific options to come before the
> >> subcommand name was a mistake.  It may be easier for users that
> >> both "-b branch add" and "add -b branch" are accepted, but I
> >> have to wonder if it would really hurt if we made "-b branch
> >> add" a syntax error.
> >
> > I will recode it to have all options except for --quiet (which is
> > inverse of -v or --verbose) be mentioned after the subcommand.
>
> Just a word of caution when dealing with me.
>
> Unlike Linus, I am not always right.  And other people on the

I will cautiously remember the caution :).

> list who are here longer already know this. I am reasonably sure
> that some of them will disagree with me on design issues like
> this one; I mildly suspect that this forbidding "-b branch add"
> might be met with resistance from existing users.
>
> You do not have to agree with me on every little detail I
> mention.  If you feel a design issue might be contentious, it
> could turn out to be a better use of your time to keep the code
> as it is while waiting to see if other people would offer better
> alternatives.

Actually the best design, IMHO, is to have separate commands itself
for them, that is submodule-add, submodule-init, submodule-update,
submodule-status or submodule. I think this would also make it
coherent with other commands such as git-ls, git-merge, git-show. In
that way we could have a common .sh file that will contain the common
functions and can be accessed from the command shell scripts. This
would also make it quite simple.

>
> > Actually module_$command is not possible because only add's module is
> > module_add rest are modules_$command....
>
> Is there a fundamental reason why you cannot rename them to be
> more consistent?

In fact it is consistent, add works on a single module only, whereas
rest of the command works either on 1 or more. Thus having plural
(modules) is logical.

>

Best regards,

-- 
Imran M Yousuf

Re: [PATCH] - Updated usage and simplified sub-command action invocation

[Imran M Yousuf]

On Jan 10, 2008 12:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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". Thus not satisfying the following case -
> >
> > 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 find this too verbose with too little information.
>
> If I am reading you correctly, what you meant was that the way
> command parser was structured made subcommand names such as
> "init" and "update" reserved words, and it was impossible to use
> them as arguments to commands.
>
> You could have said something like this instead:
>
>         The command parser incorrectly made subcommand names to
>         git-submodule reserved, refusing them to be used as
>         parameters to subcommands.  For example,
>
>                 $ git submodule add init update
>
>         to add a submodule whose (symbolic) name is "init" and
>         that resides at path "update" was refused.
>
> That would have been much cleaner and easier on the reader than
> having to decipher what the 20+ command shell script sequence
> was doing.
>
> I do agree that the breakage is worth fixing, though.
>
> > +# Synopsis of this commands are as follows
> > +# git-submodule [--quiet] [-b branch] add <repository> [<path>]
> > +# git-submodule [--quiet] [--cached] [status] [--] [<path>...]
> > +# git-submodule [--quiet] init [--] [<path>...]
> > +# git-submodule [--quiet] update [--] [<path>...]
>
> I somehow feel that syntactically the original implementation
> that allowed subcommand specific options to come before the
> subcommand name was a mistake.  It may be easier for users that
> both "-b branch add" and "add -b branch" are accepted, but I
> have to wonder if it would really hurt if we made "-b branch
> add" a syntax error.

Just a point in this regard, if we allow to have both "-b branch add"
and "add -b branch" in that case there will be code redundancy as
there will have to parsing of "-b branch" in the subcommand parsing
and in the command module (module_add). I will be implementing now, to
support both; with the exception for --quiet.

>
> So how about reorganizing the top-level option parser like this:
>
>         while :
>         do
>                 case $# in 0) break ;; esac
>                 case "$1" in
>                 add | status | init | update)
>                         # we have found subcommand.
>                         command="$1"
>                         shift
>                         break ;;
>                 --)
>                         # end of parameters
>                         shift
>                         break ;;
>                 --quiet)
>                         quiet=1
>                         ;;
>                 -*)
>                         die "unknown option $1"
>                 esac
>                 shift
>         done
>         test -n "$command" || command=$default_command
>         module_$command "$@"
>
> And then make individual command implementations responsible for
> parsing their own options (and perhaps the common ones, to allow
> "git submodule add --quiet", but that is optional), like:
>
>         module_add () {
>                 while :
>                 do
>                         case $# in 0) break ;; esac
>                         case "$1" in
>                         --cached)
>                                 cached=1
>                                 ;;
>                         -b | --branch)
>                                 shift
>                                 branch="$1"
>                                 test -n "$branch" ||
>                                 die "no branch name after -b?"
>                                 ;;
>                         --)
>                                 shift
>                                 break
>                                 ;;
>                         --quiet)
>                                 quiet=1
>                                 ;;
>                         -*)
>                                 die "unknown option $1"
>                         esac
>                         shift
>                 done
>                 repo=$1
>                 path=$2
>                 ...
>         }
>
> In the above illustration I did not bother eliminating cut&paste
> duplication, but there may be a better way to share the piece to
> parse common options across subcommands option parsers and the
> toplevel one.
>
>



-- 
Imran M Yousuf

Re: [PATCH] - Updated usage and simplified sub-command action invocation

[Junio C Hamano]

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

>> > Actually module_$command is not possible because only add's module is
>> > module_add rest are modules_$command....
>>
>> Is there a fundamental reason why you cannot rename them to be
>> more consistent?
>
> In fact it is consistent, add works on a single module only, whereas
> rest of the command works either on 1 or more. Thus having plural
> (modules) is logical.

It certainly is consistent in _that_ meaning of the word, but I
was not talking about that consistency, which is less useful in
this context.

The consistency I was talking about was "A subcommand called $foo
is always handled by a shell function called cmd_$foo".  That is
also a consistency, and it is of much more useful kind in a
situation like this, namely, a command dispatcher.

If you have show_blobs() and show_commit() subroutines, former
of which takes 1 or more blobs while the latter of which can
only take 1 commit, being consistent in your meaning might help
the programmers avoiding a mistake to pass two or more commits
to a non-existent show_commits().  In that sense, your kind of
consistency is not totally useless.

However, it is not so useful in a context where there is one
call site for each of the functions, like a command dispatcher
scenario.