{"thread":{"id":"45476","subject":"[PATCHv3 0/3] short status: improve reporting for submodule changes","startedAt":"2017-03-23T00:43:42Z","lastAt":"2017-03-30T18:28:58Z","messageCount":73,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"315027","messageId":"20170323004329.15892-1-sbeller@google.com","threadId":"45476","inReplyTo":null,"subject":"[PATCHv3 0/3] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T00:43:26Z","receivedAt":"2017-03-23T00:43:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This comes as a series; first I'd like to refactor is_submodule_modified\nto take advantage of the new porcelain=2 plumbing switch to check for changes\nin the submodule.\n\nOn top of the refactoring comes the actual change, which moved the\nrewriting of the submodule change indicator letter to the collection part.\n\nThanks,\nStefan\n\n\nStefan Beller (3):\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c, is_submodule_modified: stricter checking for submodules\n short status: improve reporting for submodule changes\n\n Documentation/git-status.txt | 9 +++++++\n submodule.c | 58 +++++++++++++++++++++-----------------------\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 24 ++++++++++++++++++\n wt-status.c | 13 ++++++++--\n 5 files changed, 84 insertions(+), 38 deletions(-)\n\n-- \n2.12.1.432.gfe308fe33c.dirty\n\n"},{"id":"315028","messageId":"20170323004329.15892-2-sbeller@google.com","threadId":"45476","inReplyTo":"20170323004329.15892-1-sbeller@google.com","subject":"[PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T00:43:27Z","receivedAt":"2017-03-23T00:43:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Migrate 'is_submodule_modified' to the new porcelain format of\ngit-status.\n\nAs the old porcelain only reported ' M' for submodules, no\nmatter what happened inside the submodule (untracked files,\nchanges to tracked files or move of HEAD), the new API\nproperly reports the different scenarios.\n\nIn a followup patch we will make use of these finer grained\nreporting for git-status.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 53 ++++++++++++++++++++++++-----------------------------\n 1 file changed, 24 insertions(+), 29 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3200b7bb2b..d355ddb46b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1041,17 +1041,9 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n-\tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {\n-\t\t\"status\",\n-\t\t\"--porcelain\",\n-\t\tNULL,\n-\t\tNULL,\n-\t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n-\tconst char *line, *next_line;\n \tconst char *git_dir;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n@@ -1066,42 +1058,45 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t}\n \tstrbuf_reset(&buf);\n \n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain=2\", NULL);\n \tif (ignore_untracked)\n-\t\targv[2] = \"-uno\";\n+\t\targv_array_push(&cp.args, \"-uno\");\n \n-\tcp.argv = argv;\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n+\t\tdie(\"Could not run 'git status --porcelain=2' in submodule %s\", path);\n \n-\tlen = strbuf_read(&buf, cp.out, 1024);\n-\tline = buf.buf;\n-\twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?')) {\n+\twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n+\t\t/* regular untracked files */\n+\t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\t\t\tbreak;\n-\t\t} else {\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\tif (ignore_untracked ||\n-\t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n-\t\t\t\tbreak;\n+\n+\t\t/* regular unmerged and renamed files */\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t buf.buf[0] == '1' ||\n+\t\t buf.buf[0] == '2') {\n+\t\t\tif (buf.buf[5] == 'S') {\n+\t\t\t\t/* nested submodule handling */\n+\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t\t\tif (buf.buf[8] == 'U')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\t\t\t} else\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \t\t}\n-\t\tnext_line = strchr(line, '\\n');\n-\t\tif (!next_line)\n-\t\t\tbreak;\n-\t\tnext_line++;\n-\t\tlen -= (next_line - line);\n-\t\tline = next_line;\n+\n+\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&\n+\t\t dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\tbreak;\n \t}\n \tclose(cp.out);\n \n \tif (finish_command(&cp))\n-\t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n+\t\tdie(\"'git status --porcelain=2' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n \treturn dirty_submodule;\n-- \n2.12.1.432.gfe308fe33c.dirty\n\n"},{"id":"315029","messageId":"20170323004329.15892-3-sbeller@google.com","threadId":"45476","inReplyTo":"20170323004329.15892-1-sbeller@google.com","subject":"[PATCH 2/3] submodule.c, is_submodule_modified: stricter checking for submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T00:43:28Z","receivedAt":"2017-03-23T00:43:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By having a stricter check in the superproject we catch errors earlier,\ninstead of spawning a child process to tell us.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d355ddb46b..66335159ae 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1050,11 +1050,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tgit_dir = read_gitfile(buf.buf);\n \tif (!git_dir)\n \t\tgit_dir = buf.buf;\n-\tif (!is_directory(git_dir)) {\n+\tif (!is_git_directory(git_dir)) {\n+\t\tif (is_directory(git_dir))\n+\t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n \t\tstrbuf_release(&buf);\n \t\t/* The submodule is not checked out, so it is not modified */\n \t\treturn 0;\n-\n \t}\n \tstrbuf_reset(&buf);\n \n-- \n2.12.1.432.gfe308fe33c.dirty\n\n"},{"id":"315030","messageId":"20170323004329.15892-4-sbeller@google.com","threadId":"45476","inReplyTo":"20170323004329.15892-1-sbeller@google.com","subject":"[PATCH 3/3] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T00:43:29Z","receivedAt":"2017-03-23T00:43:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If I add an untracked file to a submodule or modify a tracked file,\ncurrently \"git status --short\" treats the change in the same way as\nchanges to the current HEAD of the submodule:\n\n $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit\n $ echo hello >gerrit/plugins/replication/stray-file\n $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap\n $ git -C gerrit status --short\n M plugins/replication\n\nThis is by analogy with ordinary files, where \"M\" represents a change\nthat has not been added yet to the index. But this change cannot be\nadded to the index without entering the submodule, \"git add\"-ing it,\nand running \"git commit\", so the analogy is counterproductive.\n\nIntroduce new status letters \" ?\" and \" m\" for this. These are similar\nto the existing \"??\" and \" M\" but mean that the submodule (not the\nparent project) has new untracked files and modified files, respectively.\nThe user can use \"git add\" and \"git commit\" from within the submodule to\nadd them.\n\nChanges to the submodule's HEAD commit can be recorded in the index with\na plain \"git add -u\" and are shown with \" M\", like today.\n\nTo avoid excessive clutter, show at most one of \" ?\", \" m\", and \" M\" for\nthe submodule. They represent increasing levels of change --- the last\none that applies is shown (e.g., \" m\" if there are both modified files\nand untracked files in the submodule, or \" M\" if the submodule's HEAD\nhas been modified and it has untracked files).\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which shares code with \"status --short\". We only change\n\"git status --short\".\n\nNon-short \"git status\" and \"git status --porcelain=2\" already handle\nthese cases by showing more detail:\n\n $ git -C gerrit status --porcelain=2\n 1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication\n $ git -C gerrit status\n[...]\n modified: plugins/replication (modified content, untracked content)\n\nScripts caring about these distinctions should use --porcelain=2.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n Documentation/git-status.txt | 9 +++++++++\n t/t3600-rm.sh | 18 +++++++++++++-----\n t/t7506-status-submodule.sh | 24 ++++++++++++++++++++++++\n wt-status.c | 13 +++++++++++--\n 4 files changed, 57 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..01b457c322 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,13 @@ in which case `XY` are `!!`.\n ! ! ignored\n -------------------------------------------------\n \n+Submodules have more state and instead report\n+\t\tM the submodule has a different HEAD than\n+\t\t recorded in the index\n+\t\tm the submodule has modified content\n+\t\t? the submodule has untracked files\n+\n+\n If -b is used the short-format status is preceded by a line\n \n ## branchname tracking info\n@@ -210,6 +217,8 @@ field from the first filename). Third, filenames containing special\n characters are not specially formatted; no quoting or\n backslash-escaping is performed.\n \n+Any submodule changes are reported as modified `M` instead of `m` or single `?`.\n+\n Porcelain Format Version 2\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5aa6db584c..b58793448b 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -268,6 +268,14 @@ cat >expect.modified <expect.modified_inside <expect.modified_untracked <expect.cached <actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex d31b34da83..98dff01947 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -50,6 +50,15 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with modified file in submodule (short)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n@@ -64,6 +73,14 @@ test_expect_success 'status with added file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with added file in submodule (short)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n@@ -83,6 +100,13 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with untracked file in submodule (short)' '\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t ? sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added and untracked file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \techo \"content\" >sub/new-file &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 308cf3779e..9909fd0e57 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -431,10 +431,19 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t}\n \t\tif (!d->worktree_status)\n \t\t\td->worktree_status = p->status;\n-\t\td->dirty_submodule = p->two->dirty_submodule;\n-\t\tif (S_ISGITLINK(p->two->mode))\n+\t\tif (S_ISGITLINK(p->two->mode)) {\n+\t\t\td->dirty_submodule = p->two->dirty_submodule;\n \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n \t\t\t\t\t\t\t &p->two->oid);\n+\t\t\tif (s->status_format == STATUS_FORMAT_SHORT) {\n+\t\t\t\tif (d->new_submodule_commits)\n+\t\t\t\t\td->worktree_status = 'M';\n+\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\t\t\t\td->worktree_status = 'm';\n+\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\t\td->worktree_status = '?';\n+\t\t\t}\n+\t\t}\n \n \t\tswitch (p->status) {\n \t\tcase DIFF_STATUS_ADDED:\n-- \n2.12.1.432.gfe308fe33c.dirty\n\n"},{"id":"315032","messageId":"20170323005341.GH26108@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170323004329.15892-2-sbeller@google.com","subject":"Re: [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-23T00:53:41Z","receivedAt":"2017-03-23T00:54:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Migrate 'is_submodule_modified' to the new porcelain format of\n> git-status.\n>\n> As the old porcelain only reported ' M' for submodules, no\n> matter what happened inside the submodule (untracked files,\n> changes to tracked files or move of HEAD), the new API\n> properly reports the different scenarios.\n[...]\n> submodule.c | 53 ++++++++++++++++++++++++-----------------------------\n> 1 file changed, 24 insertions(+), 29 deletions(-)\n\nNeat. Is this something that could be covered in tests, or should I\nbe patient and rely on patch 3/3 for that?\n\nI think this would be easier to understand if it were two patches: one\nthat switched to --porcelain=2 with no change in behavior, and another\nthat took advantage of --porcelain=2 to return richer information. As\nis, I had trouble verifying that this isn't going to break anything\n--- there's not enough local information here and in submodule.h to\ntell what callers may rely on and I didn't audit callers.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"315033","messageId":"20170323005437.GI26108@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170323004329.15892-3-sbeller@google.com","subject":"Re: [PATCH 2/3] submodule.c, is_submodule_modified: stricter checking for submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-23T00:54:37Z","receivedAt":"2017-03-23T00:54:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> By having a stricter check in the superproject we catch errors earlier,\n> instead of spawning a child process to tell us.\n\nPlus the error message is clearer.\n\n> ---\n> submodule.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"315034","messageId":"20170323010605.GJ26108@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170323004329.15892-4-sbeller@google.com","subject":"Re: [PATCH 3/3] short status: improve reporting for submodule changes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-23T01:06:05Z","receivedAt":"2017-03-23T01:06:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Documentation/git-status.txt | 9 +++++++++\n> t/t3600-rm.sh | 18 +++++++++++++-----\n> t/t7506-status-submodule.sh | 24 ++++++++++++++++++++++++\n> wt-status.c | 13 +++++++++++--\n> 4 files changed, 57 insertions(+), 7 deletions(-)\n\nVery nice.\n\n[...]\n> --- a/Documentation/git-status.txt\n> +++ b/Documentation/git-status.txt\n> @@ -181,6 +181,13 @@ in which case `XY` are `!!`.\n\nThe documentation is clear.\n\n[...]\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n\nThe updated behavior shown in this test makes sense.\n\n[...]\n> --- a/t/t7506-status-submodule.sh\n> +++ b/t/t7506-status-submodule.sh\n\nThis test has good coverage and describes a good behavior.\n\n[...]\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -431,10 +431,19 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n> \t\t}\n> \t\tif (!d->worktree_status)\n> \t\t\td->worktree_status = p->status;\n> -\t\td->dirty_submodule = p->two->dirty_submodule;\n> -\t\tif (S_ISGITLINK(p->two->mode))\n> +\t\tif (S_ISGITLINK(p->two->mode)) {\n> +\t\t\td->dirty_submodule = p->two->dirty_submodule;\n> \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n> \t\t\t\t\t\t\t &p->two->oid);\n> +\t\t\tif (s->status_format == STATUS_FORMAT_SHORT) {\n> +\t\t\t\tif (d->new_submodule_commits)\n> +\t\t\t\t\td->worktree_status = 'M';\n\nCan these be new DIFF_STATUS_* letters in diff.h?\n\nI hadn't realized before that the worktree_status usually is taken\nverbatim from diff_filepair. Now I understand better what you were\ntalking about offline earlier today (sorry for the confusion).\n\nWe probably don't want the diff machinery to have to be aware of the\nvarious status_format modes, so doing this here seems sensible to me.\nUnfortunately it's also a reminder that \"git diff --name-status\" and\n--diff-filter could benefit from a similar change. (Unfortunate\nbecause that's a harder change to make without breaking scripts.)\n\n> +\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n> +\t\t\t\t\td->worktree_status = 'm';\n> +\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n> +\t\t\t\t\td->worktree_status = '?';\n> +\t\t\t}\n> +\t\t}\n\nGiven the above, this implementation looks good. So for what it's worth,\nthis patch is\nReviewed-by: Jonathan Nieder \n\nPatch 1/3 is the one that gives me pause, since I hadn't been able to\nchase down all callers. Would it be feasible to split that into two:\none patch to switch to --porcelain=2 but not change behavior, and a\nseparate patch to improve what is stored in the dirty_submodule flags?\n\nThanks,\nJonathan\n"},{"id":"315043","messageId":"xmqqo9wsfrsr.fsf@gitster.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170323005341.GH26108@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T06:09:08Z","receivedAt":"2017-03-23T08:05:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Stefan Beller wrote:\n>\n>> Migrate 'is_submodule_modified' to the new porcelain format of\n>> git-status.\n>>\n>> As the old porcelain only reported ' M' for submodules, no\n>> matter what happened inside the submodule (untracked files,\n>> changes to tracked files or move of HEAD), the new API\n>> properly reports the different scenarios.\n> [...]\n>> submodule.c | 53 ++++++++++++++++++++++++-----------------------------\n>> 1 file changed, 24 insertions(+), 29 deletions(-)\n>\n> Neat. Is this something that could be covered in tests, or should I\n> be patient and rely on patch 3/3 for that?\n>\n> I think this would be easier to understand if it were two patches: one\n> that switched to --porcelain=2 with no change in behavior, and another\n> that took advantage of --porcelain=2 to return richer information. \n\nThat sounds like a sensible organization.\n\n> As is, I had trouble verifying that this isn't going to break\n> anything --- there's not enough local information here and in\n> submodule.h to tell what callers may rely on and I didn't audit\n> callers.\n"},{"id":"315145","messageId":"CAGZ79kZriqCE4-9bVQTNGXEZsJi-Z+ryp3QRu-MC8Sn1kaGSdg@mail.gmail.com","threadId":"45476","inReplyTo":"xmqqo9wsfrsr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T18:47:19Z","receivedAt":"2017-03-23T18:47:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 22, 2017 at 11:09 PM, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n>> Stefan Beller wrote:\n>>\n>>> Migrate 'is_submodule_modified' to the new porcelain format of\n>>> git-status.\n>>>\n>>> As the old porcelain only reported ' M' for submodules, no\n>>> matter what happened inside the submodule (untracked files,\n>>> changes to tracked files or move of HEAD), the new API\n>>> properly reports the different scenarios.\n>> [...]\n>>> submodule.c | 53 ++++++++++++++++++++++++-----------------------------\n>>> 1 file changed, 24 insertions(+), 29 deletions(-)\n>>\n>> Neat. Is this something that could be covered in tests, or should I\n>> be patient and rely on patch 3/3 for that?\n\nI am not sure how to cover it in tests properly, as we do not expose this\nfunction to the outside directly.\n\nThis function is used in only one place (diff-lib.c, in\nmatch_stat_with_submodule,\nwhich itself is used in run_diff_files and get_stat_data), which is deep down\nin the diff library.\n\n>> I think this would be easier to understand if it were two patches: one\n>> that switched to --porcelain=2 with no change in behavior,\n\nI don't think so as it would double the code to review.\nI'll see if I can present this conversion in an easier way.\n\n> That sounds like a sensible organization.\n\nok.\n\nThanks,\nStefan\n"},{"id":"315180","messageId":"20170323210924.26818-1-sbeller@google.com","threadId":"45476","inReplyTo":"20170323004329.15892-1-sbeller@google.com","subject":"[PATCH 0/8] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:16Z","receivedAt":"2017-03-23T21:09:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v4:\n* broken down in a lot of tiny patches.\n\nJonathan wrote:\n> Patch 1/3 is the one that gives me pause, since I hadn't been able to\n> chase down all callers. Would it be feasible to split that into two:\n> one patch to switch to --porcelain=2 but not change behavior, and a\n> separate patch to improve what is stored in the dirty_submodule flags?\n\nThis part is in the latest patch now.\n\nThanks,\nStefan\n\n\nv3:\nThis comes as a series; first I'd like to refactor is_submodule_modified\nto take advantage of the new porcelain=2 plumbing switch to check for changes\nin the submodule.\n\nOn top of the refactoring comes the actual change, which moved the\nrewriting of the submodule change indicator letter to the collection part.\n\nThanks,\nStefan\n\nStefan Beller (8):\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c: use argv_array in is_submodule_modified\n submodule.c: convert is_submodule_modified to use\n strbuf_getwholeline_fd\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c: factor out early loop termination in\n is_submodule_modified\n submodule.c: stricter checking for submodules in is_submodule_modified\n short status: improve reporting for submodule changes\n submodule.c: correctly handle nested submodules in\n is_submodule_modified\n\n Documentation/git-status.txt | 9 +++++++\n submodule.c | 56 ++++++++++++++++++++-----------------------\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 13 ++++++++--\n 5 files changed, 116 insertions(+), 37 deletions(-)\n\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315181","messageId":"20170323210924.26818-4-sbeller@google.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"[PATCH 3/8] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:19Z","receivedAt":"2017-03-23T21:09:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of implementing line reading yet again, make use of our beautiful\nlibrary functions.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 14 ++------------\n 1 file changed, 2 insertions(+), 12 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex dc61ca7103..f01e84faff 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1041,11 +1041,9 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n-\tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n-\tconst char *line, *next_line;\n \tconst char *git_dir;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n@@ -1072,10 +1070,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tif (start_command(&cp))\n \t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n \n-\tlen = strbuf_read(&buf, cp.out, 1024);\n-\tline = buf.buf;\n-\twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?')) {\n+\twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n+\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n \t\t\t\tbreak;\n@@ -1085,12 +1081,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n \t\t\t\tbreak;\n \t\t}\n-\t\tnext_line = strchr(line, '\\n');\n-\t\tif (!next_line)\n-\t\t\tbreak;\n-\t\tnext_line++;\n-\t\tlen -= (next_line - line);\n-\t\tline = next_line;\n \t}\n \tclose(cp.out);\n \n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315182","messageId":"20170323210924.26818-2-sbeller@google.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"[PATCH 1/8] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:17Z","receivedAt":"2017-03-23T21:09:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Migrate 'is_submodule_modified' to the new porcelain format of\ngit-status.\n\nAs the old porcelain only reported ' M' for submodules, no\nmatter what happened inside the submodule (untracked files,\nchanges to tracked files or move of HEAD), the new API\nproperly reports the different scenarios.\n\nIn a followup patch we will make use of these finer grained\nreporting for git-status.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 53 ++++++++++++++++++++++++-----------------------------\n 1 file changed, 24 insertions(+), 29 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3200b7bb2b..d355ddb46b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1041,17 +1041,9 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n-\tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {\n-\t\t\"status\",\n-\t\t\"--porcelain\",\n-\t\tNULL,\n-\t\tNULL,\n-\t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n-\tconst char *line, *next_line;\n \tconst char *git_dir;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n@@ -1066,42 +1058,45 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t}\n \tstrbuf_reset(&buf);\n \n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain=2\", NULL);\n \tif (ignore_untracked)\n-\t\targv[2] = \"-uno\";\n+\t\targv_array_push(&cp.args, \"-uno\");\n \n-\tcp.argv = argv;\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n+\t\tdie(\"Could not run 'git status --porcelain=2' in submodule %s\", path);\n \n-\tlen = strbuf_read(&buf, cp.out, 1024);\n-\tline = buf.buf;\n-\twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?')) {\n+\twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n+\t\t/* regular untracked files */\n+\t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\t\t\tbreak;\n-\t\t} else {\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\tif (ignore_untracked ||\n-\t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n-\t\t\t\tbreak;\n+\n+\t\t/* regular unmerged and renamed files */\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t buf.buf[0] == '1' ||\n+\t\t buf.buf[0] == '2') {\n+\t\t\tif (buf.buf[5] == 'S') {\n+\t\t\t\t/* nested submodule handling */\n+\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t\t\tif (buf.buf[8] == 'U')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\t\t\t} else\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \t\t}\n-\t\tnext_line = strchr(line, '\\n');\n-\t\tif (!next_line)\n-\t\t\tbreak;\n-\t\tnext_line++;\n-\t\tlen -= (next_line - line);\n-\t\tline = next_line;\n+\n+\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&\n+\t\t dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\tbreak;\n \t}\n \tclose(cp.out);\n \n \tif (finish_command(&cp))\n-\t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n+\t\tdie(\"'git status --porcelain=2' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n \treturn dirty_submodule;\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315183","messageId":"20170323210924.26818-6-sbeller@google.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"[PATCH 5/8] submodule.c: factor out early loop termination in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:21Z","receivedAt":"2017-03-23T21:09:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes it easier for a follow up patch.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex da1db90dda..93d6f08b50 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1072,16 +1072,14 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \n \twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n \t\t/* regular untracked files */\n-\t\tif (buf.buf[0] == '?') {\n+\t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\t\t\tbreak;\n-\t\t} else {\n+\t\telse\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\tif (ignore_untracked ||\n-\t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n-\t\t\t\tbreak;\n-\t\t}\n+\n+\t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n+\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))\n+\t\t\tbreak;\n \t}\n \tclose(cp.out);\n \n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315184","messageId":"20170323210924.26818-3-sbeller@google.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"[PATCH 2/8] submodule.c: use argv_array in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:18Z","receivedAt":"2017-03-23T21:09:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"struct argv_array is easier to use and maintain\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 43 +++++++++++++++++++++----------------------\n 1 file changed, 21 insertions(+), 22 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d355ddb46b..dc61ca7103 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1041,9 +1041,11 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n+\tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n+\tconst char *line, *next_line;\n \tconst char *git_dir;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n@@ -1068,35 +1070,32 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain=2' in submodule %s\", path);\n+\t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n \n-\twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n-\t\t/* regular untracked files */\n-\t\tif (buf.buf[0] == '?')\n+\tlen = strbuf_read(&buf, cp.out, 1024);\n+\tline = buf.buf;\n+\twhile (len > 2) {\n+\t\tif ((line[0] == '?') && (line[1] == '?')) {\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\n-\t\t/* regular unmerged and renamed files */\n-\t\tif (buf.buf[0] == 'u' ||\n-\t\t buf.buf[0] == '1' ||\n-\t\t buf.buf[0] == '2') {\n-\t\t\tif (buf.buf[5] == 'S') {\n-\t\t\t\t/* nested submodule handling */\n-\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n-\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\t\tif (buf.buf[8] == 'U')\n-\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\t} else\n-\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t}\n-\n-\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&\n-\t\t dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\t\t\tbreak;\n+\t\t} else {\n+\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t\tif (ignore_untracked ||\n+\t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n \t\t\t\tbreak;\n+\t\t}\n+\t\tnext_line = strchr(line, '\\n');\n+\t\tif (!next_line)\n+\t\t\tbreak;\n+\t\tnext_line++;\n+\t\tlen -= (next_line - line);\n+\t\tline = next_line;\n \t}\n \tclose(cp.out);\n \n \tif (finish_command(&cp))\n-\t\tdie(\"'git status --porcelain=2' failed in submodule %s\", path);\n+\t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n \treturn dirty_submodule;\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315185","messageId":"20170323210924.26818-5-sbeller@google.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"[PATCH 4/8] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:20Z","receivedAt":"2017-03-23T21:09:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Migrate 'is_submodule_modified' to the new porcelain format of\ngit-status. This conversion attempts to convert faithfully, i.e.\nthe behavior ought to be exactly the same.\n\nAs the output in the parsing only distinguishes between untracked files\nand the rest, this is easy to port to the new format, as we only\nneed to identify untracked files and the rest is handled in the \"else\"\ncase.\n\nuntracked files are indicated by only a single question mark instead of\ntwo question marks, so the conversion is easy.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex f01e84faff..da1db90dda 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1068,10 +1068,11 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n+\t\tdie(\"Could not run 'git status --porcelain=2' in submodule %s\", path);\n \n \twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n-\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {\n+\t\t/* regular untracked files */\n+\t\tif (buf.buf[0] == '?') {\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n \t\t\t\tbreak;\n@@ -1085,7 +1086,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tclose(cp.out);\n \n \tif (finish_command(&cp))\n-\t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n+\t\tdie(\"'git status --porcelain=2' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n \treturn dirty_submodule;\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315186","messageId":"20170323210924.26818-9-sbeller@google.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"[PATCH 8/8] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:24Z","receivedAt":"2017-03-23T21:09:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n submodule.c | 16 ++++++++++++++--\n t/t3600-rm.sh | 2 +-\n t/t7506-status-submodule.sh | 2 +-\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex e06e52b993..0f477f3a4e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1075,8 +1075,20 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t/* regular untracked files */\n \t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\telse\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\n+\t\t/* regular unmerged and renamed files */\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t buf.buf[0] == '1' ||\n+\t\t buf.buf[0] == '2') {\n+\t\t\tif (buf.buf[5] == 'S') {\n+\t\t\t\t/* nested submodule handling */\n+\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t\t\tif (buf.buf[8] == 'U')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\t\t\t} else\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t}\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n \t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex a6e5c5bd56..b58793448b 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified_inside actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex ad46384064..e3cdcede72 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -324,7 +324,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain)'\n test_expect_success 'status with untracked file in nested submodule (short)' '\n \tgit -C super status --short >output &&\n \tdiff output - <<-\\EOF\n-\t m sub1\n+\t ? sub1\n \tEOF\n '\n \n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315187","messageId":"20170323210924.26818-7-sbeller@google.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"[PATCH 6/8] submodule.c: stricter checking for submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:22Z","receivedAt":"2017-03-23T21:09:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By having a stricter check in the superproject we catch errors earlier,\ninstead of spawning a child process to tell us.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 93d6f08b50..e06e52b993 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1050,11 +1050,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tgit_dir = read_gitfile(buf.buf);\n \tif (!git_dir)\n \t\tgit_dir = buf.buf;\n-\tif (!is_directory(git_dir)) {\n+\tif (!is_git_directory(git_dir)) {\n+\t\tif (is_directory(git_dir))\n+\t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n \t\tstrbuf_release(&buf);\n \t\t/* The submodule is not checked out, so it is not modified */\n \t\treturn 0;\n-\n \t}\n \tstrbuf_reset(&buf);\n \n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315188","messageId":"20170323210924.26818-8-sbeller@google.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"[PATCH 7/8] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:09:23Z","receivedAt":"2017-03-23T21:09:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If I add an untracked file to a submodule or modify a tracked file,\ncurrently \"git status --short\" treats the change in the same way as\nchanges to the current HEAD of the submodule:\n\n $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit\n $ echo hello >gerrit/plugins/replication/stray-file\n $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap\n $ git -C gerrit status --short\n M plugins/replication\n\nThis is by analogy with ordinary files, where \"M\" represents a change\nthat has not been added yet to the index. But this change cannot be\nadded to the index without entering the submodule, \"git add\"-ing it,\nand running \"git commit\", so the analogy is counterproductive.\n\nIntroduce new status letters \" ?\" and \" m\" for this. These are similar\nto the existing \"??\" and \" M\" but mean that the submodule (not the\nparent project) has new untracked files and modified files, respectively.\nThe user can use \"git add\" and \"git commit\" from within the submodule to\nadd them.\n\nChanges to the submodule's HEAD commit can be recorded in the index with\na plain \"git add -u\" and are shown with \" M\", like today.\n\nTo avoid excessive clutter, show at most one of \" ?\", \" m\", and \" M\" for\nthe submodule. They represent increasing levels of change --- the last\none that applies is shown (e.g., \" m\" if there are both modified files\nand untracked files in the submodule, or \" M\" if the submodule's HEAD\nhas been modified and it has untracked files).\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which shares code with \"status --short\". We only change\n\"git status --short\".\n\nNon-short \"git status\" and \"git status --porcelain=2\" already handle\nthese cases by showing more detail:\n\n $ git -C gerrit status --porcelain=2\n 1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication\n $ git -C gerrit status\n[...]\n modified: plugins/replication (modified content, untracked content)\n\nScripts caring about these distinctions should use --porcelain=2.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n Documentation/git-status.txt | 9 +++++++\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 13 ++++++++--\n 4 files changed, 90 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..01b457c322 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,13 @@ in which case `XY` are `!!`.\n ! ! ignored\n -------------------------------------------------\n \n+Submodules have more state and instead report\n+\t\tM the submodule has a different HEAD than\n+\t\t recorded in the index\n+\t\tm the submodule has modified content\n+\t\t? the submodule has untracked files\n+\n+\n If -b is used the short-format status is preceded by a line\n \n ## branchname tracking info\n@@ -210,6 +217,8 @@ field from the first filename). Third, filenames containing special\n characters are not specially formatted; no quoting or\n backslash-escaping is performed.\n \n+Any submodule changes are reported as modified `M` instead of `m` or single `?`.\n+\n Porcelain Format Version 2\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5aa6db584c..a6e5c5bd56 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -268,6 +268,14 @@ cat >expect.modified <expect.modified_inside <expect.modified_untracked <expect.cached <actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex d31b34da83..ad46384064 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -50,6 +50,15 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with modified file in submodule (short)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n@@ -64,6 +73,14 @@ test_expect_success 'status with added file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with added file in submodule (short)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n@@ -83,6 +100,13 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with untracked file in submodule (short)' '\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t ? sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added and untracked file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \techo \"content\" >sub/new-file &&\n@@ -271,4 +295,37 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \ttest_cmp diff_submodule_actual diff_submodule_expect\n '\n \n+test_expect_success 'setup superproject with untracked file in nested submodule' '\n+\t(\n+\t\tcd super &&\n+\t\tgit clean -dfx &&\n+\t\trm .gitmodules &&\n+\t\tgit submodule add -f ./sub1 &&\n+\t\tgit submodule add -f ./sub2 &&\n+\t\tgit commit -a -m \"messy merge in superproject\" &&\n+\t\t(\n+\t\t\tcd sub1 &&\n+\t\t\tgit submodule add ../sub2 &&\n+\t\t\tgit commit -a -m \"add sub2 to sub1\"\n+\t\t) &&\n+\t\tgit add sub1 &&\n+\t\tgit commit -a -m \"update sub1 to contain nested sub\"\n+\t) &&\n+\techo untracked >super/sub1/sub2/untracked\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\tEOF\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 308cf3779e..9909fd0e57 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -431,10 +431,19 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t}\n \t\tif (!d->worktree_status)\n \t\t\td->worktree_status = p->status;\n-\t\td->dirty_submodule = p->two->dirty_submodule;\n-\t\tif (S_ISGITLINK(p->two->mode))\n+\t\tif (S_ISGITLINK(p->two->mode)) {\n+\t\t\td->dirty_submodule = p->two->dirty_submodule;\n \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n \t\t\t\t\t\t\t &p->two->oid);\n+\t\t\tif (s->status_format == STATUS_FORMAT_SHORT) {\n+\t\t\t\tif (d->new_submodule_commits)\n+\t\t\t\t\td->worktree_status = 'M';\n+\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\t\t\t\td->worktree_status = 'm';\n+\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\t\td->worktree_status = '?';\n+\t\t\t}\n+\t\t}\n \n \t\tswitch (p->status) {\n \t\tcase DIFF_STATUS_ADDED:\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315189","messageId":"CAGZ79kZP6JhgcFQ5+Ytc6LexpA7C4EwR-7C0QZkiWhfrRCpd6g@mail.gmail.com","threadId":"45476","inReplyTo":"20170323210924.26818-1-sbeller@google.com","subject":"Re: [PATCH 0/8] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T21:11:14Z","receivedAt":"2017-03-23T21:11:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 23, 2017 at 2:09 PM, Stefan Beller wrote:\n> v4:\n> * broken down in a lot of tiny patches.\n>\n\nexcept for the first 2 patches, which are a rebase error :(\n\nPlease ignore.\n"},{"id":"315199","messageId":"20170323223338.32274-1-sbeller@google.com","threadId":"45476","inReplyTo":"CAGZ79kZP6JhgcFQ5+Ytc6LexpA7C4EwR-7C0QZkiWhfrRCpd6g@mail.gmail.com","subject":"[PATCH v5 0/7] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T22:33:31Z","receivedAt":"2017-03-23T22:34:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v5:\n* fixed rebase error in the first 2 patches\n* the last 3 patches introduce behavior change outside the scope of is_modified_submodule\n whereas the first 4 ought to just be local changes.\n \nThanks,\nStefan\n\nv4:\n* broken down in a lot of tiny patches.\n\nJonathan wrote:\n> Patch 1/3 is the one that gives me pause, since I hadn't been able to\n> chase down all callers. Would it be feasible to split that into two:\n> one patch to switch to --porcelain=2 but not change behavior, and a\n> separate patch to improve what is stored in the dirty_submodule flags?\n\nThis part is in the latest patch now.\n\nThanks,\nStefan\n\n\nv3:\nThis comes as a series; first I'd like to refactor is_submodule_modified\nto take advantage of the new porcelain=2 plumbing switch to check for changes\nin the submodule.\n\nOn top of the refactoring comes the actual change, which moved the\nrewriting of the submodule change indicator letter to the collection part.\n\nThanks,\nStefan\n\nStefan Beller (8):\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c: use argv_array in is_submodule_modified\n submodule.c: convert is_submodule_modified to use\n strbuf_getwholeline_fd\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c: factor out early loop termination in\n is_submodule_modified\n submodule.c: stricter checking for submodules in is_submodule_modified\n short status: improve reporting for submodule changes\n submodule.c: correctly handle nested submodules in\n is_submodule_modified\n\n Documentation/git-status.txt | 9 +++++++\n submodule.c | 56 ++++++++++++++++++++-----------------------\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 13 ++++++++--\n 5 files changed, 116 insertions(+), 37 deletions(-)\n\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315200","messageId":"20170323223338.32274-2-sbeller@google.com","threadId":"45476","inReplyTo":"20170323223338.32274-1-sbeller@google.com","subject":"[PATCH 1/7] submodule.c: use argv_array in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T22:33:32Z","receivedAt":"2017-03-23T22:34:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"struct argv_array is easier to use and maintain\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3200b7bb2b..2c667ac95a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n \tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {\n-\t\t\"status\",\n-\t\t\"--porcelain\",\n-\t\tNULL,\n-\t\tNULL,\n-\t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n \tconst char *line, *next_line;\n@@ -1066,10 +1060,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t}\n \tstrbuf_reset(&buf);\n \n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", NULL);\n \tif (ignore_untracked)\n-\t\targv[2] = \"-uno\";\n+\t\targv_array_push(&cp.args, \"-uno\");\n \n-\tcp.argv = argv;\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315201","messageId":"20170323223338.32274-5-sbeller@google.com","threadId":"45476","inReplyTo":"20170323223338.32274-1-sbeller@google.com","subject":"[PATCH 4/7] submodule.c: factor out early loop termination in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T22:33:35Z","receivedAt":"2017-03-23T22:34:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes it easier for a follow up patch.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex da1db90dda..93d6f08b50 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1072,16 +1072,14 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \n \twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n \t\t/* regular untracked files */\n-\t\tif (buf.buf[0] == '?') {\n+\t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\t\t\tbreak;\n-\t\t} else {\n+\t\telse\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\tif (ignore_untracked ||\n-\t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n-\t\t\t\tbreak;\n-\t\t}\n+\n+\t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n+\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))\n+\t\t\tbreak;\n \t}\n \tclose(cp.out);\n \n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315202","messageId":"20170323223338.32274-6-sbeller@google.com","threadId":"45476","inReplyTo":"20170323223338.32274-1-sbeller@google.com","subject":"[PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T22:33:36Z","receivedAt":"2017-03-23T22:34:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By having a stricter check in the superproject we catch errors earlier,\ninstead of spawning a child process to tell us.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 93d6f08b50..e06e52b993 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1050,11 +1050,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tgit_dir = read_gitfile(buf.buf);\n \tif (!git_dir)\n \t\tgit_dir = buf.buf;\n-\tif (!is_directory(git_dir)) {\n+\tif (!is_git_directory(git_dir)) {\n+\t\tif (is_directory(git_dir))\n+\t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n \t\tstrbuf_release(&buf);\n \t\t/* The submodule is not checked out, so it is not modified */\n \t\treturn 0;\n-\n \t}\n \tstrbuf_reset(&buf);\n \n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315203","messageId":"20170323223338.32274-3-sbeller@google.com","threadId":"45476","inReplyTo":"20170323223338.32274-1-sbeller@google.com","subject":"[PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T22:33:33Z","receivedAt":"2017-03-23T22:34:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of implementing line reading yet again, make use of our beautiful\nlibrary functions.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 14 ++------------\n 1 file changed, 2 insertions(+), 12 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2c667ac95a..c1b7b78260 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1041,11 +1041,9 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n-\tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n-\tconst char *line, *next_line;\n \tconst char *git_dir;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n@@ -1072,10 +1070,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tif (start_command(&cp))\n \t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n \n-\tlen = strbuf_read(&buf, cp.out, 1024);\n-\tline = buf.buf;\n-\twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?')) {\n+\twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n+\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n \t\t\t\tbreak;\n@@ -1085,12 +1081,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n \t\t\t\tbreak;\n \t\t}\n-\t\tnext_line = strchr(line, '\\n');\n-\t\tif (!next_line)\n-\t\t\tbreak;\n-\t\tnext_line++;\n-\t\tlen -= (next_line - line);\n-\t\tline = next_line;\n \t}\n \tclose(cp.out);\n \n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315204","messageId":"20170323223338.32274-7-sbeller@google.com","threadId":"45476","inReplyTo":"20170323223338.32274-1-sbeller@google.com","subject":"[PATCH 6/7] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T22:33:37Z","receivedAt":"2017-03-23T22:34:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If I add an untracked file to a submodule or modify a tracked file,\ncurrently \"git status --short\" treats the change in the same way as\nchanges to the current HEAD of the submodule:\n\n $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit\n $ echo hello >gerrit/plugins/replication/stray-file\n $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap\n $ git -C gerrit status --short\n M plugins/replication\n\nThis is by analogy with ordinary files, where \"M\" represents a change\nthat has not been added yet to the index. But this change cannot be\nadded to the index without entering the submodule, \"git add\"-ing it,\nand running \"git commit\", so the analogy is counterproductive.\n\nIntroduce new status letters \" ?\" and \" m\" for this. These are similar\nto the existing \"??\" and \" M\" but mean that the submodule (not the\nparent project) has new untracked files and modified files, respectively.\nThe user can use \"git add\" and \"git commit\" from within the submodule to\nadd them.\n\nChanges to the submodule's HEAD commit can be recorded in the index with\na plain \"git add -u\" and are shown with \" M\", like today.\n\nTo avoid excessive clutter, show at most one of \" ?\", \" m\", and \" M\" for\nthe submodule. They represent increasing levels of change --- the last\none that applies is shown (e.g., \" m\" if there are both modified files\nand untracked files in the submodule, or \" M\" if the submodule's HEAD\nhas been modified and it has untracked files).\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which shares code with \"status --short\". We only change\n\"git status --short\".\n\nNon-short \"git status\" and \"git status --porcelain=2\" already handle\nthese cases by showing more detail:\n\n $ git -C gerrit status --porcelain=2\n 1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication\n $ git -C gerrit status\n[...]\n modified: plugins/replication (modified content, untracked content)\n\nScripts caring about these distinctions should use --porcelain=2.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n Documentation/git-status.txt | 9 +++++++\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 13 ++++++++--\n 4 files changed, 90 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..01b457c322 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,13 @@ in which case `XY` are `!!`.\n ! ! ignored\n -------------------------------------------------\n \n+Submodules have more state and instead report\n+\t\tM the submodule has a different HEAD than\n+\t\t recorded in the index\n+\t\tm the submodule has modified content\n+\t\t? the submodule has untracked files\n+\n+\n If -b is used the short-format status is preceded by a line\n \n ## branchname tracking info\n@@ -210,6 +217,8 @@ field from the first filename). Third, filenames containing special\n characters are not specially formatted; no quoting or\n backslash-escaping is performed.\n \n+Any submodule changes are reported as modified `M` instead of `m` or single `?`.\n+\n Porcelain Format Version 2\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5aa6db584c..a6e5c5bd56 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -268,6 +268,14 @@ cat >expect.modified <expect.modified_inside <expect.modified_untracked <expect.cached <actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex d31b34da83..ad46384064 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -50,6 +50,15 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with modified file in submodule (short)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n@@ -64,6 +73,14 @@ test_expect_success 'status with added file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with added file in submodule (short)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n@@ -83,6 +100,13 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with untracked file in submodule (short)' '\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t ? sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added and untracked file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \techo \"content\" >sub/new-file &&\n@@ -271,4 +295,37 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \ttest_cmp diff_submodule_actual diff_submodule_expect\n '\n \n+test_expect_success 'setup superproject with untracked file in nested submodule' '\n+\t(\n+\t\tcd super &&\n+\t\tgit clean -dfx &&\n+\t\trm .gitmodules &&\n+\t\tgit submodule add -f ./sub1 &&\n+\t\tgit submodule add -f ./sub2 &&\n+\t\tgit commit -a -m \"messy merge in superproject\" &&\n+\t\t(\n+\t\t\tcd sub1 &&\n+\t\t\tgit submodule add ../sub2 &&\n+\t\t\tgit commit -a -m \"add sub2 to sub1\"\n+\t\t) &&\n+\t\tgit add sub1 &&\n+\t\tgit commit -a -m \"update sub1 to contain nested sub\"\n+\t) &&\n+\techo untracked >super/sub1/sub2/untracked\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\tEOF\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 308cf3779e..9909fd0e57 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -431,10 +431,19 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t}\n \t\tif (!d->worktree_status)\n \t\t\td->worktree_status = p->status;\n-\t\td->dirty_submodule = p->two->dirty_submodule;\n-\t\tif (S_ISGITLINK(p->two->mode))\n+\t\tif (S_ISGITLINK(p->two->mode)) {\n+\t\t\td->dirty_submodule = p->two->dirty_submodule;\n \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n \t\t\t\t\t\t\t &p->two->oid);\n+\t\t\tif (s->status_format == STATUS_FORMAT_SHORT) {\n+\t\t\t\tif (d->new_submodule_commits)\n+\t\t\t\t\td->worktree_status = 'M';\n+\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\t\t\t\td->worktree_status = 'm';\n+\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\t\td->worktree_status = '?';\n+\t\t\t}\n+\t\t}\n \n \t\tswitch (p->status) {\n \t\tcase DIFF_STATUS_ADDED:\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315205","messageId":"20170323223338.32274-4-sbeller@google.com","threadId":"45476","inReplyTo":"20170323223338.32274-1-sbeller@google.com","subject":"[PATCH 3/7] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T22:33:34Z","receivedAt":"2017-03-23T22:34:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Migrate 'is_submodule_modified' to the new porcelain format of\ngit-status. This conversion attempts to convert faithfully, i.e.\nthe behavior ought to be exactly the same.\n\nAs the output in the parsing only distinguishes between untracked files\nand the rest, this is easy to port to the new format, as we only\nneed to identify untracked files and the rest is handled in the \"else\"\ncase.\n\nuntracked files are indicated by only a single question mark instead of\ntwo question marks, so the conversion is easy.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex c1b7b78260..da1db90dda 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1058,7 +1058,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t}\n \tstrbuf_reset(&buf);\n \n-\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", NULL);\n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain=2\", NULL);\n \tif (ignore_untracked)\n \t\targv_array_push(&cp.args, \"-uno\");\n \n@@ -1068,10 +1068,11 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n+\t\tdie(\"Could not run 'git status --porcelain=2' in submodule %s\", path);\n \n \twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n-\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {\n+\t\t/* regular untracked files */\n+\t\tif (buf.buf[0] == '?') {\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n \t\t\t\tbreak;\n@@ -1085,7 +1086,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tclose(cp.out);\n \n \tif (finish_command(&cp))\n-\t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n+\t\tdie(\"'git status --porcelain=2' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n \treturn dirty_submodule;\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315206","messageId":"20170323223338.32274-8-sbeller@google.com","threadId":"45476","inReplyTo":"20170323223338.32274-1-sbeller@google.com","subject":"[PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T22:33:38Z","receivedAt":"2017-03-23T22:34:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a nested submodule has untracked files, it would be reported as\n\"modified submodule\" in the superproject, because submodules are not\nparsed correctly in is_submodule_modified as they are bucketed into\nthe modified pile as \"they are not an untracked file\".\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 16 ++++++++++++++--\n t/t3600-rm.sh | 2 +-\n t/t7506-status-submodule.sh | 2 +-\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex e06e52b993..0f477f3a4e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1075,8 +1075,20 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t/* regular untracked files */\n \t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\telse\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\n+\t\t/* regular unmerged and renamed files */\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t buf.buf[0] == '1' ||\n+\t\t buf.buf[0] == '2') {\n+\t\t\tif (buf.buf[5] == 'S') {\n+\t\t\t\t/* nested submodule handling */\n+\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t\t\tif (buf.buf[8] == 'U')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\t\t\t} else\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t}\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n \t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex a6e5c5bd56..b58793448b 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified_inside actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex ad46384064..e3cdcede72 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -324,7 +324,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain)'\n test_expect_success 'status with untracked file in nested submodule (short)' '\n \tgit -C super status --short >output &&\n \tdiff output - <<-\\EOF\n-\t m sub1\n+\t ? sub1\n \tEOF\n '\n \n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315207","messageId":"20170323223621.GC20794@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170323223338.32274-2-sbeller@google.com","subject":"Re: [PATCH 1/7] submodule.c: use argv_array in is_submodule_modified","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-23T22:36:21Z","receivedAt":"2017-03-23T22:36:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> struct argv_array is easier to use and maintain\n\nYes!\n\n[...]\n> submodule.c | 10 ++--------\n> 1 file changed, 2 insertions(+), 8 deletions(-)\n\nI also like the diffstat. :)\n\n[...]\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n> {\n> \tssize_t len;\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> -\tconst char *argv[] = {\n> -\t\t\"status\",\n> -\t\t\"--porcelain\",\n> -\t\tNULL,\n> -\t\tNULL,\n> -\t};\n\nand the avoidance of this kind of fixed-size magic.\n\nReviewed-by: Jonathan Nieder \n\nThank you.\n"},{"id":"315209","messageId":"20170323225019.GD20794@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170323223338.32274-3-sbeller@google.com","subject":"Re: [PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-23T22:50:19Z","receivedAt":"2017-03-23T22:50:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Instead of implementing line reading yet again, make use of our beautiful\n> library functions.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 14 ++------------\n> 1 file changed, 2 insertions(+), 12 deletions(-)\n\nThis changes buffering behavior in two ways:\n\n- by using strbuf_getwholeline_fd instead of strbuf_read, we avoid\n having to allocate memory for the entire child process output at\n once. That is, we limit maximum memory usage (good).\n\n- by using strbuf_getwholeline_fd instead of strbuf_read, we xread\n one byte at a time instead of larger chunks. That means more\n overhead due to context switches (bad).\n\nSome callers of getwholeline_fd need the one-byte-at-a-time thing to\navoid waiting too long for input, and in some cases the alternative is\ndeadlock. We know this caller doesn't fall into that category because\nit was doing fine slurping the entire file at once. As the\ngetwholeline_fd API doc comment explains:\n\n * It reads one character at a time, so it is very slow. Do not\n * use it unless you need the correct position in the file\n * descriptor.\n\nCan this caller use xfdopen and strbuf_getwholeline instead to get\nback the benefit of buffering (i.e., something like the below)?\n\nAnother consequence of switching to streaming is that we may close\nbefore the child finishes. Do we have to worry about handling SIGPIPE\nin the child? I haven't checked how this handles that --- a test\nmight be useful.\n\nThanks and hope that helps,\nJonathan\n\ndiff --git i/submodule.c w/submodule.c\nindex c1b7b78260..184d5739fc 100644\n--- i/submodule.c\n+++ w/submodule.c\n@@ -1043,6 +1043,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *fp;\n \tunsigned dirty_submodule = 0;\n \tconst char *git_dir;\n \n@@ -1070,7 +1071,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tif (start_command(&cp))\n \t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n \n-\twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n+\tfp = xfdopen(cp.out, \"r\");\n+\twhile (strbuf_getwholeline(&buf, fp, '\\n') != EOF) {\n \t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n@@ -1082,7 +1084,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t\t\tbreak;\n \t\t}\n \t}\n-\tclose(cp.out);\n+\tfclose(fp);\n \n \tif (finish_command(&cp))\n \t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n"},{"id":"315211","messageId":"CAGZ79kYPdGVBUnTgGyiBB5ApC9eS14p7n1QjEWApSrmF38t9Rw@mail.gmail.com","threadId":"45476","inReplyTo":"20170323225019.GD20794@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T23:04:16Z","receivedAt":"2017-03-23T23:04:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 23, 2017 at 3:50 PM, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>\n>> Instead of implementing line reading yet again, make use of our beautiful\n>> library functions.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> submodule.c | 14 ++------------\n>> 1 file changed, 2 insertions(+), 12 deletions(-)\n>\n> This changes buffering behavior in two ways:\n>\n> - by using strbuf_getwholeline_fd instead of strbuf_read, we avoid\n> having to allocate memory for the entire child process output at\n> once. That is, we limit maximum memory usage (good).\n>\n> - by using strbuf_getwholeline_fd instead of strbuf_read, we xread\n> one byte at a time instead of larger chunks. That means more\n> overhead due to context switches (bad).\n\nThanks for careful reading!\n\n>\n> Some callers of getwholeline_fd need the one-byte-at-a-time thing to\n> avoid waiting too long for input, and in some cases the alternative is\n> deadlock. We know this caller doesn't fall into that category because\n> it was doing fine slurping the entire file at once. As the\n> getwholeline_fd API doc comment explains:\n>\n> * It reads one character at a time, so it is very slow. Do not\n> * use it unless you need the correct position in the file\n> * descriptor.\n>\n> Can this caller use xfdopen and strbuf_getwholeline instead to get\n> back the benefit of buffering (i.e., something like the below)?\n\nThe code below makes sense to me.\n\n>\n> Another consequence of switching to streaming is that we may close\n> before the child finishes.\n\nWe could have had closing before the child finished before as well:\n* the first read happens with strbuf_read(&buf, cp.out, 1024);\n* it contains a line indicating a modified file\n* The condition breaks out of while:\n if (ignore_untracked ||\n (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n break;\n* after the while loop we have the close();\n* but we only had one read, which is not the complete output of the child.\n\n> Do we have to worry about handling SIGPIPE\n> in the child? I haven't checked how this handles that --- a test\n> might be useful.\n\nThis patch doesn't make it worse in that respect.\n\nThanks,\nStefan\n"},{"id":"315212","messageId":"CAGZ79kYkqgY8n=C2ms8rsaRz9iZ66p_YXDfm9tR=MymuH-HHPw@mail.gmail.com","threadId":"45476","inReplyTo":"CAGZ79kYPdGVBUnTgGyiBB5ApC9eS14p7n1QjEWApSrmF38t9Rw@mail.gmail.com","subject":"Re: [PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T23:11:05Z","receivedAt":"2017-03-23T23:11:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 23, 2017 at 4:04 PM, Stefan Beller wrote:\n>\n> We could have had closing before the child finished before as well:\n> * the first read happens with strbuf_read(&buf, cp.out, 1024);\n\nThe 1024 is only a hint. So it actually reads the output in full.\n\nSo I guess I'll come up with a test for SIGPIPE, then.\n"},{"id":"315293","messageId":"20170324182902.19280-1-sbeller@google.com","threadId":"45476","inReplyTo":"20170323223338.32274-7-sbeller@google.com","subject":"[PATCH v6 0/7] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T18:28:55Z","receivedAt":"2017-03-24T18:29:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v6:\n* kill the child once we know all information that we ask for, as an optimization\n* reordered the patches for that\n* strbuf_getwholeline instead of its _fd version.\n\nv5:\n* fixed rebase error in the first 2 patches\n* the last 3 patches introduce behavior change outside the scope of is_modified_submodule\n whereas the first 4 ought to just be local changes.\n \nThanks,\nStefan\n\nv4:\n* broken down in a lot of tiny patches.\n\nJonathan wrote:\n> Patch 1/3 is the one that gives me pause, since I hadn't been able to\n> chase down all callers. Would it be feasible to split that into two:\n> one patch to switch to --porcelain=2 but not change behavior, and a\n> separate patch to improve what is stored in the dirty_submodule flags?\n\nThis part is in the latest patch now.\n\nThanks,\nStefan\n\n\nv3:\nThis comes as a series; first I'd like to refactor is_submodule_modified\nto take advantage of the new porcelain=2 plumbing switch to check for changes\nin the submodule.\n\nOn top of the refactoring comes the actual change, which moved the\nrewriting of the submodule change indicator letter to the collection part.\n\nThanks,\nStefan\n\nStefan Beller (8):\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c: use argv_array in is_submodule_modified\n submodule.c: convert is_submodule_modified to use\n strbuf_getwholeline_fd\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c: factor out early loop termination in\n is_submodule_modified\n submodule.c: stricter checking for submodules in is_submodule_modified\n short status: improve reporting for submodule changes\n submodule.c: correctly handle nested submodules in\n is_submodule_modified\n\n Documentation/git-status.txt | 9 +++++++\n submodule.c | 56 ++++++++++++++++++++-----------------------\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 13 ++++++++--\n 5 files changed, 116 insertions(+), 37 deletions(-)\n\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315294","messageId":"20170324182902.19280-2-sbeller@google.com","threadId":"45476","inReplyTo":"20170324182902.19280-1-sbeller@google.com","subject":"[PATCH 1/7] submodule.c: use argv_array in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T18:28:56Z","receivedAt":"2017-03-24T18:29:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"struct argv_array is easier to use and maintain\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3200b7bb2b..2c667ac95a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n \tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {\n-\t\t\"status\",\n-\t\t\"--porcelain\",\n-\t\tNULL,\n-\t\tNULL,\n-\t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n \tconst char *line, *next_line;\n@@ -1066,10 +1060,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t}\n \tstrbuf_reset(&buf);\n \n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", NULL);\n \tif (ignore_untracked)\n-\t\targv[2] = \"-uno\";\n+\t\targv_array_push(&cp.args, \"-uno\");\n \n-\tcp.argv = argv;\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n-- \n2.12.1.437.g2b7623d507\n\n"},{"id":"315295","messageId":"20170324182902.19280-3-sbeller@google.com","threadId":"45476","inReplyTo":"20170324182902.19280-1-sbeller@google.com","subject":"[PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T18:28:57Z","receivedAt":"2017-03-24T18:29:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes it easier for a follow up patch.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2c667ac95a..e52cb8a958 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1075,16 +1075,15 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tlen = strbuf_read(&buf, cp.out, 1024);\n \tline = buf.buf;\n \twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?')) {\n+\t\tif ((line[0] == '?') && (line[1] == '?'))\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\t\t\tbreak;\n-\t\t} else {\n+\t\telse\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\tif (ignore_untracked ||\n-\t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n-\t\t\t\tbreak;\n-\t\t}\n+\n+\t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n+\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))\n+\t\t\tbreak;\n+\n \t\tnext_line = strchr(line, '\\n');\n \t\tif (!next_line)\n \t\t\tbreak;\n-- \n2.12.1.437.g2b7623d507\n\n"},{"id":"315296","messageId":"20170324182902.19280-4-sbeller@google.com","threadId":"45476","inReplyTo":"20170324182902.19280-1-sbeller@google.com","subject":"[PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T18:28:58Z","receivedAt":"2017-03-24T18:29:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of implementing line reading yet again, make use of our beautiful\nlibrary function to read one line. By using strbuf_getwholeline instead\nof strbuf_read, we avoid having to allocate memory for the entire child\nprocess output at once. That is, we limit maximum memory usage.\nOnce we know all information that we care about, we can terminate\nthe child early. In that case we do not care about its exit code as well.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 31 +++++++++++++++----------------\n 1 file changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex e52cb8a958..0c43f9f2b1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1041,12 +1041,12 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n-\tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *fp;\n \tunsigned dirty_submodule = 0;\n-\tconst char *line, *next_line;\n \tconst char *git_dir;\n+\tint ignore_cp_exit_code = 0;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n \tgit_dir = read_gitfile(buf.buf);\n@@ -1072,28 +1072,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tif (start_command(&cp))\n \t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n \n-\tlen = strbuf_read(&buf, cp.out, 1024);\n-\tline = buf.buf;\n-\twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?'))\n+\tfp = xfdopen(cp.out, \"r\");\n+\twhile (strbuf_getwholeline(&buf, fp, '\\n') != EOF) {\n+\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?'))\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\telse\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n-\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))\n-\t\t\tbreak;\n-\n-\t\tnext_line = strchr(line, '\\n');\n-\t\tif (!next_line)\n+\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked)) {\n+\t\t\t/*\n+\t\t\t * We're not interested in any further information from\n+\t\t\t * the child any more, no output nor its exit code.\n+\t\t\t */\n+\t\t\tkill(cp.pid, SIGTERM);\n+\t\t\tignore_cp_exit_code = 1;\n \t\t\tbreak;\n-\t\tnext_line++;\n-\t\tlen -= (next_line - line);\n-\t\tline = next_line;\n+\t\t}\n \t}\n-\tclose(cp.out);\n+\tfclose(fp);\n \n-\tif (finish_command(&cp))\n+\tif (finish_command(&cp) && !ignore_cp_exit_code)\n \t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n-- \n2.12.1.437.g2b7623d507\n\n"},{"id":"315297","messageId":"20170324182902.19280-6-sbeller@google.com","threadId":"45476","inReplyTo":"20170324182902.19280-1-sbeller@google.com","subject":"[PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T18:29:00Z","receivedAt":"2017-03-24T18:29:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By having a stricter check in the superproject we catch errors earlier,\ninstead of spawning a child process to tell us.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 256f15fde1..467f1de763 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1052,11 +1052,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tgit_dir = read_gitfile(buf.buf);\n \tif (!git_dir)\n \t\tgit_dir = buf.buf;\n-\tif (!is_directory(git_dir)) {\n+\tif (!is_git_directory(git_dir)) {\n+\t\tif (is_directory(git_dir))\n+\t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n \t\tstrbuf_release(&buf);\n \t\t/* The submodule is not checked out, so it is not modified */\n \t\treturn 0;\n-\n \t}\n \tstrbuf_reset(&buf);\n \n-- \n2.12.1.437.g2b7623d507\n\n"},{"id":"315298","messageId":"20170324182902.19280-7-sbeller@google.com","threadId":"45476","inReplyTo":"20170324182902.19280-1-sbeller@google.com","subject":"[PATCH 6/7] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T18:29:01Z","receivedAt":"2017-03-24T18:29:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If I add an untracked file to a submodule or modify a tracked file,\ncurrently \"git status --short\" treats the change in the same way as\nchanges to the current HEAD of the submodule:\n\n $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit\n $ echo hello >gerrit/plugins/replication/stray-file\n $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap\n $ git -C gerrit status --short\n M plugins/replication\n\nThis is by analogy with ordinary files, where \"M\" represents a change\nthat has not been added yet to the index. But this change cannot be\nadded to the index without entering the submodule, \"git add\"-ing it,\nand running \"git commit\", so the analogy is counterproductive.\n\nIntroduce new status letters \" ?\" and \" m\" for this. These are similar\nto the existing \"??\" and \" M\" but mean that the submodule (not the\nparent project) has new untracked files and modified files, respectively.\nThe user can use \"git add\" and \"git commit\" from within the submodule to\nadd them.\n\nChanges to the submodule's HEAD commit can be recorded in the index with\na plain \"git add -u\" and are shown with \" M\", like today.\n\nTo avoid excessive clutter, show at most one of \" ?\", \" m\", and \" M\" for\nthe submodule. They represent increasing levels of change --- the last\none that applies is shown (e.g., \" m\" if there are both modified files\nand untracked files in the submodule, or \" M\" if the submodule's HEAD\nhas been modified and it has untracked files).\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which shares code with \"status --short\". We only change\n\"git status --short\".\n\nNon-short \"git status\" and \"git status --porcelain=2\" already handle\nthese cases by showing more detail:\n\n $ git -C gerrit status --porcelain=2\n 1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication\n $ git -C gerrit status\n[...]\n modified: plugins/replication (modified content, untracked content)\n\nScripts caring about these distinctions should use --porcelain=2.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n Documentation/git-status.txt | 9 +++++++\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 13 ++++++++--\n 4 files changed, 90 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..01b457c322 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,13 @@ in which case `XY` are `!!`.\n ! ! ignored\n -------------------------------------------------\n \n+Submodules have more state and instead report\n+\t\tM the submodule has a different HEAD than\n+\t\t recorded in the index\n+\t\tm the submodule has modified content\n+\t\t? the submodule has untracked files\n+\n+\n If -b is used the short-format status is preceded by a line\n \n ## branchname tracking info\n@@ -210,6 +217,8 @@ field from the first filename). Third, filenames containing special\n characters are not specially formatted; no quoting or\n backslash-escaping is performed.\n \n+Any submodule changes are reported as modified `M` instead of `m` or single `?`.\n+\n Porcelain Format Version 2\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5aa6db584c..a6e5c5bd56 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -268,6 +268,14 @@ cat >expect.modified <expect.modified_inside <expect.modified_untracked <expect.cached <actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex d31b34da83..ad46384064 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -50,6 +50,15 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with modified file in submodule (short)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n@@ -64,6 +73,14 @@ test_expect_success 'status with added file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with added file in submodule (short)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n@@ -83,6 +100,13 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with untracked file in submodule (short)' '\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t ? sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added and untracked file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \techo \"content\" >sub/new-file &&\n@@ -271,4 +295,37 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \ttest_cmp diff_submodule_actual diff_submodule_expect\n '\n \n+test_expect_success 'setup superproject with untracked file in nested submodule' '\n+\t(\n+\t\tcd super &&\n+\t\tgit clean -dfx &&\n+\t\trm .gitmodules &&\n+\t\tgit submodule add -f ./sub1 &&\n+\t\tgit submodule add -f ./sub2 &&\n+\t\tgit commit -a -m \"messy merge in superproject\" &&\n+\t\t(\n+\t\t\tcd sub1 &&\n+\t\t\tgit submodule add ../sub2 &&\n+\t\t\tgit commit -a -m \"add sub2 to sub1\"\n+\t\t) &&\n+\t\tgit add sub1 &&\n+\t\tgit commit -a -m \"update sub1 to contain nested sub\"\n+\t) &&\n+\techo untracked >super/sub1/sub2/untracked\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\tEOF\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 308cf3779e..9909fd0e57 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -431,10 +431,19 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t}\n \t\tif (!d->worktree_status)\n \t\t\td->worktree_status = p->status;\n-\t\td->dirty_submodule = p->two->dirty_submodule;\n-\t\tif (S_ISGITLINK(p->two->mode))\n+\t\tif (S_ISGITLINK(p->two->mode)) {\n+\t\t\td->dirty_submodule = p->two->dirty_submodule;\n \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n \t\t\t\t\t\t\t &p->two->oid);\n+\t\t\tif (s->status_format == STATUS_FORMAT_SHORT) {\n+\t\t\t\tif (d->new_submodule_commits)\n+\t\t\t\t\td->worktree_status = 'M';\n+\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\t\t\t\td->worktree_status = 'm';\n+\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\t\td->worktree_status = '?';\n+\t\t\t}\n+\t\t}\n \n \t\tswitch (p->status) {\n \t\tcase DIFF_STATUS_ADDED:\n-- \n2.12.1.437.g2b7623d507\n\n"},{"id":"315299","messageId":"20170324182902.19280-8-sbeller@google.com","threadId":"45476","inReplyTo":"20170324182902.19280-1-sbeller@google.com","subject":"[PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T18:29:02Z","receivedAt":"2017-03-24T18:29:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a nested submodule has untracked files, it would be reported as\n\"modified submodule\" in the superproject, because submodules are not\nparsed correctly in is_submodule_modified as they are bucketed into\nthe modified pile as \"they are not an untracked file\".\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 16 ++++++++++++++--\n t/t3600-rm.sh | 2 +-\n t/t7506-status-submodule.sh | 2 +-\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 467f1de763..ec7e9b1b06 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1078,8 +1078,20 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t/* regular untracked files */\n \t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\telse\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\n+\t\t/* regular unmerged and renamed files */\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t buf.buf[0] == '1' ||\n+\t\t buf.buf[0] == '2') {\n+\t\t\tif (buf.buf[5] == 'S') {\n+\t\t\t\t/* nested submodule handling */\n+\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t\t\tif (buf.buf[8] == 'U')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\t\t\t} else\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t}\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n \t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked)) {\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex a6e5c5bd56..b58793448b 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified_inside actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex ad46384064..e3cdcede72 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -324,7 +324,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain)'\n test_expect_success 'status with untracked file in nested submodule (short)' '\n \tgit -C super status --short >output &&\n \tdiff output - <<-\\EOF\n-\t m sub1\n+\t ? sub1\n \tEOF\n '\n \n-- \n2.12.1.437.g2b7623d507\n\n"},{"id":"315300","messageId":"20170324182902.19280-5-sbeller@google.com","threadId":"45476","inReplyTo":"20170324182902.19280-1-sbeller@google.com","subject":"[PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-24T18:28:59Z","receivedAt":"2017-03-24T18:29:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Migrate 'is_submodule_modified' to the new porcelain format of\ngit-status. This conversion attempts to convert faithfully, i.e.\nthe behavior ought to be exactly the same.\n\nAs the output in the parsing only distinguishes between untracked files\nand the rest, this is easy to port to the new format, as we only\nneed to identify untracked files and the rest is handled in the \"else\"\ncase.\n\nuntracked files are indicated by only a single question mark instead of\ntwo question marks, so the conversion is easy.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0c43f9f2b1..256f15fde1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1060,7 +1060,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t}\n \tstrbuf_reset(&buf);\n \n-\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", NULL);\n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain=2\", NULL);\n \tif (ignore_untracked)\n \t\targv_array_push(&cp.args, \"-uno\");\n \n@@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n+\t\tdie(\"Could not run 'git status --porcelain=2' in submodule %s\", path);\n \n \tfp = xfdopen(cp.out, \"r\");\n \twhile (strbuf_getwholeline(&buf, fp, '\\n') != EOF) {\n-\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?'))\n+\t\t/* regular untracked files */\n+\t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\telse\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n@@ -1093,7 +1094,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tfclose(fp);\n \n \tif (finish_command(&cp) && !ignore_cp_exit_code)\n-\t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n+\t\tdie(\"'git status --porcelain=2' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n \treturn dirty_submodule;\n-- \n2.12.1.437.g2b7623d507\n\n"},{"id":"315338","messageId":"20170324222557.GF31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170324182902.19280-2-sbeller@google.com","subject":"Re: [PATCH 1/7] submodule.c: use argv_array in is_submodule_modified","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T22:25:57Z","receivedAt":"2017-03-24T22:26:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> struct argv_array is easier to use and maintain\n\nMissing '.' at end of sentence.\n\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 10 ++--------\n> 1 file changed, 2 insertions(+), 8 deletions(-)\n\nWith or without that tweak, I still like this as much as last time. :)\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"315339","messageId":"20170324223040.GG31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170324182902.19280-3-sbeller@google.com","subject":"Re: [PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T22:30:40Z","receivedAt":"2017-03-24T22:30:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1075,16 +1075,15 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n> \tlen = strbuf_read(&buf, cp.out, 1024);\n> \tline = buf.buf;\n> \twhile (len > 2) {\n> -\t\tif ((line[0] == '?') && (line[1] == '?')) {\n> +\t\tif ((line[0] == '?') && (line[1] == '?'))\n> \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n> -\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n> -\t\t\t\tbreak;\n> -\t\t} else {\n> +\t\telse\n> \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n> -\t\t\tif (ignore_untracked ||\n> -\t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n> -\t\t\t\tbreak;\n> -\t\t}\n> +\n> +\t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n> +\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))\n\nnit: long line\n\nReviewed-by: Jonathan Nieder \n\ndiff --git i/submodule.c w/submodule.c\nindex aba89661a7..8934d97359 100644\n--- i/submodule.c\n+++ w/submodule.c\n@@ -1087,8 +1087,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n-\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))\n+\t\t (ignore_untracked\n+\t\t || (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))) {\n \t\t\tbreak;\n+\t\t}\n \n \t\tnext_line = strchr(line, '\\n');\n \t\tif (!next_line)\n"},{"id":"315340","messageId":"20170324223848.GH31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170324182902.19280-4-sbeller@google.com","subject":"Re: [PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T22:38:48Z","receivedAt":"2017-03-24T22:38:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Instead of implementing line reading yet again, make use of our beautiful\n> library function to read one line. By using strbuf_getwholeline instead\n> of strbuf_read, we avoid having to allocate memory for the entire child\n> process output at once. That is, we limit maximum memory usage.\n\nIt also overlaps work a little better.\n\n> Once we know all information that we care about, we can terminate\n> the child early. In that case we do not care about its exit code as well.\n\nShould this say something about SIGPIPE?\n\n[...]\n> +++ b/submodule.c\n[...]\n> @@ -1072,28 +1072,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n[...]\n> +\t\t\t/*\n> +\t\t\t * We're not interested in any further information from\n> +\t\t\t * the child any more, no output nor its exit code.\n> +\t\t\t */\n\nlanguage nit: s/, no output/: neither output/\n\n[...]\n> -\tif (finish_command(&cp))\n> +\tif (finish_command(&cp) && !ignore_cp_exit_code)\n\nfinish_command complains if the child dies of SIGTERM:\n\n\t\terror: status died of signal 15\n\nwait_or_whine(cp->pid, cp->argv[0], 1) doesn't do that but is meant for\nsignal handling. Maybe we should rely on SIGPIPE instead (which\nwait_or_whine always silences) and avoid the kill() call.\n\nCan there be a test for this case (i.e. having lots of untracked files\nin the submodule so the child process fills its pipe buffer and has to\nexit early)?\n\nThanks,\nJonathan\n"},{"id":"315341","messageId":"20170324224124.GI31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170324182902.19280-5-sbeller@google.com","subject":"Re: [PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T22:41:24Z","receivedAt":"2017-03-24T22:41:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n[...]\n> --- a/submodule.c\n> +++ b/submodule.c\n[...]\n> @@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n[...]\n> \twhile (strbuf_getwholeline(&buf, fp, '\\n') != EOF) {\n> -\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?'))\n> +\t\t/* regular untracked files */\n> +\t\tif (buf.buf[0] == '?')\n> \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n> \t\telse\n> \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n\nAt first I didn't believe it could be so simple. :)\n\nUnlike ordinary files that use 1, 2, or u, ignored files use '!'.\nYou're not passing --ignored so you don't have to worry about them.\n\nReviewed-by: Jonathan Nieder \n\nVery nice.\n"},{"id":"315342","messageId":"20170324224207.GJ31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170324182902.19280-6-sbeller@google.com","subject":"Re: [PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T22:42:07Z","receivedAt":"2017-03-24T22:42:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> By having a stricter check in the superproject we catch errors earlier,\n> instead of spawning a child process to tell us.\n> \n> Signed-off-by: Stefan Beller \n> Reviewed-by: Jonathan Nieder \n\nYep. :)\n"},{"id":"315345","messageId":"20170324230657.GK31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170324182902.19280-7-sbeller@google.com","subject":"Re: [PATCH 6/7] short status: improve reporting for submodule changes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T23:06:57Z","receivedAt":"2017-03-24T23:07:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> +++ b/wt-status.c\n> @@ -431,10 +431,19 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n> \t\t}\n> \t\tif (!d->worktree_status)\n> \t\t\td->worktree_status = p->status;\n> -\t\td->dirty_submodule = p->two->dirty_submodule;\n> -\t\tif (S_ISGITLINK(p->two->mode))\n> +\t\tif (S_ISGITLINK(p->two->mode)) {\n> +\t\t\td->dirty_submodule = p->two->dirty_submodule;\n\nThis is to simplify because dirty_submodule is just going to stay as 0\nin the !S_ISGITLINK(p->two->mode) case, right?\n\n> \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n> \t\t\t\t\t\t\t &p->two->oid);\n> +\t\t\tif (s->status_format == STATUS_FORMAT_SHORT) {\n> +\t\t\t\tif (d->new_submodule_commits)\n> +\t\t\t\t\td->worktree_status = 'M';\n> +\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n> +\t\t\t\t\td->worktree_status = 'm';\n> +\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n> +\t\t\t\t\td->worktree_status = '?';\n> +\t\t\t}\n> +\t\t}\n\nMakes sense.\n\nThis patch also goes past the right margin. Maybe this code to compute\nd->worktree_status could be its own function?\n\nWith or without such a change,\nReviewed-by: Jonathan Nieder \n\ndiff --git i/wt-status.c w/wt-status.c\nindex 9909fd0e57..0375484962 100644\n--- i/wt-status.c\n+++ w/wt-status.c\n@@ -407,6 +407,16 @@ static void wt_longstatus_print_change_data(struct wt_status *s,\n \tstrbuf_release(&twobuf);\n }\n \n+static char short_submodule_status(struct wt_status_change_data *d) {\n+\tif (d->new_submodule_commits)\n+\t\treturn 'M';\n+\tif (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\treturn 'm';\n+\tif (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\treturn '?';\n+\treturn d->worktree_status;\n+}\n+\n static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\n@@ -435,14 +445,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t\td->dirty_submodule = p->two->dirty_submodule;\n \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n \t\t\t\t\t\t\t &p->two->oid);\n-\t\t\tif (s->status_format == STATUS_FORMAT_SHORT) {\n-\t\t\t\tif (d->new_submodule_commits)\n-\t\t\t\t\td->worktree_status = 'M';\n-\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\t\t\t\td->worktree_status = 'm';\n-\t\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n-\t\t\t\t\td->worktree_status = '?';\n-\t\t\t}\n+\t\t\tif (s->status_format == STATUS_FORMAT_SHORT)\n+\t\t\t\td->worktree_status = short_submodule_status(d);\n \t\t}\n \n \t\tswitch (p->status) {\n"},{"id":"315356","messageId":"20170324233156.GL31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170324182902.19280-8-sbeller@google.com","subject":"Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T23:31:56Z","receivedAt":"2017-03-24T23:32:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> When a nested submodule has untracked files, it would be reported as\n> \"modified submodule\" in the superproject, because submodules are not\n> parsed correctly in is_submodule_modified as they are bucketed into\n> the modified pile as \"they are not an untracked file\".\n> \n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 16 ++++++++++++++--\n> t/t3600-rm.sh | 2 +-\n> t/t7506-status-submodule.sh | 2 +-\n> 3 files changed, 16 insertions(+), 4 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 467f1de763..ec7e9b1b06 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1078,8 +1078,20 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n> \t\t/* regular untracked files */\n> \t\tif (buf.buf[0] == '?')\n> \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n> -\t\telse\n> -\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n> +\n> +\t\t/* regular unmerged and renamed files */\n> +\t\tif (buf.buf[0] == 'u' ||\n> +\t\t buf.buf[0] == '1' ||\n> +\t\t buf.buf[0] == '2') {\n> +\t\t\tif (buf.buf[5] == 'S') {\n\nCan this overflow the buffer? Submodule state is supposed to be 4\ncharacters, so could do\n\n\t\t\t/*\n\t\t\t * T XY SSSS:\n\t\t\t * T = line type, XY = status, SSSS = submodule state\n\t\t\t */\n\t\t\tif (buf.len < 1 + 1 + 2 + 1 + 4)\n\t\t\t\tdie(\"BUG: invalid status --porcelain=2 line %s\",\n\t\t\t\t buf.buf);\n\n\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n\t\t\t\t/* untracked file */\n\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n\n\t\t\tif (memcmp(buf.buf + 5, \"S..U\", 4))\n\t\t\t\t/* other change */\n\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n\t\t}\n\n> +\t\t\t\t/* nested submodule handling */\n> +\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n> +\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n> +\t\t\t\tif (buf.buf[8] == 'U')\n> +\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n> +\t\t\t} else\n> +\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n> +\t\t}\n\nI get lost in these cases. What does it mean if we see S..., for\nexample?\n\nAs an example, if I am understanding correctly, before this patch, if\nI have a submodule-in-submodule:\n\n\t$ find . -name .git\n\t.git\n\tsub/.git\n\tsub/subsub/.git\n\nand I put a stray file in the sub-sub module:\n\n\t$ echo stray-file >sub/subsub/x.o\n\nthen status --porcelain=2 tells me that sub is modified:\n\n\t$ git status --porcelain=2\n\t1 .M S.M. [...] sub\n\nWhat should it say after the patch? Is the XY field \"..\" or \".M\"?\n\nSome tests covering these cases with --porcelain=2 and a brief mention\nin documentation may help.\n\nThanks,\nJonathan\n\ndiff --git i/submodule.c w/submodule.c\nindex ec7e9b1b06..aec1b2cdca 100644\n--- i/submodule.c\n+++ w/submodule.c\n@@ -1083,13 +1083,18 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\tif (buf.buf[0] == 'u' ||\n \t\t buf.buf[0] == '1' ||\n \t\t buf.buf[0] == '2') {\n-\t\t\tif (buf.buf[5] == 'S') {\n-\t\t\t\t/* nested submodule handling */\n-\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n-\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\t\tif (buf.buf[8] == 'U')\n-\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\t} else\n+\t\t\t/*\n+\t\t\t * T XY SSSS:\n+\t\t\t * T = line type, XY = status, SSSS = submodule state\n+\t\t\t */\n+\t\t\tif (buf.len < 1 + 1 + 2 + 1 + 4)\n+\t\t\t\tdie(\"BUG: invalid status --porcelain=2 line %s\",\n+\t\t\t\t buf.buf);\n+\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n+\t\t\t\t/* untracked file */\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\t\t\tif (memcmp(buf.buf + 5, \"S..U\", 4))\n+\t\t\t\t/* other change */\n \t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \t\t}\n \n"},{"id":"315361","messageId":"CAGZ79kYQNqz0BKRU+Sc9XRbuw5N-0stt0Um2h4YEyKNQPLyHUw@mail.gmail.com","threadId":"45476","inReplyTo":"20170324223848.GH31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:12:00Z","receivedAt":"2017-03-25T00:20:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 24, 2017 at 3:38 PM, Jonathan Nieder wrote:\n> It also overlaps work a little better.\n\nmentioned\n\n>> Once we know all information that we care about, we can terminate\n>> the child early. In that case we do not care about its exit code as well.\n>\n> Should this say something about SIGPIPE?\n\ndone\n\n> language nit: s/, no output/: neither output/\n\ndone\n\n> wait_or_whine(cp->pid, cp->argv[0], 1) doesn't do that but is meant for\n> signal handling. Maybe we should rely on SIGPIPE instead (which\n> wait_or_whine always silences) and avoid the kill() call.\n\ndone\n\n> Can there be a test for this case (i.e. having lots of untracked files\n> in the submodule so the child process fills its pipe buffer and has to\n> exit early)?\n\ndone\n"},{"id":"315362","messageId":"CAGZ79kaYSMoMEQ8y+xwLT65suva_DY3ZWKfKSeCPPu2hgtYt-w@mail.gmail.com","threadId":"45476","inReplyTo":"20170324233156.GL31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:25:42Z","receivedAt":"2017-03-25T00:26:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 24, 2017 at 4:31 PM, Jonathan Nieder wrote:\n>\n> Can this overflow the buffer? Submodule state is supposed to be 4\n> characters, so could do\n>\n> /*\n> * T XY SSSS:\n> * T = line type, XY = status, SSSS = submodule state\n> */\n> if (buf.len < 1 + 1 + 2 + 1 + 4)\n> die(\"BUG: invalid status --porcelain=2 line %s\",\n> buf.buf);\n>\n> if (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n> /* untracked file */\n> dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n>\n> if (memcmp(buf.buf + 5, \"S..U\", 4))\n> /* other change */\n> dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n\ngood idea, will use.\n\n\n> }\n>\n>> + /* nested submodule handling */\n>> + if (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n>> + dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n>> + if (buf.buf[8] == 'U')\n>> + dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n>> + } else\n>> + dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n>> + }\n>\n> I get lost in these cases. What does it mean if we see S..., for\n> example?\n\nThat means there is a submodule with no new commits, no modified files\nand no untracked files. (\"Why did we even output this?\", oh because of\nit is in either 2 (moved) or because the hashes are different, i.e.\nwe already added the new HEAD of the submodule)\n\n>\n> Some tests covering these cases with --porcelain=2 and a brief mention\n> in documentation may help.\n\nok, will do.\n"},{"id":"315363","messageId":"20170325003610.15282-1-sbeller@google.com","threadId":"45476","inReplyTo":"20170324182902.19280-1-sbeller@google.com","subject":"[PATCH v7 0/7] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:36:03Z","receivedAt":"2017-03-25T00:36:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v7:\n* taken all of Jonathan minor nits, so patch 1..6 should be good to go\n* patch 7 lacks tests and documentation (according to Jonathan...)\n but as it is the last patch, just fixing a minor detail we can leave it off.\n\nJunio, please take patch 1-6 as usual, I will be out until next Wednesday.\n\n\nThanks,\nStefan\n\nv6:\n* kill the child once we know all information that we ask for, as an optimization\n* reordered the patches for that\n* strbuf_getwholeline instead of its _fd version.\n\nv5:\n* fixed rebase error in the first 2 patches\n* the last 3 patches introduce behavior change outside the scope of is_modified_submodule\n whereas the first 4 ought to just be local changes.\n \nThanks,\nStefan\n\nv4:\n* broken down in a lot of tiny patches.\n\nJonathan wrote:\n> Patch 1/3 is the one that gives me pause, since I hadn't been able to\n> chase down all callers. Would it be feasible to split that into two:\n> one patch to switch to --porcelain=2 but not change behavior, and a\n> separate patch to improve what is stored in the dirty_submodule flags?\n\nThis part is in the latest patch now.\n\nThanks,\nStefan\n\n\nv3:\nThis comes as a series; first I'd like to refactor is_submodule_modified\nto take advantage of the new porcelain=2 plumbing switch to check for changes\nin the submodule.\n\nOn top of the refactoring comes the actual change, which moved the\nrewriting of the submodule change indicator letter to the collection part.\n\nThanks,\nStefan\n\nStefan Beller (8):\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c: use argv_array in is_submodule_modified\n submodule.c: convert is_submodule_modified to use\n strbuf_getwholeline_fd\n submodule.c: port is_submodule_modified to use porcelain 2\n submodule.c: factor out early loop termination in\n is_submodule_modified\n submodule.c: stricter checking for submodules in is_submodule_modified\n short status: improve reporting for submodule changes\n submodule.c: correctly handle nested submodules in\n is_submodule_modified\n\n Documentation/git-status.txt | 9 +++++++\n submodule.c | 56 ++++++++++++++++++++-----------------------\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 13 ++++++++--\n 5 files changed, 116 insertions(+), 37 deletions(-)\n\n-- \n2.12.1.438.gb674c4c09c\n\n"},{"id":"315364","messageId":"20170325003610.15282-2-sbeller@google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"[PATCH 1/7] submodule.c: use argv_array in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:36:04Z","receivedAt":"2017-03-25T00:36:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"struct argv_array is easier to use and maintain.\n\nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \n---\n submodule.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3200b7bb2b..2c667ac95a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n \tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {\n-\t\t\"status\",\n-\t\t\"--porcelain\",\n-\t\tNULL,\n-\t\tNULL,\n-\t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n \tconst char *line, *next_line;\n@@ -1066,10 +1060,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t}\n \tstrbuf_reset(&buf);\n \n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", NULL);\n \tif (ignore_untracked)\n-\t\targv[2] = \"-uno\";\n+\t\targv_array_push(&cp.args, \"-uno\");\n \n-\tcp.argv = argv;\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n-- \n2.12.0.rc1.49.gdeb397943c.dirty\n\n"},{"id":"315365","messageId":"20170325003610.15282-3-sbeller@google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"[PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:36:05Z","receivedAt":"2017-03-25T00:36:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes it easier for a follow up patch.\n\nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \n---\n submodule.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2c667ac95a..93e3fefd39 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1075,16 +1075,16 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tlen = strbuf_read(&buf, cp.out, 1024);\n \tline = buf.buf;\n \twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?')) {\n+\t\tif ((line[0] == '?') && (line[1] == '?'))\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\t\t\tbreak;\n-\t\t} else {\n+\t\telse\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\tif (ignore_untracked ||\n-\t\t\t (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n-\t\t\t\tbreak;\n-\t\t}\n+\n+\t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n+\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\n+\t\t ignore_untracked))\n+\t\t\tbreak;\n+\n \t\tnext_line = strchr(line, '\\n');\n \t\tif (!next_line)\n \t\t\tbreak;\n-- \n2.12.0.rc1.49.gdeb397943c.dirty\n\n"},{"id":"315366","messageId":"20170325003610.15282-4-sbeller@google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"[PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:36:06Z","receivedAt":"2017-03-25T00:36:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of implementing line reading yet again, make use of our beautiful\nlibrary function to read one line. By using strbuf_getwholeline instead\nof strbuf_read, we avoid having to allocate memory for the entire child\nprocess output at once. That is, we limit maximum memory usage.\nAlso we can start processing the output as it comes in, no need to\nwait for all of it.\n\nOnce we know all information that we care about, we can terminate\nthe child early. In that case we do not care about its exit code as well.\nBy just closing our side of the pipe the child process will get a SIGPIPE\nsignal, which it will not report nor do we report it in finish_command,\nac78663b0d (run-command: don't warn on SIGPIPE deaths, 2015-12-29).\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 30 ++++++++++++++----------------\n t/t7506-status-submodule.sh | 18 +++++++++++++++++-\n 2 files changed, 31 insertions(+), 17 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 93e3fefd39..e72781d9f2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1041,12 +1041,12 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n-\tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *fp;\n \tunsigned dirty_submodule = 0;\n-\tconst char *line, *next_line;\n \tconst char *git_dir;\n+\tint ignore_cp_exit_code = 0;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n \tgit_dir = read_gitfile(buf.buf);\n@@ -1072,29 +1072,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tif (start_command(&cp))\n \t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n \n-\tlen = strbuf_read(&buf, cp.out, 1024);\n-\tline = buf.buf;\n-\twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?'))\n+\tfp = xfdopen(cp.out, \"r\");\n+\twhile (strbuf_getwholeline(&buf, fp, '\\n') != EOF) {\n+\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?'))\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\telse\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n \t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\n-\t\t ignore_untracked))\n-\t\t\tbreak;\n-\n-\t\tnext_line = strchr(line, '\\n');\n-\t\tif (!next_line)\n+\t\t ignore_untracked)) {\n+\t\t\t/*\n+\t\t\t * We're not interested in any further information from\n+\t\t\t * the child any more, neither output nor its exit code.\n+\t\t\t */\n+\t\t\tignore_cp_exit_code = 1;\n \t\t\tbreak;\n-\t\tnext_line++;\n-\t\tlen -= (next_line - line);\n-\t\tline = next_line;\n+\t\t}\n \t}\n-\tclose(cp.out);\n+\tfclose(fp);\n \n-\tif (finish_command(&cp))\n+\tif (finish_command(&cp) && !ignore_cp_exit_code)\n \t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex d31b34da83..51f8d0d034 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -177,8 +177,24 @@ test_expect_success 'status with added file in modified submodule with .git file\n \ttest_i18ngrep \"modified: sub (new commits, modified content)\" output\n '\n \n+test_expect_success 'status with a lot of untracked files in the submodule' '\n+\t(\n+\t\tcd sub\n+\t\ti=0 &&\n+\t\twhile test $i -lt 1024\n+\t\tdo\n+\t\t\t>some-file-$i\n+\t\t\ti=$(( $i + 1 ))\n+\t\tdone\n+\t) &&\n+\tgit status --porcelain sub 2>err.actual &&\n+\ttest_must_be_empty err.actual &&\n+\trm err.actual\n+'\n+\n test_expect_success 'rm submodule contents' '\n-\trm -rf sub/* sub/.git\n+\trm -rf sub &&\n+\tmkdir sub\n '\n \n test_expect_success 'status clean (empty submodule dir)' '\n-- \n2.12.0.rc1.49.gdeb397943c.dirty\n\n"},{"id":"315367","messageId":"20170325003610.15282-5-sbeller@google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"[PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:36:07Z","receivedAt":"2017-03-25T00:36:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Migrate 'is_submodule_modified' to the new porcelain format of\ngit-status. This conversion attempts to convert faithfully, i.e.\nthe behavior ought to be exactly the same.\n\nAs the output in the parsing only distinguishes between untracked files\nand the rest, this is easy to port to the new format, as we only\nneed to identify untracked files and the rest is handled in the \"else\"\ncase.\n\nuntracked files are indicated by only a single question mark instead of\ntwo question marks, so the conversion is easy.\n\nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \n---\n submodule.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex e72781d9f2..5865795b9f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1060,7 +1060,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t}\n \tstrbuf_reset(&buf);\n \n-\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", NULL);\n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain=2\", NULL);\n \tif (ignore_untracked)\n \t\targv_array_push(&cp.args, \"-uno\");\n \n@@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n+\t\tdie(\"Could not run 'git status --porcelain=2' in submodule %s\", path);\n \n \tfp = xfdopen(cp.out, \"r\");\n \twhile (strbuf_getwholeline(&buf, fp, '\\n') != EOF) {\n-\t\tif ((buf.buf[0] == '?') && (buf.buf[1] == '?'))\n+\t\t/* regular untracked files */\n+\t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \t\telse\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n@@ -1093,7 +1094,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tfclose(fp);\n \n \tif (finish_command(&cp) && !ignore_cp_exit_code)\n-\t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n+\t\tdie(\"'git status --porcelain=2' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n \treturn dirty_submodule;\n-- \n2.12.0.rc1.49.gdeb397943c.dirty\n\n"},{"id":"315368","messageId":"20170325003610.15282-8-sbeller@google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"[PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:36:10Z","receivedAt":"2017-03-25T00:36:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a nested submodule has untracked files, it would be reported as\n\"modified submodule\" in the superproject, because submodules are not\nparsed correctly in is_submodule_modified as they are bucketed into\nthe modified pile as \"they are not an untracked file\".\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 23 +++++++++++++++++++++--\n t/t3600-rm.sh | 2 +-\n t/t7506-status-submodule.sh | 2 +-\n 3 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex fa21c7bb72..730cc9513a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1078,8 +1078,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t/* regular untracked files */\n \t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\telse\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t buf.buf[0] == '1' ||\n+\t\t buf.buf[0] == '2') {\n+\t\t\t/*\n+\t\t\t * T XY SSSS:\n+\t\t\t * T = line type, XY = status, SSSS = submodule state\n+\t\t\t */\n+\t\t\tif (buf.len < 1 + 1 + 2 + 1 + 4)\n+\t\t\t\tdie(\"BUG: invalid status --porcelain=2 line %s\",\n+\t\t\t\t buf.buf);\n+\n+\t\t\t/* regular unmerged and renamed files */\n+\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n+\t\t\t\t/* nested untracked file */\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\n+\t\t\tif (memcmp(buf.buf + 5, \"S..U\", 4))\n+\t\t\t\t/* other change */\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t}\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n \t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex a6e5c5bd56..b58793448b 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified_inside actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 6d3acb4a5a..ab822c79e6 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -340,7 +340,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain)'\n test_expect_success 'status with untracked file in nested submodule (short)' '\n \tgit -C super status --short >output &&\n \tdiff output - <<-\\EOF\n-\t m sub1\n+\t ? sub1\n \tEOF\n '\n \n-- \n2.12.0.rc1.49.gdeb397943c.dirty\n\n"},{"id":"315369","messageId":"20170325003610.15282-7-sbeller@google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"[PATCH 6/7] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:36:09Z","receivedAt":"2017-03-25T00:36:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If I add an untracked file to a submodule or modify a tracked file,\ncurrently \"git status --short\" treats the change in the same way as\nchanges to the current HEAD of the submodule:\n\n $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit\n $ echo hello >gerrit/plugins/replication/stray-file\n $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap\n $ git -C gerrit status --short\n M plugins/replication\n\nThis is by analogy with ordinary files, where \"M\" represents a change\nthat has not been added yet to the index. But this change cannot be\nadded to the index without entering the submodule, \"git add\"-ing it,\nand running \"git commit\", so the analogy is counterproductive.\n\nIntroduce new status letters \" ?\" and \" m\" for this. These are similar\nto the existing \"??\" and \" M\" but mean that the submodule (not the\nparent project) has new untracked files and modified files, respectively.\nThe user can use \"git add\" and \"git commit\" from within the submodule to\nadd them.\n\nChanges to the submodule's HEAD commit can be recorded in the index with\na plain \"git add -u\" and are shown with \" M\", like today.\n\nTo avoid excessive clutter, show at most one of \" ?\", \" m\", and \" M\" for\nthe submodule. They represent increasing levels of change --- the last\none that applies is shown (e.g., \" m\" if there are both modified files\nand untracked files in the submodule, or \" M\" if the submodule's HEAD\nhas been modified and it has untracked files).\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which shares code with \"status --short\". We only change\n\"git status --short\".\n\nNon-short \"git status\" and \"git status --porcelain=2\" already handle\nthese cases by showing more detail:\n\n $ git -C gerrit status --porcelain=2\n 1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication\n $ git -C gerrit status\n[...]\n modified: plugins/replication (modified content, untracked content)\n\nScripts caring about these distinctions should use --porcelain=2.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \n---\n Documentation/git-status.txt | 9 +++++++\n t/t3600-rm.sh | 18 ++++++++++----\n t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 17 +++++++++++--\n 4 files changed, 94 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..01b457c322 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,13 @@ in which case `XY` are `!!`.\n ! ! ignored\n -------------------------------------------------\n \n+Submodules have more state and instead report\n+\t\tM the submodule has a different HEAD than\n+\t\t recorded in the index\n+\t\tm the submodule has modified content\n+\t\t? the submodule has untracked files\n+\n+\n If -b is used the short-format status is preceded by a line\n \n ## branchname tracking info\n@@ -210,6 +217,8 @@ field from the first filename). Third, filenames containing special\n characters are not specially formatted; no quoting or\n backslash-escaping is performed.\n \n+Any submodule changes are reported as modified `M` instead of `m` or single `?`.\n+\n Porcelain Format Version 2\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5aa6db584c..a6e5c5bd56 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -268,6 +268,14 @@ cat >expect.modified <expect.modified_inside <expect.modified_untracked <expect.cached <actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 51f8d0d034..6d3acb4a5a 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -50,6 +50,15 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with modified file in submodule (short)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n@@ -64,6 +73,14 @@ test_expect_success 'status with added file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with added file in submodule (short)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n@@ -83,6 +100,13 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with untracked file in submodule (short)' '\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t ? sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added and untracked file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \techo \"content\" >sub/new-file &&\n@@ -287,4 +311,37 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \ttest_cmp diff_submodule_actual diff_submodule_expect\n '\n \n+test_expect_success 'setup superproject with untracked file in nested submodule' '\n+\t(\n+\t\tcd super &&\n+\t\tgit clean -dfx &&\n+\t\trm .gitmodules &&\n+\t\tgit submodule add -f ./sub1 &&\n+\t\tgit submodule add -f ./sub2 &&\n+\t\tgit commit -a -m \"messy merge in superproject\" &&\n+\t\t(\n+\t\t\tcd sub1 &&\n+\t\t\tgit submodule add ../sub2 &&\n+\t\t\tgit commit -a -m \"add sub2 to sub1\"\n+\t\t) &&\n+\t\tgit add sub1 &&\n+\t\tgit commit -a -m \"update sub1 to contain nested sub\"\n+\t) &&\n+\techo untracked >super/sub1/sub2/untracked\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\tEOF\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 308cf3779e..0375484962 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -407,6 +407,16 @@ static void wt_longstatus_print_change_data(struct wt_status *s,\n \tstrbuf_release(&twobuf);\n }\n \n+static char short_submodule_status(struct wt_status_change_data *d) {\n+\tif (d->new_submodule_commits)\n+\t\treturn 'M';\n+\tif (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\treturn 'm';\n+\tif (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\treturn '?';\n+\treturn d->worktree_status;\n+}\n+\n static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\n@@ -431,10 +441,13 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t}\n \t\tif (!d->worktree_status)\n \t\t\td->worktree_status = p->status;\n-\t\td->dirty_submodule = p->two->dirty_submodule;\n-\t\tif (S_ISGITLINK(p->two->mode))\n+\t\tif (S_ISGITLINK(p->two->mode)) {\n+\t\t\td->dirty_submodule = p->two->dirty_submodule;\n \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n \t\t\t\t\t\t\t &p->two->oid);\n+\t\t\tif (s->status_format == STATUS_FORMAT_SHORT)\n+\t\t\t\td->worktree_status = short_submodule_status(d);\n+\t\t}\n \n \t\tswitch (p->status) {\n \t\tcase DIFF_STATUS_ADDED:\n-- \n2.12.0.rc1.49.gdeb397943c.dirty\n\n"},{"id":"315370","messageId":"20170325003610.15282-6-sbeller@google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"[PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-25T00:36:08Z","receivedAt":"2017-03-25T00:36:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By having a stricter check in the superproject we catch errors earlier,\ninstead of spawning a child process to tell us.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5865795b9f..fa21c7bb72 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1052,11 +1052,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \tgit_dir = read_gitfile(buf.buf);\n \tif (!git_dir)\n \t\tgit_dir = buf.buf;\n-\tif (!is_directory(git_dir)) {\n+\tif (!is_git_directory(git_dir)) {\n+\t\tif (is_directory(git_dir))\n+\t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n \t\tstrbuf_release(&buf);\n \t\t/* The submodule is not checked out, so it is not modified */\n \t\treturn 0;\n-\n \t}\n \tstrbuf_reset(&buf);\n \n-- \n2.12.0.rc1.49.gdeb397943c.dirty\n\n"},{"id":"315372","messageId":"20170325013541.GN31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"Re: [PATCH v7 0/7] short status: improve reporting for submodule changes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-25T01:35:41Z","receivedAt":"2017-03-25T01:35:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> v7:\n> * taken all of Jonathan minor nits, so patch 1..6 should be good to go\n> * patch 7 lacks tests and documentation (according to Jonathan...)\n> but as it is the last patch, just fixing a minor detail we can leave it off.\n>\n> Junio, please take patch 1-6 as usual, I will be out until next Wednesday.\n[...]\n> Stefan Beller (8):\n> submodule.c: port is_submodule_modified to use porcelain 2\n> submodule.c: use argv_array in is_submodule_modified\n> submodule.c: convert is_submodule_modified to use\n> strbuf_getwholeline_fd\n> submodule.c: port is_submodule_modified to use porcelain 2\n> submodule.c: factor out early loop termination in\n> is_submodule_modified\n> submodule.c: stricter checking for submodules in is_submodule_modified\n> short status: improve reporting for submodule changes\n> submodule.c: correctly handle nested submodules in\n> is_submodule_modified\n>\n> Documentation/git-status.txt | 9 +++++++\n> submodule.c | 56 ++++++++++++++++++++-----------------------\n> t/t3600-rm.sh | 18 ++++++++++----\n> t/t7506-status-submodule.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++\n> wt-status.c | 13 ++++++++--\n> 5 files changed, 116 insertions(+), 37 deletions(-)\n\nPatches 1-6 are\n\nReviewed-by: Jonathan Nieder \n\nThe effect of patch 7 on --porcelain=2 output is subtle enough that I\ndon't feel I understand it. I think it heads in a good direction but\nindeed, some tests could help to illustrate the desired behavior.\n\nThanks for your patient work.\nJonathan\n"},{"id":"315537","messageId":"xmqqpoh2z93q.fsf@gitster.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170325003610.15282-8-sbeller@google.com","subject":"Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-27T21:46:17Z","receivedAt":"2017-03-27T21:53:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> When a nested submodule has untracked files, it would be reported as\n> \"modified submodule\" in the superproject, because submodules are not\n> parsed correctly in is_submodule_modified as they are bucketed into\n> the modified pile as \"they are not an untracked file\".\n\nI cannot quite parse the above.\n\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 23 +++++++++++++++++++++--\n> t/t3600-rm.sh | 2 +-\n> t/t7506-status-submodule.sh | 2 +-\n> 3 files changed, 23 insertions(+), 4 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index fa21c7bb72..730cc9513a 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1078,8 +1078,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n> \t\t/* regular untracked files */\n> \t\tif (buf.buf[0] == '?')\n> \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n> -\t\telse\n> -\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n> +\n> +\t\tif (buf.buf[0] == 'u' ||\n> +\t\t buf.buf[0] == '1' ||\n> +\t\t buf.buf[0] == '2') {\n> +\t\t\t/*\n> +\t\t\t * T XY SSSS:\n> +\t\t\t * T = line type, XY = status, SSSS = submodule state\n> +\t\t\t */\n> +\t\t\tif (buf.len < 1 + 1 + 2 + 1 + 4)\n> +\t\t\t\tdie(\"BUG: invalid status --porcelain=2 line %s\",\n> +\t\t\t\t buf.buf);\n> +\n> +\t\t\t/* regular unmerged and renamed files */\n> +\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n> +\t\t\t\t/* nested untracked file */\n> +\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n\nOK, we have untracked one.\n\n> +\t\t\tif (memcmp(buf.buf + 5, \"S..U\", 4))\n> +\t\t\t\t/* other change */\n> +\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n\nAnd for other cases like C at buf.buf[6] or M at buf.buf[7],\ni.e. where the submodule not just has untracked files but has real\nchanges, we say it is truly MODIFIED here.\n\nIf there are changes to paths that is not a submodule but a tracked\nfile in the submodule in question would have N at buf.buf[5] and is\nalso caught with the same \"not S..U so that's MODIFIED\" logic.\n\nOK.\n\nShouldn't this done as part of 4/7 where is_submodule_modified()\nstarts reading from the porcelain v2 output? 4/7 does adjust for\nthe change from double question mark (porcelain v1) to a single one\nfor untracked, but at the same time it needs to prepare for these\n'u' (unmerged), '1' (normal modification) and '2' (mods with rename)\nto appear in the output, no?\n\nIOW, with 4/7 and 7/7 done as separate steps, isn't the system\nbroken between these steps?\n\n"},{"id":"315549","messageId":"20170328010533.GQ31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"xmqqpoh2z93q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-28T01:05:33Z","receivedAt":"2017-03-28T01:06:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Shouldn't this done as part of 4/7 where is_submodule_modified()\n> starts reading from the porcelain v2 output? 4/7 does adjust for\n> the change from double question mark (porcelain v1) to a single one\n> for untracked, but at the same time it needs to prepare for these\n> 'u' (unmerged), '1' (normal modification) and '2' (mods with rename)\n> to appear in the output, no?\n>\n> IOW, with 4/7 and 7/7 done as separate steps, isn't the system\n> broken between these steps?\n\nNo. Both before and after patch 4, this code has to determine two\ndetails from a submodule:\n\n 1. Does it have untracked files?\n 2. Does it have any modifications to tracked files (including\n submodules)?\n\nUsing porcelain v1 format, (1) is represented by a \"??\" line and (2)\nis represented by any other line. Using porcelain v2 format, (1) is\nrepresented by a \"u\" line and (2) is represented by any other line.\n\nSo patch 4 does not intend to change behavior.\n\nThis patch 7 is trying to do something more subtle. Suppose I have a\nsuperproject 'parent', with a submodule 'parent/sub', which itself\ncontains a submodule 'parent/sub/subsub'. Now suppose I run, from\nwithin 'parent':\n\n\techo hi >sub/subsub/stray-file\n\nBoth before and after patch 4, if I run \"git status\" from 'parent'\nthen I will learn that \"sub\" was modified. \"git status\" within 'sub'\nwould tell me that \"subsub\" has an untracked file.\n\nBut from the end user's point of view, even when running in \"parent\",\nwhat I want to know is that there is an untracked file. Treating it\nas a modification instead of untracked file is confusing and does\nnot answer the user's actual question. That is what patch 7 tries to\nfix.\n\nIn other words, patch 7 is about changing that list of two questions\nfrom before. Something like\n\n 1. Does it or any submodule contained within it have untracked files,\n that I could add with \"git add -N --recurse-submodules\"?\n\n 2. Does it or any submodule contained within it have modified files,\n that I could add with \"git add -u --recurse-submodules\"?\n\n 3. Does it or any submodule contained within it have a changed HEAD,\n that I could also add with \"git add -u --recurse-submodules\"?\n\nQuestion (3) didn't come up before because when there are no nested\nsubmodules, the diff machinery answers it (saving us from getting the\nanswer from the status --porcelain we recurse to).\n\nThanks and hope that helps,\nJonathan\n"},{"id":"315622","messageId":"CAGZ79kYmWW5tp26aCkWeYjyp8KeP2jcL43JGiPO3pHW=wo7uzw@mail.gmail.com","threadId":"45476","inReplyTo":"xmqqpoh2z93q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-28T21:20:08Z","receivedAt":"2017-03-28T21:20:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Mar 27, 2017 at 2:46 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> When a nested submodule has untracked files, it would be reported as\n>> \"modified submodule\" in the superproject, because submodules are not\n>> parsed correctly in is_submodule_modified as they are bucketed into\n>> the modified pile as \"they are not an untracked file\".\n>\n> I cannot quite parse the above.\n\nI tried to describe the example Jonathan gave in his reply in a shorter form.\nI'll\n>> + /* regular unmerged and renamed files */\n>> + if (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n>> + /* nested untracked file */\n>> + dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n>\n> OK, we have untracked one.\n>\n>> + if (memcmp(buf.buf + 5, \"S..U\", 4))\n>> + /* other change */\n>> + dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n>\n> And for other cases like C at buf.buf[6] or M at buf.buf[7],\n> i.e. where the submodule not just has untracked files but has real\n> changes, we say it is truly MODIFIED here.\n>\n> If there are changes to paths that is not a submodule but a tracked\n> file in the submodule in question would have N at buf.buf[5] and is\n> also caught with the same \"not S..U so that's MODIFIED\" logic.\n>\n> OK.\n\nok, thanks for checking.\n\n>\n> Shouldn't this done as part of 4/7 where is_submodule_modified()\n> starts reading from the porcelain v2 output?\n\nI did that in an earlier version of the series. However the change from\nporcelain=1 to 2 should not be observable by the end user.\n\n> 4/7 does adjust for\n> the change from double question mark (porcelain v1) to a single one\n> for untracked, but at the same time it needs to prepare for these\n> 'u' (unmerged), '1' (normal modification) and '2' (mods with rename)\n> to appear in the output, no?\n\nNo, up to patch 5/7 we only had refactors, no user visible changes\nintended. And until then we had \"has untracked files\" and \"everything\nelse\". The nice part of the conversion was to cover the \"everything else\"\npart easily.\n\nThis patch transforms it into \"has untracked files or submodule reports\nuntracked files (possibly nested)\" and \"everything else\", but the former\nis more complicated to compute.\n\n> IOW, with 4/7 and 7/7 done as separate steps, isn't the system\n> broken between these steps?\n\nNo, see Jonathans answer.\n\nWe could argue, that 6/7 and 7/7 done as separate steps is broken,\nbecause since 6/7 we promise to report untracked files inside\nsubmodules as \" ?\", but we do not report them properly for nested\nsubmodules.\n\nSuppose I have a superproject 'parent', with two submodules 'parent/sub'\nand 'parent/sub1'. 'parent/sub' itself contains a submodule 'parent/sub/subsub'.\nNow suppose I run, from within 'parent':\n\n echo hi >sub/subsub/stray-file\n echo hi >sub1/stray-file\n\nwith patches 1..5:\n git status --short\n M sub\n M sub1\n\npatch 6:\n git status --short\n m sub\n ? sub1\n\nwith patch 7:\n git status --short\n ? sub\n ? sub1\n\nThe documentation update in patch 6 is unclear what to expect from\nnested submodules, so that documentation is technically not wrong,\nbut maybe misleading.\n\nI want to resend patch 7 with a documentation update as well as tests.\n\nThanks,\nStefan\n"},{"id":"315628","messageId":"20170328230938.9887-1-sbeller@google.com","threadId":"45476","inReplyTo":"20170325003610.15282-1-sbeller@google.com","subject":"[PATCH v8 0/7] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-28T23:09:36Z","receivedAt":"2017-03-28T23:10:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v8:\n* This is a resend of the last two patches, i.e. these two patches apply\n at 5c896f7c3ec (origin/sb/submodule-short-status^^)\n* below is a diff of this patch series against origin/sb/submodule-short-status\n* add tests showing the subtle bug fix in case of nesting.\n* add a bit of documentation\n\nv7:\nprevious work at\nhttps://public-inbox.org/git/20170325003610.15282-1-sbeller@google.com/\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex 01b457c322..452c6eb875 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -187,6 +187,8 @@ Submodules have more state and instead report\n \t\tm the submodule has modified content\n \t\t? the submodule has untracked files\n \n+Note that 'm' and '?' are applied recursively, e.g. if a nested submodule\n+in a submodule contains an untracked file, this is reported as '?' as well.\n \n If -b is used the short-format status is preceded by a line\n \ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex ab822c79e6..4d6d8f6817 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -327,20 +327,65 @@ test_expect_success 'setup superproject with untracked file in nested submodule'\n \t\tgit add sub1 &&\n \t\tgit commit -a -m \"update sub1 to contain nested sub\"\n \t) &&\n-\techo untracked >super/sub1/sub2/untracked\n+\techo \"{ \\$7=\\\"HASH\\\"; \\$8=\\\"HASH\\\"; print }\" >suppress_hashes.awk &&\n+\techo \"suppress_hashes.awk\" >>.git/info/exclude &&\n+\techo \"output2\" >>.git/info/exclude &&\n+\techo content >super/sub1/sub2/file &&\n+\techo content >super/sub2/file\n '\n \n test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n \tgit -C super status --porcelain >output &&\n \tdiff output - <<-\\EOF\n \t M sub1\n+\t M sub2\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '\n+\tgit -C super status --porcelain=2 >output &&\n+\tawk -f suppress_hashes.awk output >output2 &&\n+\tdiff output2 - <<-\\EOF\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub1\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub2\n \tEOF\n '\n \n test_expect_success 'status with untracked file in nested submodule (short)' '\n \tgit -C super status --short >output &&\n \tdiff output - <<-\\EOF\n-\t ? sub1\n+\t m sub1\n+\t ? sub2\n+\tEOF\n+'\n+\n+test_expect_success 'setup superproject with modified file in nested submodule' '\n+\tgit -C super/sub1/sub2 add file &&\n+\tgit -C super/sub2 add file\n+'\n+\n+test_expect_success 'status with added file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\t M sub2\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in nested submodule (porcelain=2)' '\n+\tgit -C super status --porcelain=2 >output &&\n+\tawk -f suppress_hashes.awk output >output2 &&\n+\tdiff output2 - <<-\\EOF\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub2\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\t m sub2\n \tEOF\n '\n \n"},{"id":"315629","messageId":"20170328230938.9887-3-sbeller@google.com","threadId":"45476","inReplyTo":"20170328230938.9887-1-sbeller@google.com","subject":"[PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-28T23:09:38Z","receivedAt":"2017-03-28T23:10:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Suppose I have a superproject 'super', with two submodules 'super/sub'\nand 'super/sub1'. 'super/sub' itself contains a submodule\n'super/sub/subsub'. Now suppose I run, from within 'super':\n\n echo hi >sub/subsub/stray-file\n echo hi >sub1/stray-file\n\nCurrently we get would see the following output in git-status:\n\n git status --short\n m sub\n ? sub1\n\nWith this patch applied, the untracked file in the nested submodule is\nturned into an untracked file on the 'super' level as well.\n\n git status --short\n ? sub\n ? sub1\n\nThis doesn't change the output of 'git status --porcelain=1' for nested\nsubmodules, because its output is always ' M' for either untracked files\nor local modifications no matter the nesting level of the submodule.\n\n'git status --porcelain=2' is affected by this change in a nested\nsubmodule, though. Without this patch it would report the direct submodule\nas modified and having no untracked files. With this patch it would report\nuntracked files. Chalk this up as a bug fix.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-status.txt | 2 ++\n submodule.c | 23 +++++++++++++++++++++--\n t/t3600-rm.sh | 2 +-\n t/t7506-status-submodule.sh | 2 +-\n 4 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex 01b457c322..452c6eb875 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -187,6 +187,8 @@ Submodules have more state and instead report\n \t\tm the submodule has modified content\n \t\t? the submodule has untracked files\n \n+Note that 'm' and '?' are applied recursively, e.g. if a nested submodule\n+in a submodule contains an untracked file, this is reported as '?' as well.\n \n If -b is used the short-format status is preceded by a line\n \ndiff --git a/submodule.c b/submodule.c\nindex fa21c7bb72..730cc9513a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1078,8 +1078,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t/* regular untracked files */\n \t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\telse\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t buf.buf[0] == '1' ||\n+\t\t buf.buf[0] == '2') {\n+\t\t\t/*\n+\t\t\t * T XY SSSS:\n+\t\t\t * T = line type, XY = status, SSSS = submodule state\n+\t\t\t */\n+\t\t\tif (buf.len < 1 + 1 + 2 + 1 + 4)\n+\t\t\t\tdie(\"BUG: invalid status --porcelain=2 line %s\",\n+\t\t\t\t buf.buf);\n+\n+\t\t\t/* regular unmerged and renamed files */\n+\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n+\t\t\t\t/* nested untracked file */\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\n+\t\t\tif (memcmp(buf.buf + 5, \"S..U\", 4))\n+\t\t\t\t/* other change */\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t}\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n \t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex a6e5c5bd56..b58793448b 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified_inside actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex fd057751df..4d6d8f6817 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -346,7 +346,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain=2\n \tgit -C super status --porcelain=2 >output &&\n \tawk -f suppress_hashes.awk output >output2 &&\n \tdiff output2 - <<-\\EOF\n-\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub1\n \t1 .M S..U 160000 160000 160000 HASH HASH sub2\n \tEOF\n '\n-- \n2.12.1.438.g67623a8358\n\n"},{"id":"315630","messageId":"20170328230938.9887-2-sbeller@google.com","threadId":"45476","inReplyTo":"20170328230938.9887-1-sbeller@google.com","subject":"[PATCH 1/2] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-28T23:09:37Z","receivedAt":"2017-03-28T23:10:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If I add an untracked file to a submodule or modify a tracked file,\ncurrently \"git status --short\" treats the change in the same way as\nchanges to the current HEAD of the submodule:\n\n $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit\n $ echo hello >gerrit/plugins/replication/stray-file\n $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap\n $ git -C gerrit status --short\n M plugins/replication\n\nThis is by analogy with ordinary files, where \"M\" represents a change\nthat has not been added yet to the index. But this change cannot be\nadded to the index without entering the submodule, \"git add\"-ing it,\nand running \"git commit\", so the analogy is counterproductive.\n\nIntroduce new status letters \" ?\" and \" m\" for this. These are similar\nto the existing \"??\" and \" M\" but mean that the submodule (not the\nparent project) has new untracked files and modified files, respectively.\nThe user can use \"git add\" and \"git commit\" from within the submodule to\nadd them.\n\nChanges to the submodule's HEAD commit can be recorded in the index with\na plain \"git add -u\" and are shown with \" M\", like today.\n\nTo avoid excessive clutter, show at most one of \" ?\", \" m\", and \" M\" for\nthe submodule. They represent increasing levels of change --- the last\none that applies is shown (e.g., \" m\" if there are both modified files\nand untracked files in the submodule, or \" M\" if the submodule's HEAD\nhas been modified and it has untracked files).\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which shares code with \"status --short\". We only change\n\"git status --short\".\n\nNon-short \"git status\" and \"git status --porcelain=2\" already handle\nthese cases by showing more detail:\n\n $ git -C gerrit status --porcelain=2\n 1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication\n $ git -C gerrit status\n[...]\n modified: plugins/replication (modified content, untracked content)\n\nScripts caring about these distinctions should use --porcelain=2.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \n---\n Documentation/git-status.txt | 9 ++++\n t/t3600-rm.sh | 18 +++++---\n t/t7506-status-submodule.sh | 102 +++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 17 +++++++-\n 4 files changed, 139 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..01b457c322 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,13 @@ in which case `XY` are `!!`.\n ! ! ignored\n -------------------------------------------------\n \n+Submodules have more state and instead report\n+\t\tM the submodule has a different HEAD than\n+\t\t recorded in the index\n+\t\tm the submodule has modified content\n+\t\t? the submodule has untracked files\n+\n+\n If -b is used the short-format status is preceded by a line\n \n ## branchname tracking info\n@@ -210,6 +217,8 @@ field from the first filename). Third, filenames containing special\n characters are not specially formatted; no quoting or\n backslash-escaping is performed.\n \n+Any submodule changes are reported as modified `M` instead of `m` or single `?`.\n+\n Porcelain Format Version 2\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5aa6db584c..a6e5c5bd56 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -268,6 +268,14 @@ cat >expect.modified <expect.modified_inside <expect.modified_untracked <expect.cached <actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 51f8d0d034..fd057751df 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -50,6 +50,15 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with modified file in submodule (short)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n@@ -64,6 +73,14 @@ test_expect_success 'status with added file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with added file in submodule (short)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n@@ -83,6 +100,13 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with untracked file in submodule (short)' '\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t ? sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added and untracked file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \techo \"content\" >sub/new-file &&\n@@ -287,4 +311,82 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \ttest_cmp diff_submodule_actual diff_submodule_expect\n '\n \n+test_expect_success 'setup superproject with untracked file in nested submodule' '\n+\t(\n+\t\tcd super &&\n+\t\tgit clean -dfx &&\n+\t\trm .gitmodules &&\n+\t\tgit submodule add -f ./sub1 &&\n+\t\tgit submodule add -f ./sub2 &&\n+\t\tgit commit -a -m \"messy merge in superproject\" &&\n+\t\t(\n+\t\t\tcd sub1 &&\n+\t\t\tgit submodule add ../sub2 &&\n+\t\t\tgit commit -a -m \"add sub2 to sub1\"\n+\t\t) &&\n+\t\tgit add sub1 &&\n+\t\tgit commit -a -m \"update sub1 to contain nested sub\"\n+\t) &&\n+\techo \"{ \\$7=\\\"HASH\\\"; \\$8=\\\"HASH\\\"; print }\" >suppress_hashes.awk &&\n+\techo \"suppress_hashes.awk\" >>.git/info/exclude &&\n+\techo \"output2\" >>.git/info/exclude &&\n+\techo content >super/sub1/sub2/file &&\n+\techo content >super/sub2/file\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\t M sub2\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '\n+\tgit -C super status --porcelain=2 >output &&\n+\tawk -f suppress_hashes.awk output >output2 &&\n+\tdiff output2 - <<-\\EOF\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub2\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\t ? sub2\n+\tEOF\n+'\n+\n+test_expect_success 'setup superproject with modified file in nested submodule' '\n+\tgit -C super/sub1/sub2 add file &&\n+\tgit -C super/sub2 add file\n+'\n+\n+test_expect_success 'status with added file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\t M sub2\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in nested submodule (porcelain=2)' '\n+\tgit -C super status --porcelain=2 >output &&\n+\tawk -f suppress_hashes.awk output >output2 &&\n+\tdiff output2 - <<-\\EOF\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub2\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\t m sub2\n+\tEOF\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 308cf3779e..0375484962 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -407,6 +407,16 @@ static void wt_longstatus_print_change_data(struct wt_status *s,\n \tstrbuf_release(&twobuf);\n }\n \n+static char short_submodule_status(struct wt_status_change_data *d) {\n+\tif (d->new_submodule_commits)\n+\t\treturn 'M';\n+\tif (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\treturn 'm';\n+\tif (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\treturn '?';\n+\treturn d->worktree_status;\n+}\n+\n static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\n@@ -431,10 +441,13 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t}\n \t\tif (!d->worktree_status)\n \t\t\td->worktree_status = p->status;\n-\t\td->dirty_submodule = p->two->dirty_submodule;\n-\t\tif (S_ISGITLINK(p->two->mode))\n+\t\tif (S_ISGITLINK(p->two->mode)) {\n+\t\t\td->dirty_submodule = p->two->dirty_submodule;\n \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n \t\t\t\t\t\t\t &p->two->oid);\n+\t\t\tif (s->status_format == STATUS_FORMAT_SHORT)\n+\t\t\t\td->worktree_status = short_submodule_status(d);\n+\t\t}\n \n \t\tswitch (p->status) {\n \t\tcase DIFF_STATUS_ADDED:\n-- \n2.12.1.438.g67623a8358\n\n"},{"id":"315633","messageId":"20170328232446.GT31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170328230938.9887-2-sbeller@google.com","subject":"Re: [PATCH 1/2] short status: improve reporting for submodule changes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-28T23:24:46Z","receivedAt":"2017-03-28T23:24:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n[...]\n> +++ b/t/t7506-status-submodule.sh\n[...]\n> @@ -287,4 +311,82 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n> \ttest_cmp diff_submodule_actual diff_submodule_expect\n> '\n> \n> +test_expect_success 'setup superproject with untracked file in nested submodule' '\n> +\t(\n> +\t\tcd super &&\n> +\t\tgit clean -dfx &&\n> +\t\trm .gitmodules &&\n> +\t\tgit submodule add -f ./sub1 &&\n> +\t\tgit submodule add -f ./sub2 &&\n> +\t\tgit commit -a -m \"messy merge in superproject\" &&\n> +\t\t(\n> +\t\t\tcd sub1 &&\n> +\t\t\tgit submodule add ../sub2 &&\n> +\t\t\tgit commit -a -m \"add sub2 to sub1\"\n> +\t\t) &&\n> +\t\tgit add sub1 &&\n> +\t\tgit commit -a -m \"update sub1 to contain nested sub\"\n> +\t) &&\n> +\techo \"{ \\$7=\\\"HASH\\\"; \\$8=\\\"HASH\\\"; print }\" >suppress_hashes.awk &&\n> +\techo \"suppress_hashes.awk\" >>.git/info/exclude &&\n\nI had some trouble following what suppress_hashes.awk does at first.\n\nSome other examples that could be worth imitating:\n\n- diff-lib.sh has some sanitize_diff functions using 'sed'\n- t4202 has a sanitize_output functino, also using 'sed'\n- grepping for $_x40 finds some other examples (these will be fun to\n change when the hash function changes, but at least they're easy to\n find).\n\nThe main general idea I have in mind is that providing a function at the\ntop of the file for the test to use instead of a script that interacts\nwith what git is tracking can make things easier to understand.\n\nAlso, could there be a comment with a diagram describing the setup\n(sub1 vs sub2, etc)?\n\n[...]\n> +test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n> +\tgit -C super status --porcelain >output &&\n> +\tdiff output - <<-\\EOF\n> +\t M sub1\n> +\t M sub2\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '\n> +\tgit -C super status --porcelain=2 >output &&\n> +\tawk -f suppress_hashes.awk output >output2 &&\n> +\tdiff output2 - <<-\\EOF\n> +\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n> +\t1 .M S..U 160000 160000 160000 HASH HASH sub2\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'status with untracked file in nested submodule (short)' '\n> +\tgit -C super status --short >output &&\n> +\tdiff output - <<-\\EOF\n> +\t m sub1\n> +\t ? sub2\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'setup superproject with modified file in nested submodule' '\n> +\tgit -C super/sub1/sub2 add file &&\n> +\tgit -C super/sub2 add file\n> +'\n> +\n> +test_expect_success 'status with added file in nested submodule (porcelain)' '\n> +\tgit -C super status --porcelain >output &&\n> +\tdiff output - <<-\\EOF\n> +\t M sub1\n> +\t M sub2\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'status with added file in nested submodule (porcelain=2)' '\n> +\tgit -C super status --porcelain=2 >output &&\n> +\tawk -f suppress_hashes.awk output >output2 &&\n> +\tdiff output2 - <<-\\EOF\n> +\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n> +\t1 .M S.M. 160000 160000 160000 HASH HASH sub2\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'status with added file in nested submodule (short)' '\n> +\tgit -C super status --short >output &&\n> +\tdiff output - <<-\\EOF\n> +\t m sub1\n> +\t m sub2\n> +\tEOF\n\nHow does ordinary non --short \"git status\" handle these cases?\n\nWhat should happen when there is a new untracked repository within a\nsubmodule?\n\nWhat should happen if there is both a modified tracked file and an\nuntracked file in a sub-submodule?\n\n\n> +'\n> +\n> test_done\n\nVery nice. Thanks --- this was exactly what I was looking for.\n\nThe rest looks good.\n\nSincerely,\nJonathan\n"},{"id":"315634","messageId":"20170328234217.GU31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170328230938.9887-3-sbeller@google.com","subject":"Re: [PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-28T23:42:17Z","receivedAt":"2017-03-28T23:42:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Suppose I have a superproject 'super', with two submodules 'super/sub'\n> and 'super/sub1'. 'super/sub' itself contains a submodule\n> 'super/sub/subsub'. Now suppose I run, from within 'super':\n>\n> echo hi >sub/subsub/stray-file\n> echo hi >sub1/stray-file\n>\n> Currently we get would see the following output in git-status:\n>\n> git status --short\n> m sub\n> ? sub1\n>\n> With this patch applied, the untracked file in the nested submodule is\n> turned into an untracked file on the 'super' level as well.\n\ns/turned into/displayed as/\n\n> git status --short\n> ? sub\n> ? sub1\n>\n> This doesn't change the output of 'git status --porcelain=1' for nested\n> submodules, because its output is always ' M' for either untracked files\n> or local modifications no matter the nesting level of the submodule.\n>\n> 'git status --porcelain=2' is affected by this change in a nested\n> submodule, though. Without this patch it would report the direct submodule\n> as modified and having no untracked files. With this patch it would report\n> untracked files. Chalk this up as a bug fix.\n\nI think that's reasonable, and the documentation does a good job of\ndescribing it.\n\nDoes this have any effect on the default mode of 'git status' (without\n--short or --porcelain)?\n\n[...]\n> --- a/Documentation/git-status.txt\n> +++ b/Documentation/git-status.txt\n> @@ -187,6 +187,8 @@ Submodules have more state and instead report\n> \t\tm the submodule has modified content\n> \t\t? the submodule has untracked files\n> \n> +Note that 'm' and '?' are applied recursively, e.g. if a nested submodule\n> +in a submodule contains an untracked file, this is reported as '?' as well.\n\nLanguage nits:\n\n* Can simplify by leaving out \"Note that \".\n* s/, e\\.g\\./. For example,/\n\nShould this say a brief word about rationale? The obvious way to\ndescribe it would involve \"git add --recurse-submodules\", which alas\ndoesn't exist yet. But we could say\n\n this is reported as '?' as well,\n since the change cannot be added using \"git add -u\" from within any of the\n submodules.\n\n[...]\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1078,8 +1078,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n> \t\t/* regular untracked files */\n> \t\tif (buf.buf[0] == '?')\n> \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n> -\t\telse\n> -\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n> +\n> +\t\tif (buf.buf[0] == 'u' ||\n> +\t\t buf.buf[0] == '1' ||\n> +\t\t buf.buf[0] == '2') {\n> +\t\t\t/*\n> +\t\t\t * T XY SSSS:\n> +\t\t\t * T = line type, XY = status, SSSS = submodule state\n> +\t\t\t */\n> +\t\t\tif (buf.len < 1 + 1 + 2 + 1 + 4)\n\noptional: Can shorten:\n\n\t\t\t/* T = line type, XY = status, SSSS = submodule state */\n\t\t\tif (buf.len < strlen(\"T XY SSSS\"))\n\t\t\t\t...\n\nThat produces the same code at run time because compilers are able to\ninline the strlen of a constant. What you already have also seems\nsensible, though.\n\n[...]\n> +\t\t\t\tdie(\"BUG: invalid status --porcelain=2 line %s\",\n> +\t\t\t\t buf.buf);\n> +\n> +\t\t\t/* regular unmerged and renamed files */\n> +\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n> +\t\t\t\t/* nested untracked file */\n> +\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n> +\n> +\t\t\tif (memcmp(buf.buf + 5, \"S..U\", 4))\n> +\t\t\t\t/* other change */\n> +\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n\nsanity check: What does this do for a \"2\" line indicating a sub-submodule\nthat has been renamed that contains an untracked file? Do we need to\nrely on some other indication to show this as a change?\n\nEnumerating some more cases, since I keep finding myself getting lost:\n\n - if the HEAD commit of \"sub\" changes, we show this as \" M sub\".\n What should we show if the HEAD commit of \"sub/subsub\" changes?\n I think this should be \" m\".\n\n - if \"sub\" is renamed, we show this as \"R sub -> newname\".\n What should we show if \"sub/subsub\" is renamed? It is tempting\n to show this as \" m\".\n\n - if \"sub\" is deleted, we show this as \"D sub\". What should we\n show if \"sub/subsub\" is deleted? I think this is \" m\".\n\nIt keeps getting more complicated, but I think this is making sense.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"315711","messageId":"CAGZ79kbKgkHNjPXusAeoG112WEZZ801suxmcbei8hF7++5m-9A@mail.gmail.com","threadId":"45476","inReplyTo":"20170328234217.GU31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-29T22:00:42Z","receivedAt":"2017-03-29T22:00:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> sanity check: What does this do for a \"2\" line indicating a sub-submodule\n> that has been renamed that contains an untracked file? Do we need to\n> rely on some other indication to show this as a change?\n\nOh. :(\n\nIn case of 'u' and '2' we need to set DIRTY_SUBMODULE_MODIFIED\nadditionally. will fix in a reroll.\n\n>\n> Enumerating some more cases, since I keep finding myself getting lost:\n>\n> - if the HEAD commit of \"sub\" changes, we show this as \" M sub\".\n> What should we show if the HEAD commit of \"sub/subsub\" changes?\n> I think this should be \" m\".\n>\n> - if \"sub\" is renamed, we show this as \"R sub -> newname\".\n> What should we show if \"sub/subsub\" is renamed? It is tempting\n> to show this as \" m\".\n>\n> - if \"sub\" is deleted, we show this as \"D sub\". What should we\n> show if \"sub/subsub\" is deleted? I think this is \" m\".\n\nAll these cases are ' m', which I agree with, as it is a \"modification\nthat cannot be git-add'ed in the superproject\".\n\nWe might be inclined to later come up with ' d' for a deleted nested\nsubmodule, but I do not think it is worth the effort.\n\nThanks,\nStefan\n"},{"id":"315715","messageId":"20170329222616.11077-1-sbeller@google.com","threadId":"45476","inReplyTo":"20170328230938.9887-1-sbeller@google.com","subject":"[PATCHv9 (6,7)/7] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-29T22:26:14Z","receivedAt":"2017-03-29T22:26:28Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v9:\n* This is a resend of the last two patches, i.e. these two patches apply\n at 5c896f7c3ec (origin/sb/submodule-short-status^^)\n* below is a diff of this patch series against origin/sb/submodule-short-status\n* better tests, refined documentation, thanks for the review, Jonathan!\n\nThanks,\nStefan\n\nprevious work:\nhttps://public-inbox.org/git/20170328230938.9887-1-sbeller@google.com/\n\nStefan Beller (2):\n short status: improve reporting for submodule changes\n submodule.c: correctly handle nested submodules in\n is_submodule_modified\n\n Documentation/git-status.txt | 13 +++++\n submodule.c | 21 +++++++-\n t/t3600-rm.sh | 18 +++++--\n t/t7506-status-submodule.sh | 117 +++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 17 ++++++-\n 5 files changed, 177 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex 01b457c322..d70abc6afe 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -186,7 +186,11 @@ Submodules have more state and instead report\n \t\t recorded in the index\n \t\tm the submodule has modified content\n \t\t? the submodule has untracked files\n+since modified content or untracked files in a submodule cannot be added\n+via `git add` in the superproject to prepare a commit.\n \n+'m' and '?' are applied recursively. For example if a nested submodule\n+in a submodule contains an untracked file, this is reported as '?' as well.\n \n If -b is used the short-format status is preceded by a line\n \ndiff --git a/submodule.c b/submodule.c\nindex 730cc9513a..3da65100e3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1082,20 +1082,18 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\tif (buf.buf[0] == 'u' ||\n \t\t buf.buf[0] == '1' ||\n \t\t buf.buf[0] == '2') {\n-\t\t\t/*\n-\t\t\t * T XY SSSS:\n-\t\t\t * T = line type, XY = status, SSSS = submodule state\n-\t\t\t */\n-\t\t\tif (buf.len < 1 + 1 + 2 + 1 + 4)\n+\t\t\t/* T = line type, XY = status, SSSS = submodule state */\n+\t\t\tif (buf.len < strlen(\"T XY SSSS\"))\n \t\t\t\tdie(\"BUG: invalid status --porcelain=2 line %s\",\n \t\t\t\t buf.buf);\n \n-\t\t\t/* regular unmerged and renamed files */\n \t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n \t\t\t\t/* nested untracked file */\n \t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \n-\t\t\tif (memcmp(buf.buf + 5, \"S..U\", 4))\n+\t\t\tif (buf.buf[0] == 'u' ||\n+\t\t\t buf.buf[0] == '2' ||\n+\t\t\t memcmp(buf.buf + 5, \"S..U\", 4))\n \t\t\t\t/* other change */\n \t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \t\t}\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex ab822c79e6..055c90736e 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -17,6 +17,12 @@ test_create_repo_with_commit () {\n \t)\n }\n \n+sanitize_output () {\n+\tsed -e \"s/$_x40/HASH/\" -e \"s/$_x40/HASH/\" output >output2 &&\n+\tmv output2 output\n+}\n+\n+\n test_expect_success 'setup' '\n \ttest_create_repo_with_commit sub &&\n \techo output > .gitignore &&\n@@ -311,6 +317,10 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \ttest_cmp diff_submodule_actual diff_submodule_expect\n '\n \n+# We'll setup different cases for further testing:\n+# sub1 will contain a nested submodule,\n+# sub2 will have an untracked file\n+# sub3 will have an untracked repository\n test_expect_success 'setup superproject with untracked file in nested submodule' '\n \t(\n \t\tcd super &&\n@@ -318,6 +328,7 @@ test_expect_success 'setup superproject with untracked file in nested submodule'\n \t\trm .gitmodules &&\n \t\tgit submodule add -f ./sub1 &&\n \t\tgit submodule add -f ./sub2 &&\n+\t\tgit submodule add -f ./sub1 sub3 &&\n \t\tgit commit -a -m \"messy merge in superproject\" &&\n \t\t(\n \t\t\tcd sub1 &&\n@@ -327,13 +338,27 @@ test_expect_success 'setup superproject with untracked file in nested submodule'\n \t\tgit add sub1 &&\n \t\tgit commit -a -m \"update sub1 to contain nested sub\"\n \t) &&\n-\techo untracked >super/sub1/sub2/untracked\n+\techo content >super/sub1/sub2/file &&\n+\techo content >super/sub2/file &&\n+\tgit -C super/sub3 clone ../../sub2 untracked_repository\n '\n \n test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n \tgit -C super status --porcelain >output &&\n \tdiff output - <<-\\EOF\n \t M sub1\n+\t M sub2\n+\t M sub3\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '\n+\tgit -C super status --porcelain=2 >output &&\n+\tsanitize_output output &&\n+\tdiff output - <<-\\EOF\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub1\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub2\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub3\n \tEOF\n '\n \n@@ -341,6 +366,41 @@ test_expect_success 'status with untracked file in nested submodule (short)' '\n \tgit -C super status --short >output &&\n \tdiff output - <<-\\EOF\n \t ? sub1\n+\t ? sub2\n+\t ? sub3\n+\tEOF\n+'\n+\n+test_expect_success 'setup superproject with modified file in nested submodule' '\n+\tgit -C super/sub1/sub2 add file &&\n+\tgit -C super/sub2 add file\n+'\n+\n+test_expect_success 'status with added file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\t M sub2\n+\t M sub3\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in nested submodule (porcelain=2)' '\n+\tgit -C super status --porcelain=2 >output &&\n+\tsanitize_output output &&\n+\tdiff output - <<-\\EOF\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub2\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub3\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\t m sub2\n+\t ? sub3\n \tEOF\n '\n \n\n\n"},{"id":"315716","messageId":"20170329222616.11077-2-sbeller@google.com","threadId":"45476","inReplyTo":"20170329222616.11077-1-sbeller@google.com","subject":"[PATCH 1/2] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-29T22:26:15Z","receivedAt":"2017-03-29T22:26:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If I add an untracked file to a submodule or modify a tracked file,\ncurrently \"git status --short\" treats the change in the same way as\nchanges to the current HEAD of the submodule:\n\n $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit\n $ echo hello >gerrit/plugins/replication/stray-file\n $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap\n $ git -C gerrit status --short\n M plugins/replication\n\nThis is by analogy with ordinary files, where \"M\" represents a change\nthat has not been added yet to the index. But this change cannot be\nadded to the index without entering the submodule, \"git add\"-ing it,\nand running \"git commit\", so the analogy is counterproductive.\n\nIntroduce new status letters \" ?\" and \" m\" for this. These are similar\nto the existing \"??\" and \" M\" but mean that the submodule (not the\nparent project) has new untracked files and modified files, respectively.\nThe user can use \"git add\" and \"git commit\" from within the submodule to\nadd them.\n\nChanges to the submodule's HEAD commit can be recorded in the index with\na plain \"git add -u\" and are shown with \" M\", like today.\n\nTo avoid excessive clutter, show at most one of \" ?\", \" m\", and \" M\" for\nthe submodule. They represent increasing levels of change --- the last\none that applies is shown (e.g., \" m\" if there are both modified files\nand untracked files in the submodule, or \" M\" if the submodule's HEAD\nhas been modified and it has untracked files).\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which shares code with \"status --short\". We only change\n\"git status --short\".\n\nNon-short \"git status\" and \"git status --porcelain=2\" already handle\nthese cases by showing more detail:\n\n $ git -C gerrit status --porcelain=2\n 1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication\n $ git -C gerrit status\n[...]\n modified: plugins/replication (modified content, untracked content)\n\nScripts caring about these distinctions should use --porcelain=2.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \n---\n Documentation/git-status.txt | 11 ++++\n t/t3600-rm.sh | 18 +++++--\n t/t7506-status-submodule.sh | 117 +++++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 17 ++++++-\n 4 files changed, 156 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..67f1a910f3 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,15 @@ in which case `XY` are `!!`.\n ! ! ignored\n -------------------------------------------------\n \n+Submodules have more state and instead report\n+\t\tM the submodule has a different HEAD than\n+\t\t recorded in the index\n+\t\tm the submodule has modified content\n+\t\t? the submodule has untracked files\n+since modified content or untracked files in a submodule cannot be added\n+via `git add` in the superproject to prepare a commit.\n+\n+\n If -b is used the short-format status is preceded by a line\n \n ## branchname tracking info\n@@ -210,6 +219,8 @@ field from the first filename). Third, filenames containing special\n characters are not specially formatted; no quoting or\n backslash-escaping is performed.\n \n+Any submodule changes are reported as modified `M` instead of `m` or single `?`.\n+\n Porcelain Format Version 2\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5aa6db584c..a6e5c5bd56 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -268,6 +268,14 @@ cat >expect.modified <expect.modified_inside <expect.modified_untracked <expect.cached <actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified actual &&\n+\ttest_cmp expect.modified_inside actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 51f8d0d034..1fa2ff2909 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -17,6 +17,12 @@ test_create_repo_with_commit () {\n \t)\n }\n \n+sanitize_output () {\n+\tsed -e \"s/$_x40/HASH/\" -e \"s/$_x40/HASH/\" output >output2 &&\n+\tmv output2 output\n+}\n+\n+\n test_expect_success 'setup' '\n \ttest_create_repo_with_commit sub &&\n \techo output > .gitignore &&\n@@ -50,6 +56,15 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with modified file in submodule (short)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n@@ -64,6 +79,14 @@ test_expect_success 'status with added file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with added file in submodule (short)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub\n+\tEOF\n+'\n+\n test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n@@ -83,6 +106,13 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n \n+test_expect_success 'status with untracked file in submodule (short)' '\n+\tgit status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t ? sub\n+\tEOF\n+'\n+\n test_expect_success 'status with added and untracked file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \techo \"content\" >sub/new-file &&\n@@ -287,4 +317,91 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '\n \ttest_cmp diff_submodule_actual diff_submodule_expect\n '\n \n+# We'll setup different cases for further testing:\n+# sub1 will contain a nested submodule,\n+# sub2 will have an untracked file\n+# sub3 will have an untracked repository\n+test_expect_success 'setup superproject with untracked file in nested submodule' '\n+\t(\n+\t\tcd super &&\n+\t\tgit clean -dfx &&\n+\t\trm .gitmodules &&\n+\t\tgit submodule add -f ./sub1 &&\n+\t\tgit submodule add -f ./sub2 &&\n+\t\tgit submodule add -f ./sub1 sub3 &&\n+\t\tgit commit -a -m \"messy merge in superproject\" &&\n+\t\t(\n+\t\t\tcd sub1 &&\n+\t\t\tgit submodule add ../sub2 &&\n+\t\t\tgit commit -a -m \"add sub2 to sub1\"\n+\t\t) &&\n+\t\tgit add sub1 &&\n+\t\tgit commit -a -m \"update sub1 to contain nested sub\"\n+\t) &&\n+\techo content >super/sub1/sub2/file &&\n+\techo content >super/sub2/file &&\n+\tgit -C super/sub3 clone ../../sub2 untracked_repository\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\t M sub2\n+\t M sub3\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '\n+\tgit -C super status --porcelain=2 >output &&\n+\tsanitize_output output &&\n+\tdiff output - <<-\\EOF\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub2\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub3\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\t ? sub2\n+\t ? sub3\n+\tEOF\n+'\n+\n+test_expect_success 'setup superproject with modified file in nested submodule' '\n+\tgit -C super/sub1/sub2 add file &&\n+\tgit -C super/sub2 add file\n+'\n+\n+test_expect_success 'status with added file in nested submodule (porcelain)' '\n+\tgit -C super status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub1\n+\t M sub2\n+\t M sub3\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in nested submodule (porcelain=2)' '\n+\tgit -C super status --porcelain=2 >output &&\n+\tsanitize_output output &&\n+\tdiff output - <<-\\EOF\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n+\t1 .M S.M. 160000 160000 160000 HASH HASH sub2\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub3\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in nested submodule (short)' '\n+\tgit -C super status --short >output &&\n+\tdiff output - <<-\\EOF\n+\t m sub1\n+\t m sub2\n+\t ? sub3\n+\tEOF\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 308cf3779e..0375484962 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -407,6 +407,16 @@ static void wt_longstatus_print_change_data(struct wt_status *s,\n \tstrbuf_release(&twobuf);\n }\n \n+static char short_submodule_status(struct wt_status_change_data *d) {\n+\tif (d->new_submodule_commits)\n+\t\treturn 'M';\n+\tif (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\treturn 'm';\n+\tif (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\treturn '?';\n+\treturn d->worktree_status;\n+}\n+\n static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\n@@ -431,10 +441,13 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\t}\n \t\tif (!d->worktree_status)\n \t\t\td->worktree_status = p->status;\n-\t\td->dirty_submodule = p->two->dirty_submodule;\n-\t\tif (S_ISGITLINK(p->two->mode))\n+\t\tif (S_ISGITLINK(p->two->mode)) {\n+\t\t\td->dirty_submodule = p->two->dirty_submodule;\n \t\t\td->new_submodule_commits = !!oidcmp(&p->one->oid,\n \t\t\t\t\t\t\t &p->two->oid);\n+\t\t\tif (s->status_format == STATUS_FORMAT_SHORT)\n+\t\t\t\td->worktree_status = short_submodule_status(d);\n+\t\t}\n \n \t\tswitch (p->status) {\n \t\tcase DIFF_STATUS_ADDED:\n-- \n2.12.1.442.ge9452a8fbc\n\n"},{"id":"315717","messageId":"20170329222616.11077-3-sbeller@google.com","threadId":"45476","inReplyTo":"20170329222616.11077-1-sbeller@google.com","subject":"[PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-29T22:26:16Z","receivedAt":"2017-03-29T22:26:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Suppose I have a superproject 'super', with two submodules 'super/sub'\nand 'super/sub1'. 'super/sub' itself contains a submodule\n'super/sub/subsub'. Now suppose I run, from within 'super':\n\n echo hi >sub/subsub/stray-file\n echo hi >sub1/stray-file\n\nCurrently we get would see the following output in git-status:\n\n git status --short\n m sub\n ? sub1\n\nWith this patch applied, the untracked file in the nested submodule is\ndisplayed as an untracked file on the 'super' level as well.\n\n git status --short\n ? sub\n ? sub1\n\nThis doesn't change the output of 'git status --porcelain=1' for nested\nsubmodules, because its output is always ' M' for either untracked files\nor local modifications no matter the nesting level of the submodule.\n\n'git status --porcelain=2' is affected by this change in a nested\nsubmodule, though. Without this patch it would report the direct submodule\nas modified and having no untracked files. With this patch it would report\nuntracked files. Chalk this up as a bug fix.\n\nThis bug fix also affects the default output (non-short, non-porcelain)\nof git-status, which is not tested here.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-status.txt | 2 ++\n submodule.c | 21 +++++++++++++++++++--\n t/t3600-rm.sh | 2 +-\n t/t7506-status-submodule.sh | 4 ++--\n 4 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex 67f1a910f3..d70abc6afe 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -189,6 +189,8 @@ Submodules have more state and instead report\n since modified content or untracked files in a submodule cannot be added\n via `git add` in the superproject to prepare a commit.\n \n+'m' and '?' are applied recursively. For example if a nested submodule\n+in a submodule contains an untracked file, this is reported as '?' as well.\n \n If -b is used the short-format status is preceded by a line\n \ndiff --git a/submodule.c b/submodule.c\nindex fa21c7bb72..3da65100e3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1078,8 +1078,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t/* regular untracked files */\n \t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\telse\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t buf.buf[0] == '1' ||\n+\t\t buf.buf[0] == '2') {\n+\t\t\t/* T = line type, XY = status, SSSS = submodule state */\n+\t\t\tif (buf.len < strlen(\"T XY SSSS\"))\n+\t\t\t\tdie(\"BUG: invalid status --porcelain=2 line %s\",\n+\t\t\t\t buf.buf);\n+\n+\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n+\t\t\t\t/* nested untracked file */\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\n+\t\t\tif (buf.buf[0] == 'u' ||\n+\t\t\t buf.buf[0] == '2' ||\n+\t\t\t memcmp(buf.buf + 5, \"S..U\", 4))\n+\t\t\t\t/* other change */\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t}\n \n \t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n \t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex a6e5c5bd56..b58793448b 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi\n \ttest -d submod &&\n \ttest -f submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\ttest_cmp expect.modified_inside actual &&\n+\ttest_cmp expect.modified_untracked actual &&\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 1fa2ff2909..055c90736e 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -356,7 +356,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain=2\n \tgit -C super status --porcelain=2 >output &&\n \tsanitize_output output &&\n \tdiff output - <<-\\EOF\n-\t1 .M S.M. 160000 160000 160000 HASH HASH sub1\n+\t1 .M S..U 160000 160000 160000 HASH HASH sub1\n \t1 .M S..U 160000 160000 160000 HASH HASH sub2\n \t1 .M S..U 160000 160000 160000 HASH HASH sub3\n \tEOF\n@@ -365,7 +365,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain=2\n test_expect_success 'status with untracked file in nested submodule (short)' '\n \tgit -C super status --short >output &&\n \tdiff output - <<-\\EOF\n-\t m sub1\n+\t ? sub1\n \t ? sub2\n \t ? sub3\n \tEOF\n-- \n2.12.1.442.ge9452a8fbc\n\n"},{"id":"315721","messageId":"20170329230925.GY31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170329222616.11077-2-sbeller@google.com","subject":"Re: [PATCH 1/2] short status: improve reporting for submodule changes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-29T23:09:25Z","receivedAt":"2017-03-29T23:09:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n> Reviewed-by: Jonathan Nieder \n> ---\n> Documentation/git-status.txt | 11 ++++\n> t/t3600-rm.sh | 18 +++++--\n> t/t7506-status-submodule.sh | 117 +++++++++++++++++++++++++++++++++++++++++++\n> wt-status.c | 17 ++++++-\n> 4 files changed, 156 insertions(+), 7 deletions(-)\n\nYes, this looks good.\n\nThank you,\nJonathan\n"},{"id":"315722","messageId":"20170329231308.GZ31294@aiede.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170329222616.11077-3-sbeller@google.com","subject":"Re: [PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-29T23:13:09Z","receivedAt":"2017-03-29T23:13:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> This bug fix also affects the default output (non-short, non-porcelain)\n> of git-status, which is not tested here.\n\nDo you have an example? (In just the commit message would be fine, in\ntests would be even better.)\n\n> Signed-off-by: Stefan Beller \n> ---\n> Documentation/git-status.txt | 2 ++\n> submodule.c | 21 +++++++++++++++++++--\n> t/t3600-rm.sh | 2 +-\n> t/t7506-status-submodule.sh | 4 ++--\n> 4 files changed, 24 insertions(+), 5 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nbut I would be a lot more comfortable after looking at the change to\n\"git status\" output. (E.g. a test demonstrating it can happen in a\nfollowup change if that's simpler.)\n\nThanks for your patient work on this.\n"},{"id":"315803","messageId":"xmqqr31e4ohx.fsf@gitster.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170328010533.GQ31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T18:18:34Z","receivedAt":"2017-03-30T18:18:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>> Shouldn't this done as part of 4/7 where is_submodule_modified()\n>> starts reading from the porcelain v2 output? 4/7 does adjust for\n>> the change from double question mark (porcelain v1) to a single one\n>> for untracked, but at the same time it needs to prepare for these\n>> 'u' (unmerged), '1' (normal modification) and '2' (mods with rename)\n>> to appear in the output, no?\n>>\n>> IOW, with 4/7 and 7/7 done as separate steps, isn't the system\n>> broken between these steps?\n>\n> No. Both before and after patch 4, this code has to determine two\n> details from a submodule:\n> ...\n> Question (3) didn't come up before because when there are no nested\n> submodules, the diff machinery answers it (saving us from getting the\n> answer from the status --porcelain we recurse to).\n\nThanks.\n"},{"id":"315807","messageId":"xmqqmvc24o0t.fsf@gitster.mtv.corp.google.com","threadId":"45476","inReplyTo":"20170329231308.GZ31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T18:28:50Z","receivedAt":"2017-03-30T18:28:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Stefan Beller wrote:\n>\n>> This bug fix also affects the default output (non-short, non-porcelain)\n>> of git-status, which is not tested here.\n>\n> Do you have an example? (In just the commit message would be fine, in\n> tests would be even better.)\n>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> Documentation/git-status.txt | 2 ++\n>> submodule.c | 21 +++++++++++++++++++--\n>> t/t3600-rm.sh | 2 +-\n>> t/t7506-status-submodule.sh | 4 ++--\n>> 4 files changed, 24 insertions(+), 5 deletions(-)\n>\n> Reviewed-by: Jonathan Nieder \n>\n> but I would be a lot more comfortable after looking at the change to\n> \"git status\" output. (E.g. a test demonstrating it can happen in a\n> followup change if that's simpler.)\n>\n> Thanks for your patient work on this.\n\nThank you both. I re-read the whole thing and it feels to me that\nthe topic is now ready for 'next'.\n"}]}