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

From: Maarten Bosmans <mkbosmans@gmail.com>
Date: Feb 6, 2024, 09:55 UTC
Message-ID: <CA+CvcKTtcHCCKucQ0h1dnaDAMNfErJ+a1CXEVi=ZE5dv57Tb3A@mail.gmail.com>
In-Reply-To: <xmqqil32l0i6.fsf@gitster.g>

Op di 6 feb 2024 om 04:44 schreef Junio C Hamano <gitster@pobox.com>:

Show 19 quoted lines

>
> Maarten Bosmans <mkbosmans@gmail.com> writes:
> > Avoid the need to launch a subprocess by calling stream_blob_to_fd
> > directly.  This does not only get rid of the overhead of a separate
> > child process, but also avoids the initalization of the whole log
> > machinery that `git show` does.  That is needed for example to show
> > decorated commits and applying the mailmap.  For simply displaying
> > a blob however, the only useful thing show does is enabling the pager.
> >
> > Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
> > ---
> >  builtin/notes.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
>
> I am not sure if we want to accept an approach that feels somewhat
> narrow/short sighted, like this patch.  When "git show" learns an
> improved way to show blob objects, who will update the code this
> patch touches to teach it to use the same improved way to show the
> notes?

Yes, you would loose some of the flexibility by just calling out to another git command. But whether that is actually an issue depends on the way git show would be extended. As I mentioned in the cover letter, there is some handling of textconv being done in git show for the blob case. I though it would not be applicable to note blobs, but am not entirely sure.

To address your concern and the textconv case, instead of calling stream_blob_to_fd() directly, show_blob_object() could be used. Right now that is a static function in building/log.c I guess that needs to be moved then to log.c (or show.c)?

I'm still not sure that is a good approach though, as the notes edit/append subcommands use repo_read_object_file() directly to fill the current note contents. So anything fancy that git show would do to blob output is not reflected when editing a note.

Show 6 quoted lines

> I actually was hoping, after seeing the use case description in the
> cover letter, that the series would be introducing a batch mode
> interface to allow callers to ask notes for many objects and have
> the command respond notes for these objects in a way that which
> piece of output corresponds to which object in the request, reducing
> the process overhead amortised over many objects.

That is also a cool idea. That would probably use the functionality of the cat-file batch mode, right? In that case you loose any future changes to git show anyway. At least the current behavior of git show when fed multiple blob hashes is to simply output them directly after another, without any way of identifying the parts.

My concern would be that this additional functionality would not get used much, as it requires a bit more than just a quick-n-dirty bash script with some loops and command substitutions. If that approach is reasonably fast (the object of this patch series), then there is not much to be gained in a practical sense by a batch mode. To me it seems the git notes code is already a bit undermaintained currently, so I'd rather keep it generic, instead of customizing it to more specific use cases.

Maarten