Re: [PATCH 1/2] range-diff: fix a crash in parsing git-log output

From: Jeff King <peff@peff.net>
Date: Apr 15, 2020, 22:29 UTC
Message-ID: <20200415222905.GA3595509@coredump.intra.peff.net>
In-Reply-To: <20200415220242.GA45241@syl.local>

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