[PATCH 0/3] range-diff: don't compare notes

[PATCH 0/3] range-diff: don't compare notes

[Denton Liu]

When I was using range-diff at $DAYJOB earlier, I realised that it
includes commit notes as part of the commit message comparison. This is
undesired behaviour so this patchset documents it and stops it from
happening.

Denton Liu (3):
  t3206: remove spaces after redirect operators
  t3206: demonstrate failure with notes
  range-diff: use --no-notes to generate patches

 range-diff.c          |  2 +-
 t/t3206-range-diff.sh | 17 ++++++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.24.0.420.g9ac4901264

[PATCH 1/3] t3206: remove spaces after redirect operators

[Denton Liu]

For shell scripts, the usual convention is for there to be no space
after redirection operators, (e.g. `>file`, not `> file`). Remove the
one instance of this.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 0579cd9969..87c6c029db 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -8,7 +8,7 @@ test_description='range-diff tests'
 # harm than good.  We need some real history.
 
 test_expect_success 'setup' '
-	git fast-import < "$TEST_DIRECTORY"/t3206/history.export &&
+	git fast-import <"$TEST_DIRECTORY"/t3206/history.export &&
 	test_oid_cache <<-EOF
 	# topic
 	t1 sha1:4de457d
-- 
2.24.0.420.g9ac4901264

[PATCH 2/3] t3206: demonstrate failure with notes

[Denton Liu]

When a commit being range-diff'd has a note attached to it, the note
will be compared as well. However, this shouldn't happen since the
purpose of range-diff is to compare the difference between two
commit-ranges and, since the note attached to a commit is mutable, they
shouldn't be included as part of the comparison.

Demonstrate this failure in t3206.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 87c6c029db..29b9a6f854 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -505,4 +505,19 @@ test_expect_success 'range-diff overrides diff.noprefix internally' '
 	git -c diff.noprefix=true range-diff HEAD^...
 '
 
+test_expect_failure 'range-diff does not compare notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git range-diff --no-color master..topic master..unmodified \
+		>actual &&
+	cat >expected <<-EOF &&
+	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
+	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
+	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
+	4:  $(test_oid t4) = 4:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
2.24.0.420.g9ac4901264

[PATCH 3/3] range-diff: use --no-notes to generate patches

[Denton Liu]

When a commit being range-diff'd has a note attached to it, the note
will be compared as well. However, this shouldn't happen since the
purpose of range-diff is to compare the difference between two
commit-ranges and, since the note attached to a commit is mutable, they
shouldn't be included as part of the comparison.

Add `--no-notes` to the `git log` call and mark the corresponding test
as passing.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 range-diff.c          | 2 +-
 t/t3206-range-diff.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index 7fed5a3b4b..725930ee92 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -52,7 +52,7 @@ static int read_patches(const char *range, struct string_list *list)
 
 	argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
 			"--reverse", "--date-order", "--decorate=no",
