{"thread":{"id":"44052","subject":"[RFC/PATCH] ls-files: adding support for submodules","startedAt":"2016-09-09T21:54:32Z","lastAt":"2016-09-15T21:37:11Z","messageCount":22,"participants":["Brandon Williams","Jeff King","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"301571","messageId":"1473458004-41460-1-git-send-email-bmwill@google.com","threadId":"44052","inReplyTo":null,"subject":"[RFC/PATCH] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-09T21:53:24Z","receivedAt":"2016-09-09T21:54:32Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Allow ls-files to recognize submodules in order to retrieve a list of\nfiles from a repository's submodules.  This is done by forking off a\nprocess to recursively call ls-files on all submodules.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\nHey git developers!\n\nI'm new to the community and this is the first patch for an open source project\nthat I have worked on.\n\nI'm looking forward to working on the project!\nBrandon Williams\n\n Documentation/git-ls-files.txt         |   7 ++-\n builtin/ls-files.c                     |  58 +++++++++++++++++++\n t/t3007-ls-files-recurse-submodules.sh | 103 +++++++++++++++++++++++++++++++++\n 3 files changed, 167 insertions(+), 1 deletion(-)\n create mode 100644 t/t3007-ls-files-recurse-submodules.sh\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 0d933ac..446209e 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -18,7 +18,8 @@ SYNOPSIS\n \t\t[--exclude-per-directory=<file>]\n \t\t[--exclude-standard]\n \t\t[--error-unmatch] [--with-tree=<tree-ish>]\n-\t\t[--full-name] [--abbrev] [--] [<file>...]\n+\t\t[--full-name] [--recurse-submodules]\n+\t\t[--abbrev] [--] [<file>...]\n \n DESCRIPTION\n -----------\n@@ -137,6 +138,10 @@ a space) at the start of each line:\n \toption forces paths to be output relative to the project\n \ttop directory.\n \n+--recurse-submodules::\n+\tRecursively calls ls-files on each submodule in the repository.\n+\tCurrently there is only support for the --cached mode.\n+\n --abbrev[=<n>]::\n \tInstead of showing the full 40-byte hexadecimal object\n \tlines, show only a partial prefix.\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 00ea91a..c428a51 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -14,6 +14,7 @@\n #include \"resolve-undo.h\"\n #include \"string-list.h\"\n #include \"pathspec.h\"\n+#include \"run-command.h\"\n \n static int abbrev;\n static int show_deleted;\n@@ -28,6 +29,7 @@ static int show_valid_bit;\n static int line_terminator = '\\n';\n static int debug_mode;\n static int show_eol;\n+static int recurse_submodules;\n \n static const char *prefix;\n static int max_prefix_len;\n@@ -152,6 +154,45 @@ static void show_killed_files(struct dir_struct *dir)\n \t}\n }\n \n+/**\n+ *  Recursively call ls-files on a submodule\n+ */\n+static void show_gitlink(const struct cache_entry *ce)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf name = STRBUF_INIT;\n+\tint submodule_name_len;\n+\tFILE *fp;\n+\n+\targv_array_push(&cp.args, \"ls-files\");\n+\targv_array_push(&cp.args, \"--recurse-submodules\");\n+\tcp.git_cmd = 1;\n+\tcp.dir = ce->name;\n+\tcp.out = -1;\n+\tstart_command(&cp);\n+\tfp = fdopen(cp.out, \"r\");\n+\n+\t/*\n+\t * The ls-files child process produces filenames relative to\n+\t * the submodule. Prefix each line with the submodule path\n+\t * to make it relative to the current repository.\n+\t */\n+\tstrbuf_addstr(&name, ce->name);\n+\tstrbuf_addch(&name, '/');\n+\tsubmodule_name_len = name.len;\n+\twhile (strbuf_getline(&buf, fp) != EOF) {\n+\t\tstrbuf_addbuf(&name, &buf);\n+\t\twrite_name(name.buf);\n+\t\tstrbuf_setlen(&name, submodule_name_len);\n+\t}\n+\n+\tfinish_command(&cp);\n+\tstrbuf_release(&buf);\n+\tstrbuf_release(&name);\n+\tfclose(fp);\n+}\n+\n static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n {\n \tint len = max_prefix_len;\n@@ -163,6 +204,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n \t\t\t    len, ps_matched,\n \t\t\t    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))\n \t\treturn;\n+\tif (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {\n+\t\tshow_gitlink(ce);\n+\t\treturn;\n+\t}\n \n \tif (tag && *tag && show_valid_bit &&\n \t    (ce->ce_flags & CE_VALID)) {\n@@ -468,6 +513,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\t{ OPTION_SET_INT, 0, \"full-name\", &prefix_len, NULL,\n \t\t\tN_(\"make the output relative to the project top directory\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },\n+\t\tOPT_BOOL(0, \"recurse-submodules\", &recurse_submodules,\n+\t\t\tN_(\"recurse through submodules\")),\n \t\tOPT_BOOL(0, \"error-unmatch\", &error_unmatch,\n \t\t\tN_(\"if any <file> is not in the index, treat this as an error\")),\n \t\tOPT_STRING(0, \"with-tree\", &with_tree, N_(\"tree-ish\"),\n@@ -519,6 +566,17 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n+\tif (recurse_submodules &&\n+\t    (show_stage || show_deleted || show_others || show_unmerged ||\n+\t     show_killed || show_modified || show_resolve_undo ||\n+\t     show_valid_bit || show_tag || show_eol))\n+\t\tdie(\"ls-files --recurse-submodules can only be used in \"\n+\t\t    \"--cached mode\");\n+\n+\tif (recurse_submodules && argc)\n+\t\tdie(\"ls-files --recurse-submodules does not support path \"\n+\t\t    \"arguments\");\n+\n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD |\n \t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nnew file mode 100644\nindex 0000000..78deded\n--- /dev/null\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -0,0 +1,103 @@\n+#!/bin/sh\n+\n+test_description='Test ls-files recurse-submodules feature\n+\n+This test verifies the recurse-submodules feature correctly lists files from\n+submodules.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup directory structure and submodules' '\n+\techo a >a &&\n+\tmkdir b &&\n+\techo b >b/b &&\n+\tgit add a b &&\n+\tgit commit -m \"add a and b\" &&\n+\tmkdir submodule &&\n+\t(\n+\t\tcd submodule &&\n+\t\tgit init &&\n+\t\techo c >c &&\n+\t\tgit add c &&\n+\t\tgit commit -m \"add c\"\n+\t) &&\n+\tgit submodule add ./submodule &&\n+\tgit commit -m \"added submodule\"\n+'\n+\n+cat >expect <<EOF\n+.gitmodules\n+a\n+b/b\n+submodule/c\n+EOF\n+\n+test_expect_success 'ls-files correctly outputs files in submodule' '\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files does not output files not added to a repo' '\n+\techo a >not_added &&\n+\techo b >b/not_added &&\n+\t(\n+\t\tcd submodule &&\n+\t\techo c >not_added\n+\t) &&\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<EOF\n+.gitmodules\n+a\n+b/b\n+submodule/.gitmodules\n+submodule/c\n+submodule/subsub/d\n+EOF\n+\n+test_expect_success 'ls-files recurses more than 1 level' '\n+\t(\n+\t\tcd submodule &&\n+\t\tmkdir subsub &&\n+\t\t(\n+\t\t\tcd subsub &&\n+\t\t\tgit init &&\n+\t\t\techo d >d &&\n+\t\t\tgit add d &&\n+\t\t\tgit commit -m \"add d\"\n+\t\t) &&\n+\t\tgit submodule add ./subsub &&\n+\t\tgit commit -m \"added subsub\"\n+\t) &&\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect_error <<EOF\n+fatal: ls-files --recurse-submodules does not support path arguments\n+EOF\n+\n+test_expect_success 'error when using path arguments' '\n+\ttest_must_fail git ls-files --recurse-submodules b 2>actual &&\n+\ttest_cmp expect_error actual\n+'\n+\n+cat >expect_error <<EOF\n+fatal: ls-files --recurse-submodules can only be used in --cached mode\n+EOF\n+\n+test_expect_success 'error when using different modes' '\n+\tfor opt in {v,t}; do\n+\t\ttest_must_fail git ls-files --recurse-submodules -$opt 2>actual &&\n+\t\ttest_cmp expect_error actual\n+\tdone &&\n+\tfor opt in {deleted,modified,others,ignored,stage,killed,unmerged,eol}; do\n+\t\ttest_must_fail git ls-files --recurse-submodules --$opt 2>actual &&\n+\t\ttest_cmp expect_error actual\n+\tdone\n+'\n+\n+test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301575","messageId":"20160909223513.3rirneqxmrcyi4k4@sigill.intra.peff.net","threadId":"44052","inReplyTo":"1473458004-41460-1-git-send-email-bmwill@google.com","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T22:35:14Z","receivedAt":"2016-09-09T22:35:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 09, 2016 at 02:53:24PM -0700, Brandon Williams wrote:\n\n> Allow ls-files to recognize submodules in order to retrieve a list of\n> files from a repository's submodules.  This is done by forking off a\n> process to recursively call ls-files on all submodules.\n> \n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n> Hey git developers!\n> \n> I'm new to the community and this is the first patch for an open source project\n> that I have worked on.\n> \n> I'm looking forward to working on the project!\n\nWelcome. :)\n\nSubmodules are not really my area of expertise, so I don't have any\ncommentary on the goal of the patch, except that it sounds reasonable to\nmy layman's ears.\n\nThe implementation looks fairly clean. A few comments:\n\n> +static void show_gitlink(const struct cache_entry *ce)\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf name = STRBUF_INIT;\n> +\tint submodule_name_len;\n> +\tFILE *fp;\n> +\n> +\targv_array_push(&cp.args, \"ls-files\");\n> +\targv_array_push(&cp.args, \"--recurse-submodules\");\n> +\tcp.git_cmd = 1;\n> +\tcp.dir = ce->name;\n> +\tcp.out = -1;\n> +\tstart_command(&cp);\n> +\tfp = fdopen(cp.out, \"r\");\n\nYou should error-check the result of start_command(). I guess the\nreasonable outcome would be to die(), as it is a sign that we could not\nfork, find git, etc.\n\nDitto for fdopen (you can use xfdopen for that).\n\n> +\t/*\n> +\t * The ls-files child process produces filenames relative to\n> +\t * the submodule. Prefix each line with the submodule path\n> +\t * to make it relative to the current repository.\n> +\t */\n> +\tstrbuf_addstr(&name, ce->name);\n> +\tstrbuf_addch(&name, '/');\n> +\tsubmodule_name_len = name.len;\n> +\twhile (strbuf_getline(&buf, fp) != EOF) {\n> +\t\tstrbuf_addbuf(&name, &buf);\n> +\t\twrite_name(name.buf);\n> +\t\tstrbuf_setlen(&name, submodule_name_len);\n> +\t}\n\nWhat happens if the filename in the submodule needs quoting? You'll get\nthe quoted value in your buffer, and then re-quote it again in\nwrite_name().\n\nThe simplest thing would probably be to use \"ls-files -z\" for the\nrecursive invocation, and then split on NUL bytes (we have\nstrbuf_getline_nul for that).\n\n> +\tfinish_command(&cp);\n\nWhat should happen if finish_command() tells us that the ls-files\nsub-process reported an error? It may not be worth aborting the rest of\nthe listing, but we might want to propagate that in our own return code.\n\n> +\tstrbuf_release(&buf);\n> +\tstrbuf_release(&name);\n> +\tfclose(fp);\n> +}\n\nA minor style nit, but I would generally fclose(fp) before running\nfinish_command() (i.e., resource clean up in the reverse order of\nallocation). It doesn't matter in this case because \"fp\" is output from\nthe process, and we know we've already read to EOF. For other cases, it\ncould cause a deadlock (e.g., we end up in wait() for the child process\nto finish, but it is blocked in write() waiting for us to read). So I\nthink it's a good habit to get into.\n\n> @@ -519,6 +566,17 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n>  \tif (require_work_tree && !is_inside_work_tree())\n>  \t\tsetup_work_tree();\n>  \n> +\tif (recurse_submodules &&\n> +\t    (show_stage || show_deleted || show_others || show_unmerged ||\n> +\t     show_killed || show_modified || show_resolve_undo ||\n> +\t     show_valid_bit || show_tag || show_eol))\n> +\t\tdie(\"ls-files --recurse-submodules can only be used in \"\n> +\t\t    \"--cached mode\");\n\nWoah, that list of variables is getting rather long. This is not a\nproblem introduced by your patch, so it's not a blocker. But I wonder if\nsome of them are mutually exclusive and could be collapsed to a single\nvariable.\n\nI guess the reason for this \"only with --cached\" is that you do not\npropagate the options down to the recursive process. If we were to do\nthat, then this big list of restrictions would go away. I'd be OK with\nstarting with more limited functionality like your patch, though. I\nthink doing the recursive thing correctly would also involve parsing the\noutput of each to append the filename prefix.\n\nSo I suppose another option would be to teach ls-files a \"prefix\" option\nto add to each filename, and just pass in the submodule path. Then you\ncan let the sub-processes write directly to the common stdout, and I\nthink it would be safe to blindly pass the parent argv into the child\nprocesses.\n\n-Peff\n"},{"id":"301576","messageId":"xmqqsht88zov.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"20160909223513.3rirneqxmrcyi4k4@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T22:40:16Z","receivedAt":"2016-09-09T22:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I suppose another option would be to teach ls-files a \"prefix\" option\n> to add to each filename, and just pass in the submodule path. Then you\n> can let the sub-processes write directly to the common stdout, and I\n> think it would be safe to blindly pass the parent argv into the child\n> processes.\n\nI think that is a sensible way to do this, instead of reading from -z\nand showing things depending on various output modes you were told\nto use.\n"},{"id":"301579","messageId":"xmqqh99o8ypm.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"1473458004-41460-1-git-send-email-bmwill@google.com","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T23:01:25Z","receivedAt":"2016-09-09T23:01:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Allow ls-files to recognize submodules in order to retrieve a list of\n> files from a repository's submodules.  This is done by forking off a\n> process to recursively call ls-files on all submodules.\n\nWhile I see why \"ls-files --recurse-submodules\" sounds nice (\"hey, I\ncan get list of _all_ the files here\"), and I am quite happy with\nthe quality of implementation (not just the code but its\ndocumentation and test) especially from a first-time contributor, I\nam not quite sure what the utility of this new feature would be,\nespecially given that the command is a plumbing, i.e. meant to be a\nuseful building block for scripts.\n\nIf I get\n\n\t$ git ls-files --recurse-submodules\n\tMakefile\n        lib/Makefile\n        lib/hello.py\n\tmain.py\n\tgoodbye.py\n\nout of the command, what can I do with it without knowing where the\nsubmodule boundaries are?  It's not like I can just do\n\n\tgit ls-files --recurse-submodule |\n        while read path\n        do\n        \tgit update-index --add \"$path\"\n\tdone\n\nwhen \"lib/\" is a submodule.  Instead, I'd need to go to \"lib/\" and\nthen add \"Makefile\" and \"hello.py\" from there.\n\n> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\n> new file mode 100644\n> index 0000000..78deded\n> --- /dev/null\n> +++ b/t/t3007-ls-files-recurse-submodules.sh\n> @@ -0,0 +1,103 @@\n> +...\n> +test_expect_success 'setup directory structure and submodules' '\n> +...\n> +'\n> +\n> +cat >expect <<EOF\n> +.gitmodules\n> +a\n> +b/b\n> +submodule/c\n> +EOF\n\nWe used to do that when we didn't know better.\n\nPlease don't do things outside test_expect_* block, especially in a\nnew script.\n"},{"id":"301584","messageId":"CAGZ79kaWNykEAZcjZ2Y19j2_3TF+qcz+vb8zSi1U+me3k+06LA@mail.gmail.com","threadId":"44052","inReplyTo":"xmqqh99o8ypm.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-09T23:47:45Z","receivedAt":"2016-09-09T23:48:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 9, 2016 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Brandon Williams <bmwill@google.com> writes:\n>\n>> Allow ls-files to recognize submodules in order to retrieve a list of\n>> files from a repository's submodules.  This is done by forking off a\n>> process to recursively call ls-files on all submodules.\n>\n> While I see why \"ls-files --recurse-submodules\" sounds nice (\"hey, I\n> can get list of _all_ the files here\"), and I am quite happy with\n> the quality of implementation (not just the code but its\n> documentation and test)\n\nThe plan is to hook the ls-files machinery into\ngit-grep as the way of obtaining files to grep for a pattern.\n\nAs git-grep has a thread pool already, getting the list of files\nsingle threaded  is more beneficial as e.g. calling git-grep recursively\nfor submodules, as then you may have a fork bomb.\n\n>\n>> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\n\n> We used to do that when we didn't know better.\n>\n\nOops, a lot of stuff slipped through the first internal review.\n"},{"id":"301674","messageId":"xmqqmvje6qbe.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"CAGZ79kaWNykEAZcjZ2Y19j2_3TF+qcz+vb8zSi1U+me3k+06LA@mail.gmail.com","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-11T22:10:13Z","receivedAt":"2016-09-11T22:10:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The plan is to hook the ls-files machinery into\n> git-grep as the way of obtaining files to grep for a pattern.\n\nThat does not make much sense to me for exactly the same reason why\nthe \"grab the list of paths and run 'git add' on them\" example in\nthe message you are responding to does not make sense.  The use of\nthe thread-pool would still need to honor the submodule boundary so\nthat one thread may be assigned files in the top-level superproject\nwhile another may be assigned files in lib/ submodule repository,\nand the latter would be doing a rough equivalent of \"git -C lib\ngrep\" perhaps with a new option \"--output-path-prefix=lib/\" that\nmakes any and all paths that are reported from the command prefixed\nwith the specified string, so the result of its grepping in Makefile\nmay be reported as findings in lib/Makefile.\n\nFor that, it is not sufficient for the enumeration of paths done in\nthe top-level to just list lib/Makefile and lib/hello.py along with\nMakefile and main.py, is it?  You would somehow need to have a way\nto tell that 'lib/' and everything in there is inside a separate\nrepository.  Without knowing that \"lib/\" is its own repository, you\nwould not even know which files under \"lib/\" hierarchy in the\nfilesystem are actually tracked files, which you would learn only by\nreading lib/.git/index, or what textconv filtering needs to be done\non them, which you would learn only by reading lib/.gitattributes\nand/or lib/.git/config.\n\nSo a \"ls-files\" that is done internally in the end-user facing \"git\ngrep --recurse-submodules\" needs to be run _without_ recursing\nitself at least once to learn \"lib/\" is a submodule.  A flat \"here\nare everything we have\" does not sound like a good building block.\n"},{"id":"301684","messageId":"20160912005106.fj7adhmtfklv4vir@sigill.intra.peff.net","threadId":"44052","inReplyTo":"xmqqmvje6qbe.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-12T00:51:06Z","receivedAt":"2016-09-12T00:51:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 11, 2016 at 03:10:13PM -0700, Junio C Hamano wrote:\n\n> So a \"ls-files\" that is done internally in the end-user facing \"git\n> grep --recurse-submodules\" needs to be run _without_ recursing\n> itself at least once to learn \"lib/\" is a submodule.  A flat \"here\n> are everything we have\" does not sound like a good building block.\n\nI do not use submodules myself, but I could imagine that you may have\nscripts outside of git that do not care about the submodule divisions at\nall, and would be happy with the flat block. E.g., our \"make tags\"\ntarget uses \"git ls-files\" so find all of the source files. I could\nimagine projects with submodules that would want to do so recursively (I\ncould also imagine projects that do _not_ want to do so; it would depend\non your workflow and how tightly bound the submodules are). Another\nplausible example would be something grep-like that has features\ngit-grep does not (I don't use \"ack\", but perhaps \"git ls-files\n--recurse-submodules -z | xargs --null ack ...\" is something people\nwould want to do).\n\n-Peff\n"},{"id":"301685","messageId":"20160912005229.6njhgfq7h6cb34s4@sigill.intra.peff.net","threadId":"44052","inReplyTo":"20160912005106.fj7adhmtfklv4vir@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-12T00:52:30Z","receivedAt":"2016-09-12T00:52:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 11, 2016 at 08:51:06PM -0400, Jeff King wrote:\n\n> On Sun, Sep 11, 2016 at 03:10:13PM -0700, Junio C Hamano wrote:\n> \n> > So a \"ls-files\" that is done internally in the end-user facing \"git\n> > grep --recurse-submodules\" needs to be run _without_ recursing\n> > itself at least once to learn \"lib/\" is a submodule.  A flat \"here\n> > are everything we have\" does not sound like a good building block.\n> \n> I do not use submodules myself, but I could imagine that you may have\n> scripts outside of git that do not care about the submodule divisions at\n> all, and would be happy with the flat block. E.g., our \"make tags\"\n> target uses \"git ls-files\" so find all of the source files. I could\n> imagine projects with submodules that would want to do so recursively (I\n> could also imagine projects that do _not_ want to do so; it would depend\n> on your workflow and how tightly bound the submodules are). Another\n> plausible example would be something grep-like that has features\n> git-grep does not (I don't use \"ack\", but perhaps \"git ls-files\n> --recurse-submodules -z | xargs --null ack ...\" is something people\n> would want to do).\n\nNone of that negates your point, btw, which is that this does not seem\nlike a great building block for \"git grep --recurse-submodules\". Just\nthat it seems plausible to me that people could find recursive\n\"ls-files\" useful on its own.\n\n-Peff\n"},{"id":"301686","messageId":"xmqqlgyx6g9k.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"xmqqmvje6qbe.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-12T01:47:19Z","receivedAt":"2016-09-12T01:47:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> So a \"ls-files\" that is done internally in the end-user facing \"git\n> grep --recurse-submodules\" needs to be run _without_ recursing\n> itself at least once to learn \"lib/\" is a submodule.  A flat \"here\n> are everything we have\" does not sound like a good building block.\n\n... unless you are _only_ interested in grepping (or in general\nworking outside Git repository) in the files in the working tree,\ni.e. \"git grep\" without <tree-ish> nor \"--cached\".\n\nA lot of the time you are interested in the current state of files,\nnot even in \"the state recorded in the tip of the history\" but in\n\"the state as I have in my working tree, the state as my compiler\nsees it\".\n\nI am a bit torn.  Clearly this is an important special case, but it\nwould make the codepath for object database case and working tree\ncase even further apart between \"git grep [--cached | <tree-ish>]\"\nand the \"find in the working tree\" codepath, which does not sound\nfriendly to the codebase.\n\n"},{"id":"301726","messageId":"xmqqmvjd3y5l.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"20160912005229.6njhgfq7h6cb34s4@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-12T16:01:26Z","receivedAt":"2016-09-12T16:01:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> I do not use submodules myself, but I could imagine that you may have\n>> scripts outside of git that do not care about the submodule divisions at\n>> all, and would be happy with the flat block. ...\n>> git-grep does not (I don't use \"ack\", but perhaps \"git ls-files\n>> --recurse-submodules -z | xargs --null ack ...\" is something people\n>> would want to do).\n>\n> None of that negates your point, btw, which is that this does not seem\n> like a great building block for \"git grep --recurse-submodules\". Just\n> that it seems plausible to me that people could find recursive\n> \"ls-files\" useful on its own.\n\nI do think it is a good argument why ls-files (with or without -z)\nthat recurses into submodules would help \"git grep\" that does not\nlook at the object store and does the search in the working tree\nfiles that are tracked.  In normal Git world view, if you want a\nlist of files that are tracked, you are the one who is supposed to\ngo to the repository and look at the index there, but if you can\nsomehow magically get that list out-of-band, the \"in the working\ntree files\" mode of \"git grep\" can work just like the \"xargs -z ack\"\npipeline in your example.\n\nSo I tend to agree that the enhancement in question is a useful\nthing on its own, at least the part that gives a list of paths in\nthe index.\n\nI do not know if all other operationg modes are useful, though.  For\neaxmple, the mode that lists untracked paths might be useful to do a\nrecursive \"git clean\".  On the other hand, \"ls-files -s\" with the\npatch may produce the \"correct\" result (i.e. the object name given\nfor each path would be the same one that are found in the index of\nthe submodule the path was taken from), but the correctness may not\nnecessarily translate to usefulness, exactly because you need to\nknow which submodule's repository has the named object that is not\ngiven by the flattened list.\n\nHaving to say that \"this command produces correct result, but not\nall correct things are necessarily useful\" makes me wonder if that\nis the direction in which we would want to go.  For example, what\nwould be our answer to an end-user question: I now know that the\nrecursive ls-files can give me all the untracked files in the\ntop-level and submodules.  How can I feed that to \"git add\" to\nupdate their respective index files with them?\n\nI am not convinced that we would always want to make \"git add\nlib/Makefile\" to automagically run \"git -C lib/ add Makefile\" when\nlib/ is a submodule (for that matter, even if we wanted to, I do not\nknow if that is something we can efficiently do with pathspecs that\nhave wildcards).\n\nSo...\n\n\n\n"},{"id":"301734","messageId":"CAKoko1rNUaoC7pKH=aWQQP83AoeZjakFSNq_w-G2OL78ZKg+=w@mail.gmail.com","threadId":"44052","inReplyTo":"CAKoko1oSEac_Nr1SkRB=dM_r3Jnew1Et2ZKj716iU3JLyHe2GQ@mail.gmail.com","subject":"Re: [RFC/PATCH] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-12T17:39:59Z","receivedAt":"2016-09-12T17:40:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Thanks for all the comments.  What it sounds like is that using ls-files as\na means to power a recursive git-grep may not be like the best approach (I\nassumed that would be the case but thought it a decent place to start).\n\nI agree that not all operating modes would be useful for a recursive\nls-files, which is why I initially don't have support for them.  I guess\nthe question would be which modes would be worth supporting in a recursive\ncase?\n"},{"id":"301796","messageId":"1473726835-143289-1-git-send-email-bmwill@google.com","threadId":"44052","inReplyTo":"1473458004-41460-1-git-send-email-bmwill@google.com","subject":"[PATCH v2] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-13T00:33:55Z","receivedAt":"2016-09-13T00:34:38Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Allow ls-files to recognize submodules in order to retrieve a list of\nfiles from a repository's submodules.  This is done by forking off a\nprocess to recursively call ls-files on all submodules. Also added an\noutput-path-prefix command in order to prepend paths to child processes.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n Documentation/git-ls-files.txt         | 11 +++-\n builtin/ls-files.c                     | 60 +++++++++++++++++++++\n t/t3007-ls-files-recurse-submodules.sh | 99 ++++++++++++++++++++++++++++++++++\n 3 files changed, 169 insertions(+), 1 deletion(-)\n create mode 100755 t/t3007-ls-files-recurse-submodules.sh\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 0d933ac..a623ebf 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -18,7 +18,9 @@ SYNOPSIS\n \t\t[--exclude-per-directory=<file>]\n \t\t[--exclude-standard]\n \t\t[--error-unmatch] [--with-tree=<tree-ish>]\n-\t\t[--full-name] [--abbrev] [--] [<file>...]\n+\t\t[--full-name] [--recurse-submodules]\n+\t\t[--output-path-prefix=<path>]\n+\t\t[--abbrev] [--] [<file>...]\n \n DESCRIPTION\n -----------\n@@ -137,6 +139,13 @@ a space) at the start of each line:\n \toption forces paths to be output relative to the project\n \ttop directory.\n \n+--recurse-submodules::\n+\tRecursively calls ls-files on each submodule in the repository.\n+\tCurrently there is only support for the --cached mode.\n+\n+--output-path-prefix=<path>::\n+\tPrepend the provided path to the output of each file\n+\n --abbrev[=<n>]::\n \tInstead of showing the full 40-byte hexadecimal object\n \tlines, show only a partial prefix.\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 00ea91a..c0bce00 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -14,6 +14,7 @@\n #include \"resolve-undo.h\"\n #include \"string-list.h\"\n #include \"pathspec.h\"\n+#include \"run-command.h\"\n \n static int abbrev;\n static int show_deleted;\n@@ -28,6 +29,8 @@ static int show_valid_bit;\n static int line_terminator = '\\n';\n static int debug_mode;\n static int show_eol;\n+static const char *output_path_prefix = \"\";\n+static int recurse_submodules;\n \n static const char *prefix;\n static int max_prefix_len;\n@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)\n static void write_name(const char *name)\n {\n \t/*\n+\t * NEEDSWORK: To make this thread-safe, full_name would have to be owned\n+\t * by the caller.\n+\t *\n+\t * full_name get reused across output lines to minimize the allocation\n+\t * churn.\n+\t */\n+\tstatic struct strbuf full_name = STRBUF_INIT;\n+\tif (output_path_prefix != '\\0') {\n+\t\tstrbuf_reset(&full_name);\n+\t\tstrbuf_addstr(&full_name, output_path_prefix);\n+\t\tstrbuf_addstr(&full_name, name);\n+\t\tname = full_name.buf;\n+\t}\n+\n+\t/*\n \t * With \"--full-name\", prefix_len=0; this caller needs to pass\n \t * an empty string in that case (a NULL is good for \"\").\n \t */\n@@ -152,6 +170,25 @@ static void show_killed_files(struct dir_struct *dir)\n \t}\n }\n \n+/**\n+ * Recursively call ls-files on a submodule\n+ */\n+static void show_gitlink(const struct cache_entry *ce)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tint status;\n+\n+\targv_array_push(&cp.args, \"ls-files\");\n+\targv_array_push(&cp.args, \"--recurse-submodules\");\n+\targv_array_pushf(&cp.args, \"--output-path-prefix=%s%s/\",\n+\t\t\t output_path_prefix, ce->name);\n+\tcp.git_cmd = 1;\n+\tcp.dir = ce->name;\n+\tstatus = run_command(&cp);\n+\tif (status)\n+\t\texit(status);\n+}\n+\n static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n {\n \tint len = max_prefix_len;\n@@ -163,6 +200,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n \t\t\t    len, ps_matched,\n \t\t\t    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))\n \t\treturn;\n+\tif (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {\n+\t\tshow_gitlink(ce);\n+\t\treturn;\n+\t}\n \n \tif (tag && *tag && show_valid_bit &&\n \t    (ce->ce_flags & CE_VALID)) {\n@@ -468,6 +509,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\t{ OPTION_SET_INT, 0, \"full-name\", &prefix_len, NULL,\n \t\t\tN_(\"make the output relative to the project top directory\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },\n+\t\tOPT_STRING(0, \"output-path-prefix\", &output_path_prefix,\n+\t\t\tN_(\"path\"), N_(\"prepend <path> to each file\")),\n+\t\tOPT_BOOL(0, \"recurse-submodules\", &recurse_submodules,\n+\t\t\tN_(\"recurse through submodules\")),\n \t\tOPT_BOOL(0, \"error-unmatch\", &error_unmatch,\n \t\t\tN_(\"if any <file> is not in the index, treat this as an error\")),\n \t\tOPT_STRING(0, \"with-tree\", &with_tree, N_(\"tree-ish\"),\n@@ -519,6 +564,21 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n+\tif (recurse_submodules &&\n+\t    (show_stage || show_deleted || show_others || show_unmerged ||\n+\t     show_killed || show_modified || show_resolve_undo ||\n+\t     show_valid_bit || show_tag || show_eol))\n+\t\tdie(\"ls-files --recurse-submodules can only be used in \"\n+\t\t    \"--cached mode\");\n+\n+\tif (recurse_submodules && error_unmatch)\n+\t\tdie(\"ls-files --recurse-submodules does not support \"\n+\t\t    \"--error-unmatch\");\n+\n+\tif (recurse_submodules && argc)\n+\t\tdie(\"ls-files --recurse-submodules does not support path \"\n+\t\t    \"arguments\");\n+\n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD |\n \t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nnew file mode 100755\nindex 0000000..caf3815\n--- /dev/null\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -0,0 +1,99 @@\n+#!/bin/sh\n+\n+test_description='Test ls-files recurse-submodules feature\n+\n+This test verifies the recurse-submodules feature correctly lists files from\n+submodules.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup directory structure and submodules' '\n+\techo a >a &&\n+\tmkdir b &&\n+\techo b >b/b &&\n+\tgit add a b &&\n+\tgit commit -m \"add a and b\" &&\n+\tgit init submodule &&\n+\techo c >submodule/c &&\n+\tgit -C submodule add c &&\n+\tgit -C submodule commit -m \"add c\" &&\n+\tgit submodule add ./submodule &&\n+\tgit commit -m \"added submodule\"\n+'\n+\n+test_expect_success 'ls-files correctly outputs files in submodule' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/c\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files does not output files not added to a repo' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/c\n+\tEOF\n+\n+\techo a >not_added &&\n+\techo b >b/not_added &&\n+\techo c >submodule/not_added &&\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files recurses more than 1 level' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/.gitmodules\n+\tsubmodule/c\n+\tsubmodule/subsub/d\n+\tEOF\n+\n+\tgit init submodule/subsub &&\n+\techo d >submodule/subsub/d &&\n+\tgit -C submodule/subsub add d &&\n+\tgit -C submodule/subsub commit -m \"add d\" &&\n+\tgit -C submodule submodule add ./subsub &&\n+\tgit -C submodule commit -m \"added subsub\" &&\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules does not support using path arguments' '\n+\ttest_must_fail git ls-files --recurse-submodules b 2>actual &&\n+\ttest_i18ngrep \"does not support path arguments\" actual\n+'\n+\n+test_expect_success '--recurse-submodules does not support --error-unmatch' '\n+\ttest_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&\n+\ttest_i18ngrep \"does not support --error-unmatch\" actual\n+'\n+\n+test_incompatible_with_recurse_submodules () {\n+\ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n+\t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n+\t\ttest_i18ngrep 'can only be used in --cached mode' actual\n+\t\"\n+}\n+\n+test_incompatible_with_recurse_submodules -v\n+test_incompatible_with_recurse_submodules -t\n+test_incompatible_with_recurse_submodules --deleted\n+test_incompatible_with_recurse_submodules --modified\n+test_incompatible_with_recurse_submodules --others\n+test_incompatible_with_recurse_submodules --stage\n+test_incompatible_with_recurse_submodules --killed\n+test_incompatible_with_recurse_submodules --unmerged\n+test_incompatible_with_recurse_submodules --eol\n+\n+test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301816","messageId":"CAKoko1pc7bB85cjho7mg0mDQrDjmfvpAVC8FS=VtQ+Dwk6eCYA@mail.gmail.com","threadId":"44052","inReplyTo":"1473726835-143289-1-git-send-email-bmwill@google.com","subject":"Re: [PATCH v2] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-13T03:35:57Z","receivedAt":"2016-09-13T03:36:04Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":">  static void write_name(const char *name)\n>  {\n>         /*\n> +        * NEEDSWORK: To make this thread-safe, full_name would have to be owned\n> +        * by the caller.\n> +        *\n> +        * full_name get reused across output lines to minimize the allocation\n> +        * churn.\n> +        */\n> +       static struct strbuf full_name = STRBUF_INIT;\n> +       if (output_path_prefix != '\\0') {\n\nIt was pointed out to me that this should be:\n    if (*output_path_prefix != '\\0') {\n\n\n> +               strbuf_reset(&full_name);\n> +               strbuf_addstr(&full_name, output_path_prefix);\n> +               strbuf_addstr(&full_name, name);\n> +               name = full_name.buf;\n> +       }\n"},{"id":"301850","messageId":"xmqqzinbvk15.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"1473726835-143289-1-git-send-email-bmwill@google.com","subject":"Re: [PATCH v2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T16:31:18Z","receivedAt":"2016-09-13T16:31:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Allow ls-files to recognize submodules in order to retrieve a list of\n> files from a repository's submodules.  This is done by forking off a\n> process to recursively call ls-files on all submodules. Also added an\n> output-path-prefix command in order to prepend paths to child processes.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n\n> @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)\n>  static void write_name(const char *name)\n>  {\n>  \t/*\n> +\t * NEEDSWORK: To make this thread-safe, full_name would have to be owned\n> +\t * by the caller.\n> +\t *\n> +\t * full_name get reused across output lines to minimize the allocation\n> +\t * churn.\n> +\t */\n> +\tstatic struct strbuf full_name = STRBUF_INIT;\n> +\tif (output_path_prefix != '\\0') {\n> +\t\tstrbuf_reset(&full_name);\n> +\t\tstrbuf_addstr(&full_name, output_path_prefix);\n> +\t\tstrbuf_addstr(&full_name, name);\n> +\t\tname = full_name.buf;\n> +\t}\n\nAt first glance it was surprising that no test caught this lack of\ndereference; the reason is because you initialize output_path_prefix\nto an empty string, not NULL, causing full_name.buf always used,\nwhich does not have an impact on the output.\n\nI think initializing it to NULL is a more typical way to say \"this\noption has not been given\", and if you took that route, the\ncondition would become\n\n\tif (output_path_prefix && *output_path_prefix) {\n        \t...\n\nIn any case, the fact that only this much change was required to add\noutput-path-prefix shows two good things: (1) the original code was\nalready well structured, funneling any pathname we need to emit\nthrough this single function so that we can do this kind of updates,\nand (2) the author of the patch was competent to spot this single\npoint that needs to be updated.\n\nNice.\n\n> +\tstatus = run_command(&cp);\n> +\tif (status)\n> +\t\texit(status);\n\nrun_command()'s return value comes from either start_command() or\nfinish_command().  These signal failure by returning a non-zero\nvalue, and in practice they are negative small integers.  Feeding\nnegative value to exit() is not quite kosher.  Perhaps exit(128)\nto mimick as if we called die() is better.\n\nIf your primary interest is to support the \"find in the working tree\nfiles that are tracked, recursively in submodules\" grep, I think\nthis \"when we hit a submodule, spawn a separate ls-files in there\"\nis sufficient and a solid base to build on it.\n\nOn the other hand, if you are more ambitious and \"grep\" is merely an\nexample of things that can be helped by having a list of paths\nacross module boundaries, we may want to \"libify\" ls-files in such a\nway that a single process can instantiate one or more instances of\n\"ls-files machinery\", that takes which repository to work in and\nother arguments that specifies which paths to report, and instead of\nalways showing the result to the standard output, makes repeated\ncalls to a callback function to report the discovered path and other\nattributes associated with the path that were asked for (the object\nname, values of tag_*, etc.), without spawning a separate \"ls-files\"\nprocess.\n\nThe latter would be a lot bigger task and I do not necessarily think\nit is needed, but that is one possible future direction to keep in\nmind.\n\nThanks, will queue with a minimum fix.\n"},{"id":"302017","messageId":"xmqqeg4knanq.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"xmqqzinbvk15.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:57:45Z","receivedAt":"2016-09-15T20:57:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thanks, will queue with a minimum fix.\n\nSo here are two squashable patches, one is the \"minimum\" one, the\nother is a bit more invasive one to use \"a pointer to an optional\nsetting is set to NULL\" convention.  I am undecided, and I'll stay\nto be without further comments from others, on the latter one.\n\nI understand that many internal changes in your work environment are\ntitled like \"DOing X\", but our convention around here is to label\nthem \"DO X\", as if you are giving an order to somebody else, either\ntelling the codebase \"to be like so\", or telling the patch-monkey\nmaintainer \"to make it so\".  So I'd retitle it\n\n\tls-files: optionally recurse into submodules\n\nor something like that.  It is an added advantage of being a lot\nmore descriptive than \"adding support\", which does not say what kind\nof support it is adding.\n"},{"id":"302018","messageId":"xmqqa8f8nan2.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"xmqqzinbvk15.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:58:09Z","receivedAt":"2016-09-15T20:58:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is an absolute mininum fix ;-)\n\n builtin/ls-files.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex c0bce00..6e78c71 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -78,7 +78,7 @@ static void write_name(const char *name)\n \t * churn.\n \t */\n \tstatic struct strbuf full_name = STRBUF_INIT;\n-\tif (output_path_prefix != '\\0') {\n+\tif (*output_path_prefix) {\n \t\tstrbuf_reset(&full_name);\n \t\tstrbuf_addstr(&full_name, output_path_prefix);\n \t\tstrbuf_addstr(&full_name, name);\n-- \n2.10.0-458-g97b4043\n\n"},{"id":"302019","messageId":"xmqq60pwnamj.fsf@gitster.mtv.corp.google.com","threadId":"44052","inReplyTo":"xmqqzinbvk15.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:58:28Z","receivedAt":"2016-09-15T20:58:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If we were to follow the convention to leave an optional string\nvariable to NULL, we'd need to do this on top.  I am not sure if it\nis a good change, though.\n---\n builtin/ls-files.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 6e78c71..687e475 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -29,7 +29,7 @@ static int show_valid_bit;\n static int line_terminator = '\\n';\n static int debug_mode;\n static int show_eol;\n-static const char *output_path_prefix = \"\";\n+static const char *output_path_prefix;\n static int recurse_submodules;\n \n static const char *prefix;\n@@ -78,7 +78,7 @@ static void write_name(const char *name)\n \t * churn.\n \t */\n \tstatic struct strbuf full_name = STRBUF_INIT;\n-\tif (*output_path_prefix) {\n+\tif (output_path_prefix && *output_path_prefix) {\n \t\tstrbuf_reset(&full_name);\n \t\tstrbuf_addstr(&full_name, output_path_prefix);\n \t\tstrbuf_addstr(&full_name, name);\n@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)\n \targv_array_push(&cp.args, \"ls-files\");\n \targv_array_push(&cp.args, \"--recurse-submodules\");\n \targv_array_pushf(&cp.args, \"--output-path-prefix=%s%s/\",\n-\t\t\t output_path_prefix, ce->name);\n+\t\t\t output_path_prefix ? output_path_prefix : \"\",\n+\t\t\t ce->name);\n \tcp.git_cmd = 1;\n \tcp.dir = ce->name;\n \tstatus = run_command(&cp);\n-- \n2.10.0-458-g97b4043\n\n"},{"id":"302020","messageId":"20160915205109.12240-1-gitster@pobox.com","threadId":"44052","inReplyTo":"xmqqzinbvk15.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:51:07Z","receivedAt":"2016-09-15T20:59:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thanks, will queue with a minimum fix.\n\nSo here are two squashable patches, one is the \"minimum\" one, the\nother is a bit more invasive one to use \"a pointer to an optional\nsetting is set to NULL\" convention.  I am undecided, and I'll stay\nto be without further comments from others, on the latter one.\n\nI understand that many internal changes in your work environment\ntitles their changes like \"DOing X\", but our convention around here\nis to label them \"DO X\", as if you are giving an order to somebody\nelse, either telling the codebase \"to be like so\", or telling the\npatch-monkey maintainer \"to make it so\".  So I'd retitle it\n\n\tls-files: optionally recurse into submodules\n\nor something like that.  It is an added advantage of being a lot\nmore descriptive than \"adding support\", which does not say what kind\nof support it is adding.\n"},{"id":"302021","messageId":"20160915205109.12240-3-gitster@pobox.com","threadId":"44052","inReplyTo":"20160915205109.12240-1-gitster@pobox.com","subject":"[PATCH 2/2] SQUASH??? Undecided","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:51:09Z","receivedAt":"2016-09-15T20:59:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If we were to follow the convention to leave an optional string\nvariable to NULL, we'd need to do this on top.  I am not sure if it\nis a good change, though.\n---\n builtin/ls-files.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 6e78c71..687e475 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -29,7 +29,7 @@ static int show_valid_bit;\n static int line_terminator = '\\n';\n static int debug_mode;\n static int show_eol;\n-static const char *output_path_prefix = \"\";\n+static const char *output_path_prefix;\n static int recurse_submodules;\n \n static const char *prefix;\n@@ -78,7 +78,7 @@ static void write_name(const char *name)\n \t * churn.\n \t */\n \tstatic struct strbuf full_name = STRBUF_INIT;\n-\tif (*output_path_prefix) {\n+\tif (output_path_prefix && *output_path_prefix) {\n \t\tstrbuf_reset(&full_name);\n \t\tstrbuf_addstr(&full_name, output_path_prefix);\n \t\tstrbuf_addstr(&full_name, name);\n@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)\n \targv_array_push(&cp.args, \"ls-files\");\n \targv_array_push(&cp.args, \"--recurse-submodules\");\n \targv_array_pushf(&cp.args, \"--output-path-prefix=%s%s/\",\n-\t\t\t output_path_prefix, ce->name);\n+\t\t\t output_path_prefix ? output_path_prefix : \"\",\n+\t\t\t ce->name);\n \tcp.git_cmd = 1;\n \tcp.dir = ce->name;\n \tstatus = run_command(&cp);\n-- \n2.10.0-458-g97b4043\n\n"},{"id":"302022","messageId":"20160915205109.12240-2-gitster@pobox.com","threadId":"44052","inReplyTo":"20160915205109.12240-1-gitster@pobox.com","subject":"[PATCH 1/2] SQUASH???","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T20:51:08Z","receivedAt":"2016-09-15T21:00:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"---\n builtin/ls-files.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex c0bce00..6e78c71 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -78,7 +78,7 @@ static void write_name(const char *name)\n \t * churn.\n \t */\n \tstatic struct strbuf full_name = STRBUF_INIT;\n-\tif (output_path_prefix != '\\0') {\n+\tif (*output_path_prefix) {\n \t\tstrbuf_reset(&full_name);\n \t\tstrbuf_addstr(&full_name, output_path_prefix);\n \t\tstrbuf_addstr(&full_name, name);\n-- \n2.10.0-458-g97b4043\n\n"},{"id":"302024","messageId":"CAGZ79kaCVZ-Z+XSYWK6YtkT8L=pDrDQE-pAyseHNf5w2NO5XMw@mail.gmail.com","threadId":"44052","inReplyTo":"20160915205109.12240-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] SQUASH??? Undecided","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-15T21:12:32Z","receivedAt":"2016-09-15T21:12:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+ cc Brandon\n\nOn Thu, Sep 15, 2016 at 1:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> If we were to follow the convention to leave an optional string\n> variable to NULL, we'd need to do this on top.  I am not sure if it\n> is a good change, though.\n\nI think it is a good change.\n\nThanks,\nStefan\n\n> ---\n>  builtin/ls-files.c | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 6e78c71..687e475 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -29,7 +29,7 @@ static int show_valid_bit;\n>  static int line_terminator = '\\n';\n>  static int debug_mode;\n>  static int show_eol;\n> -static const char *output_path_prefix = \"\";\n> +static const char *output_path_prefix;\n>  static int recurse_submodules;\n>\n>  static const char *prefix;\n> @@ -78,7 +78,7 @@ static void write_name(const char *name)\n>          * churn.\n>          */\n>         static struct strbuf full_name = STRBUF_INIT;\n> -       if (*output_path_prefix) {\n> +       if (output_path_prefix && *output_path_prefix) {\n>                 strbuf_reset(&full_name);\n>                 strbuf_addstr(&full_name, output_path_prefix);\n>                 strbuf_addstr(&full_name, name);\n> @@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)\n>         argv_array_push(&cp.args, \"ls-files\");\n>         argv_array_push(&cp.args, \"--recurse-submodules\");\n>         argv_array_pushf(&cp.args, \"--output-path-prefix=%s%s/\",\n> -                        output_path_prefix, ce->name);\n> +                        output_path_prefix ? output_path_prefix : \"\",\n> +                        ce->name);\n>         cp.git_cmd = 1;\n>         cp.dir = ce->name;\n>         status = run_command(&cp);\n> --\n> 2.10.0-458-g97b4043\n>\n"},{"id":"302028","messageId":"CAKoko1qj4FS=g-H0UzeTmKNoJQyxmf5bBsQJFhJjH1hYyFtQcQ@mail.gmail.com","threadId":"44052","inReplyTo":"CAGZ79kaCVZ-Z+XSYWK6YtkT8L=pDrDQE-pAyseHNf5w2NO5XMw@mail.gmail.com","subject":"Re: [PATCH 2/2] SQUASH??? Undecided","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-15T21:37:04Z","receivedAt":"2016-09-15T21:37:11Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Yeah if that is the convention then I have no problem with the change.\n\n-Brandon\n\nOn Thu, Sep 15, 2016 at 2:12 PM, Stefan Beller <sbeller@google.com> wrote:\n> + cc Brandon\n>\n> On Thu, Sep 15, 2016 at 1:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> If we were to follow the convention to leave an optional string\n>> variable to NULL, we'd need to do this on top.  I am not sure if it\n>> is a good change, though.\n>\n> I think it is a good change.\n>\n> Thanks,\n> Stefan\n>\n>> ---\n>>  builtin/ls-files.c | 7 ++++---\n>>  1 file changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n>> index 6e78c71..687e475 100644\n>> --- a/builtin/ls-files.c\n>> +++ b/builtin/ls-files.c\n>> @@ -29,7 +29,7 @@ static int show_valid_bit;\n>>  static int line_terminator = '\\n';\n>>  static int debug_mode;\n>>  static int show_eol;\n>> -static const char *output_path_prefix = \"\";\n>> +static const char *output_path_prefix;\n>>  static int recurse_submodules;\n>>\n>>  static const char *prefix;\n>> @@ -78,7 +78,7 @@ static void write_name(const char *name)\n>>          * churn.\n>>          */\n>>         static struct strbuf full_name = STRBUF_INIT;\n>> -       if (*output_path_prefix) {\n>> +       if (output_path_prefix && *output_path_prefix) {\n>>                 strbuf_reset(&full_name);\n>>                 strbuf_addstr(&full_name, output_path_prefix);\n>>                 strbuf_addstr(&full_name, name);\n>> @@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)\n>>         argv_array_push(&cp.args, \"ls-files\");\n>>         argv_array_push(&cp.args, \"--recurse-submodules\");\n>>         argv_array_pushf(&cp.args, \"--output-path-prefix=%s%s/\",\n>> -                        output_path_prefix, ce->name);\n>> +                        output_path_prefix ? output_path_prefix : \"\",\n>> +                        ce->name);\n>>         cp.git_cmd = 1;\n>>         cp.dir = ce->name;\n>>         status = run_command(&cp);\n>> --\n>> 2.10.0-458-g97b4043\n>>\n"}]}