Re: [PATCH] Fix `git rev-list --show-notes HEAD` when there are no notes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]