Re: [PATCH 1/2] range-diff: fix a crash in parsing git-log output
On Wed, Apr 15, 2020 at 04:02:42PM -0600, Taylor Blau wrote:
Show 13 quoted lines
> Subject: [PATCH] diff-tree.c: load notes machinery with '--notes'
>
> Since its introduction in 7249e91 (revision.c: support --notes
> command-line option, 2011-03-29), combining '--notes' with '--pretty'
> causes 'git diff-tree' to fail a runtime assertion:
>
> $ 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.
Yes, I think that's the problem that I saw. And this definitely fixes that case, but I think there's another related one.
$ git notes add -m foo
$ git log -1 --format='%h %N'
94316974f7 foo
$ git rev-list -1 HEAD | git diff-tree --stdin --format='%h %N' -s
94316974f7e27dccc6b87f3946bce5d2fc252dc2 %N
This is true even with your patch. With your patch I can add --notes to get the right output:
$ git rev-list -1 HEAD | git diff-tree --stdin --format='%h %N' -s --notes
94316974f7e27dccc6b87f3946bce5d2fc252dc2 foo
(It's also slightly curious that %h doesn't abbreviate in diff-tree; I guess this is a side effect of the plumbing having no default abbrev setting; it may be simplest to just live with it).
Show 6 quoted lines
> @@ -126,6 +126,8 @@ 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);
> + if (opt->show_notes)
> + load_display_notes(&opt->notes_opt);
In git-log we have the equivalent of these new lines, but just before it we check the userformat, too:
memset(&w, 0, sizeof(w));
userformat_find_requirements(NULL, &w);
if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
rev->show_notes = 1;
if (rev->show_notes)
load_display_notes(&rev->notes_opt);I think we'd want to do the same here. Even though it's plumbing, I can't think of any reason why somebody would _not_ want notes to be auto-enabled when they say "%N".
Show 7 quoted lines
> Ordinarily, this failure isn't triggered, because it requires passing
> both '--notes' and '--pretty'. Specifically, passing '--pretty' sets
> '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. This doesn't occur without
> '--pretty' since we never call 'format_display_notes()' without it.
It's not just --pretty, of course, but any option that causes us to actually try to format notes (--format, --oneline, etc).
-Peff
Vasil Dimov via GitGitGadget
