Re: [PATCH v3 3/4] get rid of git submodule summary --for-status
Am 29.08.2013 21:54, schrieb Jens Lehmann: Am 29.08.2013 15:05, schrieb Matthieu Moy: The --for-status option was an undocumented option used only by wt-status.c, which inserted a header and commented out the output. We can achieve the same result within wt-status.c, without polluting the submodule command-line options. This will make it easier to disable the comments from wt-status.c later. Cool, thanks for implementing this! But unfortunately this change collides with bc/submodule-status-ignored (I added Brian to the CC) which is currently on its way to next. Your patch will break the fix in the second commit, because that's only enabled when the submodule script sees the --for-status option. A solution for that would be to rebase your patches on top of pu, drop the first two hunks of the change to git-submodule.sh and still pass the --for-status option to git-submodule.sh. This would move adding the comment characters into wt-status.c but will still enable the script to honor the ignore=all setting when called by status. I think we should go that route, --for-status is an internal option and nobody should rely on its behavior. Signed-off-by: Matthieu Moy matthieu@imag.fr --- git-submodule.sh | 17 + t/t7401-submodule-summary.sh | 13 - wt-status.c | 29 +++-- 3 files changed, 28 insertions(+), 31 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 2979197..fccdec9 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -965,7 +965,6 @@ set_name_rev () { # cmd_summary() { summary_limit=-1 -for_status= diff_cmd=diff-index # parse $args after submodule ... summary. @@ -978,9 +977,6 @@ cmd_summary() { --files) files=$1 ;; ---for-status) -for_status=$1 -;; -n|--summary-limit) summary_limit=$2 isnumber $summary_limit || usage Please drop the two hunks above ... @@ -1149,18 +1145,7 @@ cmd_summary() { echo fi echo -done | -if test -n $for_status; then -if [ -n $files ]; then -gettextln Submodules changed but not updated: | git stripspace -c -else -gettextln Submodule changes to be committed: | git stripspace -c -fi -printf \n | git stripspace -c -git stripspace -c -else -cat -fi +done } # # List all submodules, prefixed with: diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh index ac2434c..b435d03 100755 --- a/t/t7401-submodule-summary.sh +++ b/t/t7401-submodule-summary.sh @@ -262,19 +262,6 @@ EOF test_cmp expected actual -test_expect_success '--for-status' -git submodule summary --for-status HEAD^ actual -test_i18ncmp actual - EOF -# Submodule changes to be committed: -# -# * sm1 $head6...000: -# -# * sm2 000...$head7 (2): -#Add foo9 -# -EOF - - ... and just remove the # from the expected output here. This test can be removed when we use test_expect_success 'fail when using --files together with --cached' test_must_fail git submodule summary --files --cached diff --git a/wt-status.c b/wt-status.c index 958a53c..d91661d 100644 --- a/wt-status.c +++ b/wt-status.c @@ -665,6 +665,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt char index[PATH_MAX]; const char *env[] = { NULL, NULL }; struct argv_array argv = ARGV_ARRAY_INIT; +struct strbuf cmd_stdout = STRBUF_INIT; +struct strbuf summary = STRBUF_INIT; +char *summary_content; +size_t len; sprintf(summary_limit, %d, s-submodule_summary); snprintf(index, sizeof(index), GIT_INDEX_FILE=%s, s-index_file); @@ -673,7 +677,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt argv_array_push(argv, submodule); argv_array_push(argv, summary); argv_array_push(argv, uncommitted ? --files : --cached); -argv_array_push(argv, --for-status); And the line above has to stay. argv_array_push(argv, --summary-limit); argv_array_push(argv, summary_limit); if (!uncommitted) @@ -685,9 +688,31 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt sm_summary.git_cmd = 1; sm_summary.no_stdin = 1; fflush(s-fp); -sm_summary.out = dup(fileno(s-fp));/* run_command closes it */ +sm_summary.out = -1; + run_command(sm_summary); argv_array_clear(argv); + +len = strbuf_read(cmd_stdout, sm_summary.out, 1024); + +/* prepend header, only if there's an actual output */ +if (len) { +if
Re: [PATCH v3 3/4] get rid of git submodule summary --for-status
Am 31.08.2013 19:08, schrieb brian m. carlson: On Fri, Aug 30, 2013 at 10:08:53PM +0200, Jens Lehmann wrote: Am 30.08.2013 21:51, schrieb Jens Lehmann: Am 30.08.2013 21:40, schrieb Jens Lehmann: Am 29.08.2013 23:23, schrieb Matthieu Moy: Jens Lehmann jens.lehm...@web.de writes: Am 29.08.2013 15:05, schrieb Matthieu Moy: Because of the missing quotes around $for_status, it seems the test is unconditionnaly true: $ test -n t ; echo $? 0 $ test -n ; echo $? 0 Right you are, I did not notice the missing in my review. Looks like we also should add one or more tests making sure that submodule summary and status never honor the ignore settings. How do we want to handle this? I can send a reroll and include some new tests, but if this code is going away, then there's no point. A reroll would be great, as I think your patch is a bugfix that should go in rather soonish no matter how we continue with the comment signs. Two new tests (one for submodule summary and one for submodule status) with both the global ignore setting and a submodule specific one set to all showing no impact on the output would suffice (and trigger the then also fixed missing bug ;-). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] get rid of git submodule summary --for-status
On Fri, Aug 30, 2013 at 10:08:53PM +0200, Jens Lehmann wrote: Am 30.08.2013 21:51, schrieb Jens Lehmann: Am 30.08.2013 21:40, schrieb Jens Lehmann: Am 29.08.2013 23:23, schrieb Matthieu Moy: Jens Lehmann jens.lehm...@web.de writes: Am 29.08.2013 15:05, schrieb Matthieu Moy: Because of the missing quotes around $for_status, it seems the test is unconditionnaly true: $ test -n t ; echo $? 0 $ test -n ; echo $? 0 Right you are, I did not notice the missing in my review. Looks like we also should add one or more tests making sure that submodule summary and status never honor the ignore settings. How do we want to handle this? I can send a reroll and include some new tests, but if this code is going away, then there's no point. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH v3 3/4] get rid of git submodule summary --for-status
Am 30.08.2013 21:51, schrieb Jens Lehmann: Am 30.08.2013 21:40, schrieb Jens Lehmann: Am 29.08.2013 23:23, schrieb Matthieu Moy: Jens Lehmann jens.lehm...@web.de writes: Am 29.08.2013 15:05, schrieb Matthieu Moy: The --for-status option was an undocumented option used only by wt-status.c, which inserted a header and commented out the output. We can achieve the same result within wt-status.c, without polluting the submodule command-line options. This will make it easier to disable the comments from wt-status.c later. Cool, thanks for implementing this! But unfortunately this change collides with bc/submodule-status-ignored (I added Brian to the CC) which is currently on its way to next. Thanks for pointing that out. The patch looks buggy: Ok, I'll tak Sorry, I accidentally hit Send ... :-( Ok, I'll take a look and will comment on that soon. --- a/git-submodule.sh +++ b/git-submodule.sh @@ -1036,6 +1036,13 @@ cmd_summary() { do # Always show modules deleted or type-changed (blob-module) test $status = D -o $status = T echo $sm_path continue + # Respect the ignore setting for --for-status. + if test -n $for_status + then + name=$(module_name $sm_path) + ignore_config=$(get_submodule_config $name ignore none) + test $status != A -a $ignore_config = all continue + fi Because of the missing quotes around $for_status, it seems the test is unconditionnaly true: $ test -n t ; echo $? 0 $ test -n ; echo $? 0 Right you are, I did not notice the missing in my review. Looks like we also should add one or more tests making sure that submodule summary and status never honor the ignore settings. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] get rid of git submodule summary --for-status
Am 30.08.2013 21:40, schrieb Jens Lehmann: Am 29.08.2013 23:23, schrieb Matthieu Moy: Jens Lehmann jens.lehm...@web.de writes: Am 29.08.2013 15:05, schrieb Matthieu Moy: The --for-status option was an undocumented option used only by wt-status.c, which inserted a header and commented out the output. We can achieve the same result within wt-status.c, without polluting the submodule command-line options. This will make it easier to disable the comments from wt-status.c later. Cool, thanks for implementing this! But unfortunately this change collides with bc/submodule-status-ignored (I added Brian to the CC) which is currently on its way to next. Thanks for pointing that out. The patch looks buggy: Ok, I'll tak Sorry, I accidentally hit Send ... :-( Ok, I'll take a look and will comment on that soon. --- a/git-submodule.sh +++ b/git-submodule.sh @@ -1036,6 +1036,13 @@ cmd_summary() { do # Always show modules deleted or type-changed (blob-module) test $status = D -o $status = T echo $sm_path continue + # Respect the ignore setting for --for-status. + if test -n $for_status + then + name=$(module_name $sm_path) + ignore_config=$(get_submodule_config $name ignore none) + test $status != A -a $ignore_config = all continue + fi Because of the missing quotes around $for_status, it seems the test is unconditionnaly true: $ test -n t ; echo $? 0 $ test -n ; echo $? 0 This makes me wonder why the ignore configuration should be considered only with --for-status. Why not turn that into Please don't. That changes the default behavior of submodule summary, which never ignores any submodules. The ignore logic was added to core git after commands like diff and status learned to check submodules for modifications too. That was bad for people who used submodules to store many and/or huge files in a way that wouldn't slow down diff or status, as it slowed them down again. The ignore option allowed them to continue using submodules for that purpose. They still need to have the submodule script ignore the ignore settings, because running them is the point in time they want to take the extra effort to look into those submodules they normally ignore. And that's why the submodule totally lacks any option to control the ignore behavior, which we would also have to add if we would follow your proposal. So I think it's either changing the default behavior of --for-status or adding another option (--for-status-wo-comment or such) which will honor the ignore setting only when called from status. --- a/git-submodule.sh +++ b/git-submodule.sh @@ -1036,6 +1036,13 @@ cmd_summary() { do # Always show modules deleted or type-changed (blob-module) test $status = D -o $status = T echo $sm_path continue +# Respect the ignore setting +name=$(module_name $sm_path) +ignore_config=$(get_submodule_config $name ignore none) +test $status != A -a $ignore_config = all continue ? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] get rid of git submodule summary --for-status
Am 29.08.2013 23:23, schrieb Matthieu Moy: Jens Lehmann jens.lehm...@web.de writes: Am 29.08.2013 15:05, schrieb Matthieu Moy: The --for-status option was an undocumented option used only by wt-status.c, which inserted a header and commented out the output. We can achieve the same result within wt-status.c, without polluting the submodule command-line options. This will make it easier to disable the comments from wt-status.c later. Cool, thanks for implementing this! But unfortunately this change collides with bc/submodule-status-ignored (I added Brian to the CC) which is currently on its way to next. Thanks for pointing that out. The patch looks buggy: Ok, I'll tak --- a/git-submodule.sh +++ b/git-submodule.sh @@ -1036,6 +1036,13 @@ cmd_summary() { do # Always show modules deleted or type-changed (blob-module) test $status = D -o $status = T echo $sm_path continue + # Respect the ignore setting for --for-status. + if test -n $for_status + then + name=$(module_name $sm_path) + ignore_config=$(get_submodule_config $name ignore none) + test $status != A -a $ignore_config = all continue + fi Because of the missing quotes around $for_status, it seems the test is unconditionnaly true: $ test -n t ; echo $? 0 $ test -n ; echo $? 0 This makes me wonder why the ignore configuration should be considered only with --for-status. Why not turn that into --- a/git-submodule.sh +++ b/git-submodule.sh @@ -1036,6 +1036,13 @@ cmd_summary() { do # Always show modules deleted or type-changed (blob-module) test $status = D -o $status = T echo $sm_path continue + # Respect the ignore setting + name=$(module_name $sm_path) + ignore_config=$(get_submodule_config $name ignore none) + test $status != A -a $ignore_config = all continue ? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] get rid of git submodule summary --for-status
Matthieu Moy matthieu@imag.fr writes: + /* prepend header, only if there's an actual output */ + if (len) { + if (uncommitted) + strbuf_addstr(summary, _(Submodules changed but not updated:)); + else + strbuf_addstr(summary, _(Submodule changes to be committed:)); + strbuf_addstr(summary, \n\n); + } + strbuf_addbuf(summary, cmd_stdout); + strbuf_release(cmd_stdout); + + summary_content = strbuf_detach(summary, len); + strbuf_add_commented_lines(summary, summary_content, len); + free(summary_content); + + summary_content = strbuf_detach(summary, len); + fprintf(s-fp, summary_content); + free(summary_content); This fprintf() looks bogus to me. How about adding this on top? wt-status.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/wt-status.c b/wt-status.c index d91661d..1f17652 100644 --- a/wt-status.c +++ b/wt-status.c @@ -710,9 +710,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt strbuf_add_commented_lines(summary, summary_content, len); free(summary_content); - summary_content = strbuf_detach(summary, len); - fprintf(s-fp, summary_content); - free(summary_content); + fputs(summary.buf, s-fp); + strbuf_release(summary); } static void wt_status_print_other(struct wt_status *s, -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] get rid of git submodule summary --for-status
Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@imag.fr writes: +/* prepend header, only if there's an actual output */ +if (len) { +if (uncommitted) +strbuf_addstr(summary, _(Submodules changed but not updated:)); +else +strbuf_addstr(summary, _(Submodule changes to be committed:)); +strbuf_addstr(summary, \n\n); +} +strbuf_addbuf(summary, cmd_stdout); +strbuf_release(cmd_stdout); + +summary_content = strbuf_detach(summary, len); +strbuf_add_commented_lines(summary, summary_content, len); +free(summary_content); + +summary_content = strbuf_detach(summary, len); +fprintf(s-fp, summary_content); +free(summary_content); This fprintf() looks bogus to me. Oops, indeed. I forgot the %s. How about adding this on top? Your solution is better, yes. wt-status.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/wt-status.c b/wt-status.c index d91661d..1f17652 100644 --- a/wt-status.c +++ b/wt-status.c @@ -710,9 +710,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt strbuf_add_commented_lines(summary, summary_content, len); free(summary_content); - summary_content = strbuf_detach(summary, len); - fprintf(s-fp, summary_content); - free(summary_content); + fputs(summary.buf, s-fp); + strbuf_release(summary); } static void wt_status_print_other(struct wt_status *s, -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html