git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: git@vger.kernel.org
Cc: "Eric Sunshine" <sunshine@sunshineco.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2] range-diff: always pass at least minimal diff options
Date: Mon,  3 Dec 2018 21:07:34 +0100	[thread overview]
Message-ID: <20181203200734.527341-1-martin.agren@gmail.com> (raw)
In-Reply-To: <CAN0heSrfH39-37KDU3XDhxiYs1_3eUMdjbdAm37cPAmnOYUZMA@mail.gmail.com>

Commit d8981c3f88 ("format-patch: do not let its diff-options affect
--range-diff", 2018-11-30) taught `show_range_diff()` to accept a
NULL-pointer as an indication that it should use its own "reasonable
default". That fixed a regression from a5170794 ("Merge branch
'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced
a regression of its own.

In particular, it means we forget the `file` member of the diff options,
so rather than placing a range-diff in the cover-letter, we write it to
stdout. In order to fix this, rewrite the two callers adjusted by
d8981c3f88 to instead create a "dummy" set of diff options where they
only fill in which file to use.

Plus, turn off coloring to make sure we don't write any color codes.
Maybe we could do `opts.use_color = opts.file != stdout`, but for now,
I'd much rather always write uncolored output than write color codes
where there shouldn't be any.

Modify and extend the existing tests to try and verify that the right
contents end up in the right place.

Don't revert `show_range_diff()`, i.e., let it keep accepting NULL.
Rather than removing what is dead code and figuring out it isn't
actually dead and we've broken 2.20, just leave it for now.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Here's another attempt at fixing this recent regression.

 t/t3206-range-diff.sh | 20 +++++++++++++-------
 builtin/log.c         | 13 ++++++++++++-
 log-tree.c            | 13 ++++++++++++-
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..048feaf6dd 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' '
 for prev in topic master..topic
 do
 	test_expect_success "format-patch --range-diff=$prev" '
-		git format-patch --stdout --cover-letter --range-diff=$prev \
+		git format-patch --cover-letter --range-diff=$prev \
 			master..unmodified >actual &&
-		grep "= 1: .* s/5/A" actual &&
-		grep "= 2: .* s/4/A" actual &&
-		grep "= 3: .* s/11/B" actual &&
-		grep "= 4: .* s/12/B" actual
+		test_when_finished "rm 000?-*" &&
+		test_line_count = 5 actual &&
+		test_i18ngrep "^Range-diff:$" 0000-* &&
+		grep "= 1: .* s/5/A" 0000-* &&
+		grep "= 2: .* s/4/A" 0000-* &&
+		grep "= 3: .* s/11/B" 0000-* &&
+		grep "= 4: .* s/12/B" 0000-*
 	'
 done
 
 test_expect_success 'format-patch --range-diff as commentary' '
-	git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
-	test_i18ngrep "^Range-diff:$" actual
+	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
+	test_when_finished "rm 0001-*" &&
+	test_line_count = 1 actual &&
+	test_i18ngrep "^Range-diff:$" 0001-* &&
+	grep "> 1: .* new message" 0001-*
 '
 
 test_done
diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..e42487b46d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,20 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}
 
 	if (rev->rdiff1) {
+		/*
+		 * (At least for now) we only want to pass down
+		 * the file handle where we want the range-diff
+		 * to appear. Avoid any other diff options until
+		 * we know how we want to handle them.
+		 */
+		struct diff_options opts;
+		diff_setup(&opts);
+		opts.file = rev->diffopt.file;
+		opts.use_color = 0;
+		diff_setup_done(&opts);
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, NULL);
+				rev->creation_factor, 1, &opts);
 	}
 }
 
diff --git a/log-tree.c b/log-tree.c
index b243779a0b..fd79a3ec37 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,25 @@ void show_log(struct rev_info *opt)
 
 	if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
 		struct diff_queue_struct dq;
