[PATCH] diff-tree.c: load notes machinery when required

[PATCH] diff-tree.c: load notes machinery when required

[Taylor Blau]

Since its introduction in 7249e91 (revision.c: support --notes
command-line option, 2011-03-29), combining '--notes' with any option
that causes us to format notes (e.g., '--pretty', '--format="%N"', etc)
results in a failed assertion at runtime.

  $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes
  commit 8f3d9f354286745c751374f5f1fcafee6b3f3136
  git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.
  Aborted

This failure is due to diff-tree not calling 'load_display_notes' to
initialize the notes machinery.

Ordinarily, this failure isn't triggered, because it requires passing
both '--notes' and another of the above mentioned options. In the case
of '--pretty', for example, we set 'opt->verbose_header', causing
'show_log()' to eventually call 'format_display_notes()', which expects
a non-NULL 'display_note_trees'.

Without initializing the notes machinery, 'display_note_trees' remains
NULL, and thus triggers an assertion failure.

Fix this by initializing the notes machinery after parsing our options,
and harden this behavior against regression with a test in t4013. (Note
that the added ref in this test requires updating two unrelated tests
which use 'log --all', and thus need to learn about the new refs).

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
This is the remainder of the fix that I started earlier today, with some
additional suggestions from Peff incorporated.

 builtin/diff-tree.c                          |  9 +++++++++
 t/t4013-diff-various.sh                      | 11 +++++++++++
 t/t4013/diff.diff-tree_--format=%N_note      |  6 ++++++
 t/t4013/diff.diff-tree_--pretty_--notes_note | 12 ++++++++++++
 t/t4013/diff.log_--decorate=full_--all       | 15 +++++++++++++++
 t/t4013/diff.log_--decorate_--all            | 15 +++++++++++++++
 6 files changed, 68 insertions(+)
 create mode 100644 t/t4013/diff.diff-tree_--format=%N_note
 create mode 100644 t/t4013/diff.diff-tree_--pretty_--notes_note

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index cb9ea79367..11551a20cc 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -109,6 +109,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct object *tree1, *tree2;
 	static struct rev_info *opt = &log_tree_opt;
 	struct setup_revision_opt s_r_opt;
+	struct userformat_want w;
 	int read_stdin = 0;

 	if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -127,6 +128,14 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	precompose_argv(argc, argv);
 	argc = setup_revisions(argc, argv, opt, &s_r_opt);