-			"--no-prefix",
+			"--no-prefix", "--no-notes",
 			/*
 			 * Choose indicators that are not used anywhere
 			 * else in diffs, but still look reasonable
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 29b9a6f854..671703f85b 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -505,7 +505,7 @@ test_expect_success 'range-diff overrides diff.noprefix internally' '
 	git -c diff.noprefix=true range-diff HEAD^...
 '
 
-test_expect_failure 'range-diff does not compare notes' '
+test_expect_success 'range-diff does not compare notes' '
 	git notes add -m "topic note" topic &&
 	git notes add -m "unmodified note" unmodified &&
 	test_when_finished git notes remove topic unmodified &&
-- 
2.24.0.420.g9ac4901264

Re: [PATCH 0/3] range-diff: don't compare notes

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> When I was using range-diff at $DAYJOB earlier, I realised that it
> includes commit notes as part of the commit message comparison. This is
> undesired behaviour so this patchset documents it and stops it from
> happening.

I actually wish it allowed me to compare them _with_ --notes=<ref>
specified by the user.  If you are passing --no-notes through,
perhaps you can also pass through such an option while at it.

Re: [PATCH 2/3] t3206: demonstrate failure with notes

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> When a commit being range-diff'd has a note attached to it, the note
> will be compared as well. However, this shouldn't happen since the
> purpose of range-diff is to compare the difference between two
> commit-ranges and, since the note attached to a commit is mutable, they
> shouldn't be included as part of the comparison.

I do not agree with the reasoning.  Commits in the new iteration of
the same series would have note attached to them that are different
from the note attached to corresponding commits in the old iteration.

Imagine that "git am" gets enhanced to store the per-iteration
comments you write under the three-dash lines (e.g. "fixed typo in
the log message pointed out by X") as notes attached to each of the
resulting commits.  It does make sense to compare the commits _with_
notes by default, if these notes are configured to be shown in "git
show" output by default for the user.

I do think that it may be a good idea to optionally allow comparison
without notes, when "--no-notes" is given to range-diff on the
command line, by passing the option through, though.

Thanks.

[PATCH v2 0/8] range-diff: learn `--notes`

[Denton Liu]

This patchset teaches range-diff and format-patch to pass `--notes`
options down to the `git log` machinery. This should make the behaviour
more configurable for users who either don't want notes to be displayed
or want multiple notes refs to be displayed.

Changes since v1:

* Complete overhaul of approach

Denton Liu (8):
  argv-array: add space after `while`
  rev-list-options.txt: remove reference to --show-notes
  t3206: remove spaces after redirect operators
  t3206: s/expected/expect/
  t3206: demonstrate current notes behavior
  range-diff: output `## Notes ##` header
  range-diff: passthrough --[no-]notes to `git log`
  format-patch: pass notes configuration to range-diff

 Documentation/git-range-diff.txt   |   6 +-
 Documentation/rev-list-options.txt |   2 +-
 argv-array.c                       |   2 +-
 builtin/log.c                      |  24 ++-
 builtin/range-diff.c               |   6 +-
 log-tree.c                         |   2 +-
 range-diff.c                       |  21 ++-
 range-diff.h                       |   4 +-
 t/t3206-range-diff.sh              | 264 +++++++++++++++++++++++++----
 9 files changed, 286 insertions(+), 45 deletions(-)

-- 
2.24.0.420.g9ac4901264

[PATCH v2 1/8] argv-array: add space after `while`

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 argv-array.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/argv-array.c b/argv-array.c
index f352ea9357..61ef8c0dfd 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -46,7 +46,7 @@ void argv_array_pushl(struct argv_array *array, ...)
 	const char *arg;
 
 	va_start(ap, array);
-	while((arg = va_arg(ap, const char *)))
+	while ((arg = va_arg(ap, const char *)))
 		argv_array_push(array, arg);
 	va_end(ap);
 }
-- 
2.24.0.420.g9ac4901264

[PATCH v2 2/8] rev-list-options.txt: remove reference to --show-notes

[Denton Liu]

In ab18b2c0df ("log/pretty-options: Document --[no-]notes and deprecate
old notes options", 2011-03-30), the `--show-notes` option was
deprecated. However, this reference to it still remains. Change it to
reference the replacement option: `--notes`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/rev-list-options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 90ff9e2bea..311bc06a9b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -58,7 +58,7 @@ endif::git-rev-list[]
 	`--all-match`).
 ifndef::git-rev-list[]
 +
-When `--show-notes` is in effect, the message from the notes is
+When `--notes` is in effect, the message from the notes is
 matched as if it were part of the log message.
 endif::git-rev-list[]
 
-- 
2.24.0.420.g9ac4901264

[PATCH v2 3/8] t3206: remove spaces after redirect operators

[Denton Liu]

For shell scripts, the usual convention is for there to be no space
after redirection operators, (e.g. `>file`, not `> file`). Remove the
one instance of this.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 0579cd9969..87c6c029db 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -8,7 +8,7 @@ test_description='range-diff tests'
 # harm than good.  We need some real history.
 
 test_expect_success 'setup' '
-	git fast-import < "$TEST_DIRECTORY"/t3206/history.export &&
+	git fast-import <"$TEST_DIRECTORY"/t3206/history.export &&
 	test_oid_cache <<-EOF
 	# topic
 	t1 sha1:4de457d
-- 
2.24.0.420.g9ac4901264

[PATCH v2 4/8] t3206: s/expected/expect/

[Denton Liu]

For test cases, the usual convention is to name expected output files
"expect", not "expected". Replace all instances of "expected" with
"expect".

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 64 +++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 87c6c029db..e14b4951bb 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -121,88 +121,88 @@ test_expect_success 'setup' '
 test_expect_success 'simple A..B A..C (unmodified)' '
 	git range-diff --no-color master..topic master..unmodified \
 		>actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
 	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
 	4:  $(test_oid t4) = 4:  $(test_oid u4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'simple B...C (unmodified)' '
 	git range-diff --no-color topic...unmodified >actual &&
-	# same "expected" as above
-	test_cmp expected actual
+	# same "expect" as above
+	test_cmp expect actual
 '
 
 test_expect_success 'simple A B C (unmodified)' '
 	git range-diff --no-color master topic unmodified >actual &&
-	# same "expected" as above
-	test_cmp expected actual
+	# same "expect" as above
+	test_cmp expect actual
 '
 
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid r1) s/5/A/
 	3:  $(test_oid t3) = 2:  $(test_oid r2) s/11/B/
 	4:  $(test_oid t4) = 3:  $(test_oid r3) s/12/B/
 	2:  $(test_oid t2) = 4:  $(test_oid r4) s/4/A/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'removed a commit' '
 	git range-diff --no-color master topic removed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid d1) s/5/A/
 	2:  $(test_oid t2) < -:  $(test_oid __) s/4/A/
 	3:  $(test_oid t3) = 2:  $(test_oid d2) s/11/B/
 	4:  $(test_oid t4) = 3:  $(test_oid d3) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'added a commit' '
 	git range-diff --no-color master topic added >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid a1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid a2) s/4/A/
 	-:  $(test_oid __) > 3:  $(test_oid a3) s/6/A/
 	3:  $(test_oid t3) = 4:  $(test_oid a4) s/11/B/
 	4:  $(test_oid t4) = 5:  $(test_oid a5) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'new base, A B C' '
 	git range-diff --no-color master topic rebased >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid b1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid b2) s/4/A/
 	3:  $(test_oid t3) = 3:  $(test_oid b3) s/11/B/
 	4:  $(test_oid t4) = 4:  $(test_oid b4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'new base, B...C' '
 	# this syntax includes the commits from master!
 	git range-diff --no-color topic...rebased >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	-:  $(test_oid __) > 1:  $(test_oid b5) unrelated
 	1:  $(test_oid t1) = 2:  $(test_oid b1) s/5/A/
 	2:  $(test_oid t2) = 3:  $(test_oid b2) s/4/A/
 	3:  $(test_oid t3) = 4:  $(test_oid b3) s/11/B/
 	4:  $(test_oid t4) = 5:  $(test_oid b4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'changed commit' '
 	git range-diff --no-color topic...changed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid c2) s/4/A/
 	3:  $(test_oid t3) ! 3:  $(test_oid c3) s/11/B/
@@ -226,23 +226,23 @@ test_expect_success 'changed commit' '
 	     +B
 	      13
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'changed commit with --no-patch diff option' '
 	git range-diff --no-color --no-patch topic...changed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid c2) s/4/A/
 	3:  $(test_oid t3) ! 3:  $(test_oid c3) s/11/B/
 	4:  $(test_oid t4) ! 4:  $(test_oid c4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'changed commit with --stat diff option' '
 	git range-diff --no-color --stat topic...changed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
@@ -256,12 +256,12 @@ test_expect_success 'changed commit with --stat diff option' '
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'changed commit with sm config' '
 	git range-diff --no-color --submodule=log topic...changed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid c2) s/4/A/
 	3:  $(test_oid t3) ! 3:  $(test_oid c3) s/11/B/
@@ -285,12 +285,12 @@ test_expect_success 'changed commit with sm config' '
 	     +B
 	      13
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'renamed file' '
 	git range-diff --no-color --submodule=log topic...renamed-file >actual &&
-	sed s/Z/\ /g >expected <<-EOF &&
+	sed s/Z/\ /g >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid n1) s/5/A/
 	2:  $(test_oid t2) ! 2:  $(test_oid n2) s/4/A/
 	    @@ Metadata
@@ -330,12 +330,12 @@ test_expect_success 'renamed file' '
 	    Z 10
 	    Z B
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'file with mode only change' '
 	git range-diff --no-color --submodule=log topic...mode-only-change >actual &&
-	sed s/Z/\ /g >expected <<-EOF &&
+	sed s/Z/\ /g >expect <<-EOF &&
 	1:  fccce22 ! 1:  4d39cb3 s/4/A/
 	    @@ Metadata
 	    ZAuthor: Thomas Rast <trast@inf.ethz.ch>
@@ -370,12 +370,12 @@ test_expect_success 'file with mode only change' '
 	    + ## other-file (mode change 100644 => 100755) ##
 	3:  a63e992 = 3:  4c1e0f5 s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'file added and later removed' '
 	git range-diff --no-color --submodule=log topic...added-removed >actual &&
-	sed s/Z/\ /g >expected <<-EOF &&
+	sed s/Z/\ /g >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid s1) s/5/A/
 	2:  $(test_oid t2) ! 2:  $(test_oid s2) s/4/A/
 	    @@ Metadata
@@ -411,7 +411,7 @@ test_expect_success 'file added and later removed' '
 	    + ## new-file (deleted) ##
 	4:  $(test_oid t4) = 4:  $(test_oid s4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'no commits on one side' '
@@ -421,7 +421,7 @@ test_expect_success 'no commits on one side' '
 
 test_expect_success 'changed message' '
 	git range-diff --no-color topic...changed-message >actual &&
-	sed s/Z/\ /g >expected <<-EOF &&
+	sed s/Z/\ /g >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid m1) s/5/A/
 	2:  $(test_oid t2) ! 2:  $(test_oid m2) s/4/A/
 	    @@ Metadata
@@ -436,7 +436,7 @@ test_expect_success 'changed message' '
 	3:  $(test_oid t3) = 3:  $(test_oid m3) s/11/B/
 	4:  $(test_oid t4) = 4:  $(test_oid m4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'dual-coloring' '
-- 
2.24.0.420.g9ac4901264

[PATCH v2 5/8] t3206: demonstrate current notes behavior

[Denton Liu]

The test suite had a blindspot where it did not check the behavior of
range-diff and format-patch when notes were present. Cover this
blindspot.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e14b4951bb..69babdcf03 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -505,4 +505,56 @@ test_expect_success 'range-diff overrides diff.noprefix internally' '
 	git -c diff.noprefix=true range-diff HEAD^...
 '
 
+test_expect_success 'range-diff compares notes by default' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git range-diff --no-color master..topic master..unmodified \
+		>actual &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
+	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
+	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
+	4:  $(test_oid t4) ! 4:  $(test_oid u4) s/12/B/
+	    @@ Metadata
+	    Z
+	    Z ## Commit message ##
+	    Z    s/12/B/
+	    -    topic note
+	    +    unmodified note
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'format-patch --range-diff compares notes by default' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git format-patch --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	    @@ Metadata
+	    Z
+	    Z ## Commit message ##
+	    Z    s/12/B/
+	    -    topic note
+	    +    unmodified note
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	sed "/@@ Metadata/,/@@ file: A/!d" 0000-* >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.24.0.420.g9ac4901264

[PATCH v2 6/8] range-diff: output `## Notes ##` header

[Denton Liu]

When notes were included in the output of range-diff, they were just
mashed together with the rest of the commit message. As a result, users
wouldn't be able to clearly distinguish where the commit message ended
and where the notes started.

Output a `## Notes ##` header when notes are detected so that notes can
be compared more clearly.

Note that we handle case of `Notes (<ref>): -> ## Notes (<ref>) ##` with
this code as well. We can't test this in this patch, however, since
there is currently no way to pass along different notes refs to `git
log`. This will be fixed in a future patch.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 range-diff.c          |  6 ++++++
 t/t3206-range-diff.sh | 14 +++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index 7fed5a3b4b..623397221d 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -144,6 +144,12 @@ static int read_patches(const char *range, struct string_list *list)
 				strbuf_addstr(&buf, line);
 				strbuf_addstr(&buf, "\n\n");
 				strbuf_addstr(&buf, " ## Commit message ##\n");
+			} else if (starts_with(line, "Notes") &&
+				   line[strlen(line) - 1] == ':') {
+				strbuf_addstr(&buf, "\n\n");
+				/* strip the trailing colon */
+				strbuf_addf(&buf, " ## %.*s ##\n",
+					    (int)(strlen(line) - 1), line);
 			} else if (starts_with(line, "    ")) {
 				p = line + len - 2;
 				while (isspace(*p) && p >= line)
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 69babdcf03..5bb33efdff 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -516,10 +516,10 @@ test_expect_success 'range-diff compares notes by default' '
 	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
 	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
 	4:  $(test_oid t4) ! 4:  $(test_oid u4) s/12/B/
-	    @@ Metadata
+	    @@ Commit message
 	    Z
-	    Z ## Commit message ##
-	    Z    s/12/B/
+	    Z
+	    Z ## Notes ##
 	    -    topic note
 	    +    unmodified note
 	    Z
@@ -543,17 +543,17 @@ test_expect_success 'format-patch --range-diff compares notes by default' '
 	grep "= 3: .* s/11/B" 0000-* &&
 	grep "! 4: .* s/12/B" 0000-* &&
 	sed s/Z/\ /g >expect <<-EOF &&
-	    @@ Metadata
+	    @@ Commit message
 	    Z
-	    Z ## Commit message ##
-	    Z    s/12/B/
+	    Z
+	    Z ## Notes ##
 	    -    topic note
 	    +    unmodified note
 	    Z
 	    Z ## file ##
 	    Z@@ file: A
 	EOF
-	sed "/@@ Metadata/,/@@ file: A/!d" 0000-* >actual &&
+	sed "/@@ Commit message/,/@@ file: A/!d" 0000-* >actual &&
 	test_cmp expect actual
 '
 
-- 
2.24.0.420.g9ac4901264

[PATCH v2 7/8] range-diff: passthrough --[no-]notes to `git log`

[Denton Liu]

When a commit being range-diff'd has a note attached to it, the note
will be compared as well. However, if a user has multiple notes refs or
if they want to suppress notes from being printed, there is currently no
way to do this.

Passthrough `---no--notes` to the `git log` call so that this option is
customizable.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-range-diff.txt |  6 +++-
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  6 +++-
 log-tree.c                       |  2 +-
 range-diff.c                     | 15 ++++++----
 range-diff.h                     |  4 ++-
 t/t3206-range-diff.sh            | 47 ++++++++++++++++++++++++++++++++
 7 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index 8a6ea2c6c5..a3d69713f7 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -57,6 +57,10 @@ to revert to color all lines according to the outer diff markers
 	See the ``Algorithm`` section below for an explanation why this is
 	needed.
 
+--[no-]notes[=<treeish>]::
+	This flag is passed to the `git log` program
+	(see linkgit:git-log[1]) that generates the patches.
+
 <range1> <range2>::
 	Compare the commits specified by the two ranges, where
 	`<range1>` is considered an older version of `<range2>`.
@@ -75,7 +79,7 @@ to revert to color all lines according to the outer diff markers
 linkgit:git-diff[1]), most notably the `--color=[<when>]` and
 `--no-color` options. These options are used when generating the "diff
 between patches", i.e. to compare the author, commit message and diff of
