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...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.
--
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

Reply via email to