On Jan 10, 2008 12:23 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> imyousuf@xxxxxxxxx writes:
>
> > From: Imran M Yousuf <imyousuf@xxxxxxxxxxxxxxxxxxxxxx>
> >
> > - 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