-corresponding old/new commits. There is currently no means to tweak the
+corresponding old/new commits. There is currently no means to tweak most of the
 diff options passed to `git log` when generating those patches.
 
 OUTPUT STABILITY
diff --git a/builtin/log.c b/builtin/log.c
index a26f223ab4..047ac4594d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1189,7 +1189,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		diff_setup_done(&opts);
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &opts);
+				rev->creation_factor, 1, &opts, NULL);
 	}
 }
 
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 9202e75544..98acf3533e 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -15,12 +15,16 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 {
 	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
 	struct diff_options diffopt = { NULL };
+	struct argv_array other_arg = ARGV_ARRAY_INIT;
 	int simple_color = -1;
 	struct option range_diff_options[] = {
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
 			    N_("use simple diff colors")),
+		OPT_PASSTHRU_ARGV(0, "notes", &other_arg,
+				  N_("notes"), N_("passed to 'git log'"),
+				  PARSE_OPT_OPTARG),
 		OPT_END()
 	};
 	struct option *options;
@@ -78,7 +82,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	FREE_AND_NULL(options);
 
 	res = show_range_diff(range1.buf, range2.buf, creation_factor,
-			      simple_color < 1, &diffopt);
+			      simple_color < 1, &diffopt, &other_arg);
 
 	strbuf_release(&range1);
 	strbuf_release(&range2);
diff --git a/log-tree.c b/log-tree.c
index 923a299e70..151e12f415 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -770,7 +770,7 @@ void show_log(struct rev_info *opt)
 		opts.use_color = opt->diffopt.use_color;
 		diff_setup_done(&opts);
 		show_range_diff(opt->rdiff1, opt->rdiff2,
-				opt->creation_factor, 1, &opts);
+				opt->creation_factor, 1, &opts, NULL);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}
diff --git a/range-diff.c b/range-diff.c
index 623397221d..f56b4012a2 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -40,7 +40,8 @@ static size_t find_end_of_line(char *buffer, unsigned long size)
  * Reads the patches into a string list, with the `util` field being populated
  * as struct object_id (will need to be free()d).
  */
-static int read_patches(const char *range, struct string_list *list)
+static int read_patches(const char *range, struct string_list *list,
+			struct argv_array *other_arg)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
@@ -61,8 +62,11 @@ static int read_patches(const char *range, struct string_list *list)
 			"--output-indicator-new=>",
 			"--output-indicator-old=<",
 			"--output-indicator-context=#",
-			"--no-abbrev-commit", range,
+			"--no-abbrev-commit",
 			NULL);
+	if (other_arg)
+		argv_array_pushv(&cp.args, other_arg->argv);
+	argv_array_push(&cp.args, range);
 	cp.out = -1;
 	cp.no_stdin = 1;
 	cp.git_cmd = 1;
@@ -502,16 +506,17 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
 
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
-		    struct diff_options *diffopt)
+		    struct diff_options *diffopt,
+		    struct argv_array *other_arg)
 {
 	int res = 0;
 
 	struct string_list branch1 = STRING_LIST_INIT_DUP;
 	struct string_list branch2 = STRING_LIST_INIT_DUP;
 
-	if (read_patches(range1, &branch1))
+	if (read_patches(range1, &branch1, other_arg))
 		res = error(_("could not parse log for '%s'"), range1);
-	if (!res && read_patches(range2, &branch2))
+	if (!res && read_patches(range2, &branch2, other_arg))
 		res = error(_("could not parse log for '%s'"), range2);
 
 	if (!res) {
diff --git a/range-diff.h b/range-diff.h
index 08a50b6e98..7d918ab9ed 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -2,6 +2,7 @@
 #define RANGE_DIFF_H
 
 #include "diff.h"
+#include "argv-array.h"
 
 #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
 
@@ -12,6 +13,7 @@
  */
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
-		    struct diff_options *diffopt);
+		    struct diff_options *diffopt,
+		    struct argv_array *other_arg);
 
 #endif
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 5bb33efdff..cea1846282 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -529,6 +529,53 @@ test_expect_success 'range-diff compares notes by default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'range-diff with --no-notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git range-diff --no-color --no-notes master..topic master..unmodified \
+		>actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
+	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
+	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
+	4:  $(test_oid t4) = 4:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'range-diff with multiple --notes' '
+	git notes --ref=note1 add -m "topic note1" topic &&
+	git notes --ref=note1 add -m "unmodified note1" unmodified &&
+	test_when_finished git notes --ref=note1 remove topic unmodified &&
+	git notes --ref=note2 add -m "topic note2" topic &&
+	git notes --ref=note2 add -m "unmodified note2" unmodified &&
+	test_when_finished git notes --ref=note2 remove topic unmodified &&
+	git range-diff --no-color --notes=note1 --notes=note2 master..topic master..unmodified \
+		>actual &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
+	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
+	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
+	4:  $(test_oid t4) ! 4:  $(test_oid u4) s/12/B/
+	    @@ Commit message
+	    Z
+	    Z
+	    Z ## Notes (note1) ##
+	    -    topic note1
+	    +    unmodified note1
+	    Z
+	    Z
+	    Z ## Notes (note2) ##
+	    -    topic note2
+	    +    unmodified note2
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'format-patch --range-diff compares notes by default' '
 	git notes add -m "topic note" topic &&
 	git notes add -m "unmodified note" unmodified &&
-- 
2.24.0.420.g9ac4901264

[PATCH v2 8/8] format-patch: pass notes configuration to range-diff

[Denton Liu]

Since format-patch accepts `--[no-]notes`, one would expect the
range-diff generated to also respect the setting. Unfortunately, the
range-diff we currently generate only uses the default option (which
always outputs default notes, even when notes are not being used
elsewhere).

Pass the notes configuration to range-diff so that it can honor it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/log.c         |  24 +++++++++-
 t/t3206-range-diff.sh | 101 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 123 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 047ac4594d..e192f219d9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1111,6 +1111,25 @@ static void prepare_cover_text(struct pretty_print_context *pp,
 	strbuf_release(&subject_sb);
 }
 
+static int get_notes_refs(struct string_list_item *item, void *arg)
+{
+	argv_array_pushf(arg, "--notes=%s", item->string);
+	return 0;
+}
+
+static void get_notes_args(struct argv_array *arg, struct rev_info *rev)
+{
+	if (!rev->show_notes) {
+		argv_array_push(arg, "--no-notes");
+	} else if (rev->notes_opt.use_default_notes > 0 ||
+		   (rev->notes_opt.use_default_notes == -1 &&
+		    !rev->notes_opt.extra_notes_refs.nr)) {
+		argv_array_push(arg, "--notes");
+	} else {
+		for_each_string_list(&rev->notes_opt.extra_notes_refs, get_notes_refs, arg);
+	}
+}
+
 static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
