Hi Peff, On 2015-08-23 19:43, Jeff King wrote: > On Sat, Aug 22, 2015 at 05:14:39PM +0200, Johannes Schindelin wrote: > >> The `format_display_notes()` function clearly assumes that the data >> structure holding the notes has been initialized already, i.e. that the >> `display_notes_trees` variable is no longer `NULL`. >> >> However, when there are no notes whatsoever, this variable is still >> `NULL`, even after initialization. >> >> So let's be graceful and just return if that data structure is `NULL`. > > Hrm. This is supposed to be made non-NULL by calling init_display_notes. > The "git-log" code does this properly, and can show notes. The > "rev-list" code does not, and hits this assert. But that also means that > it cannot actually _show_ notes, and your patch is papering over the > problem.
Good point... > it looks like [patch] is not enough to convince the pretty-printer to > actually show notes (I suspect we need something like the > pretty_ctx->notes_message setup that is in show_log()). > > I don't know how deeply anybody _cares_ about showing notes via > rev-list. It has clearly never worked. But rather than silently > accepting --show-notes (and not showing any notes!), perhaps we should > tell the user that it does not work. Likewise, the "%N" --format > specifier never expands in rev-list, and should probably be removed from > the rev-list documentation. Hmpf. I really did not want to uncover yet another rabbit hole... >> Reported in https://github.com/msysgit/git/issues/363. > > After reading your subject, I wondered why "git rev-list --show-notes > HEAD" did not crash for me (whether or not I had notes). But the key > element from that issue is the addition of "--grep", which is what > triggers us to actually look at the notes (since rev-list otherwise does > not support displaying them). And that _does_ work with my patch above. > So perhaps that is useful, though again, it has never worked in the > past (and with your patch, it would silently return no results, whether > the grep matched or not). > > Of course it's a terrible interface to make "--show-notes --grep" grep > the notes, but not actually _show_ them. :-/ It is. > So I'm not really in favor of this approach, but if we do go that route, > the comment above the declaration of format_display_notes needs to be > updated. You're right. I think the best approach for now is to error out when `--show-notes` is passed to rev-list. Do you agree? Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
