From: Jens Lehmann <Jens.Lehmann@web.de>
To: Jens Lehmann <Jens.Lehmann@web.de>
Cc: Matthieu Moy <Matthieu.Moy@imag.fr>,
git@vger.kernel.org, gitster@pobox.com,
"brian m. carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH v3 3/4] get rid of "git submodule summary --for-status"
Date: Tue, 03 Sep 2013 21:32:23 +0200 [thread overview]
Message-ID: <52263947.6040400@web.de> (raw)
In-Reply-To: <521FA6ED.9010005@web.de>
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.Moy@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...0000000:
>> -#
>> -# * sm2 0000000...$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 (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);
>> }
>>
>> static void wt_status_print_other(struct wt_status *s,
Junio already commented on this part.
next prev parent reply other threads:[~2013-09-03 19:32 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-29 13:05 [RFC/PATCH v3 0/4] Disable "git status" comment prefix Matthieu Moy
2013-08-29 13:05 ` [PATCH v3 1/4] builtin/stripspace.c: fix broken indentation Matthieu Moy
2013-08-29 13:05 ` [PATCH v3 2/4] wt-status: use argv_array API Matthieu Moy
2013-08-29 13:05 ` [PATCH v3 3/4] get rid of "git submodule summary --for-status" Matthieu Moy
2013-08-29 19:54 ` Jens Lehmann
2013-08-29 21:23 ` Matthieu Moy
2013-08-30 19:40 ` Jens Lehmann
2013-08-30 19:51 ` Jens Lehmann
2013-08-30 20:08 ` Jens Lehmann
2013-08-31 17:08 ` brian m. carlson
2013-09-01 13:47 ` Jens Lehmann
2013-09-03 19:32 ` Jens Lehmann [this message]
2013-08-29 19:56 ` Junio C Hamano
2013-08-29 21:05 ` Matthieu Moy
2013-08-29 13:05 ` [PATCH v3 4/4] status: introduce status.displayCommentChar to disable display of # Matthieu Moy
2013-08-29 16:19 ` [RFC/PATCH v3 0/4] Disable "git status" comment prefix Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52263947.6040400@web.de \
--to=jens.lehmann@web.de \
--cc=Matthieu.Moy@imag.fr \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).