@@ -1183,13 +1202,16 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		 * can be added later if deemed desirable.
 		 */
 		struct diff_options opts;
+		struct argv_array other_arg = ARGV_ARRAY_INIT;
 		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);
+		get_notes_args(&other_arg, rev);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &opts, NULL);
+				rev->creation_factor, 1, &opts, &other_arg);
+		argv_array_clear(&other_arg);
 	}
 }
 
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index cea1846282..abbf1efc20 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -576,7 +576,7 @@ test_expect_success 'range-diff with multiple --notes' '
 	test_cmp expect actual
 '
 
-test_expect_success 'format-patch --range-diff compares notes by default' '
+test_expect_success 'format-patch --range-diff does not compare notes by default' '
 	git notes add -m "topic note" topic &&
 	git notes add -m "unmodified note" unmodified &&
 	test_when_finished git notes remove topic unmodified &&
@@ -588,6 +588,40 @@ test_expect_success 'format-patch --range-diff compares notes by default' '
 	grep "= 1: .* s/5/A" 0000-* &&
 	grep "= 2: .* s/4/A" 0000-* &&
 	grep "= 3: .* s/11/B" 0000-* &&
+	grep "= 4: .* s/12/B" 0000-* &&
+	! grep "Notes" 0000-* &&
+	! grep "note" 0000-*
+'
+
+test_expect_success 'format-patch --range-diff with --no-notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git format-patch --no-notes --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
+	! grep "Notes" 0000-* &&
+	! grep "note" 0000-*
+'
+
+test_expect_success 'format-patch --range-diff with --notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git format-patch --notes --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
 	sed s/Z/\ /g >expect <<-EOF &&
 	    @@ Commit message
@@ -604,4 +638,69 @@ test_expect_success 'format-patch --range-diff compares notes by default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'format-patch --range-diff with --notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	test_config format.notes true &&
+	git format-patch --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	    @@ Commit message
+	    Z
+	    Z
+	    Z ## Notes ##
+	    -    topic note
+	    +    unmodified note
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	sed "/@@ Commit message/,/@@ file: A/!d" 0000-* >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'format-patch --range-diff with multiple notes' '
+	git notes --ref=note1 add -m "topic note1" topic &&
+	git notes --ref=note1 add -m "unmodified note1" unmodified &&
+	test_when_finished git notes --ref=note1 remove topic unmodified &&
+	git notes --ref=note2 add -m "topic note2" topic &&
+	git notes --ref=note2 add -m "unmodified note2" unmodified &&
+	test_when_finished git notes --ref=note2 remove topic unmodified &&
+	git format-patch --notes=note1 --notes=note2 --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	    @@ Commit message
+	    Z
+	    Z
+	    Z ## Notes (note1) ##
+	    -    topic note1
+	    +    unmodified note1
+	    Z
+	    Z
+	    Z ## Notes (note2) ##
+	    -    topic note2
+	    +    unmodified note2
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	sed "/@@ Commit message/,/@@ file: A/!d" 0000-* >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.24.0.420.g9ac4901264

Re: [PATCH v2 3/8] t3206: remove spaces after redirect operators

[Eric Sunshine]

On Tue, Nov 19, 2019 at 6:55 PM Denton Liu <liu.denton@gmail.com> wrote:
> For shell scripts, the usual convention is for there to be no space
> after redirection operators, (e.g. `>file`, not `> file`). Remove the
> one instance of this.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> @@ -8,7 +8,7 @@ test_description='range-diff tests'
>  test_expect_success 'setup' '
> -       git fast-import < "$TEST_DIRECTORY"/t3206/history.export &&
> +       git fast-import <"$TEST_DIRECTORY"/t3206/history.export &&
>         test_oid_cache <<-EOF
>         # topic
>         t1 sha1:4de457d

While you're here cleaning style problems, you could also change this
here-doc from -EOF to -\EOF. (This is the only here-doc in this script
which can use -\EOF; all the others need -EOF.)

Re: [PATCH v2 5/8] t3206: demonstrate current notes behavior

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> Subject: Re: [PATCH v2 5/8] t3206: demonstrate current notes behavior

This makes it sound as if "current" is either bad or stale and will
be "fixed".  In the previous round that might have been your
intention, but I do not think the focus of v2 is that, so phrasing
may want to be updated, perhaps like

    t3206: range-diff compares log message with commit notes

or something.

> The test suite had a blindspot where it did not check the behavior of
> range-diff and format-patch when notes were present. Cover this
> blindspot.

Good description.

Re: [PATCH v2 7/8] range-diff: passthrough --[no-]notes to `git log`

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> When a commit being range-diff'd has a note attached to it, the note
> will be compared as well. However, if a user has multiple notes refs or
> if they want to suppress notes from being printed, there is currently no
> way to do this.
>
> Passthrough `---no--notes` to the `git log` call so that this option is

"`--[no-]notes`" or perhaps "`--[no-]notes` and `--notes=<ref>`"?

I think the verb phrase is two words, "pass through", by the way.

> +--[no-]notes[=<treeish>]::
> +	This flag is passed to the `git log` program
> +	(see linkgit:git-log[1]) that generates the patches.

I can see this was taken from "git log --help", and it probably
needs fixing for consistency as well, but I think --notes=<ref>
would be easier to click users' minds with notes.displayRef
configuration variable.

> @@ -61,8 +62,11 @@ static int read_patches(const char *range, struct string_list *list)
>  			"--output-indicator-new=>",
>  			"--output-indicator-old=<",
>  			"--output-indicator-context=#",
> -			"--no-abbrev-commit", range,
> +			"--no-abbrev-commit",
>  			NULL);
> +	if (other_arg)
> +		argv_array_pushv(&cp.args, other_arg->argv);
> +	argv_array_push(&cp.args, range);

Makes sense.

> diff --git a/range-diff.h b/range-diff.h
> index 08a50b6e98..7d918ab9ed 100644
> --- a/range-diff.h
> +++ b/range-diff.h
> @@ -2,6 +2,7 @@
>  #define RANGE_DIFF_H
>  
>  #include "diff.h"
> +#include "argv-array.h"
>  
>  #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
>  
> @@ -12,6 +13,7 @@
>   */
>  int show_range_diff(const char *range1, const char *range2,
>  		    int creation_factor, int dual_color,
> -		    struct diff_options *diffopt);
> +		    struct diff_options *diffopt,
> +		    struct argv_array *other_arg);
>  
>  #endif

I thought a mere use of "pointer to a struct" did not have to bring
the definition of the struct into the picture?  In other words,
wouldn't it be fine to leave the other_arg a pointer to an opaque
structure by not including "argv-array.h" in this file?

Thanks.

Re: [PATCH v2 7/8] range-diff: passthrough --[no-]notes to `git log`

[Denton Liu]

Hi Junio,

On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > When a commit being range-diff'd has a note attached to it, the note
> > will be compared as well. However, if a user has multiple notes refs or
> > if they want to suppress notes from being printed, there is currently no
> > way to do this.
> >
> > Passthrough `---no--notes` to the `git log` call so that this option is
> 
> "`--[no-]notes`" or perhaps "`--[no-]notes` and `--notes=<ref>`"?

Whoops, I probably typed `ys-` in vim instead of `ys[` so I surrounded
the `no-` with the wrong characters.

> 
> I think the verb phrase is two words, "pass through", by the way.

Thanks, I didn't know this.

> 
> > +--[no-]notes[=<treeish>]::
> > +	This flag is passed to the `git log` program
> > +	(see linkgit:git-log[1]) that generates the patches.
> 
> I can see this was taken from "git log --help", and it probably
> needs fixing for consistency as well, but I think --notes=<ref>
> would be easier to click users' minds with notes.displayRef
> configuration variable.

I'll put in a cleanup patch for this as well.

> 
> > @@ -61,8 +62,11 @@ static int read_patches(const char *range, struct string_list *list)
> >  			"--output-indicator-new=>",
> >  			"--output-indicator-old=<",
> >  			"--output-indicator-context=#",
> > -			"--no-abbrev-commit", range,
> > +			"--no-abbrev-commit",
> >  			NULL);
> > +	if (other_arg)
> > +		argv_array_pushv(&cp.args, other_arg->argv);
> > +	argv_array_push(&cp.args, range);
> 
> Makes sense.
> 
> > diff --git a/range-diff.h b/range-diff.h
> > index 08a50b6e98..7d918ab9ed 100644
> > --- a/range-diff.h
> > +++ b/range-diff.h
> > @@ -2,6 +2,7 @@
> >  #define RANGE_DIFF_H
> >  
> >  #include "diff.h"
> > +#include "argv-array.h"
> >  
> >  #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
> >  
> > @@ -12,6 +13,7 @@
> >   */
> >  int show_range_diff(const char *range1, const char *range2,
> >  		    int creation_factor, int dual_color,
> > -		    struct diff_options *diffopt);
> > +		    struct diff_options *diffopt,
> > +		    struct argv_array *other_arg);
> >  
> >  #endif
> 
> I thought a mere use of "pointer to a struct" did not have to bring
> the definition of the struct into the picture?  In other words,
> wouldn't it be fine to leave the other_arg a pointer to an opaque
> structure by not including "argv-array.h" in this file?

