Maarten Bosmans [off-list ref] writes:
From: Maarten Bosmans <redacted>
From: Maarten Bosmans <redacted>
Which one of you are you? Please make up your mind and use only one
;-) IOW, the first one is unneeded, as the latter matches what you
have on the S-o-b line.
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 <redacted>
---
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?
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.
Op di 6 feb 2024 om 04:44 schreef Junio C Hamano [off-list ref]:
Maarten Bosmans [off-list ref] writes:
quoted
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 <redacted>
---
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.
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
On Tue, Feb 6, 2024, at 04:44, Junio C Hamano wrote:
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 sounds great. I imagine that would be very useful.
--
Kristoffer Haugsbakk