git submodule output on invalid command

git submodule output on invalid command

[Pieter de Bie]

If you give git submodule an invalid commands, it outputs nothing.
For example:

	Vienna:git pieter$ git submodule satsus
	Vienna:git pieter$

This is because the default command is 'status' and status accepts paths to
limit the output.

I tried to find a fix for this, but git-submodule also allows a syntax
like

	git submodule --cached status
and
	git submodule --cached

so you can't just look at the first argument to see if a command is valid.
Similarly, the default command is 'status', so something like

	Vienna:bonnenteller pieter$ git submodule vendor/
	ef38bc83b7ff4b290a6b1f4d82df03585fbb7529 vendor/plugins/will_paginate (2.3.2)

is also valid. Using that line of reasoning, something like 'git submodule satsus'
is valid and should return nothing, because there are no submodules in
the 'satsus' path. However, I still feel this should produce a warning.

I'm sure there is a nicer way to alert the user than my patch below, which
warns if the user did not supply any valid paths. Anyone else got a more
satisfying approach?

- Pieter

diff --git a/git-submodule.sh b/git-submodule.sh
index 1c39b59..3aae746 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -59,7 +59,12 @@ resolve_relative_url ()
 #
 module_list()
 {
-       git ls-files --stage -- "$@" | grep '^160000 '
+       git ls-files --stage -- "$@" | grep '^160000 ' ||
+       if test -z "$@"; then
+               die "This repository contains no submodules"
+       else
+               die "Could not find any submodules in paths $@"
+       fi
 }
 
 #

Re* git submodule output on invalid command

[Junio C Hamano]

Pieter de Bie <pdebie@ai.rug.nl> writes:

> ..., something like 'git
> submodule satsus' is valid and should return nothing, because there are
> no submodules in the 'satsus' path. However, I still feel this should
> produce a warning.
>
> I'm sure there is a nicer way to alert the user than my patch below, which
> warns if the user did not supply any valid paths. Anyone else got a more
> satisfying approach?