+		struct diff_options opts;
 
 		memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
 		DIFF_QUEUE_CLEAR(&diff_queued_diff);
 
 		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+		/*
+		 * (At least for now) we only want to pass down
+		 * the file handle where we want the range-diff
+		 * to appear. Avoid any other diff options until
+		 * we know how we want to handle them.
+		 */
+		diff_setup(&opts);
+		opts.file = opt->diffopt.file;
+		opts.use_color = 0;
+		diff_setup_done(&opts);
 		show_range_diff(opt->rdiff1, opt->rdiff2,
-				opt->creation_factor, 1, NULL);
+				opt->creation_factor, 1, &opts);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}
-- 
2.20.0.rc2.1.gfcc5f94f1e


  reply	other threads:[~2018-12-03 20:08 UTC|newest]

Thread overview: 97+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-21 15:20 [ANNOUNCE] Git v2.20.0-rc1 Junio C Hamano
2018-11-22 15:58 ` Ævar Arnfjörð Bjarmason
2018-11-22 19:27   ` Eric Sunshine
2018-11-22 21:12     ` [PATCH 0/2] format-patch: pre-2.20 range-diff regression fix Ævar Arnfjörð Bjarmason
2018-11-22 21:12     ` [PATCH 1/2] format-patch: add a more exhaustive --range-diff test Ævar Arnfjörð Bjarmason
2018-11-24  4:14       ` Junio C Hamano
2018-11-24 11:45         ` Ævar Arnfjörð Bjarmason
2018-11-22 21:12     ` [PATCH 2/2] format-patch: don't include --stat with --range-diff output Ævar Arnfjörð Bjarmason
2018-11-24  2:26       ` Junio C Hamano
2018-11-24  4:17         ` Junio C Hamano
2018-11-28 20:18           ` [PATCH 0/2] format-patch: fix root cause of recent regression Ævar Arnfjörð Bjarmason
2018-11-28 20:18           ` [PATCH 1/2] format-patch: add test for --range-diff diff output Ævar Arnfjörð Bjarmason
2018-11-28 20:18           ` [PATCH 2/2] format-patch: allow for independent diff & range-diff options Ævar Arnfjörð Bjarmason
2018-11-29  2:59             ` Junio C Hamano
2018-11-29 10:07             ` Johannes Schindelin
2018-11-29 10:30               ` Ævar Arnfjörð Bjarmason
2018-11-29 12:12                 ` Johannes Schindelin
2018-11-29 14:35                   ` Ævar Arnfjörð Bjarmason
2018-11-29 15:41                     ` Johannes Schindelin
2018-11-29 16:03                       ` Ævar Arnfjörð Bjarmason
2018-11-29 19:03                         ` Johannes Schindelin
2018-11-30  2:30                         ` Junio C Hamano
2018-11-30  4:27                           ` [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options) Junio C Hamano
2018-11-30  8:57                             ` Junio C Hamano
2018-11-30  9:24                               ` Ævar Arnfjörð Bjarmason
2018-11-30 12:32                               ` Johannes Schindelin
2018-11-30  9:31                             ` Eric Sunshine
2018-12-03 13:27                               ` Martin Ågren
2018-12-03 20:07                                 ` Martin Ågren [this message]
2018-12-03 21:21                                   ` [PATCH v3] range-diff: always pass at least minimal diff options Eric Sunshine
2018-12-04  1:35                                     ` Junio C Hamano
2018-12-04  5:40                                     ` Martin Ågren
2018-11-30  9:58                         ` [PATCH 2/2] format-patch: allow for independent diff & range-diff options Eric Sunshine
2018-11-26  7:35 ` [ANNOUNCE] Git v2.20.0-rc1 Junio C Hamano
2018-11-26 15:41   ` Elijah Newren
2018-11-27  0:40     ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2018-11-19  2:54 Git Test Coverage Report (v2.20.0-rc0) Derrick Stolee
2018-11-19 15:40 ` Derrick Stolee
2018-11-19 16:21   ` Jeff King
2018-11-19 18:44     ` Jeff King
2018-11-19 19:00   ` Ben Peart
2018-11-19 21:06     ` Derrick Stolee
2018-11-20 11:34   ` Jeff King
2018-11-20 12:17     ` Derrick Stolee
2018-11-20 12:40       ` Jeff King
2018-11-19 18:33 ` Ævar Arnfjörð Bjarmason
2018-11-19 18:51   ` [PATCH] tests: add a special setup where rebase.useBuiltin is off (Re: Git Test Coverage Report (v2.20.0-rc0)) Jonathan Nieder
2018-11-19 21:03     ` Ævar Arnfjörð Bjarmason
2018-11-19 19:10   ` Git Test Coverage Report (v2.20.0-rc0) Derrick Stolee
2018-11-19 19:39     ` Ævar Arnfjörð Bjarmason
2018-11-19 19:44       ` Derrick Stolee
2018-11-19 21:31   ` Derrick Stolee
2018-11-20 20:43     ` Johannes Schindelin
2018-11-13 12:38 [PATCH 0/1] rebase: understand -C again, refactor Johannes Schindelin via GitGitGadget
2018-11-13 12:38 ` [PATCH 1/1] rebase: really just passthru the `git am` options Johannes Schindelin via GitGitGadget
2018-11-13 13:05   ` Junio C Hamano
2018-11-13 15:05   ` Phillip Wood
2018-11-13 19:21     ` Johannes Schindelin
2018-11-13 19:58       ` Phillip Wood
2018-11-13 21:50         ` rebase-in-C stability for 2.20 Ævar Arnfjörð Bjarmason
2018-11-14  0:07           ` Stefan Beller
2018-11-14  9:01             ` [PATCH 0/2] rebase.useBuiltin doc & test mode Ævar Arnfjörð Bjarmason
2018-11-14 14:07               ` Johannes Schindelin
2018-11-14  9:01             ` [PATCH 1/2] rebase doc: document rebase.useBuiltin Ævar Arnfjörð Bjarmason
2018-11-14  9:01             ` [PATCH 2/2] tests: add a special setup where rebase.useBuiltin is off Ævar Arnfjörð Bjarmason
2018-11-14  0:36           ` rebase-in-C stability for 2.20 Elijah Newren
2018-11-14  3:39           ` Junio C Hamano
2018-11-24 20:54           ` [ANNOUNCE] Git v2.20.0-rc1 Ævar Arnfjörð Bjarmason
2018-11-25  1:00             ` Junio C Hamano
2018-11-26  6:10               ` [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1) Junio C Hamano
2018-11-28  4:31                 ` Jonathan Nieder
2018-11-28  9:23                   ` Johannes Schindelin
2018-11-28 12:21                     ` Ævar Arnfjörð Bjarmason
2018-11-29  4:58                       ` Junio C Hamano
2018-11-29 14:17                     ` Johannes Schindelin
2018-11-29 14:30                       ` Ian Jackson
2018-11-29 15:39                         ` Johannes Schindelin
2018-11-29 15:50                           ` Ian Jackson
2018-11-29 16:14                             ` Johannes Schindelin
2018-11-29 16:26                               ` Ian Jackson
2018-11-26 22:52             ` [ANNOUNCE] Git v2.20.0-rc1 Johannes Schindelin
2018-11-26 23:47               ` Johannes Schindelin
2018-11-28  4:07                 ` Junio C Hamano
2018-11-28  9:30                   ` Johannes Schindelin
2018-11-14 14:22         ` [PATCH 1/1] rebase: really just passthru the `git am` options Johannes Schindelin
2018-11-14  7:29 ` [PATCH 0/1] rebase: understand -C again, refactor Jeff King
2018-11-14 14:28   ` Johannes Schindelin
2018-11-14 16:25 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget
2018-11-14 16:25   ` [PATCH v2 1/2] rebase: really just passthru the `git am` options Johannes Schindelin via GitGitGadget
2018-11-14 16:25   ` [PATCH v2 2/2] rebase: validate -C<n> and --whitespace=<mode> parameters early Johannes Schindelin via GitGitGadget
2018-11-14 16:37     ` Phillip Wood
2018-11-14 21:24       ` Johannes Schindelin
2018-11-19 12:38     ` Ævar Arnfjörð Bjarmason
2018-11-19 21:37       ` Git Test Coverage Report (v2.20.0-rc0) Ævar Arnfjörð Bjarmason
2018-11-20 10:58       ` [PATCH v2 2/2] rebase: validate -C<n> and --whitespace=<mode> parameters early Johannes Schindelin
2018-11-20 11:42         ` [PATCH] rebase: mark a test as failing with rebase.useBuiltin=false Ævar Arnfjörð Bjarmason
2018-11-20 19:55           ` Johannes Schindelin

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181203200734.527341-1-martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).