+	memset(&w, 0, sizeof(w));
+	userformat_find_requirements(NULL, &w);
+
+	if (!opt->show_notes_given && (!opt->pretty_given || w.notes))
+		opt->show_notes = 1;
+	if (opt->show_notes)
+		load_display_notes(&opt->notes_opt);
+
 	while (--argc > 0) {
 		const char *arg = *++argv;

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index dde3f11fec..4263b95ca6 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -95,6 +95,15 @@ test_expect_success setup '
 	git commit -m "update mode" &&
 	git checkout -f master &&

+	GIT_AUTHOR_DATE="2006-06-26 00:06:00 +0000" &&
+	GIT_COMMITTER_DATE="2006-06-26 00:06:00 +0000" &&
+	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
+	git checkout -b note initial &&
+	git update-index --chmod=+x file2 &&
+	git commit -m "update mode (file2)" &&
+	git notes add -m "note" &&
+	git checkout -f master &&
+
 	# Same merge as master, but with parents reversed. Hide it in a
 	# pseudo-ref to avoid impacting tests with --all.
 	commit=$(echo reverse |
@@ -398,6 +407,8 @@ diff --no-index --raw --no-abbrev dir2 dir

 diff-tree --pretty --root --stat --compact-summary initial
 diff-tree --pretty -R --root --stat --compact-summary initial
+diff-tree --pretty --notes note
+diff-tree --format=%N note
 diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
diff --git a/t/t4013/diff.diff-tree_--format=%N_note b/t/t4013/diff.diff-tree_--format=%N_note
new file mode 100644
index 0000000000..93042ed539
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--format=%N_note
@@ -0,0 +1,6 @@
+$ git diff-tree --format=%N note
+note
+
+
+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file2
+$
diff --git a/t/t4013/diff.diff-tree_--pretty_--notes_note b/t/t4013/diff.diff-tree_--pretty_--notes_note
new file mode 100644
index 0000000000..4d0bde601c
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_--notes_note
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty --notes note
+commit a6f364368ca320bc5a92e18912e16fa6b3dff598
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode (file2)
+
+Notes:
+    note
+
+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file2
+$
diff --git a/t/t4013/diff.log_--decorate=full_--all b/t/t4013/diff.log_--decorate=full_--all
index 2afe91f116..3f9b872ece 100644
--- a/t/t4013/diff.log_--decorate=full_--all
+++ b/t/t4013/diff.log_--decorate=full_--all
@@ -5,12 +5,27 @@ Date:   Mon Jun 26 00:06:00 2006 +0000

     update mode

+commit a6f364368ca320bc5a92e18912e16fa6b3dff598 (refs/heads/note)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode (file2)
+
+Notes:
+    note
+
 commit cd4e72fd96faed3f0ba949dc42967430374e2290 (refs/heads/rearrange)
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:06:00 2006 +0000

     Rearranged lines in dir/sub

+commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 (refs/notes/commits)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    Notes added by 'git notes add'
+
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (HEAD -> refs/heads/master)
 Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
diff --git a/t/t4013/diff.log_--decorate_--all b/t/t4013/diff.log_--decorate_--all
index d0f308ab2b..f5e20e1e14 100644
--- a/t/t4013/diff.log_--decorate_--all
+++ b/t/t4013/diff.log_--decorate_--all
@@ -5,12 +5,27 @@ Date:   Mon Jun 26 00:06:00 2006 +0000

     update mode

+commit a6f364368ca320bc5a92e18912e16fa6b3dff598 (note)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode (file2)
+
+Notes:
+    note
+
 commit cd4e72fd96faed3f0ba949dc42967430374e2290 (rearrange)
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:06:00 2006 +0000

     Rearranged lines in dir/sub

+commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 (refs/notes/commits)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    Notes added by 'git notes add'
+
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (HEAD -> master)
 Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
--
2.26.0.121.gefe3874640.dirty

Re: [PATCH] diff-tree.c: load notes machinery when required

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Since its introduction in 7249e91 (revision.c: support --notes
> command-line option, 2011-03-29), combining '--notes' with any option
> that causes us to format notes (e.g., '--pretty', '--format="%N"', etc)
> results in a failed assertion at runtime.
>
>   $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes
>   commit 8f3d9f354286745c751374f5f1fcafee6b3f3136
>   git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.
>   Aborted
>
> This failure is due to diff-tree not calling 'load_display_notes' to
> initialize the notes machinery.
>
> Ordinarily, this failure isn't triggered, because it requires passing
> both '--notes' and another of the above mentioned options. In the case
> of '--pretty', for example, we set 'opt->verbose_header', causing
> 'show_log()' to eventually call 'format_display_notes()', which expects
> a non-NULL 'display_note_trees'.
>
> Without initializing the notes machinery, 'display_note_trees' remains
> NULL, and thus triggers an assertion failure.
>
> Fix this by initializing the notes machinery after parsing our options,
> and harden this behavior against regression with a test in t4013. (Note
> that the added ref in this test requires updating two unrelated tests
> which use 'log --all', and thus need to learn about the new refs).
>
> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> This is the remainder of the fix that I started earlier today, with some
> additional suggestions from Peff incorporated.

Yeah, with the auto-detection of userformat requirement, the fix
looks good to me, too.

Will queue.  Thanks.

>
>  builtin/diff-tree.c                          |  9 +++++++++
>  t/t4013-diff-various.sh                      | 11 +++++++++++
>  t/t4013/diff.diff-tree_--format=%N_note      |  6 ++++++
>  t/t4013/diff.diff-tree_--pretty_--notes_note | 12 ++++++++++++
>  t/t4013/diff.log_--decorate=full_--all       | 15 +++++++++++++++
>  t/t4013/diff.log_--decorate_--all            | 15 +++++++++++++++
>  6 files changed, 68 insertions(+)
>  create mode 100644 t/t4013/diff.diff-tree_--format=%N_note
>  create mode 100644 t/t4013/diff.diff-tree_--pretty_--notes_note
>
> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> index cb9ea79367..11551a20cc 100644
> --- a/builtin/diff-tree.c
> +++ b/builtin/diff-tree.c
> @@ -109,6 +109,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
>  	struct object *tree1, *tree2;
>  	static struct rev_info *opt = &log_tree_opt;
>  	struct setup_revision_opt s_r_opt;
> +	struct userformat_want w;
>  	int read_stdin = 0;
>
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
> @@ -127,6 +128,14 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
>  	precompose_argv(argc, argv);
>  	argc = setup_revisions(argc, argv, opt, &s_r_opt);
>
> +	memset(&w, 0, sizeof(w));
> +	userformat_find_requirements(NULL, &w);
> +
> +	if (!opt->show_notes_given && (!opt->pretty_given || w.notes))
> +		opt->show_notes = 1;
> +	if (opt->show_notes)
> +		load_display_notes(&opt->notes_opt);
> +
>  	while (--argc > 0) {
>  		const char *arg = *++argv;
>
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index dde3f11fec..4263b95ca6 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -95,6 +95,15 @@ test_expect_success setup '
>  	git commit -m "update mode" &&
>  	git checkout -f master &&
>
> +	GIT_AUTHOR_DATE="2006-06-26 00:06:00 +0000" &&
> +	GIT_COMMITTER_DATE="2006-06-26 00:06:00 +0000" &&
> +	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
> +	git checkout -b note initial &&
> +	git update-index --chmod=+x file2 &&
> +	git commit -m "update mode (file2)" &&
> +	git notes add -m "note" &&
> +	git checkout -f master &&
> +
>  	# Same merge as master, but with parents reversed. Hide it in a
>  	# pseudo-ref to avoid impacting tests with --all.
>  	commit=$(echo reverse |
> @@ -398,6 +407,8 @@ diff --no-index --raw --no-abbrev dir2 dir
>
>  diff-tree --pretty --root --stat --compact-summary initial
>  diff-tree --pretty -R --root --stat --compact-summary initial
> +diff-tree --pretty --notes note
> +diff-tree --format=%N note
>  diff-tree --stat --compact-summary initial mode
>  diff-tree -R --stat --compact-summary initial mode
>  EOF
> diff --git a/t/t4013/diff.diff-tree_--format=%N_note b/t/t4013/diff.diff-tree_--format=%N_note
> new file mode 100644
> index 0000000000..93042ed539
> --- /dev/null
> +++ b/t/t4013/diff.diff-tree_--format=%N_note
> @@ -0,0 +1,6 @@
> +$ git diff-tree --format=%N note
> +note
> +
> +
> +:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file2
> +$
> diff --git a/t/t4013/diff.diff-tree_--pretty_--notes_note b/t/t4013/diff.diff-tree_--pretty_--notes_note
> new file mode 100644
> index 0000000000..4d0bde601c
> --- /dev/null
> +++ b/t/t4013/diff.diff-tree_--pretty_--notes_note
> @@ -0,0 +1,12 @@
> +$ git diff-tree --pretty --notes note
> +commit a6f364368ca320bc5a92e18912e16fa6b3dff598
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:06:00 2006 +0000
> +
> +    update mode (file2)
> +
> +Notes:
> +    note
> +
> +:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file2
> +$
> diff --git a/t/t4013/diff.log_--decorate=full_--all b/t/t4013/diff.log_--decorate=full_--all
> index 2afe91f116..3f9b872ece 100644
> --- a/t/t4013/diff.log_--decorate=full_--all
> +++ b/t/t4013/diff.log_--decorate=full_--all
> @@ -5,12 +5,27 @@ Date:   Mon Jun 26 00:06:00 2006 +0000
>
>      update mode
>
> +commit a6f364368ca320bc5a92e18912e16fa6b3dff598 (refs/heads/note)
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:06:00 2006 +0000
> +
> +    update mode (file2)
> +
> +Notes:
> +    note
> +
>  commit cd4e72fd96faed3f0ba949dc42967430374e2290 (refs/heads/rearrange)
>  Author: A U Thor <author@example.com>
>  Date:   Mon Jun 26 00:06:00 2006 +0000
>
>      Rearranged lines in dir/sub
>
> +commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 (refs/notes/commits)
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:06:00 2006 +0000
> +
> +    Notes added by 'git notes add'
> +
>  commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (HEAD -> refs/heads/master)
>  Merge: 9a6d494 c7a2ab9
>  Author: A U Thor <author@example.com>
> diff --git a/t/t4013/diff.log_--decorate_--all b/t/t4013/diff.log_--decorate_--all
> index d0f308ab2b..f5e20e1e14 100644
> --- a/t/t4013/diff.log_--decorate_--all
> +++ b/t/t4013/diff.log_--decorate_--all
> @@ -5,12 +5,27 @@ Date:   Mon Jun 26 00:06:00 2006 +0000
>
>      update mode
>
> +commit a6f364368ca320bc5a92e18912e16fa6b3dff598 (note)
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:06:00 2006 +0000
> +
> +    update mode (file2)
> +
> +Notes:
> +    note
> +
>  commit cd4e72fd96faed3f0ba949dc42967430374e2290 (rearrange)
>  Author: A U Thor <author@example.com>
>  Date:   Mon Jun 26 00:06:00 2006 +0000
>
>      Rearranged lines in dir/sub
>
> +commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 (refs/notes/commits)
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:06:00 2006 +0000
> +
> +    Notes added by 'git notes add'
> +
>  commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (HEAD -> master)
>  Merge: 9a6d494 c7a2ab9
>  Author: A U Thor <author@example.com>
> --
> 2.26.0.121.gefe3874640.dirty

Re: [PATCH] diff-tree.c: load notes machinery when required

[Jeff King]

On Wed, Apr 15, 2020 at 06:37:38PM -0600, Taylor Blau wrote:

> Since its introduction in 7249e91 (revision.c: support --notes
> command-line option, 2011-03-29), combining '--notes' with any option
> that causes us to format notes (e.g., '--pretty', '--format="%N"', etc)
> results in a failed assertion at runtime.
> 
>   $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes
>   commit 8f3d9f354286745c751374f5f1fcafee6b3f3136
>   git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.
>   Aborted
> 
> This failure is due to diff-tree not calling 'load_display_notes' to
> initialize the notes machinery.
> 
> Ordinarily, this failure isn't triggered, because it requires passing
> both '--notes' and another of the above mentioned options. In the case
> of '--pretty', for example, we set 'opt->verbose_header', causing
> 'show_log()' to eventually call 'format_display_notes()', which expects
> a non-NULL 'display_note_trees'.

This looks much better. One question, though...

> @@ -127,6 +128,14 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
>  	precompose_argv(argc, argv);
>  	argc = setup_revisions(argc, argv, opt, &s_r_opt);
> 
> +	memset(&w, 0, sizeof(w));
> +	userformat_find_requirements(NULL, &w);
> +
> +	if (!opt->show_notes_given && (!opt->pretty_given || w.notes))
> +		opt->show_notes = 1;
> +	if (opt->show_notes)
> +		load_display_notes(&opt->notes_opt);
> +

I assume these lines were lifted from builtin/log.c. But what is
pretty_given doing here?

In git-log, it's turning on notes for the default case when no format is
given. But here, if no format has been given we _wouldn't_ want to show
notes.

I think it's relatively harmless, since our default format is not to do
any pretty-printing, and therefore we wouldn't look at the notes. But it
does cause us to call load_display_notes() when we don't need to. I
think that conditional can be simplified to just:

  if (!opt->show_notes_given && w.notes)
	opt->show_notes = 1;

> Fix this by initializing the notes machinery after parsing our options,
> and harden this behavior against regression with a test in t4013. (Note
> that the added ref in this test requires updating two unrelated tests
> which use 'log --all', and thus need to learn about the new refs).

This makes the test update rather hard to follow, but I don't think
there's an easy way around it. I wondered if we could insert and remove
our notes just for our tests, but it's hard to do with the big while
loop in that script.

I also thought it might not be that bad to just add a few tests at the
end, but there are some complications (like removing sha1s from the
output; easily done with a custom format, but the point is to test
--pretty).

So what you have is probably the most sensible thing (and the new commit
would make it easier to test other commands around notes later, if we
chose to).

> @@ -398,6 +407,8 @@ diff --no-index --raw --no-abbrev dir2 dir
> 
>  diff-tree --pretty --root --stat --compact-summary initial
>  diff-tree --pretty -R --root --stat --compact-summary initial
> +diff-tree --pretty --notes note
> +diff-tree --format=%N note
>  diff-tree --stat --compact-summary initial mode
>  diff-tree -R --stat --compact-summary initial mode

Perhaps worth testing that:

  diff-tree --pretty note

does not print any notes by default?

-Peff

Re: [PATCH] diff-tree.c: load notes machinery when required

[Taylor Blau]

On Thu, Apr 16, 2020 at 12:56:30AM -0400, Jeff King wrote:
> On Wed, Apr 15, 2020 at 06:37:38PM -0600, Taylor Blau wrote:
>
> > Since its introduction in 7249e91 (revision.c: support --notes
> > command-line option, 2011-03-29), combining '--notes' with any option
> > that causes us to format notes (e.g., '--pretty', '--format="%N"', etc)
> > results in a failed assertion at runtime.
> >
> >   $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes
> >   commit 8f3d9f354286745c751374f5f1fcafee6b3f3136
> >   git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.
> >   Aborted
> >
> > This failure is due to diff-tree not calling 'load_display_notes' to
> > initialize the notes machinery.
> >
> > Ordinarily, this failure isn't triggered, because it requires passing
> > both '--notes' and another of the above mentioned options. In the case
> > of '--pretty', for example, we set 'opt->verbose_header', causing
> > 'show_log()' to eventually call 'format_display_notes()', which expects
> > a non-NULL 'display_note_trees'.
>
> This looks much better. One question, though...
>
> > @@ -127,6 +128,14 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
> >  	precompose_argv(argc, argv);
> >  	argc = setup_revisions(argc, argv, opt, &s_r_opt);
> >
> > +	memset(&w, 0, sizeof(w));
> > +	userformat_find_requirements(NULL, &w);
> > +
> > +	if (!opt->show_notes_given && (!opt->pretty_given || w.notes))
> > +		opt->show_notes = 1;
> > +	if (opt->show_notes)
> > +		load_display_notes(&opt->notes_opt);
> > +
>
> I assume these lines were lifted from builtin/log.c. But what is
> pretty_given doing here?
>
> In git-log, it's turning on notes for the default case when no format is
> given. But here, if no format has been given we _wouldn't_ want to show
> notes.
>
> I think it's relatively harmless, since our default format is not to do
> any pretty-printing, and therefore we wouldn't look at the notes. But it
> does cause us to call load_display_notes() when we don't need to. I
> think that conditional can be simplified to just:
>
>   if (!opt->show_notes_given && w.notes)
> 	opt->show_notes = 1;

Yep, your reasoning makes perfect sense, and I think that what you
suggested is exactly right. (The original code was indeed lifted from
'git log'...)

> > Fix this by initializing the notes machinery after parsing our options,
> > and harden this behavior against regression with a test in t4013. (Note
> > that the added ref in this test requires updating two unrelated tests
> > which use 'log --all', and thus need to learn about the new refs).
>
> This makes the test update rather hard to follow, but I don't think
> there's an easy way around it. I wondered if we could insert and remove
> our notes just for our tests, but it's hard to do with the big while
> loop in that script.
>
> I also thought it might not be that bad to just add a few tests at the
> end, but there are some complications (like removing sha1s from the
> output; easily done with a custom format, but the point is to test
> --pretty).
>
> So what you have is probably the most sensible thing (and the new commit
> would make it easier to test other commands around notes later, if we
> chose to).

Yeah :/.

> > @@ -398,6 +407,8 @@ diff --no-index --raw --no-abbrev dir2 dir
> >
> >  diff-tree --pretty --root --stat --compact-summary initial
> >  diff-tree --pretty -R --root --stat --compact-summary initial
> > +diff-tree --pretty --notes note
> > +diff-tree --format=%N note
> >  diff-tree --stat --compact-summary initial mode
> >  diff-tree -R --stat --compact-summary initial mode
>
> Perhaps worth testing that:
>
>   diff-tree --pretty note
>
> does not print any notes by default?

Good idea.

> -Peff

I'll send an updated patch as a response in a separate email shortly...

Thanks,
Taylor

Re: [PATCH] diff-tree.c: load notes machinery when required

[Taylor Blau]

Here is an updated version of the patch that should be ready for
queuing.

-- 8< --

Subject: [PATCH] tempfile.c: introduce 'create_tempfile_mode'

In the next patch, 'hold_lock_file_for_update' will gain an additional
'mode' parameter to specify permissions for the associated temporary
file.

Since the lockfile.c machinery uses 'create_tempfile' which always
creates a temporary file with global read-write permissions, introduce a
variant here that allows specifying the mode.

Arguably, all temporary files should have permission 0444, since they
are likely to be renamed into place and then not written to again. This
is a much larger change than we may want to take on in this otherwise
small patch, so for the time being, make 'create_tempfile' behave as it
has always done by inlining it to 'create_tempfile_mode' with mode set
to '0666'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 tempfile.c | 6 +++---
 tempfile.h | 7 ++++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index d43ad8c191..94aa18f3f7 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)
 }

 /* Make sure errno contains a meaningful value on error */
-struct tempfile *create_tempfile(const char *path)
+struct tempfile *create_tempfile_mode(const char *path, int mode)
 {
 	struct tempfile *tempfile = new_tempfile();

 	strbuf_add_absolute_path(&tempfile->filename, path);
 	tempfile->fd = open(tempfile->filename.buf,
-			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);
+			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);
 	if (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)
 		/* Try again w/o O_CLOEXEC: the kernel might not support it */
 		tempfile->fd = open(tempfile->filename.buf,
-				    O_RDWR | O_CREAT | O_EXCL, 0666);
+				    O_RDWR | O_CREAT | O_EXCL, mode);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
 		return NULL;
diff --git a/tempfile.h b/tempfile.h
index cddda0a33c..b5760d4581 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -89,7 +89,12 @@ struct tempfile {
  * a tempfile (whose "fd" member can be used for writing to it), or
  * NULL on error. It is an error if a file already exists at that path.
  */
-struct tempfile *create_tempfile(const char *path);
+struct tempfile *create_tempfile_mode(const char *path, int mode);
+
+static inline struct tempfile *create_tempfile(const char *path)
+{
+	return create_tempfile_mode(path, 0666);
+}

 /*
  * Register an existing file as a tempfile, meaning that it will be
--
2.26.2.108.g048abe1751

Re: [PATCH] diff-tree.c: load notes machinery when required

[Jeff King]

On Mon, Apr 20, 2020 at 06:05:31PM -0600, Taylor Blau wrote:

> Here is an updated version of the patch that should be ready for
> queuing.
> 
> -- 8< --
> 
> Subject: [PATCH] tempfile.c: introduce 'create_tempfile_mode'

Er, right patch?

-Peff

Re: [PATCH] diff-tree.c: load notes machinery when required

[Taylor Blau]

On Mon, Apr 20, 2020 at 08:06:19PM -0400, Jeff King wrote:
> On Mon, Apr 20, 2020 at 06:05:31PM -0600, Taylor Blau wrote:
>
> > Here is an updated version of the patch that should be ready for
> > queuing.
> >
> > -- 8< --
> >
> > Subject: [PATCH] tempfile.c: introduce 'create_tempfile_mode'
>
> Er, right patch?

Ugh, how embarrassing. See below:

-- >8 --

Subject: [PATCH] diff-tree.c: load notes machinery when required

Since its introduction in 7249e91 (revision.c: support --notes
command-line option, 2011-03-29), combining '--notes' with any option
that causes us to format notes (e.g., '--pretty', '--format="%N"', etc)
results in a failed assertion at runtime.

  $ git rev-list HEAD | git diff-tree --stdin --pretty=medium --notes
  commit 8f3d9f354286745c751374f5f1fcafee6b3f3136
  git: notes.c:1308: format_display_notes: Assertion `display_notes_trees' failed.
  Aborted

This failure is due to diff-tree not calling 'load_display_notes' to
initialize the notes machinery.

Ordinarily, this failure isn't triggered, because it requires passing
both '--notes' and another of the above mentioned options. In the case
of '--pretty', for example, we set 'opt->verbose_header', causing
'show_log()' to eventually call 'format_display_notes()', which expects
a non-NULL 'display_note_trees'.

Without initializing the notes machinery, 'display_note_trees' remains
NULL, and thus triggers an assertion failure.

Fix this by initializing the notes machinery after parsing our options,
and harden this behavior against regression with a test in t4013. (Note
that the added ref in this test requires updating two unrelated tests
which use 'log --all', and thus need to learn about the new refs).

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/diff-tree.c                          |  9 +++++++++
 t/t4013-diff-various.sh                      | 12 ++++++++++++
 t/t4013/diff.diff-tree_--format=%N_note      |  6 ++++++
 t/t4013/diff.diff-tree_--pretty_--notes_note | 12 ++++++++++++
 t/t4013/diff.diff-tree_--pretty_note         |  9 +++++++++
 t/t4013/diff.log_--decorate=full_--all       | 15 +++++++++++++++
 t/t4013/diff.log_--decorate_--all            | 15 +++++++++++++++
 7 files changed, 78 insertions(+)
 create mode 100644 t/t4013/diff.diff-tree_--format=%N_note
 create mode 100644 t/t4013/diff.diff-tree_--pretty_--notes_note
 create mode 100644 t/t4013/diff.diff-tree_--pretty_note

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index cb9ea79367..802363d0a2 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -109,6 +109,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct object *tree1, *tree2;
 	static struct rev_info *opt = &log_tree_opt;
 	struct setup_revision_opt s_r_opt;
+	struct userformat_want w;
 	int read_stdin = 0;

 	if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -127,6 +128,14 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	precompose_argv(argc, argv);
 	argc = setup_revisions(argc, argv, opt, &s_r_opt);

+	memset(&w, 0, sizeof(w));
+	userformat_find_requirements(NULL, &w);
+
+	if (!opt->show_notes_given && w.notes)
+		opt->show_notes = 1;
+	if (opt->show_notes)
+		load_display_notes(&opt->notes_opt);
+
 	while (--argc > 0) {
 		const char *arg = *++argv;

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index dde3f11fec..3f60f7d96c 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -95,6 +95,15 @@ test_expect_success setup '
 	git commit -m "update mode" &&
 	git checkout -f master &&

+	GIT_AUTHOR_DATE="2006-06-26 00:06:00 +0000" &&
+	GIT_COMMITTER_DATE="2006-06-26 00:06:00 +0000" &&
+	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
+	git checkout -b note initial &&
+	git update-index --chmod=+x file2 &&
+	git commit -m "update mode (file2)" &&
+	git notes add -m "note" &&
+	git checkout -f master &&
+
 	# Same merge as master, but with parents reversed. Hide it in a
 	# pseudo-ref to avoid impacting tests with --all.
 	commit=$(echo reverse |
@@ -398,6 +407,9 @@ diff --no-index --raw --no-abbrev dir2 dir

 diff-tree --pretty --root --stat --compact-summary initial
 diff-tree --pretty -R --root --stat --compact-summary initial
+diff-tree --pretty note
+diff-tree --pretty --notes note
+diff-tree --format=%N note
 diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
diff --git a/t/t4013/diff.diff-tree_--format=%N_note b/t/t4013/diff.diff-tree_--format=%N_note
new file mode 100644
index 0000000000..93042ed539
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--format=%N_note
@@ -0,0 +1,6 @@
+$ git diff-tree --format=%N note
+note
+
+
+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file2
+$
diff --git a/t/t4013/diff.diff-tree_--pretty_--notes_note b/t/t4013/diff.diff-tree_--pretty_--notes_note
new file mode 100644
index 0000000000..4d0bde601c
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_--notes_note
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty --notes note
+commit a6f364368ca320bc5a92e18912e16fa6b3dff598
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode (file2)
+
+Notes:
+    note
+
+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file2
+$
diff --git a/t/t4013/diff.diff-tree_--pretty_note b/t/t4013/diff.diff-tree_--pretty_note
new file mode 100644
index 0000000000..1fa5967083
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_note
@@ -0,0 +1,9 @@
+$ git diff-tree --pretty note
+commit a6f364368ca320bc5a92e18912e16fa6b3dff598
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode (file2)
+
+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file2
+$
diff --git a/t/t4013/diff.log_--decorate=full_--all b/t/t4013/diff.log_--decorate=full_--all
index 2afe91f116..3f9b872ece 100644
--- a/t/t4013/diff.log_--decorate=full_--all
+++ b/t/t4013/diff.log_--decorate=full_--all
@@ -5,12 +5,27 @@ Date:   Mon Jun 26 00:06:00 2006 +0000

     update mode

+commit a6f364368ca320bc5a92e18912e16fa6b3dff598 (refs/heads/note)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode (file2)
+
+Notes:
+    note
+
 commit cd4e72fd96faed3f0ba949dc42967430374e2290 (refs/heads/rearrange)
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:06:00 2006 +0000

     Rearranged lines in dir/sub

+commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 (refs/notes/commits)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    Notes added by 'git notes add'
+
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (HEAD -> refs/heads/master)
 Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
diff --git a/t/t4013/diff.log_--decorate_--all b/t/t4013/diff.log_--decorate_--all
index d0f308ab2b..f5e20e1e14 100644
--- a/t/t4013/diff.log_--decorate_--all
+++ b/t/t4013/diff.log_--decorate_--all
@@ -5,12 +5,27 @@ Date:   Mon Jun 26 00:06:00 2006 +0000

     update mode

+commit a6f364368ca320bc5a92e18912e16fa6b3dff598 (note)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode (file2)
+
+Notes:
+    note
+
 commit cd4e72fd96faed3f0ba949dc42967430374e2290 (rearrange)
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:06:00 2006 +0000

     Rearranged lines in dir/sub

+commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 (refs/notes/commits)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    Notes added by 'git notes add'
+
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64 (HEAD -> master)
 Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
--
2.26.2.108.g048abe1751

Re: [PATCH] diff-tree.c: load notes machinery when required

[Jeff King]

On Mon, Apr 20, 2020 at 06:13:15PM -0600, Taylor Blau wrote:

> Subject: [PATCH] diff-tree.c: load notes machinery when required
> [...]
> +	memset(&w, 0, sizeof(w));
> +	userformat_find_requirements(NULL, &w);
> +
> +	if (!opt->show_notes_given && w.notes)
> +		opt->show_notes = 1;
> +	if (opt->show_notes)
> +		load_display_notes(&opt->notes_opt);
> +

Thanks, this version looks good.

-Peff