[PATCH] Simplified the invocation of command action in submodule

[PATCH] Simplified the invocation of command action in submodule

[imyousuf]

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

- Simplified the invocation of action.
- Changed switch case based action invoke rather more direct command
invocation. Previously first switch case was used to go through $@ and
determine the action, i.e. add, init, update etc, and second switch case
just to invoke the action. It is modified to determine the action name in
the first case structure instead and later just invoke it.

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

diff --git a/git-submodule.sh b/git-submodule.sh
index ad9fe62..8a29382 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -16,6 +16,7 @@ update=
 status=
 quiet=
 cached=
+command=
 
 #
 # print stuff on stdout unless -q was specified
@@ -293,20 +294,23 @@ modules_list()
 	done
 }
 
+# command specifies the whole function name since 
+# one of theirs prefix is module not modules
 while test $# != 0
 do
 	case "$1" in
 	add)
 		add=1
+		command="module_$1"
 		;;
 	init)
-		init=1
+		command="modules_$1"
 		;;
 	update)
-		update=1
+		command="modules_$1"
 		;;
 	status)
-		status=1
+		command="modules_list"
 		;;
 	-q|--quiet)
 		quiet=1
@@ -320,7 +324,7 @@ do
 		branch="$2"; shift
 		;;
 	--cached)
-		cached=1
+		command="modules_list"
 		;;
 	--)
 		break
@@ -345,20 +349,8 @@ case "$add,$branch" in
 	;;
 esac
 
-case "$add,$init,$update,$status,$cached" in
-1,,,,)
-	module_add "$@"
-	;;
-,1,,,)
-	modules_init "$@"
-	;;
-,,1,,)
-	modules_update "$@"
-	;;
-,,,*,*)
-	modules_list "$@"
-	;;
-*)
+if [ -z $command ]; then 
 	usage
-	;;
-esac
+else
+	"$command" "$@"
+fi
-- 
1.5.3.7

Re: [PATCH] Simplified the invocation of command action in submodule

[Junio C Hamano]

imyousuf@gmail.com writes:

> diff --git a/git-submodule.sh b/git-submodule.sh
> index ad9fe62..8a29382 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -16,6 +16,7 @@ update=
>  status=
>  quiet=
>  cached=
> +command=

Doesn't the patch make some if not all of the above variables
unused?

>  	case "$1" in
>  	add)
>  		add=1
> +		command="module_$1"
>  		;;
>  	init)
> -		init=1
> +		command="modules_$1"
>  		;;

Does the remaining code still use $add?

Re: [PATCH] Simplified the invocation of command action in submodule

[Imran M Yousuf]

Hi Junio,

Firstly, $add is still used later in the code.
Secondly, yes the variables should be deleted. Will make the change
and send the patch again; I forgot to clean the unused variables from
the declaration, sorry.

Best regards,

Imran

On Jan 9, 2008 2:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> imyousuf@gmail.com writes:
>
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index ad9fe62..8a29382 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -16,6 +16,7 @@ update=
> >  status=
> >  quiet=
> >  cached=
> > +command=
>
> Doesn't the patch make some if not all of the above variables
> unused?
>
> >       case "$1" in
> >       add)
> >               add=1
> > +             command="module_$1"
> >               ;;
> >       init)
> > -             init=1
> > +             command="modules_$1"
> >               ;;
>
> Does the remaining code still use $add?
>



-- 
Imran M Yousuf

Re: [PATCH] Simplified the invocation of command action in submodule

[Johannes Sixt]

imyousuf@gmail.com schrieb:
> @@ -16,6 +16,7 @@ update=
>  status=
>  quiet=
>  cached=
> +command=
>
>  #
>  # print stuff on stdout unless -q was specified
> @@ -293,20 +294,23 @@ modules_list()
>  	done
>  }
>
> +# command specifies the whole function name since
> +# one of theirs prefix is module not modules
>  while test $# != 0
>  do
>  	case "$1" in
>  	add)
>  		add=1
> +		command="module_$1"
>  		;;
>  	init)
> -		init=1
> +		command="modules_$1"
>  		;;
>  	update)
> -		update=1
> +		command="modules_$1"
>  		;;
>  	status)
> -		status=1
> +		command="modules_list"
>  		;;
>  	-q|--quiet)
>  		quiet=1
> @@ -320,7 +324,7 @@ do
>  		branch="$2"; shift
>  		;;
>  	--cached)
> -		cached=1
> +		command="modules_list"

