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

From: Jeff King <peff@peff.net>
Date: Apr 15, 2020, 16:23 UTC
Message-ID: <20200415162326.GG2464307@coredump.intra.peff.net>
In-Reply-To: <xmqqsgh47okk.fsf@gitster.c.googlers.com>

On Wed, Apr 15, 2020 at 08:31:39AM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> "Vasil Dimov via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: Vasil Dimov <vd@FreeBSD.org>
> >
> > `git range-diff` calls `git log` internally and tries to parse its
> > output. But `git log` output can be customized by the user in their
> > git config and for certain configurations either an error will be
> > returned by `git range-diff` or it will crash.
> >
> > To fix this explicitly set the output format of the internally
> > executed `git log` with `--pretty=medium`. Because that cancels
> > `--notes`, add explicitly `--notes` at the end.
> 
> Good finding.  
> 
> Shouldn't we also disable customizations that come from the
> configuration variables like diff.external, diff.<driver>.command?

If range-diff were a script, I would say it should be using the "rev-list | diff-tree --stdin" plumbing under the hood, rather than "log".

The read_patches() function does let callers pass options to git-log, but I don't _think_ this is exposed to the user. We only allow a few --notes options to be passed, and we should be able to apply those to diff-tree. So converting it to use plumbing might be an option.

Though I think there is another bug:

  $ 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

Another option would be for range-diff to directly call the revision-traversal plumbing itself. There may be complications there, though (or else it would have been done from the outset).

We should fix that assertion regardless, though.

-Peff