Fix embarrassing "git log --follow" bug

Previous thread: [TIG PATCH] Add missing = for comparison in obsolete actions check. by James Bowes on Monday, October 8, 2007 - 1:30 pm. (2 messages)

Next thread: Re: Git User's Survey 2007 unfinished summary continued by Jakub Narebski on Monday, October 8, 2007 - 4:55 pm. (157 messages)
To: Git Mailing List <git@...>
Cc: Junio C Hamano <junkio@...>
Date: Monday, October 8, 2007 - 4:42 pm

It turns out that I completely broke "git log --follow" with my recent
patch to revision.c ("Fix revision log diff setup, avoid unnecessary diff
generation", commit b7bb760d5ed4881422673d32f869d140221d3564).

Why? Because --follow obviously requires the diff machinery to function,
exactly the same way pickaxe does.

So everybody is away right now, but considering that nobody even noticed
this bug, I don't think it matters. But for the record, here's the trivial
one-liner fix (well, two, since I also fixed the comment).

Because of the nature of the bug, if you ask for patches when following
(which is one of the things I normally do), the bug is hidden, because
then the request for diff output will automatically also enable the diffs
themselves.

So while "git log --follow <filename>" didn't work, adding a "-p"
magically made it work again even without this fix.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
revision.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 5d294be..e76da0d 100644
--- a/revision.c
+++ b/revision.c
@@ -1241,8 +1241,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
revs->diff = 1;

- /* Pickaxe needs diffs */
- if (revs->diffopt.pickaxe)
+ /* Pickaxe and rename following needs diffs */
+ if (revs->diffopt.pickaxe || revs->diffopt.follow_renames)
revs->diff = 1;

if (revs->topo_order)
-

To: Git Mailing List <git@...>
Cc: Junio C Hamano <junkio@...>
Date: Monday, October 8, 2007 - 4:46 pm

This fixes an unnecessary empty line that we add to the log message when
we generate diffs, but don't actually end up printing any due to having
DIFF_FORMAT_NO_OUTPUT set.

This can happen with pickaxe or with rename following. The reason is that
we normally add an empty line between the commit and the diff, but we do
that even for the case where we've then suppressed the actual printing of
the diff.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
log-tree.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 2319154..62edd34 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -304,7 +304,8 @@ int log_tree_diff_flush(struct rev_info *opt)
* output for readability.
*/
show_log(opt, opt->diffopt.msg_sep);
- if (opt->verbose_header &&
+ if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
+ opt->verbose_header &&
opt->commit_format != CMIT_FMT_ONELINE) {
int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
if ((pch & opt->diffopt.output_format) == pch)
-

To: Git Mailing List <git@...>
Cc: Junio C Hamano <junkio@...>
Date: Monday, October 8, 2007 - 4:55 pm

Btw, this seems to break a few tests, where the test assumed the extra
newlines at the end of commands like "git show -s" (which also triggered
the extra unnecessary padding between commit text and the suppressed
diff).

I think the patch is correct, but the tests obviously would need fixing
before this patch should be applied.

Linus
-

To: Git Mailing List <git@...>
Cc: Junio C Hamano <junkio@...>
Date: Tuesday, October 9, 2007 - 12:35 pm

Clean up "git log" format with DIFF_FORMAT_NO_OUTPUT

This fixes an unnecessary empty line that we add to the log message when
we generate diffs, but don't actually end up printing any due to having
DIFF_FORMAT_NO_OUTPUT set.

This can happen with pickaxe or with rename following. The reason is that
we normally add an empty line between the commit and the diff, but we do
that even for the case where we've then suppressed the actual printing of
the diff.

This also updates a couple of tests that assumed the extraneous empty
line would exist at the end of output.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This is a resend of the original patch, with just the changes to the tests
added to make it pass them all. Both of the tests actually got cleaner, I
think (ie one had the sed script blindly just delete the last line in
order to match it up with the known input, for example).

Linus

log-tree.c | 3 ++-
t/t3900-i18n-commit.sh | 2 +-
t/t4013/diff.log_-SF_master | 1 -
3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 2319154..62edd34 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -304,7 +304,8 @@ int log_tree_diff_flush(struct rev_info *opt)
* output for readability.
*/
show_log(opt, opt->diffopt.msg_sep);
- if (opt->verbose_header &&
+ if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
+ opt->verbose_header &&
opt->commit_format != CMIT_FMT_ONELINE) {
int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
if ((pch & opt->diffopt.output_format) == pch)
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index fcbabe8..94b1c24 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -8,7 +8,7 @@ test_description='commit and log output encodings'
. ./test-lib.sh

compare_with () {
- git show -s $1 | sed -e '1,/^$/d' -e 's/^ //' -e '$d' >cu...

Previous thread: [TIG PATCH] Add missing = for comparison in obsolete actions check. by James Bowes on Monday, October 8, 2007 - 1:30 pm. (2 messages)

Next thread: Re: Git User's Survey 2007 unfinished summary continued by Jakub Narebski on Monday, October 8, 2007 - 4:55 pm. (157 messages)