Don't remove cached=1 because otherwise --cached is effectively ignored.

>  		;;
>  	--)
>  		break
> @@ -345,20 +349,8 @@ case "$add,$branch" in
>  	;;
>  esac
>
> -case "$add,$init,$update,$status,$cached" in
> -1,,,,)
> -	module_add "$@"
> -	;;
> -,1,,,)
> -	modules_init "$@"
> -	;;
> -,,1,,)
> -	modules_update "$@"
> -	;;
> -,,,*,*)
> -	modules_list "$@"
> -	;;
> -*)
> +if [ -z $command ]; then
>  	usage
> -	;;
> -esac
> +else
> +	"$command" "$@"
> +fi

- Previously 'git submodule' was equvalent to 'git submodule status', now
it is an error.

- Previously, passing --cached to add, init, or update was an error, now
it is not.

-- Hannes

Re: [PATCH] Simplified the invocation of command action in submodule

[Imran M Yousuf]

I already saw that mistake Johannes, thank you for pointing it out.

On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> imyousuf@gmail.com schrieb:
>
> > @@ -16,6 +16,7 @@ update=
> >  status=
> >  quiet=
> >  cached=
> > +command=
> >
> >  #
> >  # print stuff on stdout unless -q was specified
> > @@ -293,20 +294,23 @@ modules_list()
> >       done
> >  }
> >
> > +# command specifies the whole function name since
> > +# one of theirs prefix is module not modules
> >  while test $# != 0
> >  do
> >       case "$1" in
> >       add)
> >               add=1
> > +             command="module_$1"
> >               ;;
> >       init)
> > -             init=1
> > +             command="modules_$1"
> >               ;;
> >       update)
> > -             update=1
> > +             command="modules_$1"
> >               ;;
> >       status)
> > -             status=1
> > +             command="modules_list"
> >               ;;
> >       -q|--quiet)
> >               quiet=1
> > @@ -320,7 +324,7 @@ do
> >               branch="$2"; shift
> >               ;;
> >       --cached)
> > -             cached=1
> > +             command="modules_list"
>
> Don't remove cached=1 because otherwise --cached is effectively ignored.
>
> >               ;;
> >       --)
> >               break
> > @@ -345,20 +349,8 @@ case "$add,$branch" in
> >       ;;
> >  esac
> >
> > -case "$add,$init,$update,$status,$cached" in
> > -1,,,,)
> > -     module_add "$@"
> > -     ;;
> > -,1,,,)
> > -     modules_init "$@"
> > -     ;;
> > -,,1,,)
> > -     modules_update "$@"
> > -     ;;
> > -,,,*,*)
> > -     modules_list "$@"
> > -     ;;
> > -*)
> > +if [ -z $command ]; then
> >       usage
> > -     ;;
> > -esac
> > +else
> > +     "$command" "$@"
> > +fi
>
> - Previously 'git submodule' was equvalent to 'git submodule status', now
> it is an error.
>
> - Previously, passing --cached to add, init, or update was an error, now
> it is not.
>
> -- Hannes
>



-- 
Imran M Yousuf
Entrepreneur & Software Engineer
Smart IT Engineering
Dhaka, Bangladesh
Email: imran@smartitengineering.com
Mobile: +880-1711402557

Re: [PATCH] Simplified the invocation of command action in submodule

[Johannes Sixt]

-- Hannes

of them.
"that mistake" with no clue on which one you mean when I pointed out three
BTW, on this list we don't top-post. In particular not when you write only

Imran M Yousuf schrieb:
> I already saw that mistake Johannes, thank you for pointing it out.
> 
> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> imyousuf@gmail.com schrieb:
>>
[...]

Re: [PATCH] Simplified the invocation of command action in submodule

[Imran M Yousuf]

