From: Tay Ray Chuan <rctay89@gmail.com>
Date: Fri, 04 Jun 2010 08:34:28 GMT
Subject: Re: [PATCH v3 3/3] commit::print_summary(): set rev_info.always_show_header to 1
Message-ID: <AANLkTilv7gR6ewVpvwS0PPaNAj4FC446PYZAX2RKLT8O@mail.gmail.com>
In-Reply-To: <1275639660-5344-1-git-send-email-rctay89@gmail.com>

Hi Will,

sorry, I forgot to Cc you for this new version.

-- 
Cheers,
Ray Chuan

On Fri, Jun 4, 2010 at 4:21 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> This attempts to fix a regression in git-commit, where non-abbreviated
> SHA-1s were printed in the summary.
>
> One possible fix would be to set ctx.abbrev to DEFAULT_ABBREV in the
> `if` block.
>
> However, we remove this codeblock altogether, and set
> rev.always_show_header. This way, we use back the same show_log()
> mechanism (instead of format_commit_message()).
>
> Quoting log-tree.c:560:
>
>        shown = log_tree_diff(opt, commit, &log);
>        if (!shown && opt->loginfo && opt->always_show_header) {
>                log.parent = NULL;
>                show_log(opt);
>                shown = 1;
>        }
>
> This is the only area that always_show_header is checked, so the
> setting of this flag should only affect this area.
>
> Also, we now die() if log_tree_commit() returns false. Based on the
> existing underlying codepaths (log_tree_commit(), log_tree_diff(),
> log_tree_diff_flush(), to name a few), this should not occur; changes to
> these codepaths may warrant a revision of our handling of this
> situation. Tests #2 and #3 in t7502 should aid in detecting such
> breakages.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>
> This is a reworked version of the third patch of the
> 'tc/commit-abbrev-fix' series; there are no changes to the first and
> second patches.
>
> Changes from v2:
>  - shift around 2nd and 3rd paras.
>  - mention the die() and our assumption that it won't be triggered, as
>   suggested by Junio.
>
>  builtin/commit.c  |   13 ++++---------
>  t/t7502-commit.sh |    4 ++--
>  2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a4e4966..2884d0c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1148,7 +1148,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>        rev.verbose_header = 1;
>        rev.show_root_diff = 1;
>        get_commit_format(format.buf, &rev);
> -       rev.always_show_header = 0;
> +       rev.always_show_header = 1;
>        rev.diffopt.detect_rename = 1;
>        rev.diffopt.rename_limit = 100;
>        rev.diffopt.break_opt = 0;
> @@ -1162,14 +1162,9 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>                                head,
>                initial_commit ? " (root-commit)" : "");
>
> -       if (!log_tree_commit(&rev, commit)) {
> -               struct pretty_print_context ctx = {0};
> -               struct strbuf buf = STRBUF_INIT;
> -               ctx.date_mode = DATE_NORMAL;
> -               format_commit_message(commit, format.buf + 7, &buf, &ctx);
> -               printf("%s\n", buf.buf);
> -               strbuf_release(&buf);
> -       }
> +       if (!log_tree_commit(&rev, commit))
> +               die("unable to print summary");
> +
>        strbuf_release(&format);
>  }
>
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index b10541d..08c0247 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
>        check_summary_oneline "" "a change"
>  '
>
> -test_expect_failure 'output summary format for commit with an empty diff' '
> +test_expect_success 'output summary format for commit with an empty diff' '
>
>        check_summary_oneline "" "empty" "--allow-empty"
>  '
>
> -test_expect_failure 'output summary format for merges' '
> +test_expect_success 'output summary format for merges' '
>
>        git checkout -b recursive-base &&
>        test_commit base file1 &&
> --
> 1.7.1.189.g07419
>
>