* Re: [BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit
2014-05-11 23:25 [BUGFIX/RFC] git-show: fix 'git show -s' to not add extra terminator after merge commit Max Kirillov
@ 2014-05-12 16:59 ` Junio C Hamano
2014-05-12 23:26 ` Max Kirillov
0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2014-05-12 16:59 UTC (permalink / raw)
To: Max Kirillov; +Cc: git
Max Kirillov <max@max630.net> writes:
> When "git show -s" is called for merge commit it prints extra newline
> between any merge commit and the next one. This looks especially ugly for
> --oneline and other single-line formats. Looks very much like a bug.
Yeah, it is not limited to one-line formats. For example:
$ git show -s v2.0.0-rc3^0 v2.0.0-rc3^1
ends with an extra blank line before you see your prompt, but if you
swap the revs:
$ git show -s v2.0.0-rc3^1 v2.0.0-rc3^0
you do not get the extra blank line. Instead, you have an extra
blank in between. And I agree that these extra blank lines look
very much like a bug.
> This actually has broken some number of existing tests
> (their fix not included), and might break some existing
> scripts which already expect this extra newline.
>
> So, is it yet not too late to fix this?
I would prefer to see it fixed eventually.
I haven't checked if the new extra check uses the right condition
yet, but from a cursory look and with a few examples I tried
manually, the version with your change seems to behave a lot more
sensibly.
A good way to double-check may be to see the fixes to the tests to
correct their wrong expectations, and if the updated expectation is
sensible. That way, we will find if the observations we made (your
problem description and my two examples above, and a few examples I
tried manually) covered all what matters, or if there are cases
where they indeed were expecting some sensible results which this
change would break that we missed. Offhand, I doubt we would find
any case where these extra blank lines are sensible expectations,
though.
> combine-diff.c | 2 +-
> t/t7007-show.sh | 8 ++++++--
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 3b92c448..d2924de 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1331,7 +1331,7 @@ void diff_tree_combined(const unsigned char *sha1,
> if (show_log_first && i == 0) {
> show_log(rev);
>
> - if (rev->verbose_header && opt->output_format)
> + if (rev->verbose_header && opt->output_format && opt->output_format != DIFF_FORMAT_NO_OUTPUT)
Please wrap this long line, perhaps like:
if (rev->verbose_header && opt->output_format &&
opt->output_format != DIFF_FORMAT_NO_OUTPUT)
> printf("%s%c", diff_line_prefix(opt),
> opt->line_termination);
> }
> diff --git a/t/t7007-show.sh b/t/t7007-show.sh
> index e41fa00..d76c7db 100755
> --- a/t/t7007-show.sh
> +++ b/t/t7007-show.sh
> @@ -25,6 +25,7 @@ test_expect_success 'set up a bit of history' '
> git checkout -b side HEAD^^ &&
> test_commit side2 &&
> test_commit side3
> + test_merge merge main3
> '
>
> test_expect_success 'showing two commits' '
> @@ -109,8 +110,11 @@ test_expect_success 'showing range' '
> '
>
> test_expect_success '-s suppresses diff' '
> - echo main3 >expect &&
> - git show -s --format=%s main3 >actual &&
> + cat >expect <<-EOF &&
> + merge
> + main3
> + EOF
Please make it a habit to quote the EOF marker when you are not
placing things that require variable substitutions, to tell the
readers that they can coast their eyes over the here-document
without having to worry about them. For a short here-document
like this, it does not matter, but these things have a habit of
getting imitated without thinking by others, and it is better to be
defensive and consistent. I.e.
cat >expect <<-\EOF &&
merge
main3
EOF
> + git show -s --format=%s merge main3 >actual &&
> test_cmp expect actual
> '
^ permalink raw reply [flat|nested] 3+ messages in thread