On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> imyousuf@gmail.com schrieb:
>
> > @@ -16,6 +16,7 @@ update=
> >  status=
> >  quiet=
> >  cached=
> > +command=
> >
> >  #
> >  # print stuff on stdout unless -q was specified
> > @@ -293,20 +294,23 @@ modules_list()
> >       done
> >  }
> >
> > +# command specifies the whole function name since
> > +# one of theirs prefix is module not modules
> >  while test $# != 0
> >  do
> >       case "$1" in
> >       add)
> >               add=1
> > +             command="module_$1"
> >               ;;
> >       init)
> > -             init=1
> > +             command="modules_$1"
> >               ;;
> >       update)
> > -             update=1
> > +             command="modules_$1"
> >               ;;
> >       status)
> > -             status=1
> > +             command="modules_list"
> >               ;;
> >       -q|--quiet)
> >               quiet=1
> > @@ -320,7 +324,7 @@ do
> >               branch="$2"; shift
> >               ;;
> >       --cached)
> > -             cached=1
> > +             command="modules_list"
>
> Don't remove cached=1 because otherwise --cached is effectively ignored.
>
> >               ;;
> >       --)
> >               break
> > @@ -345,20 +349,8 @@ case "$add,$branch" in
> >       ;;
> >  esac
> >
> > -case "$add,$init,$update,$status,$cached" in
> > -1,,,,)
> > -     module_add "$@"
> > -     ;;
> > -,1,,,)
> > -     modules_init "$@"
> > -     ;;
> > -,,1,,)
> > -     modules_update "$@"
> > -     ;;
> > -,,,*,*)
> > -     modules_list "$@"
> > -     ;;
> > -*)
> > +if [ -z $command ]; then
> >       usage
> > -     ;;
> > -esac
> > +else
> > +     "$command" "$@"
> > +fi
>
> - Previously 'git submodule' was equvalent to 'git submodule status', now
> it is an error.

Yes, I forgot to add that status is the default command. Thanks for
pointing it out.

>
> - Previously, passing --cached to add, init, or update was an error, now
> it is not.

The usage statement and this behaviour is rather contradicting. The
usage says that --cached can be used with all commands; so I am not
sure whether using --cached with add should be an error or not. IMHO,
if the previous implementation was right than the USAGE has to be
changed, and if the previous implementation was incorrect, than if the
default command is set to status than current implementation is right.

I would like to get comment on this until I fix the patch and resend it.

>
> -- Hannes
>

Thank you,

-- 
Imran M Yousuf

Re: [PATCH] Simplified the invocation of command action in submodule

[Johannes Sixt]

Imran M Yousuf schrieb:
> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> - Previously, passing --cached to add, init, or update was an error, now
>> it is not.
> 
> The usage statement and this behaviour is rather contradicting. The
> usage says that --cached can be used with all commands; so I am not
> sure whether using --cached with add should be an error or not. IMHO,
> if the previous implementation was right than the USAGE has to be
> changed, and if the previous implementation was incorrect, than if the
> default command is set to status than current implementation is right.

I prefer that the usage statement lists one line per sub-command with the
flags that apply only to the sub-command. IOW, a usage statement that
suggests that a flag applies to all sub-commands when in reality it
doesn't is bogus, IMHO.

-- Hannes

Re: [PATCH] Simplified the invocation of command action in submodule

[Imran M Yousuf]

On Jan 9, 2008 4:01 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Imran M Yousuf schrieb:
> > On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> >> - Previously, passing --cached to add, init, or update was an error, now
> >> it is not.
> >
> > The usage statement and this behaviour is rather contradicting. The
> > usage says that --cached can be used with all commands; so I am not
> > sure whether using --cached with add should be an error or not. IMHO,
> > if the previous implementation was right than the USAGE has to be
> > changed, and if the previous implementation was incorrect, than if the
> > default command is set to status than current implementation is right.
>
> I prefer that the usage statement lists one line per sub-command with the
> flags that apply only to the sub-command. IOW, a usage statement that
> suggests that a flag applies to all sub-commands when in reality it
> doesn't is bogus, IMHO.
>

I think for this patch I will keep the usage intact and keep the
implementation coherent with the current usage and add a comment in
that place so that if required it can be changed in future.

> -- Hannes
>
>



-- 
Imran M Yousuf

Re: [PATCH] Simplified the invocation of command action in submodule

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Imran M Yousuf schrieb:
>> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> - Previously, passing --cached to add, init, or update was an error, now
>>> it is not.
>> 
>> The usage statement and this behaviour is rather contradicting. The
>> usage says that --cached can be used with all commands; so I am not
>> sure whether using --cached with add should be an error or not. IMHO,
>> if the previous implementation was right than the USAGE has to be
>> changed, and if the previous implementation was incorrect, than if the
>> default command is set to status than current implementation is right.
>
> I prefer that the usage statement lists one line per sub-command with the
> flags that apply only to the sub-command. IOW, a usage statement that
> suggests that a flag applies to all sub-commands when in reality it
> doesn't is bogus, IMHO.