Without including "argv-array.h", we get the following hdr-check error:

	$ make range-diff.hco
	    HDR range-diff.h
	In file included from range-diff.hcc:2:
	./range-diff.h:16:14: error: declaration of 'struct argv_array' will not be visible outside of this function [-Werror,-Wvisibility]
			    struct argv_array *other_arg);
				   ^
	1 error generated.
	make: *** [range-diff.hco] Error 1

I am currently using this compiler for reference:

	$ clang --version
	Apple LLVM version 10.0.1 (clang-1001.0.46.4)
	Target: x86_64-apple-darwin18.7.0
	Thread model: posix
	InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Thanks,

Denton

> 
> Thanks.

[PATCH v3 00/10] range-diff: learn `--notes`

[Denton Liu]

This patchset teaches range-diff and format-patch to pass `--notes`
options down to the `git log` machinery. This should make the behaviour
more configurable for users who either don't want notes to be displayed
or want multiple notes refs to be displayed.

Changes since v2:

* Changed --notes=<treeish> to --notes=<ref>

* Added "t3206: disable parameter substitution in heredoc"

* Updated the commit message of "range-diff: pass through --notes to `git log`"

Changes since v1:

* Complete overhaul of approach

Denton Liu (10):
  argv-array: add space after `while`
  rev-list-options.txt: remove reference to --show-notes
  pretty-options.txt: --notes accepts a ref instead of treeish
  t3206: remove spaces after redirect operators
  t3206: disable parameter substitution in heredoc
  t3206: s/expected/expect/
  t3206: range-diff compares logs with commit notes
  range-diff: output `## Notes ##` header
  range-diff: pass through --notes to `git log`
  format-patch: pass notes configuration to range-diff

 Documentation/git-range-diff.txt   |   6 +-
 Documentation/pretty-options.txt   |   8 +-
 Documentation/rev-list-options.txt |   2 +-
 argv-array.c                       |   2 +-
 builtin/log.c                      |  24 ++-
 builtin/range-diff.c               |   6 +-
 log-tree.c                         |   2 +-
 range-diff.c                       |  21 ++-
 range-diff.h                       |   4 +-
 t/t3206-range-diff.sh              | 266 +++++++++++++++++++++++++----
 10 files changed, 291 insertions(+), 50 deletions(-)

Range-diff against v2:
 1:  396ac06b0d =  1:  fd78742570 argv-array: add space after `while`
 2:  8cc7c8fe72 =  2:  e1b023a6fc rev-list-options.txt: remove reference to --show-notes
 -:  ---------- >  3:  4989956f12 pretty-options.txt: --notes accepts a ref instead of treeish
 3:  22f5f07ace =  4:  85fcacf3f9 t3206: remove spaces after redirect operators
 -:  ---------- >  5:  855a3df542 t3206: disable parameter substitution in heredoc
 4:  922db36e7e =  6:  92df18b261 t3206: s/expected/expect/
 5:  d8ecda2d5e !  7:  093d32ac4f t3206: demonstrate current notes behavior
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    t3206: demonstrate current notes behavior
    +    t3206: range-diff compares logs with commit notes
     
         The test suite had a blindspot where it did not check the behavior of
         range-diff and format-patch when notes were present. Cover this
 6:  7dd0b93b0b =  8:  2d1c849ecc range-diff: output `## Notes ##` header
 7:  587a02a39c !  9:  9c144e14c5 range-diff: passthrough --[no-]notes to `git log`
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    range-diff: passthrough --[no-]notes to `git log`
    +    range-diff: pass through --notes to `git log`
     
         When a commit being range-diff'd has a note attached to it, the note
         will be compared as well. However, if a user has multiple notes refs or
         if they want to suppress notes from being printed, there is currently no
         way to do this.
     
    -    Passthrough `---no--notes` to the `git log` call so that this option is
    -    customizable.
    +    Pass through `--[no-]notes[=<ref>]` to the `git log` call so that this
    +    option is customizable.
     
      ## Documentation/git-range-diff.txt ##
     @@ Documentation/git-range-diff.txt: to revert to color all lines according to the outer diff markers
      	See the ``Algorithm`` section below for an explanation why this is
      	needed.
      
    -+--[no-]notes[=<treeish>]::
    ++--[no-]notes[=<ref>]::
     +	This flag is passed to the `git log` program
     +	(see linkgit:git-log[1]) that generates the patches.
     +
 8:  ce8cff7d0c = 10:  86318b3fe7 format-patch: pass notes configuration to range-diff
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 01/10] argv-array: add space after `while`

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 argv-array.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/argv-array.c b/argv-array.c
index f352ea9357..61ef8c0dfd 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -46,7 +46,7 @@ void argv_array_pushl(struct argv_array *array, ...)
 	const char *arg;
 
 	va_start(ap, array);
-	while((arg = va_arg(ap, const char *)))
+	while ((arg = va_arg(ap, const char *)))
 		argv_array_push(array, arg);
 	va_end(ap);
 }
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 02/10] rev-list-options.txt: remove reference to --show-notes

[Denton Liu]

In ab18b2c0df ("log/pretty-options: Document --[no-]notes and deprecate
old notes options", 2011-03-30), the `--show-notes` option was
deprecated. However, this reference to it still remains. Change it to
reference the replacement option: `--notes`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/rev-list-options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 90ff9e2bea..311bc06a9b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -58,7 +58,7 @@ endif::git-rev-list[]
 	`--all-match`).
 ifndef::git-rev-list[]
 +
-When `--show-notes` is in effect, the message from the notes is
+When `--notes` is in effect, the message from the notes is
 matched as if it were part of the log message.
 endif::git-rev-list[]
 
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 03/10] pretty-options.txt: --notes accepts a ref instead of treeish

[Denton Liu]

Although `--notes=` accepts and handles a tree-ish just fine, it isn't a
common use-case for users to pass in bare trees. By having "treeish", it
makes it harder to click in users' minds that the argument here is the
same type as the `notes.displayRef` configuration variable, for example.

Change `treeish` to `ref` so that it will be easier for users to make
this connection.

Note that we don't completely lose the notion that `--notes=` can accept
a tree-ish. In git-notes.txt, we have

	It is also permitted for a notes ref to point directly to a tree
	object, in which case the history of the notes can be read with
	`git log -p -g <refname>`.

which means that a hardcore user who wants to take advantage of this
obscure use-case will be able to infer the connection and not be
completely left in the dark.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/pretty-options.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index e44fc8f738..6893a4a7ba 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -57,7 +57,7 @@ message by 4 spaces (i.e.  'medium', which is the default, 'full',
 and 'fuller').
 
 ifndef::git-rev-list[]
---notes[=<treeish>]::
+--notes[=<ref>]::
 	Show the notes (see linkgit:git-notes[1]) that annotate the
 	commit, when showing the commit log message.  This is the default
 	for `git log`, `git show` and `git whatchanged` commands when
@@ -68,8 +68,8 @@ By default, the notes shown are from the notes refs listed in the
 `core.notesRef` and `notes.displayRef` variables (or corresponding
 environment overrides). See linkgit:git-config[1] for more details.
 +
-With an optional '<treeish>' argument, use the treeish to find the notes
-to display.  The treeish can specify the full refname when it begins
+With an optional '<ref>' argument, use the ref to find the notes
+to display.  The ref can specify the full refname when it begins
 with `refs/notes/`; when it begins with `notes/`, `refs/` and otherwise
 `refs/notes/` is prefixed to form a full name of the ref.
 +
@@ -85,7 +85,7 @@ being displayed. Examples: "--notes=foo" will show only notes from
 	"--notes --notes=foo --no-notes --notes=bar" will only show notes
 	from "refs/notes/bar".
 
---show-notes[=<treeish>]::
+--show-notes[=<ref>]::
 --[no-]standard-notes::
 	These options are deprecated. Use the above --notes/--no-notes
 	options instead.
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 04/10] t3206: remove spaces after redirect operators

[Denton Liu]

For shell scripts, the usual convention is for there to be no space
after redirection operators, (e.g. `>file`, not `> file`). Remove the
one instance of this.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 0579cd9969..87c6c029db 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -8,7 +8,7 @@ test_description='range-diff tests'
 # harm than good.  We need some real history.
 
 test_expect_success 'setup' '
-	git fast-import < "$TEST_DIRECTORY"/t3206/history.export &&
+	git fast-import <"$TEST_DIRECTORY"/t3206/history.export &&
 	test_oid_cache <<-EOF
 	# topic
 	t1 sha1:4de457d
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 05/10] t3206: disable parameter substitution in heredoc

