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

From: Martin Ågren <martin.agren@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 the fields we absolutely require, such as output file and
color.

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.

[es: retain diff coloring when going to stdout]

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

This is a re-roll of Martin's v2[1]. The only difference from v2 is that
it retains coloring when emitting to the terminal (plus an in-code
comment was simplified).

The regression introduced by d8981c3f88, in which the range-diff only
ever gets emitted to the terminal, and never to the cover letter or
commentary section of a standalone patch, makes the --range-diff option
rather useless, so this fix probably ought to be fast-tracked. An
alternative would be to rip out all the recent "--range-diff"-related
changes and go with the --range-diff implementation which has been in
use for a few months, even if it is not perfect.

[1]: https://public-inbox.org/git/20181203200734.527341-1-martin.agren@gmail.com/

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

diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..e8e51068bd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,18 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}
 
 	if (rev->rdiff1) {
+		/*
+		 * Pass minimum required diff-options to range-diff; others
+		 * can be added later if deemed desirable.
+		 */
+		struct diff_options opts;
+		diff_setup(&opts);
+		opts.file = rev->diffopt.file;
+		opts.use_color = rev->diffopt.use_color;
+		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..10680c139e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,23 @@ 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);
+		/*
+		 * Pass minimum required diff-options to range-diff; others
+		 * can be added later if deemed desirable.
+		 */
+		diff_setup(&opts);
+		opts.file = opt->diffopt.file;
+		opts.use_color = opt->diffopt.use_color;
+		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));
 	}
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
-- 
2.20.0.rc2.403.gdbc3b29805


  reply	other threads:[~2018-12-03 21:22 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                                 ` [PATCH v2] range-diff: always pass at least minimal diff options Martin Ågren
2018-12-03 21:21                                   ` Eric Sunshine [this message]
2018-12-04  1:35                                     ` [PATCH v3] " 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=20181203212131.11299-1-sunshine@sunshineco.com \
    --to=sunshine@sunshineco.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=martin.agren@gmail.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).