{"thread":{"id":"15384","subject":"git submodule output on invalid command","startedAt":"2008-09-05T16:16:10Z","lastAt":"2008-09-06T05:03:02Z","messageCount":4,"participants":["Pieter de Bie","Junio C Hamano","David Aguilar"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"89853","messageId":"1220631370-19777-1-git-send-email-pdebie@ai.rug.nl","threadId":"15384","inReplyTo":null,"subject":"git submodule output on invalid command","fromName":"Pieter de Bie","fromEmail":"pdebie@ai.rug.nl","sentAt":"2008-09-05T16:16:10Z","receivedAt":"2008-09-05T16:16:10Z","isPatch":false,"sender":{"key":"pdebie@ai.rug.nl","avatar":null},"body":"If you give git submodule an invalid commands, it outputs nothing.\nFor example:\n\n\tVienna:git pieter$ git submodule satsus\n\tVienna:git pieter$\n\nThis is because the default command is 'status' and status accepts paths to\nlimit the output.\n\nI tried to find a fix for this, but git-submodule also allows a syntax\nlike\n\n\tgit submodule --cached status\nand\n\tgit submodule --cached\n\nso you can't just look at the first argument to see if a command is valid.\nSimilarly, the default command is 'status', so something like\n\n\tVienna:bonnenteller pieter$ git submodule vendor/\n\tef38bc83b7ff4b290a6b1f4d82df03585fbb7529 vendor/plugins/will_paginate (2.3.2)\n\nis also valid. Using that line of reasoning, something like 'git submodule satsus'\nis valid and should return nothing, because there are no submodules in\nthe 'satsus' path. However, I still feel this should produce a warning.\n\nI'm sure there is a nicer way to alert the user than my patch below, which\nwarns if the user did not supply any valid paths. Anyone else got a more\nsatisfying approach?\n\n- Pieter\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 1c39b59..3aae746 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -59,7 +59,12 @@ resolve_relative_url ()\n #\n module_list()\n {\n- git ls-files --stage -- \"$@\" | grep '^160000 '\n+ git ls-files --stage -- \"$@\" | grep '^160000 ' ||\n+ if test -z \"$@\"; then\n+ die \"This repository contains no submodules\"\n+ else\n+ die \"Could not find any submodules in paths $@\"\n+ fi\n }\n \n #\n"},{"id":"89861","messageId":"7vy726v30m.fsf@gitster.siamese.dyndns.org","threadId":"15384","inReplyTo":"1220631370-19777-1-git-send-email-pdebie@ai.rug.nl","subject":"Re* git submodule output on invalid command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-05T18:52:41Z","receivedAt":"2008-09-05T18:52:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pieter de Bie writes:\n\n> ..., something like 'git\n> submodule satsus' is valid and should return nothing, because there are\n> no submodules in the 'satsus' path. However, I still feel this should\n> produce a warning.\n>\n> I'm sure there is a nicer way to alert the user than my patch below, which\n> warns if the user did not supply any valid paths. Anyone else got a more\n> satisfying approach?\n\n\"ls-files --error-unmatch\" would warn you of mistyped nonexistent paths,\nbut \"git submodule Makefile\" would still catch the Makefile from the\ntoplevel superproject happily and will not complain without checking after\nfiltering by submodules.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 1c39b59..3aae746 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -59,7 +59,12 @@ resolve_relative_url ()\n> #\n> module_list()\n> {\n> - git ls-files --stage -- \"$@\" | grep '^160000 '\n> + git ls-files --stage -- \"$@\" | grep '^160000 ' ||\n> + if test -z \"$@\"; then\n\nShell nit; this must be \"$*\" not \"$@\", right?\n\n> + die \"This repository contains no submodules\"\n> + else\n> + die \"Could not find any submodules in paths $@\"\n\nBecause die() acts as if it is a fancier echo from output POV this does\nnot matter in practice, but as a principle, this should also be \"$*\"\ninstead. Upon seeing \"git submodule a b c\", your intention is not to pass\nthree parameters \"Could not find a\", \"b\" and \"c\" to die() but is to pass a\nsingle string that is the error message.\n\nBy the way, because this \"limiting only to submodules\" seem to appear very\noften, we might want to give ls-files a native feature to do so, perhaps\nsomething like this. Then your warning/error could become:\n\n\tgit ls-files --limit-type=submodule --error-unmatch\n\nAlthough we would need to make the error message that comes from this\ncodepath tweakable by the caller.\n\n builtin-ls-files.c | 34 ++++++++++++++++++++++++++++++++++\n 1 files changed, 34 insertions(+), 0 deletions(-)\n\ndiff --git c/builtin-ls-files.c w/builtin-ls-files.c\nindex e8d568e..69e3b5b 100644\n--- c/builtin-ls-files.c\n+++ w/builtin-ls-files.c\n@@ -28,6 +28,10 @@ static const char **pathspec;\n static int error_unmatch;\n static char *ps_matched;\n static const char *with_tree;\n+static unsigned int limit_types;\n+#define ENTRY_REGULAR 02\n+#define ENTRY_SYMLINK 01\n+#define ENTRY_SUBMODULE 04\n \n static const char *tag_cached = \"\";\n static const char *tag_unmerged = \"\";\n@@ -37,6 +41,18 @@ static const char *tag_killed = \"\";\n static const char *tag_modified = \"\";\n \n \n+static unsigned int entry_type_from_name(const char *name)\n+{\n+\tif (!strcmp(name, \"regular\"))\n+\t\treturn ENTRY_REGULAR;\n+\telse if (!strcmp(name, \"symlink\"))\n+\t\treturn ENTRY_SYMLINK;\n+\telse if (!strcmp(name, \"submodule\"))\n+\t\treturn ENTRY_SUBMODULE;\n+\telse\n+\t\tdie(\"Unknown entry type: %s\", name);\n+}\n+\n /*\n * Match a pathspec against a filename. The first \"skiplen\" characters\n * are the common prefix\n@@ -205,6 +221,20 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n \t\ttag = alttag;\n \t}\n \n+\tif (limit_types) {\n+\t\tunsigned int entry_type;\n+\t\tif (S_ISLNK(ce->ce_mode))\n+\t\t\tentry_type = ENTRY_SYMLINK;\n+\t\telse if (S_ISREG(ce->ce_mode))\n+\t\t\tentry_type = ENTRY_REGULAR;\n+\t\telse if (S_ISGITLINK(ce->ce_mode))\n+\t\t\tentry_type = ENTRY_SUBMODULE;\n+\t\telse\n+\t\t\tdie(\"Unknown type of entry %06o\", ce->ce_mode);\n+\t\tif (!(limit_types & entry_type))\n+\t\t\treturn;\n+\t}\n+\n \tif (!show_stage) {\n \t\tfputs(tag, stdout);\n \t} else {\n@@ -551,6 +581,10 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\t\twith_tree = arg + 12;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--limit-type=\")) {\n+\t\t\tlimit_types |= entry_type_from_name(arg + 13);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!prefixcmp(arg, \"--abbrev=\")) {\n \t\t\tabbrev = strtoul(arg+9, NULL, 10);\n \t\t\tif (abbrev && abbrev < MINIMUM_ABBREV)\n"},{"id":"89890","messageId":"20080906042217.GB18930@gmail.com","threadId":"15384","inReplyTo":"7vy726v30m.fsf@gitster.siamese.dyndns.org","subject":"Re: Re* git submodule output on invalid command","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2008-09-06T04:22:18Z","receivedAt":"2008-09-06T04:22:18Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On 0, Junio C Hamano wrote:\n> Pieter de Bie writes:\n> \n> > ..., something like 'git\n> > submodule satsus' is valid and should return nothing, because there are\n> > no submodules in the 'satsus' path. However, I still feel this should\n> > produce a warning.\n> >\n> > I'm sure there is a nicer way to alert the user than my patch below, which\n> > warns if the user did not supply any valid paths. Anyone else got a more\n> > satisfying approach?\n> \n> \"ls-files --error-unmatch\" would warn you of mistyped nonexistent paths,\n> but \"git submodule Makefile\" would still catch the Makefile from the\n> toplevel superproject happily and will not complain without checking after\n> filtering by submodules.\n> \n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index 1c39b59..3aae746 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -59,7 +59,12 @@ resolve_relative_url ()\n> > #\n> > module_list()\n> > {\n> > - git ls-files --stage -- \"$@\" | grep '^160000 '\n> > + git ls-files --stage -- \"$@\" | grep '^160000 ' ||\n> > + if test -z \"$@\"; then\n> \n> Shell nit; this must be \"$*\" not \"$@\", right?\n\nI added the module_list() function when moving the duplicated\ncode into a separate function. The code was lifted verbatim.\nI can submit a patch cleaning that up if it should indeed use\n\"$*\". Just let me know.\n\nThanks,\n\n-- \n\n\tDavid\n"},{"id":"89891","messageId":"7vd4jhuard.fsf@gitster.siamese.dyndns.org","threadId":"15384","inReplyTo":"20080906042217.GB18930@gmail.com","subject":"Re: Re* git submodule output on invalid command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-06T05:03:02Z","receivedAt":"2008-09-06T05:03:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> On 0, Junio C Hamano wrote:\n>> Pieter de Bie writes:\n>> ...\n>> > module_list()\n>> > {\n>> > - git ls-files --stage -- \"$@\" | grep '^160000 '\n>> > + git ls-files --stage -- \"$@\" | grep '^160000 ' ||\n>> > + if test -z \"$@\"; then\n>> \n>> Shell nit; this must be \"$*\" not \"$@\", right?\n>\n> I added the module_list() function when moving the duplicated\n> code into a separate function. The code was lifted verbatim.\n> I can submit a patch cleaning that up if it should indeed use\n> \"$*\". Just let me know.\n\nNothing you did is involved in this nit; I was talking about \"test -z\"\nargument.\n\n\tcmd \"$@\"\n\ngives N separate argument to the \"cmd\", as if each of them is surrounded\nby a dq pair, i.e.\n\n\tcmd \"$1\" \"$2\" \"$3\"...\n\nwhile\n\n\tcmd \"$*\"\n\ngives a single argument to the \"cmd\", all separated with the first\ncharacter of $IFS (typically a SP), i.e.\n\n\tcmd \"$1 $2 $3...\"\n\nwhich is what the \"test -z\" above would want to test (testing $# is Ok for\nthe purpose of this test as well).\n\nThe \"$@\" you moved is the argument given to ls-files; that one should be\n\"$@\" and replacing it to \"$*\" would be wrong.\n"}]}