[Denton Liu]

In the first heredoc, parameter substitution is not used so prevent it
from happening in the future (perhaps by accident) by escaping the limit
EOF.

The remaining heredocs use parameter substitution so they cannot be
changed.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 87c6c029db..41606775d4 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -9,7 +9,7 @@ test_description='range-diff tests'
 
 test_expect_success 'setup' '
 	git fast-import <"$TEST_DIRECTORY"/t3206/history.export &&
-	test_oid_cache <<-EOF
+	test_oid_cache <<-\EOF
 	# topic
 	t1 sha1:4de457d
 	t2 sha1:fccce22
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 06/10] t3206: s/expected/expect/

[Denton Liu]

For test cases, the usual convention is to name expected output files
"expect", not "expected". Replace all instances of "expected" with
"expect".

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 64 +++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 41606775d4..f654aed54f 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -121,88 +121,88 @@ test_expect_success 'setup' '
 test_expect_success 'simple A..B A..C (unmodified)' '
 	git range-diff --no-color master..topic master..unmodified \
 		>actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
 	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
 	4:  $(test_oid t4) = 4:  $(test_oid u4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'simple B...C (unmodified)' '
 	git range-diff --no-color topic...unmodified >actual &&
-	# same "expected" as above
-	test_cmp expected actual
+	# same "expect" as above
+	test_cmp expect actual
 '
 
 test_expect_success 'simple A B C (unmodified)' '
 	git range-diff --no-color master topic unmodified >actual &&
-	# same "expected" as above
-	test_cmp expected actual
+	# same "expect" as above
+	test_cmp expect actual
 '
 
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid r1) s/5/A/
 	3:  $(test_oid t3) = 2:  $(test_oid r2) s/11/B/
 	4:  $(test_oid t4) = 3:  $(test_oid r3) s/12/B/
 	2:  $(test_oid t2) = 4:  $(test_oid r4) s/4/A/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'removed a commit' '
 	git range-diff --no-color master topic removed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid d1) s/5/A/
 	2:  $(test_oid t2) < -:  $(test_oid __) s/4/A/
 	3:  $(test_oid t3) = 2:  $(test_oid d2) s/11/B/
 	4:  $(test_oid t4) = 3:  $(test_oid d3) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'added a commit' '
 	git range-diff --no-color master topic added >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid a1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid a2) s/4/A/
 	-:  $(test_oid __) > 3:  $(test_oid a3) s/6/A/
 	3:  $(test_oid t3) = 4:  $(test_oid a4) s/11/B/
 	4:  $(test_oid t4) = 5:  $(test_oid a5) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'new base, A B C' '
 	git range-diff --no-color master topic rebased >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid b1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid b2) s/4/A/
 	3:  $(test_oid t3) = 3:  $(test_oid b3) s/11/B/
 	4:  $(test_oid t4) = 4:  $(test_oid b4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'new base, B...C' '
 	# this syntax includes the commits from master!
 	git range-diff --no-color topic...rebased >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	-:  $(test_oid __) > 1:  $(test_oid b5) unrelated
 	1:  $(test_oid t1) = 2:  $(test_oid b1) s/5/A/
 	2:  $(test_oid t2) = 3:  $(test_oid b2) s/4/A/
 	3:  $(test_oid t3) = 4:  $(test_oid b3) s/11/B/
 	4:  $(test_oid t4) = 5:  $(test_oid b4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'changed commit' '
 	git range-diff --no-color topic...changed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid c2) s/4/A/
 	3:  $(test_oid t3) ! 3:  $(test_oid c3) s/11/B/
@@ -226,23 +226,23 @@ test_expect_success 'changed commit' '
 	     +B
 	      13
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'changed commit with --no-patch diff option' '
 	git range-diff --no-color --no-patch topic...changed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid c2) s/4/A/
 	3:  $(test_oid t3) ! 3:  $(test_oid c3) s/11/B/
 	4:  $(test_oid t4) ! 4:  $(test_oid c4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'changed commit with --stat diff option' '
 	git range-diff --no-color --stat topic...changed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
@@ -256,12 +256,12 @@ test_expect_success 'changed commit with --stat diff option' '
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'changed commit with sm config' '
 	git range-diff --no-color --submodule=log topic...changed >actual &&
-	cat >expected <<-EOF &&
+	cat >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/
 	2:  $(test_oid t2) = 2:  $(test_oid c2) s/4/A/
 	3:  $(test_oid t3) ! 3:  $(test_oid c3) s/11/B/
@@ -285,12 +285,12 @@ test_expect_success 'changed commit with sm config' '
 	     +B
 	      13
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'renamed file' '
 	git range-diff --no-color --submodule=log topic...renamed-file >actual &&
-	sed s/Z/\ /g >expected <<-EOF &&
+	sed s/Z/\ /g >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid n1) s/5/A/
 	2:  $(test_oid t2) ! 2:  $(test_oid n2) s/4/A/
 	    @@ Metadata
@@ -330,12 +330,12 @@ test_expect_success 'renamed file' '
 	    Z 10
 	    Z B
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'file with mode only change' '
 	git range-diff --no-color --submodule=log topic...mode-only-change >actual &&
-	sed s/Z/\ /g >expected <<-EOF &&
+	sed s/Z/\ /g >expect <<-EOF &&
 	1:  fccce22 ! 1:  4d39cb3 s/4/A/
 	    @@ Metadata
 	    ZAuthor: Thomas Rast <trast@inf.ethz.ch>
@@ -370,12 +370,12 @@ test_expect_success 'file with mode only change' '
 	    + ## other-file (mode change 100644 => 100755) ##
 	3:  a63e992 = 3:  4c1e0f5 s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'file added and later removed' '
 	git range-diff --no-color --submodule=log topic...added-removed >actual &&
-	sed s/Z/\ /g >expected <<-EOF &&
+	sed s/Z/\ /g >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid s1) s/5/A/
 	2:  $(test_oid t2) ! 2:  $(test_oid s2) s/4/A/
 	    @@ Metadata
@@ -411,7 +411,7 @@ test_expect_success 'file added and later removed' '
 	    + ## new-file (deleted) ##
 	4:  $(test_oid t4) = 4:  $(test_oid s4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'no commits on one side' '
@@ -421,7 +421,7 @@ test_expect_success 'no commits on one side' '
 
 test_expect_success 'changed message' '
 	git range-diff --no-color topic...changed-message >actual &&
-	sed s/Z/\ /g >expected <<-EOF &&
+	sed s/Z/\ /g >expect <<-EOF &&
 	1:  $(test_oid t1) = 1:  $(test_oid m1) s/5/A/
 	2:  $(test_oid t2) ! 2:  $(test_oid m2) s/4/A/
 	    @@ Metadata
@@ -436,7 +436,7 @@ test_expect_success 'changed message' '
 	3:  $(test_oid t3) = 3:  $(test_oid m3) s/11/B/
 	4:  $(test_oid t4) = 4:  $(test_oid m4) s/12/B/
 	EOF
-	test_cmp expected actual
+	test_cmp expect actual
 '
 
 test_expect_success 'dual-coloring' '
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 07/10] t3206: range-diff compares logs with commit notes

[Denton Liu]

The test suite had a blindspot where it did not check the behavior of
range-diff and format-patch when notes were present. Cover this
blindspot.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3206-range-diff.sh | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index f654aed54f..19ba644933 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -505,4 +505,56 @@ test_expect_success 'range-diff overrides diff.noprefix internally' '
 	git -c diff.noprefix=true range-diff HEAD^...
 '
 
+test_expect_success 'range-diff compares notes by default' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git range-diff --no-color master..topic master..unmodified \
+		>actual &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
+	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
+	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
+	4:  $(test_oid t4) ! 4:  $(test_oid u4) s/12/B/
+	    @@ Metadata
+	    Z
+	    Z ## Commit message ##
+	    Z    s/12/B/
+	    -    topic note
+	    +    unmodified note
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'format-patch --range-diff compares notes by default' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git format-patch --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	    @@ Metadata
+	    Z
+	    Z ## Commit message ##
+	    Z    s/12/B/
+	    -    topic note
+	    +    unmodified note
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	sed "/@@ Metadata/,/@@ file: A/!d" 0000-* >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 08/10] range-diff: output `## Notes ##` header

[Denton Liu]

When notes were included in the output of range-diff, they were just
mashed together with the rest of the commit message. As a result, users
wouldn't be able to clearly distinguish where the commit message ended
and where the notes started.

Output a `## Notes ##` header when notes are detected so that notes can
be compared more clearly.

