| Message ID | 20240218195938.6253-2-maarten.bosmans@vortech.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Speed up git-notes show | expand |
Maarten Bosmans <mkbosmans@gmail.com> writes: > Subject: Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c "Move" -> "move". > From: Maarten Bosmans <mkbosmans@gmail.com> > > From: Maarten Bosmans <maarten.bosmans@vortech.nl> The earlier one is not needed. Please do not include such a line. > > This way it can be used outside of builtin/log.c. > The next commit will make builtin/notes.c use it. The resulting file is only about a small part of the implementation detail of "show", and has very little to do with "log". "show.c" that happens to house show_blob_object(), a "canonical" way to display a blob object, with an anticipation that somebody may want to expand it in the future to house the "canonical" way to display a tag, a tree or a commit object, would be OK, though.
On Mon, Feb 19, 2024 at 05:22:44PM -0800, Junio C Hamano wrote: > > This way it can be used outside of builtin/log.c. > > The next commit will make builtin/notes.c use it. > > The resulting file is only about a small part of the implementation > detail of "show", and has very little to do with "log". > > "show.c" that happens to house show_blob_object(), a "canonical" way > to display a blob object, with an anticipation that somebody may > want to expand it in the future to house the "canonical" way to > display a tag, a tree or a commit object, would be OK, though. I'm not sure this function (or its siblings in builtin/log.c) counts as "canonical", since it is deeply connected to "struct rev_info". So it is appropriate for log, show, etc, but not for other commands. It felt a little funny to me to make a new file just for this one function. The related functions are all in log-tree.c. And as a bonus, the name is _already_ horribly confusing because it says "tree" but the functions are really about showing commits. ;) All that said, I'm not sure based on our previous discussion why we can't just call stream_blob_to_fd(). Looking at show_blob_object(), most of the logic is about recording the tree-context of the given name and using it for textconv. But since we know we are feeding a bare oid, that would never kick in. So I don't know if there's any value in sharing this function more widely in the first place. -Peff
Jeff King <peff@peff.net> writes: > All that said, I'm not sure based on our previous discussion why we > can't just call stream_blob_to_fd(). Looking at show_blob_object(), most > of the logic is about recording the tree-context of the given name and > using it for textconv. But since we know we are feeding a bare oid, > that would never kick in. So I don't know if there's any value in > sharing this function more widely in the first place. It is very nice that we do not need to touch this code move at all. Thanks, as usual, for a dose of sanity.
Op di 20 feb 2024 om 02:59 schreef Jeff King <peff@peff.net>: > All that said, I'm not sure based on our previous discussion why we > can't just call stream_blob_to_fd(). Looking at show_blob_object(), most > of the logic is about recording the tree-context of the given name and > using it for textconv. But since we know we are feeding a bare oid, > that would never kick in. So I don't know if there's any value in > sharing this function more widely in the first place. Indeed. My original analysis of what `git show` does when invoked by `git notes show` led to the conclusion that the only thing worthwhile to keep is the `setup_pager()` call. Thanks for confirming this. I'll reroll in a couple of days with the V1 approach back. Maarten
diff --git a/Makefile b/Makefile index 78e874099d..1c19d5c0f3 100644 --- a/Makefile +++ b/Makefile @@ -1059,6 +1059,7 @@ LIB_OBJS += list-objects-filter-options.o LIB_OBJS += list-objects-filter.o LIB_OBJS += list-objects.o LIB_OBJS += lockfile.o +LIB_OBJS += log.o LIB_OBJS += log-tree.o LIB_OBJS += ls-refs.o LIB_OBJS += mailinfo.o diff --git a/builtin/log.c b/builtin/log.c index db1808d7c1..587a4c374d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -32,7 +32,6 @@ #include "parse-options.h" #include "line-log.h" #include "branch.h" -#include "streaming.h" #include "version.h" #include "mailmap.h" #include "progress.h" @@ -42,7 +41,7 @@ #include "range-diff.h" #include "tmp-objdir.h" #include "tree.h" -#include "write-or-die.h" +#include "log.h" #define MAIL_DEFAULT_WRAP 72 #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100 @@ -653,37 +652,6 @@ static void show_tagger(const char *buf, struct rev_info *rev) strbuf_release(&out); } -static int show_blob_object(const struct object_id *oid, struct rev_info *rev, const char *obj_name) -{ - struct object_id oidc; - struct object_context obj_context; - char *buf; - unsigned long size; - - fflush(rev->diffopt.file); - if (!rev->diffopt.flags.textconv_set_via_cmdline || - !rev->diffopt.flags.allow_textconv) - return stream_blob_to_fd(1, oid, NULL, 0); - - if (get_oid_with_context(the_repository, obj_name, - GET_OID_RECORD_PATH, - &oidc, &obj_context)) - die(_("not a valid object name %s"), obj_name); - if (!obj_context.path || - !textconv_object(the_repository, obj_context.path, - obj_context.mode, &oidc, 1, &buf, &size)) { - free(obj_context.path); - return stream_blob_to_fd(1, oid, NULL, 0); - } - - if (!buf) - die(_("git show %s: bad file"), obj_name); - - write_or_die(1, buf, size); - free(obj_context.path); - return 0; -} - static int show_tag_object(const struct object_id *oid, struct rev_info *rev) { unsigned long size; @@ -770,7 +738,10 @@ int cmd_show(int argc, const char **argv, const char *prefix) const char *name = rev.pending.objects[i].name; switch (o->type) { case OBJ_BLOB: - ret = show_blob_object(&o->oid, &rev, name); + fflush(rev.diffopt.file); + bool do_textconv = rev.diffopt.flags.textconv_set_via_cmdline && + rev.diffopt.flags.allow_textconv; + ret = show_blob_object(&o->oid, name, do_textconv); break; case OBJ_TAG: { struct tag *t = (struct tag *)o; diff --git a/log.c b/log.c new file mode 100644 index 0000000000..5c77707385 --- /dev/null +++ b/log.c @@ -0,0 +1,41 @@ +#include "git-compat-util.h" +#include "gettext.h" +#include "diff.h" +#include "log.h" +#include "notes.h" +#include "object-name.h" +#include "repository.h" +#include "streaming.h" +#include "write-or-die.h" + +/* + * Print blob contents to stdout. + */ +int show_blob_object(const struct object_id *oid, const char *obj_name, bool do_textconv) +{ + struct object_id oidc; + struct object_context obj_context; + char *buf; + unsigned long size; + + if (!do_textconv) + return stream_blob_to_fd(1, oid, NULL, 0); + + if (get_oid_with_context(the_repository, obj_name, + GET_OID_RECORD_PATH, + &oidc, &obj_context)) + die(_("not a valid object name %s"), obj_name); + if (!obj_context.path || + !textconv_object(the_repository, obj_context.path, + obj_context.mode, &oidc, 1, &buf, &size)) { + free(obj_context.path); + return stream_blob_to_fd(1, oid, NULL, 0); + } + + if (!buf) + die(_("git show %s: bad file"), obj_name); + + write_or_die(1, buf, size); + free(obj_context.path); + return 0; +} diff --git a/log.h b/log.h new file mode 100644 index 0000000000..464cca52ff --- /dev/null +++ b/log.h @@ -0,0 +1,11 @@ +#ifndef LOG_H +#define LOG_H + +struct object_id; + +/* + * Print blob contents to stdout. + */ +int show_blob_object(const struct object_id *oid, const char *obj_name, bool do_textconv); + +#endif