# [PATCH v2 2/4] builtin-status: submodule summary support 15 messages from 2008-03-13 to 2008-03-14. Participants: Ping Yin, Johannes Sixt, Junio C Hamano. Thread: https://gitlist.dev/t/12668 ## Ping Yin, 2008-03-13 13:48 Subject: builtin-status submodule summary Message-ID: <1205416085-23431-1-git-send-email-pkufranky@gmail.com> URL: https://gitlist.dev/e/1205416085-23431-1-git-send-email-pkufranky%40gmail.com ``` This patch series builds on top of next. It teaches builtin-status/commit summary for submodules. It contains git-submodule summary: --for-status option builtin-status: submodule summary support builtin-status: configurable submodule summary size buitin-status: Add tests for submodule summary with diffstat git-submodule.sh | 17 +++++- t/t7401-submodule-summary.sh | 13 ++++ t/t7502-status.sh | 136 ++++++++++++++++++++++++++++++++++++++++++ wt-status.c | 38 ++++++++++++ 4 files changed, 203 insertions(+), 1 deletions(-) ``` ## Ping Yin, 2008-03-13 13:48 Subject: [PATCH v2 1/4] git-submodule summary: --for-status option Message-ID: <1205416085-23431-2-git-send-email-pkufranky@gmail.com> URL: https://gitlist.dev/e/1205416085-23431-2-git-send-email-pkufranky%40gmail.com In-Reply-To: <1205416085-23431-1-git-send-email-pkufranky@gmail.com> ``` The --for-status option is mainly used by builtin-status/commit. It adds 'Modified submodules:' line at top and '# ' prefix to all following lines. Signed-off-by: Ping Yin --- git-submodule.sh | 17 ++++++++++++++++- t/t7401-submodule-summary.sh | 13 +++++++++++++ 2 files changed, 29 insertions(+), 1 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 5f1d5ef..4baafba 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -342,6 +342,7 @@ set_name_rev () { # cmd_summary() { summary_limit=-1 + for_status= # parse $args after "submodule ... summary". while test $# -ne 0 @@ -350,6 +351,9 @@ cmd_summary() { --cached) cached="$1" ;; + --for-status) + for_status="$1" + ;; -n|--summary-limit) if summary_limit=$(($2 + 0)) 2>/dev/null && test "$summary_limit" = "$2" then @@ -398,6 +402,12 @@ cmd_summary() { ) test -n "$modules" && + if test -n "$for_status"; then + echo "# Modified submodules:" + echo "#" + else + true + fi && git diff-index $cached --raw $head -- $modules | grep -e '^:160000' -e '^:[0-7]* 160000' | cut -c2- | @@ -499,7 +509,12 @@ cmd_summary() { echo fi echo - done + done | + if test -n "$for_status"; then + sed -e "s|^|# |" -e 's|^# $|#|' + else + cat + fi } # # List all submodules, prefixed with: diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh index 0f3c42a..1dbb39d 100755 --- a/t/t7401-submodule-summary.sh +++ b/t/t7401-submodule-summary.sh @@ -192,4 +192,17 @@ test_expect_success 'given commit' " EOF " +test_expect_success '--for-status' " + git submodule summary --for-status HEAD^ >actual && + diff actual - <<-EOF +# Modified submodules: +# +# * sm1 $head6...0000000: +# +# * sm2 0000000...$head7 (2): +# > Add foo9 +# +EOF +" + test_done -- 1.5.4.3.347.g5314c ``` ## Ping Yin, 2008-03-13 13:48 Subject: [PATCH v2 2/4] builtin-status: submodule summary support Message-ID: <1205416085-23431-3-git-send-email-pkufranky@gmail.com> URL: https://gitlist.dev/e/1205416085-23431-3-git-send-email-pkufranky%40gmail.com In-Reply-To: <1205416085-23431-2-git-send-email-pkufranky@gmail.com> ``` This commit teaches 'git commit/status' show 'Modified submodules' section given by 'git submodule summary --cached --for-status' just before 'Untracked files' section. Signed-off-by: Ping Yin --- wt-status.c | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-) diff --git a/wt-status.c b/wt-status.c index 701d13d..75ee7ba 100644 --- a/wt-status.c +++ b/wt-status.c @@ -8,6 +8,7 @@ #include "revision.h" #include "diffcore.h" #include "quote.h" +#include "run-command.h" int wt_status_relative_paths = 1; int wt_status_use_color = -1; @@ -219,6 +220,30 @@ static void wt_status_print_changed(struct wt_status *s) rev.diffopt.format_callback_data = s; run_diff_files(&rev, 0); } +static void wt_status_print_submodule_summary(struct wt_status *s) +{ + struct child_process sm_summary; + const char *argv[] = { + "submodule", + "summary", + "--cached", + "--for-status", + s->amend ? "HEAD^" : "HEAD", + NULL + }; + char index[PATH_MAX]; + const char *env[2] = { index, NULL }; + snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file); + + memset(&sm_summary, 0, sizeof(sm_summary)); + sm_summary.argv = argv; + sm_summary.env = env; + sm_summary.git_cmd = 1; + sm_summary.no_stdin = 1; + fflush(s->fp); + sm_summary.out = dup(fileno(s->fp)); /* run_command closes it */ + run_command(&sm_summary); +} static void wt_status_print_untracked(struct wt_status *s) { @@ -321,6 +346,9 @@ void wt_status_print(struct wt_status *s) } wt_status_print_changed(s); + // must flush s->fp since following call will write to s->fp in a child process + fflush(s->fp); + wt_status_print_submodule_summary(s); wt_status_print_untracked(s); if (s->verbose && !s->is_initial) -- 1.5.4.3.347.g5314c ``` ## Ping Yin, 2008-03-13 13:48 Subject: [PATCH v2 3/4] builtin-status: configurable submodule summary size Message-ID: <1205416085-23431-4-git-send-email-pkufranky@gmail.com> URL: https://gitlist.dev/e/1205416085-23431-4-git-send-email-pkufranky%40gmail.com In-Reply-To: <1205416085-23431-3-git-send-email-pkufranky@gmail.com> ``` Add config variable status.submodulesummary which is passed as arg for '--summary-limit' of 'git submodule summary' to limit the submodule summary size. status.submodulesummary is 0 by default which disables the summary. Signed-off-by: Ping Yin --- wt-status.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/wt-status.c b/wt-status.c index 75ee7ba..2f47e36 100644 --- a/wt-status.c +++ b/wt-status.c @@ -12,6 +12,7 @@ int wt_status_relative_paths = 1; int wt_status_use_color = -1; +int wt_status_submodule_summary = 0; static char wt_status_colors[][COLOR_MAXLEN] = { "", /* WT_STATUS_HEADER: normal */ "\033[32m", /* WT_STATUS_UPDATED: green */ @@ -222,12 +223,17 @@ static void wt_status_print_changed(struct wt_status *s) } static void wt_status_print_submodule_summary(struct wt_status *s) { + if (wt_status_submodule_summary == 0) return; struct child_process sm_summary; + char summary_limit[64]; + sprintf(summary_limit, "%d", wt_status_submodule_summary); const char *argv[] = { "submodule", "summary", "--cached", "--for-status", + "--summary-limit", + summary_limit, s->amend ? "HEAD^" : "HEAD", NULL }; @@ -371,6 +377,10 @@ void wt_status_print(struct wt_status *s) int git_status_config(const char *k, const char *v) { + if (!strcmp(k, "status.submodulesummary")) { + wt_status_submodule_summary = atoi(v); + return 0; + } if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) { wt_status_use_color = git_config_colorbool(k, v, -1); return 0; -- 1.5.4.3.347.g5314c ``` ## Ping Yin, 2008-03-13 13:48 Subject: [PATCH v2 4/4] buitin-status: Add tests for submodule summary Message-ID: <1205416085-23431-5-git-send-email-pkufranky@gmail.com> URL: https://gitlist.dev/e/1205416085-23431-5-git-send-email-pkufranky%40gmail.com In-Reply-To: <1205416085-23431-4-git-send-email-pkufranky@gmail.com> ``` Signed-off-by: Ping Yin --- t/t7502-status.sh | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 136 insertions(+), 0 deletions(-) diff --git a/t/t7502-status.sh b/t/t7502-status.sh index e006074..7a9a08f 100755 --- a/t/t7502-status.sh +++ b/t/t7502-status.sh @@ -149,4 +149,140 @@ test_expect_success 'status of partial commit excluding new file in index' ' diff -u expect output ' +test_expect_success "setup status submodule summary" ' + test_create_repo sm && + cd sm && + : >foo && + git add foo && + git commit -m "Add foo" && + cd .. && + git add sm +' + +cat > expect <..." to unstage) +# +# new file: dir2/added +# new file: sm +# +# Changed but not updated: +# (use "git add ..." to update what will be committed) +# +# modified: dir1/modified +# +# Untracked files: +# (use "git add ..." to include in what will be committed) +# +# dir1/untracked +# dir2/modified +# dir2/untracked +# expect +# output +# untracked +EOF +test_expect_success "status submodule summary is disabled by default" ' + git status > output && + git diff expect output +' + +cd sm && +head=$(git rev-parse --verify HEAD | cut -c1-7) && +cd .. + +cat > expect <..." to unstage) +# +# new file: dir2/added +# new file: sm +# +# Changed but not updated: +# (use "git add ..." to update what will be committed) +# +# modified: dir1/modified +# +# Modified submodules: +# +# * sm 0000000...$head (1): +# > Add foo +# +# Untracked files: +# (use "git add ..." to include in what will be committed) +# +# dir1/untracked +# dir2/modified +# dir2/untracked +# expect +# output +# untracked +EOF +test_expect_success "status submodule summary" ' + git config status.submodulesummary 10 && + git status > output && + git diff expect output +' + + +cat > expect <..." to update what will be committed) +# +# modified: dir1/modified +# +# Untracked files: +# (use "git add ..." to include in what will be committed) +# +# dir1/untracked +# dir2/modified +# dir2/untracked +# expect +# output +# untracked +no changes added to commit (use "git add" and/or "git commit -a") +EOF +test_expect_success "status submodule summary (clean submodule)" ' + git commit -m "commit submodule" && + git config status.submodulesummary 10 && + ! git status > output && + git diff expect output +' + +cat > expect <..." to unstage) +# +# new file: dir2/added +# new file: sm +# +# Changed but not updated: +# (use "git add ..." to update what will be committed) +# +# modified: dir1/modified +# +# Modified submodules: +# +# * sm 0000000...$head (1): +# > Add foo +# +# Untracked files: +# (use "git add ..." to include in what will be committed) +# +# dir1/untracked +# dir2/modified +# dir2/untracked +# expect +# output +# untracked +EOF +test_expect_success "status submodule summary (--amend)" ' + git config status.submodulesummary 10 && + git status --amend > output && + git diff expect output +' + test_done -- 1.5.4.3.347.g5314c ``` ## Johannes Sixt, 2008-03-13 14:11 Subject: Re: [PATCH v2 2/4] builtin-status: submodule summary support Message-ID: <47D935FE.8010107@viscovery.net> URL: https://gitlist.dev/e/47D935FE.8010107%40viscovery.net In-Reply-To: <1205416085-23431-3-git-send-email-pkufranky@gmail.com> ``` Ping Yin schrieb: > + memset(&sm_summary, 0, sizeof(sm_summary)); > + sm_summary.argv = argv; > + sm_summary.env = env; > + sm_summary.git_cmd = 1; > + sm_summary.no_stdin = 1; > + fflush(s->fp); > + sm_summary.out = dup(fileno(s->fp)); /* run_command closes it */ The fflush() at this point makes a lot of sense, and doesn't even need a comment (IMHO). > wt_status_print_changed(s); > + // must flush s->fp since following call will write to s->fp in a child process > + fflush(s->fp); > + wt_status_print_submodule_summary(s); > wt_status_print_untracked(s); But then we don't need the fflush() here. Right? -- Hannes ``` ## Johannes Sixt, 2008-03-13 14:15 Subject: Re: [PATCH v2 3/4] builtin-status: configurable submodule summary size Message-ID: <47D936E6.10107@viscovery.net> URL: https://gitlist.dev/e/47D936E6.10107%40viscovery.net In-Reply-To: <1205416085-23431-4-git-send-email-pkufranky@gmail.com> ``` Ping Yin schrieb: > + if (wt_status_submodule_summary == 0) return; Style: if (!wt_status_submodule_summary) return; > struct child_process sm_summary; > + char summary_limit[64]; > + sprintf(summary_limit, "%d", wt_status_submodule_summary); > const char *argv[] = { Declaration after statement. That's not portable. -- Hannes ``` ## Johannes Sixt, 2008-03-13 14:21 Subject: Re: [PATCH v2 4/4] buitin-status: Add tests for submodule summary Message-ID: <47D93875.1050407@viscovery.net> URL: https://gitlist.dev/e/47D93875.1050407%40viscovery.net In-Reply-To: <1205416085-23431-5-git-send-email-pkufranky@gmail.com> ``` Ping Yin schrieb: > +cd sm && > +head=$(git rev-parse --verify HEAD | cut -c1-7) && > +cd .. I think you can make these three lines into: test_expect_success 'get short SHA1 of submodule HEAD' ' head=$(cd sm && git rev-parse --verify HEAD | cut -c1-7) ' (not tested, though). > +test_expect_success "status submodule summary (--amend)" ' Thanks for adding this one. -- Hannes ``` ## Ping Yin, 2008-03-13 18:01 Subject: Re: [PATCH v2 2/4] builtin-status: submodule summary support Message-ID: <46dff0320803131101g62c16db2wf999c851672a8e0@mail.gmail.com> URL: https://gitlist.dev/e/46dff0320803131101g62c16db2wf999c851672a8e0%40mail.gmail.com In-Reply-To: <47D935FE.8010107@viscovery.net> ``` On Thu, Mar 13, 2008 at 10:11 PM, Johannes Sixt wrote: > Ping Yin schrieb: > > > wt_status_print_changed(s); > > + // must flush s->fp since following call will write to s->fp in a child process > > + fflush(s->fp); > > + wt_status_print_submodule_summary(s); > > wt_status_print_untracked(s); > > But then we don't need the fflush() here. Right? > You are right. diff --git a/wt-status.c b/wt-status.c index 2f47e36..468c14c 100644 --- a/wt-status.c +++ b/wt-status.c @@ -352,8 +352,6 @@ void wt_status_print(struct wt_status *s) } wt_status_print_changed(s); - // must flush s->fp since following call will write to s->fp in a child process - fflush(s->fp); wt_status_print_submodule_summary(s); wt_status_print_untracked(s) -- Ping Yin ``` ## Junio C Hamano, 2008-03-13 18:04 Subject: Re: [PATCH v2 4/4] buitin-status: Add tests for submodule summary Message-ID: <7vprtyxzzd.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vprtyxzzd.fsf%40gitster.siamese.dyndns.org In-Reply-To: <47D93875.1050407@viscovery.net> ``` Johannes Sixt writes: > Ping Yin schrieb: >> +cd sm && >> +head=$(git rev-parse --verify HEAD | cut -c1-7) && >> +cd .. > > I think you can make these three lines into: > > test_expect_success 'get short SHA1 of submodule HEAD' ' > > head=$(cd sm && git rev-parse --verify HEAD | cut -c1-7) > ' "git rev-parse --short=12 --verify HEAD" or whatever minimum length you would want would free you from an ugly "pipe to cut". ``` ## Ping Yin, 2008-03-13 18:09 Subject: Re: [PATCH v2 4/4] buitin-status: Add tests for submodule summary Message-ID: <46dff0320803131109j165cfde9v85ea9b04a05819ef@mail.gmail.com> URL: https://gitlist.dev/e/46dff0320803131109j165cfde9v85ea9b04a05819ef%40mail.gmail.com In-Reply-To: <47D93875.1050407@viscovery.net> ``` On Thu, Mar 13, 2008 at 10:21 PM, Johannes Sixt wrote: > Ping Yin schrieb: > > > +cd sm && > > +head=$(git rev-parse --verify HEAD | cut -c1-7) && > > +cd .. > > I think you can make these three lines into: > > test_expect_success 'get short SHA1 of submodule HEAD' ' > > head=$(cd sm && git rev-parse --verify HEAD | cut -c1-7) > ' > > (not tested, though). Hmm, It doesn't work. Since i need $head1 outputed to file expect as follows +cat > expect <..." to unstage) +# +# new file: dir2/added +# new file: sm +# +# Changed but not updated: +# (use "git add ..." to update what will be committed) +# +# modified: dir1/modified +# +# Modified submodules: +# +# * sm 0000000...$head (1): Here we need $head. -- Ping Yin ``` ## Ping Yin, 2008-03-13 18:14 Subject: Re: [PATCH v2 4/4] buitin-status: Add tests for submodule summary Message-ID: <46dff0320803131114v3218cc5etdd729b53d3a0d44d@mail.gmail.com> URL: https://gitlist.dev/e/46dff0320803131114v3218cc5etdd729b53d3a0d44d%40mail.gmail.com In-Reply-To: <7vprtyxzzd.fsf@gitster.siamese.dyndns.org> ``` On Fri, Mar 14, 2008 at 2:04 AM, Junio C Hamano wrote: > Johannes Sixt writes: > > > Ping Yin schrieb: > >> +cd sm && > >> +head=$(git rev-parse --verify HEAD | cut -c1-7) && > >> +cd .. > > > > I think you can make these three lines into: > > > > test_expect_success 'get short SHA1 of submodule HEAD' ' > > > > head=$(cd sm && git rev-parse --verify HEAD | cut -c1-7) > > ' > > "git rev-parse --short=12 --verify HEAD" or whatever minimum length you > would want would free you from an ugly "pipe to cut". > Thanks. diff --git a/t/t7502-status.sh b/t/t7502-status.sh index 7a9a08f..80e2a7b 100755 --- a/t/t7502-status.sh +++ b/t/t7502-status.sh @@ -187,9 +187,7 @@ test_expect_success git diff expect output ' -cd sm && -head=$(git rev-parse --verify HEAD | cut -c1-7) && -cd .. +head=$(cd sm && git rev-parse --short=7 --verify HEAD) cat > expect < URL: https://gitlist.dev/e/46dff0320803140730jeef5808r7da67a103ce7b171%40mail.gmail.com In-Reply-To: <1205416085-23431-1-git-send-email-pkufranky@gmail.com> ``` On Thu, Mar 13, 2008 at 9:48 PM, Ping Yin wrote: > This patch series builds on top of next. It teaches builtin-status/commit > summary for submodules. > > It contains > git-submodule summary: --for-status option > builtin-status: submodule summary support > builtin-status: configurable submodule summary size > buitin-status: Add tests for submodule summary > > with diffstat > > git-submodule.sh | 17 +++++- > t/t7401-submodule-summary.sh | 13 ++++ > t/t7502-status.sh | 136 ++++++++++++++++++++++++++++++++++++++++++ > wt-status.c | 38 ++++++++++++ > 4 files changed, 203 insertions(+), 1 deletions(-) > IMO, git submodule summary is not so useful for me if it's not integrated into git-status. In fact i never use "git submodule summary" directly. git-status with submodule summary support is very useful to help user figure out what is going on in a more global level when cooking the commit message. So i think this series should go along with the submodule summary series. -- Ping Yin ``` ## Junio C Hamano, 2008-03-14 16:39 Subject: Re: builtin-status submodule summary Message-ID: <7vprtxp8ew.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vprtxp8ew.fsf%40gitster.siamese.dyndns.org In-Reply-To: <46dff0320803140730jeef5808r7da67a103ce7b171@mail.gmail.com> ``` "Ping Yin" writes: > So i think this series should go along with the submodule summary series. Perhaps eventually, but definitely not this round, I am afraid. The usefulness of the output in the current implementation of the "summary" itself is not even proven at this point. Nobody other than you has jumped up-and-down and said "submodule summary is great and should be in status, I did not know what I was missing!" yet. I hope we verified the code well enough to make sure that people who use "git submodule" command but do not type "git submodule summary" would not get hurt by the addition. For a new feature, that is more important than how well the new feature works and how useful the new feature is. And by shipping a release with it will give it a wider exposure and hopefully a chance for it to mature to get more useful. The series just got into a "harmless to others and is releasable" shape. Recall how many rounds "submodule summary" took to get into that state, and how much time and effort were spent on it. Use it as a yardstick to guesstimate how much further effort and time will be needed to get the changes to git-status into a reasonable shape. It may happen eventually, but not before 1.5.5. ``` ## Ping Yin, 2008-03-14 17:45 Subject: Re: builtin-status submodule summary Message-ID: <46dff0320803141045w5cf1506fw9ab676f041a4bd12@mail.gmail.com> URL: https://gitlist.dev/e/46dff0320803141045w5cf1506fw9ab676f041a4bd12%40mail.gmail.com In-Reply-To: <7vprtxp8ew.fsf@gitster.siamese.dyndns.org> ``` On Sat, Mar 15, 2008 at 12:39 AM, Junio C Hamano wrote: > "Ping Yin" writes: > > > So i think this series should go along with the submodule summary series. > > > Recall how many rounds "submodule summary" took to get into that state, > and how much time and effort were spent on it. Use it as a yardstick to > guesstimate how much further effort and time will be needed to get the > changes to git-status into a reasonable shape. > > It may happen eventually, but not before 1.5.5. > So i'll resend my patches considering suggestions from Johannes Sixt after 1.5.5. -- Ping Yin ```