{"thread":{"id":"13050","subject":"[PATCH v2 1/3] git-submodule summary: --for-status option","startedAt":"2008-04-10T15:35:24Z","lastAt":"2008-04-12T16:13:19Z","messageCount":8,"participants":["Ping Yin","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"74067","messageId":"1207841727-7840-1-git-send-email-pkufranky@gmail.com","threadId":"13050","inReplyTo":null,"subject":"[PATCH v2] builtin-status: submodule summary support","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-04-10T15:35:24Z","receivedAt":"2008-04-10T15:35:24Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"This is a resend after v1.5.5.\n"},{"id":"74065","messageId":"1207841727-7840-2-git-send-email-pkufranky@gmail.com","threadId":"13050","inReplyTo":"1207841727-7840-1-git-send-email-pkufranky@gmail.com","subject":"[PATCH v2 1/3] git-submodule summary: --for-status option","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-04-10T15:35:25Z","receivedAt":"2008-04-10T15:35:25Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"The --for-status option is mainly used by builtin-status/commit.\nIt adds 'Modified submodules:' line at top and  '# ' prefix to all\nfollowing lines.\n\nSigned-off-by: Ping Yin <pkufranky@gmail.com>\n---\n git-submodule.sh             |   17 ++++++++++++++++-\n t/t7401-submodule-summary.sh |   13 +++++++++++++\n 2 files changed, 29 insertions(+), 1 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 56ec353..d7937a5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -342,6 +342,7 @@ set_name_rev () {\n #\n cmd_summary() {\n \tsummary_limit=-1\n+\tfor_status=\n \n \t# parse $args after \"submodule ... summary\".\n \twhile test $# -ne 0\n@@ -350,6 +351,9 @@ cmd_summary() {\n \t\t--cached)\n \t\t\tcached=\"$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\tif summary_limit=$(($2 + 0)) 2>/dev/null && test \"$summary_limit\" = \"$2\"\n \t\t\tthen\n@@ -398,6 +402,12 @@ cmd_summary() {\n \t)\n \n \ttest -n \"$modules\" &&\n+\tif test -n \"$for_status\"; then\n+\t\techo \"# Modified submodules:\"\n+\t\techo \"#\"\n+\telse\n+\t\ttrue\n+\tfi &&\n \tgit diff-index $cached --raw $head -- $modules |\n \tgrep -e '^:160000' -e '^:[0-7]* 160000' |\n \tcut -c2- |\n@@ -499,7 +509,12 @@ cmd_summary() {\n \t\t\techo\n \t\tfi\n \t\techo\n-\tdone\n+\tdone |\n+\tif test -n \"$for_status\"; then\n+\t\tsed -e \"s|^|# |\" -e 's|^# $|#|'\n+\telse\n+\t\tcat\n+\tfi\n }\n #\n # List all submodules, prefixed with:\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 0f3c42a..1dbb39d 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -192,4 +192,17 @@ test_expect_success 'given commit' \"\n EOF\n \"\n \n+test_expect_success '--for-status' \"\n+    git submodule summary --for-status HEAD^ >actual &&\n+    diff actual - <<-EOF\n+# Modified submodules:\n+#\n+# * sm1 $head6...0000000:\n+#\n+# * sm2 0000000...$head7 (2):\n+#   > Add foo9\n+#\n+EOF\n+\"\n+\n test_done\n-- \n1.5.5.23.g2a5f\n"},{"id":"74066","messageId":"1207841727-7840-3-git-send-email-pkufranky@gmail.com","threadId":"13050","inReplyTo":"1207841727-7840-2-git-send-email-pkufranky@gmail.com","subject":"[PATCH v2 2/3] builtin-status: submodule summary support","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-04-10T15:35:26Z","receivedAt":"2008-04-10T15:35:26Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"This commit teaches 'git commit/status' show 'Modified submodules'\nsection given by\n\ngit submodule summary --cached --for-status --summary-limit <limit>\n\njust before 'Untracked files' section.\n\nThe <limit> is given by the config variable status.submodulesummary\nto limit the submodule summary size. status.submodulesummary is 0 by\ndefault which disables the summary.\n\nAlso mention status.submodulesummary in the documentation.\n\nSigned-off-by: Ping Yin <pkufranky@gmail.com>\n---\n Documentation/git-status.txt |    4 ++++\n wt-status.c                  |   39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex 3ea269a..32b6660 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -52,6 +52,10 @@ If the config variable `status.relativePaths` is set to false, then all\n paths shown are relative to the repository root, not to the current\n directory.\n \n+If 'status.submodulesummary' is set to a non zero number, the submodule\n+summary will be enabled and a summary of commits for modified submodules\n+will be shown (see --summary-limit option of linkgit:git-submodule[1]).\n+\n See Also\n --------\n linkgit:gitignore[5]\ndiff --git a/wt-status.c b/wt-status.c\nindex b3fd57b..369c519 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -8,9 +8,11 @@\n #include \"revision.h\"\n #include \"diffcore.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n int wt_status_relative_paths = 1;\n int wt_status_use_color = -1;\n+int wt_status_submodule_summary = 0;\n static char wt_status_colors[][COLOR_MAXLEN] = {\n \t\"\",         /* WT_STATUS_HEADER: normal */\n \t\"\\033[32m\", /* WT_STATUS_UPDATED: green */\n@@ -219,6 +221,38 @@ static void wt_status_print_changed(struct wt_status *s)\n \trev.diffopt.format_callback_data = s;\n \trun_diff_files(&rev, 0);\n }\n+static void wt_status_print_submodule_summary(struct wt_status *s)\n+{\n+\tstruct child_process sm_summary;\n+\tchar summary_limit[64];\n+\tchar index[PATH_MAX];\n+\tconst char *env[] = { index, NULL };\n+\tconst char *argv[] = {\n+\t\t\"submodule\",\n+\t\t\"summary\",\n+\t\t\"--cached\",\n+\t\t\"--for-status\",\n+\t\t\"--summary-limit\",\n+\t\tsummary_limit,\n+\t\ts->amend ? \"HEAD^\" : \"HEAD\",\n+\t\tNULL\n+\t};\n+\n+\tif (!wt_status_submodule_summary)\n+\t\treturn;\n+\n+\tsprintf(summary_limit, \"%d\", wt_status_submodule_summary);\n+\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", s->index_file);\n+\n+\tmemset(&sm_summary, 0, sizeof(sm_summary));\n+\tsm_summary.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+}\n \n static void wt_status_print_untracked(struct wt_status *s)\n {\n@@ -308,6 +342,7 @@ void wt_status_print(struct wt_status *s)\n \t}\n \n \twt_status_print_changed(s);\n+\twt_status_print_submodule_summary(s);\n \twt_status_print_untracked(s);\n \n \tif (s->verbose && !s->is_initial)\n@@ -330,6 +365,10 @@ void wt_status_print(struct wt_status *s)\n \n int git_status_config(const char *k, const char *v)\n {\n+\tif (!strcmp(k, \"status.submodulesummary\")) {\n+\t\twt_status_submodule_summary = atoi(v);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(k, \"status.color\") || !strcmp(k, \"color.status\")) {\n \t\twt_status_use_color = git_config_colorbool(k, v, -1);\n \t\treturn 0;\n-- \n1.5.5.23.g2a5f\n"},{"id":"74068","messageId":"1207841727-7840-4-git-send-email-pkufranky@gmail.com","threadId":"13050","inReplyTo":"1207841727-7840-3-git-send-email-pkufranky@gmail.com","subject":"[PATCH v2 3/3] buitin-status: Add tests for submodule summary","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-04-10T15:35:27Z","receivedAt":"2008-04-10T15:35:27Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"Signed-off-by: Ping Yin <pkufranky@gmail.com>\n---\n t/t7502-status.sh |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 134 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7502-status.sh b/t/t7502-status.sh\nindex cd08516..33882c9 100755\n--- a/t/t7502-status.sh\n+++ b/t/t7502-status.sh\n@@ -149,4 +149,138 @@ test_expect_success 'status of partial commit excluding new file in index' '\n \ttest_cmp expect output\n '\n \n+test_expect_success \"setup status submodule summary\" '\n+\ttest_create_repo sm &&\n+\tcd sm &&\n+\t: >foo &&\n+\tgit add foo &&\n+\tgit commit -m \"Add foo\" &&\n+\tcd .. &&\n+\tgit add sm\n+'\n+\n+cat > expect <<EOF\n+# On branch master\n+# Changes to be committed:\n+#   (use \"git reset HEAD <file>...\" to unstage)\n+#\n+#\tnew file:   dir2/added\n+#\tnew file:   sm\n+#\n+# Changed but not updated:\n+#   (use \"git add <file>...\" to update what will be committed)\n+#\n+#\tmodified:   dir1/modified\n+#\n+# Untracked files:\n+#   (use \"git add <file>...\" to include in what will be committed)\n+#\n+#\tdir1/untracked\n+#\tdir2/modified\n+#\tdir2/untracked\n+#\texpect\n+#\toutput\n+#\tuntracked\n+EOF\n+test_expect_success \"status submodule summary is disabled by default\" '\n+\tgit status > output &&\n+\tgit diff expect output\n+'\n+\n+head=$(cd sm && git rev-parse --short=7 --verify HEAD)\n+\n+cat > expect <<EOF\n+# On branch master\n+# Changes to be committed:\n+#   (use \"git reset HEAD <file>...\" to unstage)\n+#\n+#\tnew file:   dir2/added\n+#\tnew file:   sm\n+#\n+# Changed but not updated:\n+#   (use \"git add <file>...\" to update what will be committed)\n+#\n+#\tmodified:   dir1/modified\n+#\n+# Modified submodules:\n+#\n+# * sm 0000000...$head (1):\n+#   > Add foo\n+#\n+# Untracked files:\n+#   (use \"git add <file>...\" to include in what will be committed)\n+#\n+#\tdir1/untracked\n+#\tdir2/modified\n+#\tdir2/untracked\n+#\texpect\n+#\toutput\n+#\tuntracked\n+EOF\n+test_expect_success \"status submodule summary\" '\n+\tgit config status.submodulesummary 10 &&\n+\tgit status > output &&\n+\tgit diff expect output\n+'\n+\n+\n+cat > expect <<EOF\n+# On branch master\n+# Changed but not updated:\n+#   (use \"git add <file>...\" to update what will be committed)\n+#\n+#\tmodified:   dir1/modified\n+#\n+# Untracked files:\n+#   (use \"git add <file>...\" to include in what will be committed)\n+#\n+#\tdir1/untracked\n+#\tdir2/modified\n+#\tdir2/untracked\n+#\texpect\n+#\toutput\n+#\tuntracked\n+no changes added to commit (use \"git add\" and/or \"git commit -a\")\n+EOF\n+test_expect_success \"status submodule summary (clean submodule)\" '\n+\tgit commit -m \"commit submodule\" &&\n+\tgit config status.submodulesummary 10 &&\n+\t! git status > output &&\n+\tgit diff expect output\n+'\n+\n+cat > expect <<EOF\n+# On branch master\n+# Changes to be committed:\n+#   (use \"git reset HEAD^1 <file>...\" to unstage)\n+#\n+#\tnew file:   dir2/added\n+#\tnew file:   sm\n+#\n+# Changed but not updated:\n+#   (use \"git add <file>...\" to update what will be committed)\n+#\n+#\tmodified:   dir1/modified\n+#\n+# Modified submodules:\n+#\n+# * sm 0000000...$head (1):\n+#   > Add foo\n+#\n+# Untracked files:\n+#   (use \"git add <file>...\" to include in what will be committed)\n+#\n+#\tdir1/untracked\n+#\tdir2/modified\n+#\tdir2/untracked\n+#\texpect\n+#\toutput\n+#\tuntracked\n+EOF\n+test_expect_success \"status submodule summary (--amend)\" '\n+\tgit config status.submodulesummary 10 &&\n+\tgit status --amend > output &&\n+\tgit diff expect output\n+'\n+\n test_done\n-- \n1.5.5.23.g2a5f\n"},{"id":"74150","messageId":"7vtzi8owf9.fsf@gitster.siamese.dyndns.org","threadId":"13050","inReplyTo":"1207841727-7840-3-git-send-email-pkufranky@gmail.com","subject":"Re: [PATCH v2 2/3] builtin-status: submodule summary support","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-04-11T22:31:54Z","receivedAt":"2008-04-11T22:31:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ping Yin <pkufranky@gmail.com> writes:\n\n> This commit teaches 'git commit/status' show 'Modified submodules'\n> section given by\n>\n> git submodule summary --cached --for-status --summary-limit <limit>\n>\n> just before 'Untracked files' section.\n>\n> The <limit> is given by the config variable status.submodulesummary\n> to limit the submodule summary size. status.submodulesummary is 0 by\n> default which disables the summary.\n\nThis begs an obvious question \"What if I want to have an unlimited summary\nin the status output?\"\n\n> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\n> index 3ea269a..32b6660 100644\n> --- a/Documentation/git-status.txt\n> +++ b/Documentation/git-status.txt\n> @@ -52,6 +52,10 @@ If the config variable `status.relativePaths` is set to false, then all\n>  paths shown are relative to the repository root, not to the current\n>  directory.\n>  \n> +If 'status.submodulesummary' is set to a non zero number, the submodule\n> +summary will be enabled and a summary of commits for modified submodules\n> +will be shown (see --summary-limit option of linkgit:git-submodule[1]).\n> +\n\nThe docs quote configuration variables in typewriter face by using\nbackticks (pay attention to how status.relativePaths is quoted in your\nhunk header comment).\n\n> diff --git a/wt-status.c b/wt-status.c\n> index b3fd57b..369c519 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -8,9 +8,11 @@\n>  #include \"revision.h\"\n>  #include \"diffcore.h\"\n>  #include \"quote.h\"\n> +#include \"run-command.h\"\n>  \n>  int wt_status_relative_paths = 1;\n>  int wt_status_use_color = -1;\n> +int wt_status_submodule_summary = 0;\n\nDon't initialize to 0.  BSS takes care of it.\n\n>  static char wt_status_colors[][COLOR_MAXLEN] = {\n>  \t\"\",         /* WT_STATUS_HEADER: normal */\n>  \t\"\\033[32m\", /* WT_STATUS_UPDATED: green */\n> @@ -219,6 +221,38 @@ static void wt_status_print_changed(struct wt_status *s)\n>  \trev.diffopt.format_callback_data = s;\n>  \trun_diff_files(&rev, 0);\n>  }\n> +static void wt_status_print_submodule_summary(struct wt_status *s)\n> +{\n> ...\n> +\tmemset(&sm_summary, 0, sizeof(sm_summary));\n> +\tsm_summary.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\nI recall we had some clean-up on how file descriptors are inherited and\nduped around when run_command() runs a subprocess.  Hannes, is this dup()\nconsistent with how the things ought to be these days?\n\n> @@ -330,6 +365,10 @@ void wt_status_print(struct wt_status *s)\n>  \n>  int git_status_config(const char *k, const char *v)\n>  {\n> +\tif (!strcmp(k, \"status.submodulesummary\")) {\n> +\t\twt_status_submodule_summary = atoi(v);\n\nWith this code, you would feed a NULL to atoi() with:\n\n        [status]\n                submoduleSummary\n\nHow about making this a bool-or-number, and boolean true gives whatever\nthe default limit (may be unlimited) when no --summary-limit is given?\n\nWe may want to have git_config_bool_or_int() in config.c so that the\ncaller can tell if the value is boolean true or integer 1, which would be\na reasonable improvement independent from this submodule summary\ncodepath.\n\nint git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n{\n        *is_bool = 1;\n\tif (!value)\n\t\treturn 1;\n\tif (!*value)\n\t\treturn 0;\n\tif (!strcasecmp(value, \"true\") || !strcasecmp(value, \"yes\"))\n\t\treturn 1;\n\tif (!strcasecmp(value, \"false\") || !strcasecmp(value, \"no\"))\n\t\treturn 0;\n\t*is_bool = 0;\n\treturn git_config_int(name, value) != 0;\n}\n\nWith that, your configuration parser can do something like:\n\n\tif (!strcmp(k, \"status.submodulesummary\")) {\n\t\tint is_bool, val;\n\t\tval = git_config_bool_or_int(k, v, &b);\n\t\tif (is_bool || val <= 0) {\n                \twt_status_submodule_summary_enabled = val;\n\t\t} else {\n                \twt_status_submodule_summary = val;\n                \twt_status_submodule_summary_enabled = 1;\n\t\t}\n\t}\n\nand skip the call to wt_status_print_submodule_summary() when not\nenabled.\n\n>  int wt_status_relative_paths = 1;\n>  int wt_status_use_color = -1;\n> +int wt_status_submodule_summary = -1; /* unspecified */\n  +int wt_status_submodule_summary_enabled;\n\nThe call site in wt_status_print() would look like: \n\n\t...\n \twt_status_print_changed(s);\n\tif (wt_status_submodule_summary_enabled > 0)\n\t\twt_status_print_submodule_summary(s);\n \twt_status_print_untracked(s);\n\t...\n\nand the wt_status_print_submodule_summary() would not add --summary-limit\nparameter if wt_status_submodule_summary is left unspecified (i.e. -1).\n"},{"id":"74181","messageId":"46dff0320804112228i557652b2kcad8fe524a9638ff@mail.gmail.com","threadId":"13050","inReplyTo":"7vtzi8owf9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] builtin-status: submodule summary support","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-04-12T05:28:22Z","receivedAt":"2008-04-12T05:28:22Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On Sat, Apr 12, 2008 at 6:31 AM, Junio C Hamano <junio@pobox.com> wrote:\n\n>  int git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n>  {\n>         *is_bool = 1;\n>         if (!value)\n>                 return 1;\n>         if (!*value)\n>                 return 0;\n>         if (!strcasecmp(value, \"true\") || !strcasecmp(value, \"yes\"))\n>                 return 1;\n>         if (!strcasecmp(value, \"false\") || !strcasecmp(value, \"no\"))\n>                 return 0;\n>         *is_bool = 0;\n>         return git_config_int(name, value) != 0;\n>  }\n>\n>  With that, your configuration parser can do something like:\n>\n>\n>         if (!strcmp(k, \"status.submodulesummary\")) {\n>                 int is_bool, val;\n>                 val = git_config_bool_or_int(k, v, &b);\n>                 if (is_bool || val <= 0) {\n>                         wt_status_submodule_summary_enabled = val;\n>                 } else {\n>                         wt_status_submodule_summary = val;\n>                         wt_status_submodule_summary_enabled = 1;\n>                 }\n>         }\n>\n>  and skip the call to wt_status_print_submodule_summary() when not\n>  enabled.\n>\n>\n>  >  int wt_status_relative_paths = 1;\n>  >  int wt_status_use_color = -1;\n>  > +int wt_status_submodule_summary = -1; /* unspecified */\n>   +int wt_status_submodule_summary_enabled;\n>\n>  The call site in wt_status_print() would look like:\n>\n>         ...\n>\n>         wt_status_print_changed(s);\n>         if (wt_status_submodule_summary_enabled > 0)\n>\n>                 wt_status_print_submodule_summary(s);\n>         wt_status_print_untracked(s);\n>         ...\n>\n\nHow about the following?\n\ndiff --git a/config.c b/config.c\nindex 0624494..e614456 100644\n--- a/config.c\n+++ b/config.c\n@@ -316,6 +316,21 @@ int git_config_bool(const char *name, const char *value)\n \treturn git_config_int(name, value) != 0;\n }\n\n+int git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n+{\n+\t*is_bool = 1;\n+\tif (!value)\n+\t\treturn 1;\n+\tif (!*value)\n+\t\treturn 0;\n+\tif (!strcasecmp(value, \"true\") || !strcasecmp(value, \"yes\"))\n+\t\treturn 1;\n+\tif (!strcasecmp(value, \"false\") || !strcasecmp(value, \"no\"))\n+\t\treturn 0;\n+\t*is_bool = 0;\n+\treturn git_config_int(name, value);\n+}\n+\n int git_config_string(const char **dest, const char *var, const char *value)\n {\n \tif (!value)\ndiff --git a/wt-status.c b/wt-status.c\nindex 22385f5..3baa128 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -366,7 +366,10 @@ void wt_status_print(struct wt_status *s)\n int git_status_config(const char *k, const char *v)\n {\n \tif (!strcmp(k, \"status.submodulesummary\")) {\n-\t\twt_status_submodule_summary = atoi(v);\n+\t\tint is_bool;\n+\t\twt_status_submodule_summary = git_config_bool_or_int(k, v, &is_bool);\n+\t\tif (is_bool && wt_status_submodule_summary)\n+\t\t\twt_status_submodule_summary = -1;\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"status.color\") || !strcmp(k, \"color.status\")) {\n\n\n\n-- \nPing Yin\n"},{"id":"74203","messageId":"46dff0320804120747j424d459oc0987beb27fce1c6@mail.gmail.com","threadId":"13050","inReplyTo":"7vtzi8owf9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] builtin-status: submodule summary support","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-04-12T14:47:25Z","receivedAt":"2008-04-12T14:47:25Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On Sat, Apr 12, 2008 at 6:31 AM, Junio C Hamano <junio@pobox.com> wrote:\n>  > +static void wt_status_print_submodule_summary(struct wt_status *s)\n>  > +{\n>  > ...\n>\n> > +     memset(&sm_summary, 0, sizeof(sm_summary));\n>  > +     sm_summary.argv = argv;\n>  > +     sm_summary.env = env;\n>  > +     sm_summary.git_cmd = 1;\n>  > +     sm_summary.no_stdin = 1;\n>  > +     fflush(s->fp);\n>  > +     sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */\n>  > +     run_command(&sm_summary);\n>\n>  I recall we had some clean-up on how file descriptors are inherited and\n>  duped around when run_command() runs a subprocess.  Hannes, is this dup()\n>  consistent with how the things ought to be these days?\n\nI think dup is needed, as commit c20181e3a3 says, run_command still\nwill close passed in positive descripors.\n\n    start_command(), if .in/.out > 0, closes file descriptors, not the callers\n\n    Callers of start_command() can set the members .in and .out of struct\n    child_process to a value > 0 to specify that this descriptor is used as\n    the stdin or stdout of the child process.\n\n    Previously, if start_command() was successful, this descriptor was closed\n    upon return. Here we now make sure that the descriptor is also closed in\n    case of failures. All callers are updated not to close the file descriptor\n    themselves after start_command() was called.\n\n\n-- \nPing Yin\n"},{"id":"74218","messageId":"200804121813.20050.johannes.sixt@telecom.at","threadId":"13050","inReplyTo":"7vtzi8owf9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] builtin-status: submodule summary support","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-04-12T16:13:19Z","receivedAt":"2008-04-12T16:13:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Saturday 12 April 2008 00:31, Junio C Hamano wrote:\n> Ping Yin <pkufranky@gmail.com> writes:\n> > +static void wt_status_print_submodule_summary(struct wt_status *s)\n> > +{\n> > ...\n> > +\tmemset(&sm_summary, 0, sizeof(sm_summary));\n> > +\tsm_summary.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>\n> I recall we had some clean-up on how file descriptors are inherited and\n> duped around when run_command() runs a subprocess.  Hannes, is this dup()\n> consistent with how the things ought to be these days?\n\nYes, this dup() is required and correct.\n\n-- Hannes\n"}]}