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

["Imran M Yousuf" <imyousuf@gmail.com>]

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