Note that we handle case of `Notes (<ref>): -> ## Notes (<ref>) ##` with
this code as well. We can't test this in this patch, however, since
there is currently no way to pass along different notes refs to `git
log`. This will be fixed in a future patch.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 range-diff.c          |  6 ++++++
 t/t3206-range-diff.sh | 14 +++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index 7fed5a3b4b..623397221d 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -144,6 +144,12 @@ static int read_patches(const char *range, struct string_list *list)
 				strbuf_addstr(&buf, line);
 				strbuf_addstr(&buf, "\n\n");
 				strbuf_addstr(&buf, " ## Commit message ##\n");
+			} else if (starts_with(line, "Notes") &&
+				   line[strlen(line) - 1] == ':') {
+				strbuf_addstr(&buf, "\n\n");
+				/* strip the trailing colon */
+				strbuf_addf(&buf, " ## %.*s ##\n",
+					    (int)(strlen(line) - 1), line);
 			} else if (starts_with(line, "    ")) {
 				p = line + len - 2;
 				while (isspace(*p) && p >= line)
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 19ba644933..b936c16dd1 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -516,10 +516,10 @@ test_expect_success 'range-diff compares notes by default' '
 	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
 	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
 	4:  $(test_oid t4) ! 4:  $(test_oid u4) s/12/B/
-	    @@ Metadata
+	    @@ Commit message
 	    Z
-	    Z ## Commit message ##
-	    Z    s/12/B/
+	    Z
+	    Z ## Notes ##
 	    -    topic note
 	    +    unmodified note
 	    Z
@@ -543,17 +543,17 @@ test_expect_success 'format-patch --range-diff compares notes by default' '
 	grep "= 3: .* s/11/B" 0000-* &&
 	grep "! 4: .* s/12/B" 0000-* &&
 	sed s/Z/\ /g >expect <<-EOF &&
-	    @@ Metadata
+	    @@ Commit message
 	    Z
-	    Z ## Commit message ##
-	    Z    s/12/B/
+	    Z
+	    Z ## Notes ##
 	    -    topic note
 	    +    unmodified note
 	    Z
 	    Z ## file ##
 	    Z@@ file: A
 	EOF
-	sed "/@@ Metadata/,/@@ file: A/!d" 0000-* >actual &&
+	sed "/@@ Commit message/,/@@ file: A/!d" 0000-* >actual &&
 	test_cmp expect actual
 '
 
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 09/10] range-diff: pass through --notes to `git log`

[Denton Liu]

When a commit being range-diff'd has a note attached to it, the note
will be compared as well. However, if a user has multiple notes refs or
if they want to suppress notes from being printed, there is currently no
way to do this.

Pass through `--[no-]notes[=<ref>]` to the `git log` call so that this
option is customizable.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-range-diff.txt |  6 +++-
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  6 +++-
 log-tree.c                       |  2 +-
 range-diff.c                     | 15 ++++++----
 range-diff.h                     |  4 ++-
 t/t3206-range-diff.sh            | 47 ++++++++++++++++++++++++++++++++
 7 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index 8a6ea2c6c5..a2c25f4490 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -57,6 +57,10 @@ to revert to color all lines according to the outer diff markers
 	See the ``Algorithm`` section below for an explanation why this is
 	needed.
 
+--[no-]notes[=<ref>]::
+	This flag is passed to the `git log` program
+	(see linkgit:git-log[1]) that generates the patches.
+
 <range1> <range2>::
 	Compare the commits specified by the two ranges, where
 	`<range1>` is considered an older version of `<range2>`.
@@ -75,7 +79,7 @@ to revert to color all lines according to the outer diff markers
 linkgit:git-diff[1]), most notably the `--color=[<when>]` and
 `--no-color` options. These options are used when generating the "diff
 between patches", i.e. to compare the author, commit message and diff of
-corresponding old/new commits. There is currently no means to tweak the
+corresponding old/new commits. There is currently no means to tweak most of the
 diff options passed to `git log` when generating those patches.
 
 OUTPUT STABILITY
diff --git a/builtin/log.c b/builtin/log.c
index a26f223ab4..047ac4594d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1189,7 +1189,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		diff_setup_done(&opts);
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &opts);
+				rev->creation_factor, 1, &opts, NULL);
 	}
 }
 
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 9202e75544..98acf3533e 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -15,12 +15,16 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 {
 	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
 	struct diff_options diffopt = { NULL };
+	struct argv_array other_arg = ARGV_ARRAY_INIT;
 	int simple_color = -1;
 	struct option range_diff_options[] = {
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
 			    N_("use simple diff colors")),
+		OPT_PASSTHRU_ARGV(0, "notes", &other_arg,
+				  N_("notes"), N_("passed to 'git log'"),
+				  PARSE_OPT_OPTARG),
 		OPT_END()
 	};
 	struct option *options;
@@ -78,7 +82,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	FREE_AND_NULL(options);
 
 	res = show_range_diff(range1.buf, range2.buf, creation_factor,
-			      simple_color < 1, &diffopt);
+			      simple_color < 1, &diffopt, &other_arg);
 
 	strbuf_release(&range1);
 	strbuf_release(&range2);
diff --git a/log-tree.c b/log-tree.c
index 923a299e70..151e12f415 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -770,7 +770,7 @@ void show_log(struct rev_info *opt)
 		opts.use_color = opt->diffopt.use_color;
 		diff_setup_done(&opts);
 		show_range_diff(opt->rdiff1, opt->rdiff2,
-				opt->creation_factor, 1, &opts);
+				opt->creation_factor, 1, &opts, NULL);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}
diff --git a/range-diff.c b/range-diff.c
index 623397221d..f56b4012a2 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -40,7 +40,8 @@ static size_t find_end_of_line(char *buffer, unsigned long size)
  * Reads the patches into a string list, with the `util` field being populated
  * as struct object_id (will need to be free()d).
  */
-static int read_patches(const char *range, struct string_list *list)
+static int read_patches(const char *range, struct string_list *list,
+			struct argv_array *other_arg)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
@@ -61,8 +62,11 @@ static int read_patches(const char *range, struct string_list *list)
 			"--output-indicator-new=>",
 			"--output-indicator-old=<",
 			"--output-indicator-context=#",
-			"--no-abbrev-commit", range,
+			"--no-abbrev-commit",
 			NULL);
+	if (other_arg)
+		argv_array_pushv(&cp.args, other_arg->argv);
+	argv_array_push(&cp.args, range);
 	cp.out = -1;
 	cp.no_stdin = 1;
 	cp.git_cmd = 1;
@@ -502,16 +506,17 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
 
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
-		    struct diff_options *diffopt)
+		    struct diff_options *diffopt,
+		    struct argv_array *other_arg)
 {
 	int res = 0;
 
 	struct string_list branch1 = STRING_LIST_INIT_DUP;
 	struct string_list branch2 = STRING_LIST_INIT_DUP;
 
-	if (read_patches(range1, &branch1))
+	if (read_patches(range1, &branch1, other_arg))
 		res = error(_("could not parse log for '%s'"), range1);
-	if (!res && read_patches(range2, &branch2))
+	if (!res && read_patches(range2, &branch2, other_arg))
 		res = error(_("could not parse log for '%s'"), range2);
 
 	if (!res) {
diff --git a/range-diff.h b/range-diff.h
index 08a50b6e98..7d918ab9ed 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -2,6 +2,7 @@
 #define RANGE_DIFF_H
 
 #include "diff.h"
+#include "argv-array.h"
 
 #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
 
@@ -12,6 +13,7 @@
  */
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
-		    struct diff_options *diffopt);
+		    struct diff_options *diffopt,
+		    struct argv_array *other_arg);
 
 #endif
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index b936c16dd1..521b4a83ec 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -529,6 +529,53 @@ test_expect_success 'range-diff compares notes by default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'range-diff with --no-notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git range-diff --no-color --no-notes master..topic master..unmodified \
+		>actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
+	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
+	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
+	4:  $(test_oid t4) = 4:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'range-diff with multiple --notes' '
+	git notes --ref=note1 add -m "topic note1" topic &&
+	git notes --ref=note1 add -m "unmodified note1" unmodified &&
+	test_when_finished git notes --ref=note1 remove topic unmodified &&
+	git notes --ref=note2 add -m "topic note2" topic &&
+	git notes --ref=note2 add -m "unmodified note2" unmodified &&
+	test_when_finished git notes --ref=note2 remove topic unmodified &&
+	git range-diff --no-color --notes=note1 --notes=note2 master..topic master..unmodified \
+		>actual &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	1:  $(test_oid t1) = 1:  $(test_oid u1) s/5/A/
+	2:  $(test_oid t2) = 2:  $(test_oid u2) s/4/A/
+	3:  $(test_oid t3) = 3:  $(test_oid u3) s/11/B/
+	4:  $(test_oid t4) ! 4:  $(test_oid u4) s/12/B/
+	    @@ Commit message
+	    Z
+	    Z
+	    Z ## Notes (note1) ##
+	    -    topic note1
+	    +    unmodified note1
+	    Z
+	    Z
+	    Z ## Notes (note2) ##
+	    -    topic note2
+	    +    unmodified note2
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'format-patch --range-diff compares notes by default' '
 	git notes add -m "topic note" topic &&
 	git notes add -m "unmodified note" unmodified &&
