Re: [PATCH 1/4] notes: print note blob to stdout directly

From: Jeff King <peff@peff.net>
Date: Feb 17, 2024, 05:16 UTC
Message-ID: <20240217051650.GB539459@coredump.intra.peff.net>
In-Reply-To: <xmqqcysxkjrq.fsf@gitster.g>

On Thu, Feb 15, 2024 at 10:25:45PM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> > Of course it's reasonable to also store notes that aren't meant to be
> > displayed via git-log, etc, at all. The textconv-caching machinery
> > stores its results in a separate notes ref. Those should generally be
> > text (since the point is to come up with something diff-able). But I
> > think it would be perfectly reasonable for another mechanism to make use
> > of notes in the same way and store binary goo.
> 
> Yup.  
> 
> The question is if our current use of "git show" allows creative use
> of such binary data attached as notes.  The patch in question will
> break such users, as it would become impossible once we start
> bypassing the "git show" and emitting the binary data directly to
> the standard output stream.

Hmm, good question. I can't think offhand of a way that the user could convince "git show <oid>" to do anything different than just dumping the literal contents. It is not even handed a path that could trigger .gitattributes or similar.

The only difference I can think of is that "git show" triggers a pager by default. Probably "git notes show" should be doing the same (and honestly, that would be less confusing to me; the fact that configuring "pager.show" would affect "git notes show" is surprising to me).

Digging in the history, it looks like we use "git show" at all because this was adapted from a shell script (though that shell script probably ought to have been using cat-file in the first place; maybe back then we thought we'd support non-blobs ;) ).

Show 13 quoted lines

> Because the pathname a note blob is stored at is unpredictable and
> not under end-user control, it would not be practical to define
> different smudge filters per note object using the attribute
> mechanism, but if you limit the types of binary data you store in
> your notes (e.g., refs/notes/thumbnail may be a note ref whose
> contents are all jpeg thumbnail images, where your main history is
> your photo archive), you might be able to convince the "git show"
> invocation we use to show the note object to show that thumbnail
> image by using something involving "display" (from imagemagick---of
> course you could use other programs that allows you to view the
> image on different platforms) in your smudge filter.  Bypassing "git
> show" and sending the blob contents directly to the standard output
> would be a grave regression for such a user.

Like I said, I do not think there is a way to convince "git show" in that way. Unless perhaps something like:

  git -c pager.show='f() { cat >tmp.jpg; display tmp; }; f' \
    notes show --ref thumbnail

But that's pretty awful. Why would you do that instead of just "git notes show >tmp; display tmp" yourself? And again, even if we wanted to support such a monstrosity, I feel like setting pager.notes.show would be much more intuitive than piggy-backing on "git show".

Sometimes, of course, we have to support weird stuff anyway because it has existed for a long time and we don't want to break users. But this is really pushing my gut-feeling limit of what is reasonable / plausible for somebody to be doing.

Of course I may be missing some other case where "show" behaves in a useful way that is different than a straight dump of the blob. But if not, I'd almost say that getting rid of the extra "show" call now is a good thing, because it locks in the simple behavior. ;)

-Peff