I view the usage emitted by a command primarily as a quick
reminder for people who are _already_ familiar with the command
to help "was the option this command takes --foo or --bar?  I
can never remember which X-<" situation.  The usage string is
not a replacement of the manual page.  For that reason, I
generally prefer short and sweet one line usage for the whole
command, even if it does not exactly capture mutually
incompatible option combinations, _as long as_ the command
itself is simple enough.

As you said, however, git-submodule is a command dispatcher on
its own, and what its subcommands do are quite different, to the
point that they probably should not even be sharing the option
parser.  One line per subcommand feels more appropriate.

By the way, Imran, if the current implementation declares a
combination of "add" and "--cached" an error, and a new
implementation does not, that's called a regression.  Unless you
can prove that the combination makes sense and the existing
behaviour is a bug, in which case you can say the new
implementation fixes the bug.

In this case, module_add does not even pay attention to $cached
in the existing code.  The choice is between (1) silently ignore
user's expectation that "add --cached" would do something
different from "add" without "--cached", or (2) tell the user
that the combination does not make sense and error out.  To
people who _know_ what the command does, the choice between the
two does not make much difference (they do not give ignored
option, nor trigger the error), but to new people the latter is
often easier to use.

Re: [PATCH] Simplified the invocation of command action in submodule

[Lars Hjemli]

On Jan 9, 2008 10:51 AM, Imran M Yousuf <imyousuf@gmail.com> wrote:
> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> >
> > - Previously, passing --cached to add, init, or update was an error, now
> > it is not.
>
> The usage statement and this behaviour is rather contradicting. The
> usage says that --cached can be used with all commands; so I am not
> sure whether using --cached with add should be an error or not. IMHO,
> if the previous implementation was right than the USAGE has to be
> changed, and if the previous implementation was incorrect, than if the
> default command is set to status than current implementation is right.
>
> I would like to get comment on this until I fix the patch and resend it.

--cached only makes sense for the status subcommand, so the
usage/manpage probably should have looked like this (except for the
whitespace mangling...):

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index cffc6d4..331e806 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -10,7 +10,10 @@ SYNOPSIS
 --------
 [verse]
 'git-submodule' [--quiet] [-b branch] add <repository> [<path>]
-'git-submodule' [--quiet] [--cached] [status|init|update] [--] [<path>...]
+'git-submodule' [--quiet] [--cached] [status] [--] [<path>...]
+'git-submodule' [--quiet] init [--] [<path>...]
+'git-submodule' [--quiet] update [--] [<path>...]
+


 COMMANDS
-- 
1.5.3.7.1141.g4eb39

Re: [PATCH] Simplified the invocation of command action in submodule

[Imran M Yousuf]

On Jan 9, 2008 4:24 PM, Lars Hjemli <lh@elementstorage.no> wrote:
> On Jan 9, 2008 10:51 AM, Imran M Yousuf <imyousuf@gmail.com> wrote:
> > On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> > >
> > > - Previously, passing --cached to add, init, or update was an error, now
> > > it is not.
> >
> > The usage statement and this behaviour is rather contradicting. The
> > usage says that --cached can be used with all commands; so I am not
> > sure whether using --cached with add should be an error or not. IMHO,
> > if the previous implementation was right than the USAGE has to be
> > changed, and if the previous implementation was incorrect, than if the
> > default command is set to status than current implementation is right.
> >
> > I would like to get comment on this until I fix the patch and resend it.
>
> --cached only makes sense for the status subcommand, so the
> usage/manpage probably should have looked like this (except for the
> whitespace mangling...):
>
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index cffc6d4..331e806 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -10,7 +10,10 @@ SYNOPSIS
>  --------
>  [verse]
>  'git-submodule' [--quiet] [-b branch] add <repository> [<path>]
> -'git-submodule' [--quiet] [--cached] [status|init|update] [--] [<path>...]
> +'git-submodule' [--quiet] [--cached] [status] [--] [<path>...]
> +'git-submodule' [--quiet] init [--] [<path>...]
> +'git-submodule' [--quiet] update [--] [<path>...]
> +
>

This change makes a lot sense. Thus I will make sure that it is used
in this manner :). Thanks a lot for clarifying it Lars. I wanted to
know what is the purpose of '--'? If it is simply meant to be a
separator than fine; else I would be grateful if you would please
explain its purpose, so that I do not again implement wrongly :).

>
>  COMMANDS
> --
> 1.5.3.7.1141.g4eb39
>


-- 
Imran M Yousuf