{"thread":{"id":"34796","subject":"[RFC/PATCH v3 0/4] Disable \"git status\" comment prefix","startedAt":"2013-08-29T13:05:32Z","lastAt":"2013-09-03T19:32:23Z","messageCount":16,"participants":["Matthieu Moy","Junio C Hamano","Jens Lehmann","brian m. carlson"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"226153","messageId":"1377781536-31955-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"34796","inReplyTo":null,"subject":"[RFC/PATCH v3 0/4] Disable \"git status\" comment prefix","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-08-29T13:05:32Z","receivedAt":"2013-08-29T13:05:32Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"So, here's a reroll that makes the code cleaner.\n\nFirst patches are cleanups without behavioral changes, only the last\none does something new.\n\nI'm waiting for more comments to decide what to do with the\nconfiguration option. Right now, my preference would be to call it\nstatus.oldStyle and default it to false (i.e. change the behavior, but\nallow old-timers to get back the old one).\n\nMatthieu Moy (4):\n  builtin/stripspace.c: fix broken indentation\n  wt-status: use argv_array API\n  get rid of \"git submodule summary --for-status\"\n  status: introduce status.displayCommentChar to disable display of #\n\n Documentation/config.txt     |  5 +++\n builtin/commit.c             |  9 +++++\n builtin/stripspace.c         |  8 ++---\n git-submodule.sh             | 17 +--------\n t/t7401-submodule-summary.sh | 13 -------\n t/t7508-status.sh            | 43 +++++++++++++++++++++++\n wt-status.c                  | 82 +++++++++++++++++++++++++++++++++-----------\n wt-status.h                  |  1 +\n 8 files changed, 125 insertions(+), 53 deletions(-)\n\n-- \n1.8.4.12.gf9d53a3.dirty\n"},{"id":"226154","messageId":"1377781536-31955-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"34796","inReplyTo":"1377781536-31955-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH v3 1/4] builtin/stripspace.c: fix broken indentation","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-08-29T13:05:33Z","receivedAt":"2013-08-29T13:05:33Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n builtin/stripspace.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex e981dfb..1259ed7 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -89,11 +89,11 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \n \tif (argc == 2) {\n \t\tif (!strcmp(argv[1], \"-s\") ||\n-\t\t\t!strcmp(argv[1], \"--strip-comments\")) {\n-\t\t\t strip_comments = 1;\n+\t\t    !strcmp(argv[1], \"--strip-comments\")) {\n+\t\t\tstrip_comments = 1;\n \t\t} else if (!strcmp(argv[1], \"-c\") ||\n-\t\t\t\t\t !strcmp(argv[1], \"--comment-lines\")) {\n-\t\t\t mode = COMMENT_LINES;\n+\t\t\t   !strcmp(argv[1], \"--comment-lines\")) {\n+\t\t\tmode = COMMENT_LINES;\n \t\t} else {\n \t\t\tmode = INVAL;\n \t\t}\n-- \n1.8.4.12.gf9d53a3.dirty\n"},{"id":"226155","messageId":"1377781536-31955-3-git-send-email-Matthieu.Moy@imag.fr","threadId":"34796","inReplyTo":"1377781536-31955-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH v3 2/4] wt-status: use argv_array API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-08-29T13:05:34Z","receivedAt":"2013-08-29T13:05:34Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"No behavior change, but two slight code reorganization: argv_array_push\ndoesn't accept NULL strings, and duplicates its argument hence\nsummary_limit must be written to before being inserted into argv.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n wt-status.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex cb24f1f..958a53c 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -8,6 +8,7 @@\n #include \"diffcore.h\"\n #include \"quote.h\"\n #include \"run-command.h\"\n+#include \"argv-array.h\"\n #include \"remote.h\"\n #include \"refs.h\"\n #include \"submodule.h\"\n@@ -663,29 +664,30 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tchar summary_limit[64];\n \tchar index[PATH_MAX];\n \tconst char *env[] = { NULL, NULL };\n-\tconst char *argv[8];\n-\n-\tenv[0] =\tindex;\n-\targv[0] =\t\"submodule\";\n-\targv[1] =\t\"summary\";\n-\targv[2] =\tuncommitted ? \"--files\" : \"--cached\";\n-\targv[3] =\t\"--for-status\";\n-\targv[4] =\t\"--summary-limit\";\n-\targv[5] =\tsummary_limit;\n-\targv[6] =\tuncommitted ? NULL : (s->amend ? \"HEAD^\" : \"HEAD\");\n-\targv[7] =\tNULL;\n+\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \n \tsprintf(summary_limit, \"%d\", s->submodule_summary);\n \tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", s->index_file);\n \n+\tenv[0] = index;\n+\targv_array_push(&argv, \"submodule\");\n+\targv_array_push(&argv, \"summary\");\n+\targv_array_push(&argv, uncommitted ? \"--files\" : \"--cached\");\n+\targv_array_push(&argv, \"--for-status\");\n+\targv_array_push(&argv, \"--summary-limit\");\n+\targv_array_push(&argv, summary_limit);\n+\tif (!uncommitted)\n+\t\targv_array_push(&argv, s->amend ? \"HEAD^\" : \"HEAD\");\n+\n \tmemset(&sm_summary, 0, sizeof(sm_summary));\n-\tsm_summary.argv = argv;\n+\tsm_summary.argv = argv.argv;\n \tsm_summary.env = env;\n \tsm_summary.git_cmd = 1;\n \tsm_summary.no_stdin = 1;\n \tfflush(s->fp);\n \tsm_summary.out = dup(fileno(s->fp));    /* run_command closes it */\n \trun_command(&sm_summary);\n+\targv_array_clear(&argv);\n }\n \n static void wt_status_print_other(struct wt_status *s,\n-- \n1.8.4.12.gf9d53a3.dirty\n"},{"id":"226156","messageId":"1377781536-31955-4-git-send-email-Matthieu.Moy@imag.fr","threadId":"34796","inReplyTo":"1377781536-31955-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-08-29T13:05:35Z","receivedAt":"2013-08-29T13:05:35Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The --for-status option was an undocumented option used only by\nwt-status.c, which inserted a header and commented out the output. We can\nachieve the same result within wt-status.c, without polluting the\nsubmodule command-line options.\n\nThis will make it easier to disable the comments from wt-status.c later.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n git-submodule.sh             | 17 +----------------\n t/t7401-submodule-summary.sh | 13 -------------\n wt-status.c                  | 29 +++++++++++++++++++++++++++--\n 3 files changed, 28 insertions(+), 31 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2979197..fccdec9 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -965,7 +965,6 @@ set_name_rev () {\n #\n cmd_summary() {\n \tsummary_limit=-1\n-\tfor_status=\n \tdiff_cmd=diff-index\n \n \t# parse $args after \"submodule ... summary\".\n@@ -978,9 +977,6 @@ cmd_summary() {\n \t\t--files)\n \t\t\tfiles=\"$1\"\n \t\t\t;;\n-\t\t--for-status)\n-\t\t\tfor_status=\"$1\"\n-\t\t\t;;\n \t\t-n|--summary-limit)\n \t\t\tsummary_limit=\"$2\"\n \t\t\tisnumber \"$summary_limit\" || usage\n@@ -1149,18 +1145,7 @@ cmd_summary() {\n \t\t\techo\n \t\tfi\n \t\techo\n-\tdone |\n-\tif test -n \"$for_status\"; then\n-\t\tif [ -n \"$files\" ]; then\n-\t\t\tgettextln \"Submodules changed but not updated:\" | git stripspace -c\n-\t\telse\n-\t\t\tgettextln \"Submodule changes to be committed:\" | git stripspace -c\n-\t\tfi\n-\t\tprintf \"\\n\" | git stripspace -c\n-\t\tgit stripspace -c\n-\telse\n-\t\tcat\n-\tfi\n+\tdone\n }\n #\n # List all submodules, prefixed with:\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex ac2434c..b435d03 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -262,19 +262,6 @@ EOF\n \ttest_cmp expected actual\n \"\n \n-test_expect_success '--for-status' \"\n-\tgit submodule summary --for-status HEAD^ >actual &&\n-\ttest_i18ncmp actual - <<EOF\n-# Submodule changes to be committed:\n-#\n-# * sm1 $head6...0000000:\n-#\n-# * sm2 0000000...$head7 (2):\n-#   > Add foo9\n-#\n-EOF\n-\"\n-\n test_expect_success 'fail when using --files together with --cached' \"\n \ttest_must_fail git submodule summary --files --cached\n \"\ndiff --git a/wt-status.c b/wt-status.c\nindex 958a53c..d91661d 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -665,6 +665,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tchar index[PATH_MAX];\n \tconst char *env[] = { NULL, NULL };\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\tstruct strbuf cmd_stdout = STRBUF_INIT;\n+\tstruct strbuf summary = STRBUF_INIT;\n+\tchar *summary_content;\n+\tsize_t len;\n \n \tsprintf(summary_limit, \"%d\", s->submodule_summary);\n \tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", s->index_file);\n@@ -673,7 +677,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \targv_array_push(&argv, \"submodule\");\n \targv_array_push(&argv, \"summary\");\n \targv_array_push(&argv, uncommitted ? \"--files\" : \"--cached\");\n-\targv_array_push(&argv, \"--for-status\");\n \targv_array_push(&argv, \"--summary-limit\");\n \targv_array_push(&argv, summary_limit);\n \tif (!uncommitted)\n@@ -685,9 +688,31 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tsm_summary.git_cmd = 1;\n \tsm_summary.no_stdin = 1;\n \tfflush(s->fp);\n-\tsm_summary.out = dup(fileno(s->fp));    /* run_command closes it */\n+\tsm_summary.out = -1;\n+\n \trun_command(&sm_summary);\n \targv_array_clear(&argv);\n+\n+\tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n+\n+\t/* prepend header, only if there's an actual output */\n+\tif (len) {\n+\t\tif (uncommitted)\n+\t\t\tstrbuf_addstr(&summary, _(\"Submodules changed but not updated:\"));\n+\t\telse\n+\t\t\tstrbuf_addstr(&summary, _(\"Submodule changes to be committed:\"));\n+\t\tstrbuf_addstr(&summary, \"\\n\\n\");\n+\t}\n+\tstrbuf_addbuf(&summary, &cmd_stdout);\n+\tstrbuf_release(&cmd_stdout);\n+\n+\tsummary_content = strbuf_detach(&summary, &len);\n+\tstrbuf_add_commented_lines(&summary, summary_content, len);\n+\tfree(summary_content);\n+\n+\tsummary_content = strbuf_detach(&summary, &len);\n+\tfprintf(s->fp, summary_content);\n+\tfree(summary_content);\n }\n \n static void wt_status_print_other(struct wt_status *s,\n-- \n1.8.4.12.gf9d53a3.dirty\n"},{"id":"226157","messageId":"1377781536-31955-5-git-send-email-Matthieu.Moy@imag.fr","threadId":"34796","inReplyTo":"1377781536-31955-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH v3 4/4] status: introduce status.displayCommentChar to disable display of #","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-08-29T13:05:36Z","receivedAt":"2013-08-29T13:05:36Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Historically, \"git status\" needed to prefix each output line with '#' so\nthat the output could be added as comment to the commit message. This\nprefix comment has no real purpose when \"git status\" is ran from the\ncommand-line, and this may distract users from the real content.\n\nAllow the user to disable this prefix comment. In the long run, if users\nlike the non-prefix output, it may make sense to flip the default value\nto true.\n\nObviously, status.displayCommentChar applies to \"git status\" but is\nignored by \"git commit\", so the status is still commented in\nCOMMIT_EDITMSG.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n Documentation/config.txt |  5 +++++\n builtin/commit.c         |  9 +++++++++\n t/t7508-status.sh        | 43 +++++++++++++++++++++++++++++++++++++++++++\n wt-status.c              | 35 +++++++++++++++++++++++++----------\n wt-status.h              |  1 +\n 5 files changed, 83 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ec57a15..0afa531 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2118,6 +2118,11 @@ status.branch::\n \tSet to true to enable --branch by default in linkgit:git-status[1].\n \tThe option --no-branch takes precedence over this variable.\n \n+status.displayCommentChar::\n+\tIf set to false, linkgit:git-status[1] will not prefix each\n+\tline with the comment char (`core.commentChar`, i.e. `#` by\n+\tdefault). Defaults to true.\n+\n status.showUntrackedFiles::\n \tBy default, linkgit:git-status[1] and linkgit:git-commit[1] show\n \tfiles which are not currently tracked by Git. Directories which\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 10acc53..d4cfced 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1182,6 +1182,10 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\ts->use_color = git_config_colorbool(k, v);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"status.displaycommentchar\")) {\n+\t\ts->display_comment_char = git_config_bool(k, v);\n+\t\treturn 0;\n+\t}\n \tif (!prefixcmp(k, \"status.color.\") || !prefixcmp(k, \"color.status.\")) {\n \t\tint slot = parse_status_slot(k, 13);\n \t\tif (slot < 0)\n@@ -1496,6 +1500,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tgitmodules_config();\n \tgit_config(git_commit_config, &s);\n \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n+\t/*\n+\t * Ignore status.displayCommentChar: we do need comments in\n+\t * COMMIT_EDITMSG.\n+\t */\n+\ts.display_comment_char = 1;\n \tdetermine_whence(&s);\n \ts.colopts = 0;\n \ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex ac3d0fe..d0d7c4a 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -113,6 +113,39 @@ test_expect_success 'status (2)' '\n \ttest_i18ncmp expect output\n '\n \n+strip_comments () {\n+\tsed \"s/^\\# //; s/^\\#$//; s/^#\\t/\\t/\" <\"$1\" >\"$1\".tmp &&\n+\trm \"$1\" && mv \"$1\".tmp \"$1\"\n+}\n+\n+test_expect_success 'status with status.displayCommentChar=false' '\n+\tstrip_comments expect &&\n+\tgit -c status.displayCommentChar=false status >output &&\n+\ttest_i18ncmp expect output\n+'\n+\n+test_expect_success 'setup fake editor' '\n+\tcat >.git/editor <<-\\EOF &&\n+\t#! /bin/sh\n+\tcp \"$1\" output\n+EOF\n+\tchmod 755 .git/editor\n+'\n+\n+commit_template_commented () {\n+\t(\n+\t\tEDITOR=.git/editor &&\n+\t\texport EDITOR &&\n+\t\t# Fails due to empty message\n+\t\ttest_must_fail git commit\n+\t) &&\n+\t! grep '^[^#]' output\n+}\n+\n+test_expect_success 'commit ignores status.displayCommentChar=false in COMMIT_EDITMSG' '\n+\tcommit_template_commented\n+'\n+\n cat >expect <<\\EOF\n # On branch master\n # Changes to be committed:\n@@ -872,6 +905,16 @@ test_expect_success 'status submodule summary' '\n \ttest_i18ncmp expect output\n '\n \n+test_expect_success 'status submodule summary with status.displayCommentChar=false' '\n+\tstrip_comments expect &&\n+\tgit -c status.displayCommentChar=false status >output &&\n+\ttest_i18ncmp expect output\n+'\n+\n+test_expect_success 'commit with submodule summary ignores displayCommentChar' '\n+\tcommit_template_commented\n+'\n+\n cat >expect <<EOF\n  M dir1/modified\n A  dir2/added\ndiff --git a/wt-status.c b/wt-status.c\nindex d91661d..cdbcd6f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -46,9 +46,11 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,\n \n \tstrbuf_vaddf(&sb, fmt, ap);\n \tif (!sb.len) {\n-\t\tstrbuf_addch(&sb, comment_line_char);\n-\t\tif (!trail)\n-\t\t\tstrbuf_addch(&sb, ' ');\n+\t\tif (s->display_comment_char) {\n+\t\t\tstrbuf_addch(&sb, comment_line_char);\n+\t\t\tif (!trail)\n+\t\t\t\tstrbuf_addch(&sb, ' ');\n+\t\t}\n \t\tcolor_print_strbuf(s->fp, color, &sb);\n \t\tif (trail)\n \t\t\tfprintf(s->fp, \"%s\", trail);\n@@ -59,7 +61,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,\n \t\teol = strchr(line, '\\n');\n \n \t\tstrbuf_reset(&linebuf);\n-\t\tif (at_bol) {\n+\t\tif (at_bol && s->display_comment_char) {\n \t\t\tstrbuf_addch(&linebuf, comment_line_char);\n \t\t\tif (*line != '\\n' && *line != '\\t')\n \t\t\t\tstrbuf_addch(&linebuf, ' ');\n@@ -129,6 +131,7 @@ void wt_status_prepare(struct wt_status *s)\n \ts->untracked.strdup_strings = 1;\n \ts->ignored.strdup_strings = 1;\n \ts->show_branch = -1;  /* unspecified */\n+\ts->display_comment_char = 1;\n }\n \n static void wt_status_print_unmerged_header(struct wt_status *s)\n@@ -706,9 +709,11 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tstrbuf_addbuf(&summary, &cmd_stdout);\n \tstrbuf_release(&cmd_stdout);\n \n-\tsummary_content = strbuf_detach(&summary, &len);\n-\tstrbuf_add_commented_lines(&summary, summary_content, len);\n-\tfree(summary_content);\n+\tif (s->display_comment_char) {\n+\t\tsummary_content = strbuf_detach(&summary, &len);\n+\t\tstrbuf_add_commented_lines(&summary, summary_content, len);\n+\t\tfree(summary_content);\n+\t}\n \n \tsummary_content = strbuf_detach(&summary, &len);\n \tfprintf(s->fp, summary_content);\n@@ -793,6 +798,8 @@ static void wt_status_print_tracking(struct wt_status *s)\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *cp, *ep;\n \tstruct branch *branch;\n+\tchar comment_line_string[3];\n+\tint i;\n \n \tassert(s->branch && !s->is_initial);\n \tif (prefixcmp(s->branch, \"refs/heads/\"))\n@@ -801,12 +808,20 @@ static void wt_status_print_tracking(struct wt_status *s)\n \tif (!format_tracking_info(branch, &sb))\n \t\treturn;\n \n+\ti = 0;\n+\tif (s->display_comment_char) {\n+\t\tcomment_line_string[i++] = comment_line_char;\n+\t\tcomment_line_string[i++] = ' ';\n+\t}\n+\tcomment_line_string[i] = '\\0';\n+\n \tfor (cp = sb.buf; (ep = strchr(cp, '\\n')) != NULL; cp = ep + 1)\n \t\tcolor_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s),\n-\t\t\t\t \"%c %.*s\", comment_line_char,\n+\t\t\t\t \"%s%.*s\", comment_line_string,\n \t\t\t\t (int)(ep - cp), cp);\n-\tcolor_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), \"%c\",\n-\t\t\t comment_line_char);\n+\tif (s->display_comment_char)\n+\t\tcolor_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), \"%c\",\n+\t\t\t\t comment_line_char);\n }\n \n static int has_unmerged(struct wt_status *s)\ndiff --git a/wt-status.h b/wt-status.h\nindex fb7152e..ac02c64 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -50,6 +50,7 @@ struct wt_status {\n \tenum commit_whence whence;\n \tint nowarn;\n \tint use_color;\n+\tint display_comment_char;\n \tint relative_paths;\n \tint submodule_summary;\n \tint show_ignored_files;\n-- \n1.8.4.12.gf9d53a3.dirty\n"},{"id":"226184","messageId":"xmqqbo4g79mf.fsf@gitster.dls.corp.google.com","threadId":"34796","inReplyTo":"1377781536-31955-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [RFC/PATCH v3 0/4] Disable \"git status\" comment prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-29T16:19:20Z","receivedAt":"2013-08-29T16:19:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> ... Right now, my preference would be to call it\n> status.oldStyle and default it to false (i.e. change the behavior, but\n> allow old-timers to get back the old one).\n\nSounds sensible, at least to me.\n"},{"id":"226251","messageId":"521FA6ED.9010005@web.de","threadId":"34796","inReplyTo":"1377781536-31955-4-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-08-29T19:54:21Z","receivedAt":"2013-08-29T19:54:21Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 29.08.2013 15:05, schrieb Matthieu Moy:\n> The --for-status option was an undocumented option used only by\n> wt-status.c, which inserted a header and commented out the output. We can\n> achieve the same result within wt-status.c, without polluting the\n> submodule command-line options.\n> \n> This will make it easier to disable the comments from wt-status.c later.\n\nCool, thanks for implementing this!\n\nBut unfortunately this change collides with bc/submodule-status-ignored\n(I added Brian to the CC) which is currently on its way to next. Your\npatch will break the fix in the second commit, because that's only\nenabled when the submodule script sees the --for-status option.\n\nA solution for that would be to rebase your patches on top of pu, drop\nthe first two hunks of the change to git-submodule.sh and still pass\nthe --for-status option to git-submodule.sh. This would move adding the\ncomment characters into wt-status.c but will still enable the script to\nhonor the ignore=all setting when called by status.\n\nJunio, what is our take on changing behavior of undocumented internally\nused options? Do we have to add a new --for-status2 (which doesn't add\nthe comment characters) or is it ok to just change the behavior of the\nexisting option?\n\n> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n> ---\n>  git-submodule.sh             | 17 +----------------\n>  t/t7401-submodule-summary.sh | 13 -------------\n>  wt-status.c                  | 29 +++++++++++++++++++++++++++--\n>  3 files changed, 28 insertions(+), 31 deletions(-)\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 2979197..fccdec9 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -965,7 +965,6 @@ set_name_rev () {\n>  #\n>  cmd_summary() {\n>  \tsummary_limit=-1\n> -\tfor_status=\n>  \tdiff_cmd=diff-index\n>  \n>  \t# parse $args after \"submodule ... summary\".\n> @@ -978,9 +977,6 @@ cmd_summary() {\n>  \t\t--files)\n>  \t\t\tfiles=\"$1\"\n>  \t\t\t;;\n> -\t\t--for-status)\n> -\t\t\tfor_status=\"$1\"\n> -\t\t\t;;\n>  \t\t-n|--summary-limit)\n>  \t\t\tsummary_limit=\"$2\"\n>  \t\t\tisnumber \"$summary_limit\" || usage\n> @@ -1149,18 +1145,7 @@ cmd_summary() {\n>  \t\t\techo\n>  \t\tfi\n>  \t\techo\n> -\tdone |\n> -\tif test -n \"$for_status\"; then\n> -\t\tif [ -n \"$files\" ]; then\n> -\t\t\tgettextln \"Submodules changed but not updated:\" | git stripspace -c\n> -\t\telse\n> -\t\t\tgettextln \"Submodule changes to be committed:\" | git stripspace -c\n> -\t\tfi\n> -\t\tprintf \"\\n\" | git stripspace -c\n> -\t\tgit stripspace -c\n> -\telse\n> -\t\tcat\n> -\tfi\n> +\tdone\n>  }\n>  #\n>  # List all submodules, prefixed with:\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index ac2434c..b435d03 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -262,19 +262,6 @@ EOF\n>  \ttest_cmp expected actual\n>  \"\n>  \n> -test_expect_success '--for-status' \"\n> -\tgit submodule summary --for-status HEAD^ >actual &&\n> -\ttest_i18ncmp actual - <<EOF\n> -# Submodule changes to be committed:\n> -#\n> -# * sm1 $head6...0000000:\n> -#\n> -# * sm2 0000000...$head7 (2):\n> -#   > Add foo9\n> -#\n> -EOF\n> -\"\n> -\n>  test_expect_success 'fail when using --files together with --cached' \"\n>  \ttest_must_fail git submodule summary --files --cached\n>  \"\n> diff --git a/wt-status.c b/wt-status.c\n> index 958a53c..d91661d 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -665,6 +665,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n>  \tchar index[PATH_MAX];\n>  \tconst char *env[] = { NULL, NULL };\n>  \tstruct argv_array argv = ARGV_ARRAY_INIT;\n> +\tstruct strbuf cmd_stdout = STRBUF_INIT;\n> +\tstruct strbuf summary = STRBUF_INIT;\n> +\tchar *summary_content;\n> +\tsize_t len;\n>  \n>  \tsprintf(summary_limit, \"%d\", s->submodule_summary);\n>  \tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", s->index_file);\n> @@ -673,7 +677,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n>  \targv_array_push(&argv, \"submodule\");\n>  \targv_array_push(&argv, \"summary\");\n>  \targv_array_push(&argv, uncommitted ? \"--files\" : \"--cached\");\n> -\targv_array_push(&argv, \"--for-status\");\n>  \targv_array_push(&argv, \"--summary-limit\");\n>  \targv_array_push(&argv, summary_limit);\n>  \tif (!uncommitted)\n> @@ -685,9 +688,31 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n>  \tsm_summary.git_cmd = 1;\n>  \tsm_summary.no_stdin = 1;\n>  \tfflush(s->fp);\n> -\tsm_summary.out = dup(fileno(s->fp));    /* run_command closes it */\n> +\tsm_summary.out = -1;\n> +\n>  \trun_command(&sm_summary);\n>  \targv_array_clear(&argv);\n> +\n> +\tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n> +\n> +\t/* prepend header, only if there's an actual output */\n> +\tif (len) {\n> +\t\tif (uncommitted)\n> +\t\t\tstrbuf_addstr(&summary, _(\"Submodules changed but not updated:\"));\n> +\t\telse\n> +\t\t\tstrbuf_addstr(&summary, _(\"Submodule changes to be committed:\"));\n> +\t\tstrbuf_addstr(&summary, \"\\n\\n\");\n> +\t}\n> +\tstrbuf_addbuf(&summary, &cmd_stdout);\n> +\tstrbuf_release(&cmd_stdout);\n> +\n> +\tsummary_content = strbuf_detach(&summary, &len);\n> +\tstrbuf_add_commented_lines(&summary, summary_content, len);\n> +\tfree(summary_content);\n> +\n> +\tsummary_content = strbuf_detach(&summary, &len);\n> +\tfprintf(s->fp, summary_content);\n> +\tfree(summary_content);\n>  }\n>  \n>  static void wt_status_print_other(struct wt_status *s,\n> \n"},{"id":"226252","messageId":"xmqqsixs2run.fsf@gitster.dls.corp.google.com","threadId":"34796","inReplyTo":"1377781536-31955-4-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-29T19:56:48Z","receivedAt":"2013-08-29T19:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> +\t/* prepend header, only if there's an actual output */\n> +\tif (len) {\n> +\t\tif (uncommitted)\n> +\t\t\tstrbuf_addstr(&summary, _(\"Submodules changed but not updated:\"));\n> +\t\telse\n> +\t\t\tstrbuf_addstr(&summary, _(\"Submodule changes to be committed:\"));\n> +\t\tstrbuf_addstr(&summary, \"\\n\\n\");\n> +\t}\n> +\tstrbuf_addbuf(&summary, &cmd_stdout);\n> +\tstrbuf_release(&cmd_stdout);\n> +\n> +\tsummary_content = strbuf_detach(&summary, &len);\n> +\tstrbuf_add_commented_lines(&summary, summary_content, len);\n> +\tfree(summary_content);\n> +\n> +\tsummary_content = strbuf_detach(&summary, &len);\n> +\tfprintf(s->fp, summary_content);\n> +\tfree(summary_content);\n\nThis \"fprintf()\" looks bogus to me.  How about adding this on top?\n\n wt-status.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex d91661d..1f17652 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -710,9 +710,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tstrbuf_add_commented_lines(&summary, summary_content, len);\n \tfree(summary_content);\n \n-\tsummary_content = strbuf_detach(&summary, &len);\n-\tfprintf(s->fp, summary_content);\n-\tfree(summary_content);\n+\tfputs(summary.buf, s->fp);\n+\tstrbuf_release(&summary);\n }\n \n static void wt_status_print_other(struct wt_status *s,\n"},{"id":"226263","messageId":"vpqr4dcnr67.fsf@anie.imag.fr","threadId":"34796","inReplyTo":"xmqqsixs2run.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-08-29T21:05:52Z","receivedAt":"2013-08-29T21:05:52Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n>\n>> +\t/* prepend header, only if there's an actual output */\n>> +\tif (len) {\n>> +\t\tif (uncommitted)\n>> +\t\t\tstrbuf_addstr(&summary, _(\"Submodules changed but not updated:\"));\n>> +\t\telse\n>> +\t\t\tstrbuf_addstr(&summary, _(\"Submodule changes to be committed:\"));\n>> +\t\tstrbuf_addstr(&summary, \"\\n\\n\");\n>> +\t}\n>> +\tstrbuf_addbuf(&summary, &cmd_stdout);\n>> +\tstrbuf_release(&cmd_stdout);\n>> +\n>> +\tsummary_content = strbuf_detach(&summary, &len);\n>> +\tstrbuf_add_commented_lines(&summary, summary_content, len);\n>> +\tfree(summary_content);\n>> +\n>> +\tsummary_content = strbuf_detach(&summary, &len);\n>> +\tfprintf(s->fp, summary_content);\n>> +\tfree(summary_content);\n>\n> This \"fprintf()\" looks bogus to me.\n\nOops, indeed. I forgot the \"%s\".\n\n> How about adding this on top?\n\nYour solution is better, yes.\n\n>  wt-status.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index d91661d..1f17652 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -710,9 +710,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n>  \tstrbuf_add_commented_lines(&summary, summary_content, len);\n>  \tfree(summary_content);\n>  \n> -\tsummary_content = strbuf_detach(&summary, &len);\n> -\tfprintf(s->fp, summary_content);\n> -\tfree(summary_content);\n> +\tfputs(summary.buf, s->fp);\n> +\tstrbuf_release(&summary);\n>  }\n>  \n>  static void wt_status_print_other(struct wt_status *s,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"226265","messageId":"vpqioyonqdn.fsf@anie.imag.fr","threadId":"34796","inReplyTo":"521FA6ED.9010005@web.de","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-08-29T21:23:00Z","receivedAt":"2013-08-29T21:23:00Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Am 29.08.2013 15:05, schrieb Matthieu Moy:\n>> The --for-status option was an undocumented option used only by\n>> wt-status.c, which inserted a header and commented out the output. We can\n>> achieve the same result within wt-status.c, without polluting the\n>> submodule command-line options.\n>> \n>> This will make it easier to disable the comments from wt-status.c later.\n>\n> Cool, thanks for implementing this!\n>\n> But unfortunately this change collides with bc/submodule-status-ignored\n> (I added Brian to the CC) which is currently on its way to next.\n\nThanks for pointing that out. The patch looks buggy:\n\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1036,6 +1036,13 @@ cmd_summary() {\n                do\n                        # Always show modules deleted or type-changed (blob<->module)\n                        test $status = D -o $status = T && echo \"$sm_path\" && continue\n+                       # Respect the ignore setting for --for-status.\n+                       if test -n $for_status\n+                       then\n+                               name=$(module_name \"$sm_path\")\n+                               ignore_config=$(get_submodule_config \"$name\" ignore none)\n+                               test $status != A -a $ignore_config = all && continue\n+                       fi\n\nBecause of the missing quotes around $for_status, it seems the test is\nunconditionnaly true:\n\n$ test -n t ; echo $?\n0\n$ test -n   ; echo $?\n0\n\nThis makes me wonder why the ignore configuration should be considered\nonly with --for-status. Why not turn that into\n\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1036,6 +1036,13 @@ cmd_summary() {\n                do\n                        # Always show modules deleted or type-changed (blob<->module)\n                        test $status = D -o $status = T && echo \"$sm_path\" && continue\n+\t\t\t# Respect the ignore setting\n+\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n+\t\t\ttest $status != A -a $ignore_config = all && continue\n\n?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"226364","messageId":"5220F539.9050700@web.de","threadId":"34796","inReplyTo":"vpqioyonqdn.fsf@anie.imag.fr","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-08-30T19:40:41Z","receivedAt":"2013-08-30T19:40:41Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 29.08.2013 23:23, schrieb Matthieu Moy:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n>> Am 29.08.2013 15:05, schrieb Matthieu Moy:\n>>> The --for-status option was an undocumented option used only by\n>>> wt-status.c, which inserted a header and commented out the output. We can\n>>> achieve the same result within wt-status.c, without polluting the\n>>> submodule command-line options.\n>>>\n>>> This will make it easier to disable the comments from wt-status.c later.\n>>\n>> Cool, thanks for implementing this!\n>>\n>> But unfortunately this change collides with bc/submodule-status-ignored\n>> (I added Brian to the CC) which is currently on its way to next.\n> \n> Thanks for pointing that out. The patch looks buggy:\n\nOk, I'll tak\n\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -1036,6 +1036,13 @@ cmd_summary() {\n>                 do\n>                         # Always show modules deleted or type-changed (blob<->module)\n>                         test $status = D -o $status = T && echo \"$sm_path\" && continue\n> +                       # Respect the ignore setting for --for-status.\n> +                       if test -n $for_status\n> +                       then\n> +                               name=$(module_name \"$sm_path\")\n> +                               ignore_config=$(get_submodule_config \"$name\" ignore none)\n> +                               test $status != A -a $ignore_config = all && continue\n> +                       fi\n> \n> Because of the missing quotes around $for_status, it seems the test is\n> unconditionnaly true:\n> \n> $ test -n t ; echo $?\n> 0\n> $ test -n   ; echo $?\n> 0\n> \n> This makes me wonder why the ignore configuration should be considered\n> only with --for-status. Why not turn that into\n> \n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -1036,6 +1036,13 @@ cmd_summary() {\n>                 do\n>                         # Always show modules deleted or type-changed (blob<->module)\n>                         test $status = D -o $status = T && echo \"$sm_path\" && continue\n> +\t\t\t# Respect the ignore setting\n> +\t\t\tname=$(module_name \"$sm_path\")\n> +\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n> +\t\t\ttest $status != A -a $ignore_config = all && continue\n> \n> ?\n> \n"},{"id":"226365","messageId":"5220F7D7.2000300@web.de","threadId":"34796","inReplyTo":"5220F539.9050700@web.de","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-08-30T19:51:51Z","receivedAt":"2013-08-30T19:51:51Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 30.08.2013 21:40, schrieb Jens Lehmann:\n> Am 29.08.2013 23:23, schrieb Matthieu Moy:\n>> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>>\n>>> Am 29.08.2013 15:05, schrieb Matthieu Moy:\n>>>> The --for-status option was an undocumented option used only by\n>>>> wt-status.c, which inserted a header and commented out the output. We can\n>>>> achieve the same result within wt-status.c, without polluting the\n>>>> submodule command-line options.\n>>>>\n>>>> This will make it easier to disable the comments from wt-status.c later.\n>>>\n>>> Cool, thanks for implementing this!\n>>>\n>>> But unfortunately this change collides with bc/submodule-status-ignored\n>>> (I added Brian to the CC) which is currently on its way to next.\n>>\n>> Thanks for pointing that out. The patch looks buggy:\n> \n> Ok, I'll tak\n\nSorry, I accidentally hit \"Send\" ... :-(\n\nOk, I'll take a look and will comment on that soon.\n\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -1036,6 +1036,13 @@ cmd_summary() {\n>>                 do\n>>                         # Always show modules deleted or type-changed (blob<->module)\n>>                         test $status = D -o $status = T && echo \"$sm_path\" && continue\n>> +                       # Respect the ignore setting for --for-status.\n>> +                       if test -n $for_status\n>> +                       then\n>> +                               name=$(module_name \"$sm_path\")\n>> +                               ignore_config=$(get_submodule_config \"$name\" ignore none)\n>> +                               test $status != A -a $ignore_config = all && continue\n>> +                       fi\n>>\n>> Because of the missing quotes around $for_status, it seems the test is\n>> unconditionnaly true:\n>>\n>> $ test -n t ; echo $?\n>> 0\n>> $ test -n   ; echo $?\n>> 0\n>>\n>> This makes me wonder why the ignore configuration should be considered\n>> only with --for-status. Why not turn that into\n\nPlease don't. That changes the default behavior of submodule summary,\nwhich never ignores any submodules. The ignore logic was added to core\ngit after commands like diff and status learned to check submodules for\nmodifications too. That was bad for people who used submodules to store\nmany and/or huge files in a way that wouldn't slow down diff or status,\nas it slowed them down again. The ignore option allowed them to continue\nusing submodules for that purpose. They still need to have the submodule\nscript ignore the ignore settings, because running them is the point in\ntime they want to take the extra effort to look into those submodules\nthey normally ignore. And that's why the submodule totally lacks any\noption to control the ignore behavior, which we would also have to add\nif we would follow your proposal.\n\nSo I think it's either changing the default behavior of --for-status or\nadding another option (--for-status-wo-comment or such) which will honor\nthe ignore setting only when called from status.\n\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -1036,6 +1036,13 @@ cmd_summary() {\n>>                 do\n>>                         # Always show modules deleted or type-changed (blob<->module)\n>>                         test $status = D -o $status = T && echo \"$sm_path\" && continue\n>> +\t\t\t# Respect the ignore setting\n>> +\t\t\tname=$(module_name \"$sm_path\")\n>> +\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n>> +\t\t\ttest $status != A -a $ignore_config = all && continue\n>>\n>> ?\n>>\n> \n"},{"id":"226367","messageId":"5220FBD5.6020807@web.de","threadId":"34796","inReplyTo":"5220F7D7.2000300@web.de","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-08-30T20:08:53Z","receivedAt":"2013-08-30T20:08:53Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 30.08.2013 21:51, schrieb Jens Lehmann:\n> Am 30.08.2013 21:40, schrieb Jens Lehmann:\n>> Am 29.08.2013 23:23, schrieb Matthieu Moy:\n>>> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>>>\n>>>> Am 29.08.2013 15:05, schrieb Matthieu Moy:\n>>>>> The --for-status option was an undocumented option used only by\n>>>>> wt-status.c, which inserted a header and commented out the output. We can\n>>>>> achieve the same result within wt-status.c, without polluting the\n>>>>> submodule command-line options.\n>>>>>\n>>>>> This will make it easier to disable the comments from wt-status.c later.\n>>>>\n>>>> Cool, thanks for implementing this!\n>>>>\n>>>> But unfortunately this change collides with bc/submodule-status-ignored\n>>>> (I added Brian to the CC) which is currently on its way to next.\n>>>\n>>> Thanks for pointing that out. The patch looks buggy:\n>>\n>> Ok, I'll tak\n> \n> Sorry, I accidentally hit \"Send\" ... :-(\n> \n> Ok, I'll take a look and will comment on that soon.\n> \n>>> --- a/git-submodule.sh\n>>> +++ b/git-submodule.sh\n>>> @@ -1036,6 +1036,13 @@ cmd_summary() {\n>>>                 do\n>>>                         # Always show modules deleted or type-changed (blob<->module)\n>>>                         test $status = D -o $status = T && echo \"$sm_path\" && continue\n>>> +                       # Respect the ignore setting for --for-status.\n>>> +                       if test -n $for_status\n>>> +                       then\n>>> +                               name=$(module_name \"$sm_path\")\n>>> +                               ignore_config=$(get_submodule_config \"$name\" ignore none)\n>>> +                               test $status != A -a $ignore_config = all && continue\n>>> +                       fi\n>>>\n>>> Because of the missing quotes around $for_status, it seems the test is\n>>> unconditionnaly true:\n>>>\n>>> $ test -n t ; echo $?\n>>> 0\n>>> $ test -n   ; echo $?\n>>> 0\n\nRight you are, I did not notice the missing \"\" in my review. Looks like\nwe also should add one or more tests making sure that submodule summary\nand status never honor the ignore settings.\n"},{"id":"226423","messageId":"20130831160759.GB899703@vauxhall.crustytoothpaste.net","threadId":"34796","inReplyTo":"5220FBD5.6020807@web.de","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-08-31T17:08:30Z","receivedAt":"2013-08-31T17:08:30Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Aug 30, 2013 at 10:08:53PM +0200, Jens Lehmann wrote:\n> Am 30.08.2013 21:51, schrieb Jens Lehmann:\n> > Am 30.08.2013 21:40, schrieb Jens Lehmann:\n> >> Am 29.08.2013 23:23, schrieb Matthieu Moy:\n> >>> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> >>>\n> >>>> Am 29.08.2013 15:05, schrieb Matthieu Moy:\n> >>> Because of the missing quotes around $for_status, it seems the test is\n> >>> unconditionnaly true:\n> >>>\n> >>> $ test -n t ; echo $?\n> >>> 0\n> >>> $ test -n   ; echo $?\n> >>> 0\n> \n> Right you are, I did not notice the missing \"\" in my review. Looks like\n> we also should add one or more tests making sure that submodule summary\n> and status never honor the ignore settings.\n\nHow do we want to handle this?  I can send a reroll and include some\nnew tests, but if this code is going away, then there's no point.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"226486","messageId":"5223457F.2080608@web.de","threadId":"34796","inReplyTo":"20130831160759.GB899703@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-09-01T13:47:43Z","receivedAt":"2013-09-01T13:47:43Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 31.08.2013 19:08, schrieb brian m. carlson:\n> On Fri, Aug 30, 2013 at 10:08:53PM +0200, Jens Lehmann wrote:\n>> Am 30.08.2013 21:51, schrieb Jens Lehmann:\n>>> Am 30.08.2013 21:40, schrieb Jens Lehmann:\n>>>> Am 29.08.2013 23:23, schrieb Matthieu Moy:\n>>>>> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>>>>>\n>>>>>> Am 29.08.2013 15:05, schrieb Matthieu Moy:\n>>>>> Because of the missing quotes around $for_status, it seems the test is\n>>>>> unconditionnaly true:\n>>>>>\n>>>>> $ test -n t ; echo $?\n>>>>> 0\n>>>>> $ test -n   ; echo $?\n>>>>> 0\n>>\n>> Right you are, I did not notice the missing \"\" in my review. Looks like\n>> we also should add one or more tests making sure that submodule summary\n>> and status never honor the ignore settings.\n> \n> How do we want to handle this?  I can send a reroll and include some\n> new tests, but if this code is going away, then there's no point.\n\nA reroll would be great, as I think your patch is a bugfix that should\ngo in rather soonish no matter how we continue with the comment signs.\nTwo new tests (one for submodule summary and one for submodule status)\nwith both the global ignore setting and a submodule specific one set\nto \"all\" showing no impact on the output would suffice (and trigger the\nthen also fixed missing \"\" bug ;-).\n"},{"id":"226659","messageId":"52263947.6040400@web.de","threadId":"34796","inReplyTo":"521FA6ED.9010005@web.de","subject":"Re: [PATCH v3 3/4] get rid of \"git submodule summary --for-status\"","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-09-03T19:32:23Z","receivedAt":"2013-09-03T19:32:23Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 29.08.2013 21:54, schrieb Jens Lehmann:\n> Am 29.08.2013 15:05, schrieb Matthieu Moy:\n>> The --for-status option was an undocumented option used only by\n>> wt-status.c, which inserted a header and commented out the output. We can\n>> achieve the same result within wt-status.c, without polluting the\n>> submodule command-line options.\n>>\n>> This will make it easier to disable the comments from wt-status.c later.\n> \n> Cool, thanks for implementing this!\n> \n> But unfortunately this change collides with bc/submodule-status-ignored\n> (I added Brian to the CC) which is currently on its way to next. Your\n> patch will break the fix in the second commit, because that's only\n> enabled when the submodule script sees the --for-status option.\n> \n> A solution for that would be to rebase your patches on top of pu, drop\n> the first two hunks of the change to git-submodule.sh and still pass\n> the --for-status option to git-submodule.sh. This would move adding the\n> comment characters into wt-status.c but will still enable the script to\n> honor the ignore=all setting when called by status.\n\nI think we should go that route, --for-status is an internal option and\nnobody should rely on its behavior.\n\n>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n>> ---\n>>  git-submodule.sh             | 17 +----------------\n>>  t/t7401-submodule-summary.sh | 13 -------------\n>>  wt-status.c                  | 29 +++++++++++++++++++++++++++--\n>>  3 files changed, 28 insertions(+), 31 deletions(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 2979197..fccdec9 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -965,7 +965,6 @@ set_name_rev () {\n>>  #\n>>  cmd_summary() {\n>>  \tsummary_limit=-1\n>> -\tfor_status=\n>>  \tdiff_cmd=diff-index\n>>  \n>>  \t# parse $args after \"submodule ... summary\".\n>> @@ -978,9 +977,6 @@ cmd_summary() {\n>>  \t\t--files)\n>>  \t\t\tfiles=\"$1\"\n>>  \t\t\t;;\n>> -\t\t--for-status)\n>> -\t\t\tfor_status=\"$1\"\n>> -\t\t\t;;\n>>  \t\t-n|--summary-limit)\n>>  \t\t\tsummary_limit=\"$2\"\n>>  \t\t\tisnumber \"$summary_limit\" || usage\n\nPlease drop the two hunks above ...\n\n>> @@ -1149,18 +1145,7 @@ cmd_summary() {\n>>  \t\t\techo\n>>  \t\tfi\n>>  \t\techo\n>> -\tdone |\n>> -\tif test -n \"$for_status\"; then\n>> -\t\tif [ -n \"$files\" ]; then\n>> -\t\t\tgettextln \"Submodules changed but not updated:\" | git stripspace -c\n>> -\t\telse\n>> -\t\t\tgettextln \"Submodule changes to be committed:\" | git stripspace -c\n>> -\t\tfi\n>> -\t\tprintf \"\\n\" | git stripspace -c\n>> -\t\tgit stripspace -c\n>> -\telse\n>> -\t\tcat\n>> -\tfi\n>> +\tdone\n>>  }\n>>  #\n>>  # List all submodules, prefixed with:\n>> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n>> index ac2434c..b435d03 100755\n>> --- a/t/t7401-submodule-summary.sh\n>> +++ b/t/t7401-submodule-summary.sh\n>> @@ -262,19 +262,6 @@ EOF\n>>  \ttest_cmp expected actual\n>>  \"\n>>  \n>> -test_expect_success '--for-status' \"\n>> -\tgit submodule summary --for-status HEAD^ >actual &&\n>> -\ttest_i18ncmp actual - <<EOF\n>> -# Submodule changes to be committed:\n>> -#\n>> -# * sm1 $head6...0000000:\n>> -#\n>> -# * sm2 0000000...$head7 (2):\n>> -#   > Add foo9\n>> -#\n>> -EOF\n>> -\"\n>> -\n\n... and just remove the \"# \" from the expected output here. This\ntest can be removed when we use\n\n>>  test_expect_success 'fail when using --files together with --cached' \"\n>>  \ttest_must_fail git submodule summary --files --cached\n>>  \"\n>> diff --git a/wt-status.c b/wt-status.c\n>> index 958a53c..d91661d 100644\n>> --- a/wt-status.c\n>> +++ b/wt-status.c\n>> @@ -665,6 +665,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n>>  \tchar index[PATH_MAX];\n>>  \tconst char *env[] = { NULL, NULL };\n>>  \tstruct argv_array argv = ARGV_ARRAY_INIT;\n>> +\tstruct strbuf cmd_stdout = STRBUF_INIT;\n>> +\tstruct strbuf summary = STRBUF_INIT;\n>> +\tchar *summary_content;\n>> +\tsize_t len;\n>>  \n>>  \tsprintf(summary_limit, \"%d\", s->submodule_summary);\n>>  \tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", s->index_file);\n>> @@ -673,7 +677,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n>>  \targv_array_push(&argv, \"submodule\");\n>>  \targv_array_push(&argv, \"summary\");\n>>  \targv_array_push(&argv, uncommitted ? \"--files\" : \"--cached\");\n>> -\targv_array_push(&argv, \"--for-status\");\n\nAnd the line above has to stay.\n\n>>  \targv_array_push(&argv, \"--summary-limit\");\n>>  \targv_array_push(&argv, summary_limit);\n>>  \tif (!uncommitted)\n>> @@ -685,9 +688,31 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n>>  \tsm_summary.git_cmd = 1;\n>>  \tsm_summary.no_stdin = 1;\n>>  \tfflush(s->fp);\n>> -\tsm_summary.out = dup(fileno(s->fp));    /* run_command closes it */\n>> +\tsm_summary.out = -1;\n>> +\n>>  \trun_command(&sm_summary);\n>>  \targv_array_clear(&argv);\n>> +\n>> +\tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n>> +\n>> +\t/* prepend header, only if there's an actual output */\n>> +\tif (len) {\n>> +\t\tif (uncommitted)\n>> +\t\t\tstrbuf_addstr(&summary, _(\"Submodules changed but not updated:\"));\n>> +\t\telse\n>> +\t\t\tstrbuf_addstr(&summary, _(\"Submodule changes to be committed:\"));\n>> +\t\tstrbuf_addstr(&summary, \"\\n\\n\");\n>> +\t}\n>> +\tstrbuf_addbuf(&summary, &cmd_stdout);\n>> +\tstrbuf_release(&cmd_stdout);\n>> +\n>> +\tsummary_content = strbuf_detach(&summary, &len);\n>> +\tstrbuf_add_commented_lines(&summary, summary_content, len);\n>> +\tfree(summary_content);\n>> +\n>> +\tsummary_content = strbuf_detach(&summary, &len);\n>> +\tfprintf(s->fp, summary_content);\n>> +\tfree(summary_content);\n>>  }\n>>  \n>>  static void wt_status_print_other(struct wt_status *s,\n\nJunio already commented on this part.\n"}]}