"ls-files --error-unmatch" would warn you of mistyped nonexistent paths,
but "git submodule Makefile" would still catch the Makefile from the
toplevel superproject happily and will not complain without checking after
filtering by submodules.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 1c39b59..3aae746 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -59,7 +59,12 @@ resolve_relative_url ()
>  #
>  module_list()
>  {
> -       git ls-files --stage -- "$@" | grep '^160000 '
> +       git ls-files --stage -- "$@" | grep '^160000 ' ||
> +       if test -z "$@"; then

Shell nit; this must be "$*" not "$@", right?

> +               die "This repository contains no submodules"
> +       else
> +               die "Could not find any submodules in paths $@"

Because die() acts as if it is a fancier echo from output POV this does
not matter in practice, but as a principle, this should also be "$*"
instead.  Upon seeing "git submodule a b c", your intention is not to pass
three parameters "Could not find a", "b" and "c" to die() but is to pass a
single string that is the error message.

By the way, because this "limiting only to submodules" seem to appear very
often, we might want to give ls-files a native feature to do so, perhaps
something like this.  Then your warning/error could become:

	git ls-files --limit-type=submodule --error-unmatch

Although we would need to make the error message that comes from this
codepath tweakable by the caller.

 builtin-ls-files.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git c/builtin-ls-files.c w/builtin-ls-files.c
index e8d568e..69e3b5b 100644
--- c/builtin-ls-files.c
+++ w/builtin-ls-files.c
@@ -28,6 +28,10 @@ static const char **pathspec;
 static int error_unmatch;
 static char *ps_matched;
 static const char *with_tree;
+static unsigned int limit_types;
+#define ENTRY_REGULAR   02
+#define ENTRY_SYMLINK   01
+#define ENTRY_SUBMODULE 04
 
 static const char *tag_cached = "";
 static const char *tag_unmerged = "";
@@ -37,6 +41,18 @@ static const char *tag_killed = "";
 static const char *tag_modified = "";
 
 
+static unsigned int entry_type_from_name(const char *name)
+{
+	if (!strcmp(name, "regular"))
+		return ENTRY_REGULAR;
+	else if (!strcmp(name, "symlink"))
+		return ENTRY_SYMLINK;
+	else if (!strcmp(name, "submodule"))
+		return ENTRY_SUBMODULE;
+	else
+		die("Unknown entry type: %s", name);
+}
+
 /*
  * Match a pathspec against a filename. The first "skiplen" characters
  * are the common prefix
@@ -205,6 +221,20 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
 		tag = alttag;
 	}
 
+	if (limit_types) {
+		unsigned int entry_type;
+		if (S_ISLNK(ce->ce_mode))
+			entry_type = ENTRY_SYMLINK;
+		else if (S_ISREG(ce->ce_mode))
+			entry_type = ENTRY_REGULAR;
+		else if (S_ISGITLINK(ce->ce_mode))
+			entry_type = ENTRY_SUBMODULE;
+		else
+			die("Unknown type of entry %06o", ce->ce_mode);
+		if (!(limit_types & entry_type))
+			return;
+	}
+
 	if (!show_stage) {
 		fputs(tag, stdout);
 	} else {
@@ -551,6 +581,10 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 			with_tree = arg + 12;
 			continue;
 		}
+		if (!prefixcmp(arg, "--limit-type=")) {
+			limit_types |= entry_type_from_name(arg + 13);
+			continue;
+		}
 		if (!prefixcmp(arg, "--abbrev=")) {
 			abbrev = strtoul(arg+9, NULL, 10);
 			if (abbrev && abbrev < MINIMUM_ABBREV)

Re: Re* git submodule output on invalid command

[David Aguilar]

On  0, Junio C Hamano <gitster@pobox.com> wrote:
> Pieter de Bie <pdebie@ai.rug.nl> writes:
> 
> > ..., something like 'git
> > submodule satsus' is valid and should return nothing, because there are
> > no submodules in the 'satsus' path. However, I still feel this should
> > produce a warning.
> >
> > I'm sure there is a nicer way to alert the user than my patch below, which
> > warns if the user did not supply any valid paths. Anyone else got a more
> > satisfying approach?
> 
> "ls-files --error-unmatch" would warn you of mistyped nonexistent paths,
> but "git submodule Makefile" would still catch the Makefile from the
> toplevel superproject happily and will not complain without checking after
> filtering by submodules.
> 
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 1c39b59..3aae746 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -59,7 +59,12 @@ resolve_relative_url ()
> >  #
> >  module_list()
> >  {
> > -       git ls-files --stage -- "$@" | grep '^160000 '
> > +       git ls-files --stage -- "$@" | grep '^160000 ' ||
> > +       if test -z "$@"; then
> 
> Shell nit; this must be "$*" not "$@", right?

I added the module_list() function when moving the duplicated
code into a separate function.  The code was lifted verbatim.
I can submit a patch cleaning that up if it should indeed use
"$*".  Just let me know.

Thanks,

-- 

	David

Re: Re* git submodule output on invalid command

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> On  0, Junio C Hamano <gitster@pobox.com> wrote:
>> Pieter de Bie <pdebie@ai.rug.nl> writes:
>> ...
>> >  module_list()
>> >  {
>> > -       git ls-files --stage -- "$@" | grep '^160000 '
>> > +       git ls-files --stage -- "$@" | grep '^160000 ' ||
>> > +       if test -z "$@"; then
>> 
>> Shell nit; this must be "$*" not "$@", right?
>
> I added the module_list() function when moving the duplicated
> code into a separate function.  The code was lifted verbatim.
> I can submit a patch cleaning that up if it should indeed use
> "$*".  Just let me know.

Nothing you did is involved in this nit; I was talking about "test -z"
argument.

	cmd "$@"

gives N separate argument to the "cmd", as if each of them is surrounded
by a dq pair, i.e.

	cmd "$1" "$2" "$3"...

while

	cmd "$*"

gives a single argument to the "cmd", all separated with the first
character of $IFS (typically a SP), i.e.

	cmd "$1 $2 $3..."

which is what the "test -z" above would want to test (testing $# is Ok for
the purpose of this test as well).

The "$@" you moved is the argument given to ls-files; that one should be
"$@" and replacing it to "$*" would be wrong.