-- 
2.24.0.450.g7a9a4598a9

[PATCH v3 10/10] format-patch: pass notes configuration to range-diff

[Denton Liu]

Since format-patch accepts `--[no-]notes`, one would expect the
range-diff generated to also respect the setting. Unfortunately, the
range-diff we currently generate only uses the default option (which
always outputs default notes, even when notes are not being used
elsewhere).

Pass the notes configuration to range-diff so that it can honor it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/log.c         |  24 +++++++++-
 t/t3206-range-diff.sh | 101 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 123 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 047ac4594d..e192f219d9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1111,6 +1111,25 @@ static void prepare_cover_text(struct pretty_print_context *pp,
 	strbuf_release(&subject_sb);
 }
 
+static int get_notes_refs(struct string_list_item *item, void *arg)
+{
+	argv_array_pushf(arg, "--notes=%s", item->string);
+	return 0;
+}
+
+static void get_notes_args(struct argv_array *arg, struct rev_info *rev)
+{
+	if (!rev->show_notes) {
+		argv_array_push(arg, "--no-notes");
+	} else if (rev->notes_opt.use_default_notes > 0 ||
+		   (rev->notes_opt.use_default_notes == -1 &&
+		    !rev->notes_opt.extra_notes_refs.nr)) {
+		argv_array_push(arg, "--notes");
+	} else {
+		for_each_string_list(&rev->notes_opt.extra_notes_refs, get_notes_refs, arg);
+	}
+}
+
 static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
@@ -1183,13 +1202,16 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		 * can be added later if deemed desirable.
 		 */
 		struct diff_options opts;
+		struct argv_array other_arg = ARGV_ARRAY_INIT;
 		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);
+		get_notes_args(&other_arg, rev);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &opts, NULL);
+				rev->creation_factor, 1, &opts, &other_arg);
+		argv_array_clear(&other_arg);
 	}
 }
 
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 521b4a83ec..ec2b456dbb 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -576,7 +576,7 @@ test_expect_success 'range-diff with multiple --notes' '
 	test_cmp expect actual
 '
 
-test_expect_success 'format-patch --range-diff compares notes by default' '
+test_expect_success 'format-patch --range-diff does not compare notes by default' '
 	git notes add -m "topic note" topic &&
 	git notes add -m "unmodified note" unmodified &&
 	test_when_finished git notes remove topic unmodified &&
@@ -588,6 +588,40 @@ test_expect_success 'format-patch --range-diff compares notes by default' '
 	grep "= 1: .* s/5/A" 0000-* &&
 	grep "= 2: .* s/4/A" 0000-* &&
 	grep "= 3: .* s/11/B" 0000-* &&
+	grep "= 4: .* s/12/B" 0000-* &&
+	! grep "Notes" 0000-* &&
+	! grep "note" 0000-*
+'
+
+test_expect_success 'format-patch --range-diff with --no-notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git format-patch --no-notes --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
+	! grep "Notes" 0000-* &&
+	! grep "note" 0000-*
+'
+
+test_expect_success 'format-patch --range-diff with --notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	git format-patch --notes --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
 	sed s/Z/\ /g >expect <<-EOF &&
 	    @@ Commit message
@@ -604,4 +638,69 @@ test_expect_success 'format-patch --range-diff compares notes by default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'format-patch --range-diff with --notes' '
+	git notes add -m "topic note" topic &&
+	git notes add -m "unmodified note" unmodified &&
+	test_when_finished git notes remove topic unmodified &&
+	test_config format.notes true &&
+	git format-patch --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	    @@ Commit message
+	    Z
+	    Z
+	    Z ## Notes ##
+	    -    topic note
+	    +    unmodified note
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	sed "/@@ Commit message/,/@@ file: A/!d" 0000-* >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'format-patch --range-diff with multiple notes' '
+	git notes --ref=note1 add -m "topic note1" topic &&
+	git notes --ref=note1 add -m "unmodified note1" unmodified &&
+	test_when_finished git notes --ref=note1 remove topic unmodified &&
+	git notes --ref=note2 add -m "topic note2" topic &&
+	git notes --ref=note2 add -m "unmodified note2" unmodified &&
+	test_when_finished git notes --ref=note2 remove topic unmodified &&
+	git format-patch --notes=note1 --notes=note2 --cover-letter --range-diff=$prev \
+		master..unmodified >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-* &&
+	sed s/Z/\ /g >expect <<-EOF &&
+	    @@ Commit message
+	    Z
+	    Z
+	    Z ## Notes (note1) ##
+	    -    topic note1
+	    +    unmodified note1
+	    Z
+	    Z
+	    Z ## Notes (note2) ##
+	    -    topic note2
+	    +    unmodified note2
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	EOF
+	sed "/@@ Commit message/,/@@ file: A/!d" 0000-* >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.24.0.450.g7a9a4598a9

Re: [PATCH v2 7/8] range-diff: passthrough --[no-]notes to `git log`

[Eric Sunshine]

On Wed, Nov 20, 2019 at 2:13 PM Denton Liu <liu.denton@gmail.com> wrote:
> On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote:
> > Denton Liu <liu.denton@gmail.com> writes:
> > > +#include "argv-array.h"
> > >
> > >  int show_range_diff(const char *range1, const char *range2,
> > >                 int creation_factor, int dual_color,
> > > -               struct diff_options *diffopt);
> > > +               struct diff_options *diffopt,
> > > +               struct argv_array *other_arg);
> >
> > I thought a mere use of "pointer to a struct" did not have to bring
> > the definition of the struct into the picture?  In other words,
> > wouldn't it be fine to leave the other_arg a pointer to an opaque
> > structure by not including "argv-array.h" in this file?
>
> Without including "argv-array.h", we get the following hdr-check error:
>
>         $ make range-diff.hco
>         In file included from range-diff.hcc:2:
>         ./range-diff.h:16:14: error: declaration of 'struct argv_array' will not be visible outside of this function [-Werror,-Wvisibility]
>                             struct argv_array *other_arg);

Did you forward-declare 'argv_array' in range-diff.h? That is, rather than:

    #include "argv-array.h"

instead say:

    struct argv_array;

which tells the compiler that the type exists, thus allowing you to
deal with pointers to the struct, as long as you don't try to access
any of the struct's members in code which has seen only the forward
declaration. You would still need to #include "argv-array.h" in
range-diff.c, though.

Re: [PATCH v2 7/8] range-diff: passthrough --[no-]notes to `git log`

[Denton Liu]

Hi Eric,

On Thu, Nov 21, 2019 at 07:43:18AM -0500, Eric Sunshine wrote:
> On Wed, Nov 20, 2019 at 2:13 PM Denton Liu <liu.denton@gmail.com> wrote:
> > On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote:
> > > Denton Liu <liu.denton@gmail.com> writes:
> > > > +#include "argv-array.h"
> > > >
> > > >  int show_range_diff(const char *range1, const char *range2,
> > > >                 int creation_factor, int dual_color,
> > > > -               struct diff_options *diffopt);
> > > > +               struct diff_options *diffopt,
> > > > +               struct argv_array *other_arg);
> > >
> > > I thought a mere use of "pointer to a struct" did not have to bring
> > > the definition of the struct into the picture?  In other words,
> > > wouldn't it be fine to leave the other_arg a pointer to an opaque
> > > structure by not including "argv-array.h" in this file?
> >
> > Without including "argv-array.h", we get the following hdr-check error:
> >
> >         $ make range-diff.hco
> >         In file included from range-diff.hcc:2:
> >         ./range-diff.h:16:14: error: declaration of 'struct argv_array' will not be visible outside of this function [-Werror,-Wvisibility]
> >                             struct argv_array *other_arg);
> 
> Did you forward-declare 'argv_array' in range-diff.h? That is, rather than:
> 
>     #include "argv-array.h"
> 
> instead say:
> 
>     struct argv_array;

*facepalm* Damn, sorry for the brainfart. I completely forgot to do
that.

That being said, I'd prefer to just include the header for consistency
since we already have the #include "diff.h" up there. Or alternatively,
I could write a patch to change both into forward-declarations.

Personally, I prefer to avoid forward-declarations whenever possible
because it adds duplication. If we have a strong preference for one or
the other, should we include it in the CodingGuidelines?

Thanks,

Denton

> 
> which tells the compiler that the type exists, thus allowing you to
> deal with pointers to the struct, as long as you don't try to access
> any of the struct's members in code which has seen only the forward
> declaration. You would still need to #include "argv-array.h" in
> range-diff.c, though.