{"thread":{"id":"45391","subject":"[RFC PATCH] short status: improve reporting for submodule changes","startedAt":"2017-03-16T00:33:42Z","lastAt":"2017-03-16T22:53:46Z","messageCount":7,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"314201","messageId":"20170316003332.30918-1-sbeller@google.com","threadId":"45391","inReplyTo":null,"subject":"[RFC PATCH] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-16T00:33:32Z","receivedAt":"2017-03-16T00:33:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"While we already have a porcelain2 layer for git-status, that is accurate\nfor submodules, users still like the way they are are used to of\n'status -s'.\n\nAs a submodule has more state than a file potentially, we'll look at all\ncases:\n\n   ------ new submodule commits\n /  ----- modified files\n | /   -- untracked files\n | |  /\n | | |   current / proposed reporting\n 0 0 0     \"  \"     \"  \"\n 0 0 1     \" M\"     \" ?\"\n 0 1 0     \" M\"     \" m\"\n 0 1 1     \" M\"     \" m\"\n 1 0 0     \" M\"     \" M\"\n 1 0 1     \" M\"     \" M\"\n 1 1 0     \" M\"     \" M\"\n 1 1 1     \" M\"     \" M\"\n\nSo when there is no new commits in the submodule, we'd want to tell\nwhat actually happend to the submodule. The lower case 'm' indicates\nto the user that there is need to give a --recurse-submodules option\nfor e.g. adding or committing these changes. The \" ?\" also differentiates\nan untracked file in the submodule from a regular untracked file.\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which uses the same code as the short printing function.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/git-status.txt |  8 ++++++++\n wt-status.c                  | 15 +++++++++++++--\n wt-status.h                  |  1 +\n 3 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..26c8d832cd 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,12 @@ in which case `XY` are `!!`.\n     !           !    ignored\n     -------------------------------------------------\n \n+Submodules have more state to it, such that it reports instead:\n+\t\tM    the submodule has new commits\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 +216,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/wt-status.c b/wt-status.c\nindex a52d342695..080d97f272 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1664,9 +1664,19 @@ static void wt_shortstatus_status(struct string_list_item *it,\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_UPDATED, s), \"%c\", d->index_status);\n \telse\n \t\tputchar(' ');\n-\tif (d->worktree_status)\n+\tif (d->worktree_status) {\n+\t\tif (!s->submodule_porcelain1 &&\n+\t\t    (d->dirty_submodule || d->new_submodule_commits)) {\n+\t\t\t/* It is a submodule, and we're not in plumbing mode. */\n+\t\t\tif (d->new_submodule_commits)\n+\t\t\t\td->worktree_status = 'M';\n+\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\t\t\td->worktree_status = 'm';\n+\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\td->worktree_status = '?';\n+\t\t}\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_CHANGED, s), \"%c\", d->worktree_status);\n-\telse\n+\t} else\n \t\tputchar(' ');\n \tputchar(' ');\n \tif (s->null_termination) {\n@@ -1811,6 +1821,7 @@ static void wt_porcelain_print(struct wt_status *s)\n \ts->relative_paths = 0;\n \ts->prefix = NULL;\n \ts->no_gettext = 1;\n+\ts->submodule_porcelain1 = 1;\n \twt_shortstatus_print(s);\n }\n \ndiff --git a/wt-status.h b/wt-status.h\nindex 54fec77032..620e4d2ae4 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -70,6 +70,7 @@ struct wt_status {\n \tint display_comment_prefix;\n \tint relative_paths;\n \tint submodule_summary;\n+\tint submodule_porcelain1;\n \tint show_ignored_files;\n \tenum untracked_status_type show_untracked_files;\n \tconst char *ignore_submodule_arg;\n-- \n2.12.0.269.g1a05a5734c.dirty\n\n"},{"id":"314204","messageId":"xmqqvarac8hc.fsf@gitster.mtv.corp.google.com","threadId":"45391","inReplyTo":"20170316003332.30918-1-sbeller@google.com","subject":"Re: [RFC PATCH] short status: improve reporting for submodule changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-16T01:31:59Z","receivedAt":"2017-03-16T01:32:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> While we already have a porcelain2 layer for git-status, that is accurate\n> for submodules, users still like the way they are are used to of\n> 'status -s'.\n>\n> As a submodule has more state than a file potentially, we'll look at all\n> cases:\n>\n>    ------ new submodule commits\n>  /  ----- modified files\n>  | /   -- untracked files\n>  | |  /\n>  | | |   current / proposed reporting\n>  0 0 0     \"  \"     \"  \"\n>  0 0 1     \" M\"     \" ?\"\n>  0 1 0     \" M\"     \" m\"\n>  0 1 1     \" M\"     \" m\"\n>  1 0 0     \" M\"     \" M\"\n>  1 0 1     \" M\"     \" M\"\n>  1 1 0     \" M\"     \" M\"\n>  1 1 1     \" M\"     \" M\"\n\nYou are essentialy saying that there are three levels, 1. with\ncommit level difference, 2. the same commit with local mods, 3. no\nmods but with crufts, and instead of wasting 8 letters to express\nall combinations, the highest level is reported, right?  That sounds\nOK to me.  I am not sure if \"?\" is a good letter to use (doesn't it\nusually mean it is an untracked cruft?), though.\n\nDoes the commit level difference really mean \"has new commits\"?  It\nprobably is not new problem but an old mistake inherited from the\ncurrent code, but I suspect that you're just comparing the commit\nbound in the index of the superproject and the HEAD commit in the\nsubmodule, in which case \"newness\" does not matter a bit---\"is it\nthe same or different?\" is the question you are asking, I would\nthink.\n\n\n"},{"id":"314207","messageId":"CAGZ79kZ7rWw=q6a2AomTw20DsU1h+7ou4i8A14a2bcg+asRwLA@mail.gmail.com","threadId":"45391","inReplyTo":"xmqqvarac8hc.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-16T05:16:08Z","receivedAt":"2017-03-16T05:16:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 15, 2017 at 6:31 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> While we already have a porcelain2 layer for git-status, that is accurate\n>> for submodules, users still like the way they are are used to of\n>> 'status -s'.\n>>\n>> As a submodule has more state than a file potentially, we'll look at all\n>> cases:\n>>\n>>    ------ new submodule commits\n>>  /  ----- modified files\n>>  | /   -- untracked files\n>>  | |  /\n>>  | | |   current / proposed reporting\n>>  0 0 0     \"  \"     \"  \"\n>>  0 0 1     \" M\"     \" ?\"\n>>  0 1 0     \" M\"     \" m\"\n>>  0 1 1     \" M\"     \" m\"\n>>  1 0 0     \" M\"     \" M\"\n>>  1 0 1     \" M\"     \" M\"\n>>  1 1 0     \" M\"     \" M\"\n>>  1 1 1     \" M\"     \" M\"\n>\n> You are essentialy saying that there are three levels, 1. with\n> commit level difference, 2. the same commit with local mods, 3. no\n> mods but with crufts, and instead of wasting 8 letters to express\n> all combinations, the highest level is reported, right?  That sounds\n> OK to me.  I am not sure if \"?\" is a good letter to use (doesn't it\n> usually mean it is an untracked cruft?), though.\n\nok. it helped me, though, to picture all possibilities to come up with\nwhat I consider best for each case. Yes in the end it can be described\nas 'report highest bit'.\n\n>\n> Does the commit level difference really mean \"has new commits\"?  It\n> probably is not new problem but an old mistake inherited from the\n> current code, but I suspect that you're just comparing the commit\n> bound in the index of the superproject and the HEAD commit in the\n> submodule, in which case \"newness\" does not matter a bit---\"is it\n> the same or different?\" is the question you are asking, I would\n> think.\n\nyes, I agree. That is the actual question asked.\n\n\n>\n>\n"},{"id":"314256","messageId":"20170316204644.11616-1-sbeller@google.com","threadId":"45391","inReplyTo":"CAGZ79kZ7rWw=q6a2AomTw20DsU1h+7ou4i8A14a2bcg+asRwLA@mail.gmail.com","subject":"[PATCH v2] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-16T20:46:44Z","receivedAt":"2017-03-16T20:46:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"While we already have a porcelain2 layer for git-status, that is accurate\nfor submodules, users still like the way they are are used to of\n'status -s'.\n\nAs a submodule has more state than a file potentially, we need more letters\nindicating these states, we'll go with lower 'm' and single '?' for now.\n\nWhen there the recorded commit doesn't differ from the submodule HEAD\n(which we still want to treat as \"M\", because that can be dealt with\nin the superproject), we can have different types of change in the\nsubmodule. The lower case 'm' indicates that there are changes to tracked\nfiles in the submodule. It signals that the --recurse-submodules option\nis needed for e.g. adding or committing these changes. The \" ?\" also\ndifferentiates an untracked file in the submodule from a regular\nuntracked file.\n\nWhile making these changes, we need to make sure to not break porcelain\nlevel 1, which uses the same code as the short printing function.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/git-status.txt |  8 ++++++++\n t/t7506-status-submodule.sh  | 24 ++++++++++++++++++++++++\n wt-status.c                  | 15 +++++++++++++--\n wt-status.h                  |  1 +\n 4 files changed, 46 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex ba873657cf..b182b16c48 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -181,6 +181,12 @@ in which case `XY` are `!!`.\n     !           !    ignored\n     -------------------------------------------------\n \n+Submodules have more state to it, such that it reports instead:\n+\t\tM    the submodule has a different HEAD than recorded\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 +216,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/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 a52d342695..080d97f272 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1664,9 +1664,19 @@ static void wt_shortstatus_status(struct string_list_item *it,\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_UPDATED, s), \"%c\", d->index_status);\n \telse\n \t\tputchar(' ');\n-\tif (d->worktree_status)\n+\tif (d->worktree_status) {\n+\t\tif (!s->submodule_porcelain1 &&\n+\t\t    (d->dirty_submodule || d->new_submodule_commits)) {\n+\t\t\t/* It is a submodule, and we're not in plumbing mode. */\n+\t\t\tif (d->new_submodule_commits)\n+\t\t\t\td->worktree_status = 'M';\n+\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n+\t\t\t\td->worktree_status = 'm';\n+\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\td->worktree_status = '?';\n+\t\t}\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_CHANGED, s), \"%c\", d->worktree_status);\n-\telse\n+\t} else\n \t\tputchar(' ');\n \tputchar(' ');\n \tif (s->null_termination) {\n@@ -1811,6 +1821,7 @@ static void wt_porcelain_print(struct wt_status *s)\n \ts->relative_paths = 0;\n \ts->prefix = NULL;\n \ts->no_gettext = 1;\n+\ts->submodule_porcelain1 = 1;\n \twt_shortstatus_print(s);\n }\n \ndiff --git a/wt-status.h b/wt-status.h\nindex 54fec77032..620e4d2ae4 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -70,6 +70,7 @@ struct wt_status {\n \tint display_comment_prefix;\n \tint relative_paths;\n \tint submodule_summary;\n+\tint submodule_porcelain1;\n \tint show_ignored_files;\n \tenum untracked_status_type show_untracked_files;\n \tconst char *ignore_submodule_arg;\n-- \n2.12.0.269.g1a05a5734c.dirty\n\n"},{"id":"314268","messageId":"20170316221907.GA9135@aiede.mtv.corp.google.com","threadId":"45391","inReplyTo":"20170316204644.11616-1-sbeller@google.com","subject":"Re: [PATCH v2] short status: improve reporting for submodule changes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-16T22:19:07Z","receivedAt":"2017-03-16T22:20:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nYay, I like the change this makes.  So I'll nitpick in the hope that\nthat makes the patch more likely to stick.\n\nStefan Beller wrote:\n\n> While we already have a porcelain2 layer for git-status, that is accurate\n> for submodules, users still like the way they are are used to of\n> 'status -s'.\n\nI had to read this a few times to understand it.  Maybe explaining it\nfrom the user's point of view would work:\n\n\tIf I add an untracked file to a submodule or modify a tracked file,\n\tcurrently \"git status --short\" treats the change in the same way as\n\tchanges to the current HEAD of the submodule:\n\n\t\t$ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit\n\t\t$ echo hello >gerrit/plugins/replication/stray-file\n\t\t$ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap\n\t\t$ git -C gerrit status --short\n\t\t M plugins/replication\n\n\tThis is by analogy with ordinary files, where \"M\" represents a change\n\tthat has not been added yet to the index.  But this change cannot be\n\tadded to the index without entering the submodule, \"git add\"-ing it,\n\tand running \"git commit\", so the analogy is counterproductive.\n\n> As a submodule has more state than a file potentially, we need more letters\n> indicating these states, we'll go with lower 'm' and single '?' for now.\n>\n> When there the recorded commit doesn't differ from the submodule HEAD\n> (which we still want to treat as \"M\", because that can be dealt with\n> in the superproject), we can have different types of change in the\n> submodule. The lower case 'm' indicates that there are changes to tracked\n> files in the submodule. It signals that the --recurse-submodules option\n> is needed for e.g. adding or committing these changes. The \" ?\" also\n> differentiates an untracked file in the submodule from a regular\n> untracked file.\n\nThis could say\n\n\tIntroduce new status letters \" ?\" and \" m\" for this.  These are similar\n\tto the existing \"??\" and \" M\" but mean that the submodule (not the\n\tparent project) has new untracked files and modified files, respectively.\n\tThe user can use \"git add\" and \"git commit\" from within the submodule to\n\tadd them.\n\n\tChanges to the submodule's HEAD commit can be recorded in the index with\n\ta plain \"git add -u\" and are shown with \" M\", like today.\n\n\tTo avoid excessive clutter, show at most one of \" ?\", \" m\", and \" M\" for\n\tthe submodule.  They represent increasing levels of change --- the last\n\tone that applies is shown (e.g., \" m\" if there are both modified files\n\tand untracked files in the submodule, or \" M\" if the submodule's HEAD\n\thas been modified and it has untracked files).\n\n> While making these changes, we need to make sure to not break porcelain\n> level 1, which uses the same code as the short printing function.\n\n\tWhile making these changes, we need to make sure to not break porcelain\n\tlevel 1, which shares code with \"status --short\".  We only change\n\t\"git status --short\".\n\n\tNon-short \"git status\" and \"git status --porcelain=2\" already handle\n\tthese cases by showing more detail:\n\n\t\t$ git -C gerrit status --porcelain=2\n\t\t1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication\n\t\t$ git -C gerrit status\n\t[...]\n\t        modified:   plugins/replication (modified content, untracked content)\n\t\n\tScripts caring about these distinctions should use --porcelain=2.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n\nHaving written that, a few questions:\n\nIs it important to avoid clutter by showing the submodule only once?\nWhat would you think of showing whatever subset of those three\nstatuses apply to a given submodule as separate lines instead, to\nmatch the information that long-form \"git status\" shows?\n\nHow should a new untracked file in a submodule of a submodule be\nshown?\n\n[...]\n> --- a/Documentation/git-status.txt\n> +++ b/Documentation/git-status.txt\n> @@ -181,6 +181,12 @@ in which case `XY` are `!!`.\n>      !           !    ignored\n>      -------------------------------------------------\n>  \n> +Submodules have more state to it, such that it reports instead:\n\nLanguage nits: s/ to it//; s/, such that it reports instead/ and instead report/\n\n> +\t\tM    the submodule has a different HEAD than recorded\n\nthan recorded in the index?\n\n> +\t\tm    the submodule has modified content\n> +\t\t?    the submodule has untracked files\n[...]\n> --- a/t/t7506-status-submodule.sh\n> +++ b/t/t7506-status-submodule.sh\n\n\\o/\n\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\nnot about this patch: this could use \"git -C sub\" and avoid a\nsubshell.  But what you have here matches the existing style, so I\nwouldn't suggest changing it here.\n\n> +\techo \"changed\" >sub/foo &&\n> +\tgit status --short >output &&\n> +\tdiff output - <<-\\EOF\n\nsimilarly, this could use test_cmp, but that seems out of scope for\nthis change.\n\n[...]\n> +test_expect_success 'status with added file in submodule (short)' '\n[...]\n> +test_expect_success 'status with untracked file in submodule (short)' '\n\nTest wishlist:\n* --porcelain output for these cases\n* behavior with nested submodules\n\n[...]\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1664,9 +1664,19 @@ static void wt_shortstatus_status(struct string_list_item *it,\n>  \t\tcolor_fprintf(s->fp, color(WT_STATUS_UPDATED, s), \"%c\", d->index_status);\n>  \telse\n>  \t\tputchar(' ');\n> -\tif (d->worktree_status)\n> +\tif (d->worktree_status) {\n> +\t\tif (!s->submodule_porcelain1 &&\n\nIsn't this 's->status_format != STATUS_FORMAT_PORCELAIN'?\n\n[...]\n> +\t\t    (d->dirty_submodule || d->new_submodule_commits)) {\n> +\t\t\t/* It is a submodule, and we're not in plumbing mode. */\n> +\t\t\tif (d->new_submodule_commits)\n> +\t\t\t\td->worktree_status = 'M';\n> +\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n> +\t\t\t\td->worktree_status = 'm';\n> +\t\t\telse if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n> +\t\t\t\td->worktree_status = '?';\n> +\t\t}\n\nIs this the right place to do this?  Could wt_status_collect_changed_cb set\nworktree_status to the right value in the first place based on whether\n'status_format == STATUS_FORMAT_SHORT' or would that have undesirable\nknock-on effects elsewhere?\n\n[...]\n> --- a/wt-status.h\n> +++ b/wt-status.h\n> @@ -70,6 +70,7 @@ struct wt_status {\n>  \tint display_comment_prefix;\n>  \tint relative_paths;\n>  \tint submodule_summary;\n> +\tint submodule_porcelain1;\n\nAs described above, this shouldn't be needed since status_format\nalready has this information.\n\nThanks for this --- I like it.\n\nSincerely,\nJonathan\n"},{"id":"314283","messageId":"CAGZ79kagA38GjXcRp1XBXhjR6OLgk-zrVC5MC2eo+nZziBETLg@mail.gmail.com","threadId":"45391","inReplyTo":"20170316221907.GA9135@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] short status: improve reporting for submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-16T22:42:35Z","receivedAt":"2017-03-16T22:42:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">\n> Is it important to avoid clutter by showing the submodule only once?\n> What would you think of showing whatever subset of those three\n> statuses apply to a given submodule as separate lines instead, to\n> match the information that long-form \"git status\" shows?\n\nI considered it, but it would break the visual appeal of git status --short ?\n\n>\n> How should a new untracked file in a submodule of a submodule be\n> shown?\n\nThe same way \" ?\" indicates that (1) there is an untracked file due to\nthe question mark and (2) that you need to recurse because it differs from\n\"??\" for regular untracked files.\n\nThe problem here is that we do not know about these nested untracked files,\nbecause we use --porcelain instead of --short for submodules in\nsubmodule.c#is_submodule_modified(). I am rewriting that function anyway\nfor the \"git-describe --dirty\" bug, so maybe it's time to switch to porcelain=2\ninternally there, which can surface untracked files in nested subs.\n\n\n>\n> [...]\n>> --- a/Documentation/git-status.txt\n>> +++ b/Documentation/git-status.txt\n>> @@ -181,6 +181,12 @@ in which case `XY` are `!!`.\n>>      !           !    ignored\n>>      -------------------------------------------------\n>>\n>> +Submodules have more state to it, such that it reports instead:\n>\n> Language nits: s/ to it//; s/, such that it reports instead/ and instead report/\n>\n>> +             M    the submodule has a different HEAD than recorded\n>\n> than recorded in the index?\n\nok, will fix.\n\n\n>> +test_expect_success 'status with modified file in submodule (short)' '\n>> +     (cd sub && git reset --hard) &&\n>\n> not about this patch: this could use \"git -C sub\" and avoid a\n> subshell.  But what you have here matches the existing style, so I\n> wouldn't suggest changing it here.\n>\n>> +     echo \"changed\" >sub/foo &&\n>> +     git status --short >output &&\n>> +     diff output - <<-\\EOF\n>\n> similarly, this could use test_cmp, but that seems out of scope for\n> this change.\n\nYes I considered if I want to redo the whole file to meet\n'modern Git test suite style'. I'll think about it again.\n\n> [...]\n>> +test_expect_success 'status with added file in submodule (short)' '\n> [...]\n>> +test_expect_success 'status with untracked file in submodule (short)' '\n>\n> Test wishlist:\n> * --porcelain output for these cases\n> * behavior with nested submodules\n\nLook at the test file, please.\nI copied the tests cases (porcelain) and replaced s/porcelain/short/,\nso they exist.\n\n>\n> [...]\n>> --- a/wt-status.c\n>> +++ b/wt-status.c\n>> @@ -1664,9 +1664,19 @@ static void wt_shortstatus_status(struct string_list_item *it,\n>>               color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), \"%c\", d->index_status);\n>>       else\n>>               putchar(' ');\n>> -     if (d->worktree_status)\n>> +     if (d->worktree_status) {\n>> +             if (!s->submodule_porcelain1 &&\n>\n> Isn't this 's->status_format != STATUS_FORMAT_PORCELAIN'?\n\nyes.\n\n>> +                 (d->dirty_submodule || d->new_submodule_commits)) {\n>> +                     /* It is a submodule, and we're not in plumbing mode. */\n>> +                     if (d->new_submodule_commits)\n>> +                             d->worktree_status = 'M';\n>> +                     else if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n>> +                             d->worktree_status = 'm';\n>> +                     else if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n>> +                             d->worktree_status = '?';\n>> +             }\n>\n> Is this the right place to do this?  Could wt_status_collect_changed_cb set\n> worktree_status to the right value in the first place based on whether\n> 'status_format == STATUS_FORMAT_SHORT' or would that have undesirable\n> knock-on effects elsewhere?\n>\n\nyes that would be better. I totally missed status_format as a variable.\n\n>> +     int submodule_porcelain1;\n>\n> As described above, this shouldn't be needed since status_format\n> already has this information.\n\ngood point. will omit this variable.\n"},{"id":"314286","messageId":"20170316225307.GC9135@aiede.mtv.corp.google.com","threadId":"45391","inReplyTo":"CAGZ79kagA38GjXcRp1XBXhjR6OLgk-zrVC5MC2eo+nZziBETLg@mail.gmail.com","subject":"Re: [PATCH v2] short status: improve reporting for submodule changes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-16T22:53:07Z","receivedAt":"2017-03-16T22:53:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> Jonathan Nieder wrote:\n\n>> Is it important to avoid clutter by showing the submodule only once?\n>> What would you think of showing whatever subset of those three\n>> statuses apply to a given submodule as separate lines instead, to\n>> match the information that long-form \"git status\" shows?\n>\n> I considered it, but it would break the visual appeal of git status --short ?\n\nI could go either way.  As long as you've thought about it, I'm happy.\n\n>> How should a new untracked file in a submodule of a submodule be\n>> shown?\n>\n> The same way \" ?\" indicates that (1) there is an untracked file due to\n> the question mark and (2) that you need to recurse because it differs from\n> \"??\" for regular untracked files.\n>\n> The problem here is that we do not know about these nested untracked files,\n> because we use --porcelain instead of --short for submodules in\n> submodule.c#is_submodule_modified(). I am rewriting that function anyway\n> for the \"git-describe --dirty\" bug, so maybe it's time to switch to porcelain=2\n> internally there, which can surface untracked files in nested subs.\n\nPunting to a TODO / separate patch sounds reasonable.  Tests in this\npatch describing either the current behavior or the desired behavior\nwould be helpful, though.\n\nThanks,\nJonathan\n"}]}