[v2,1/5] log: Move show_blob_object() to log.c

Commit Message

Maarten Bosmans Feb. 18, 2024, 7:59 p.m. UTC

From: Maarten Bosmans <[e-mail]>

From: Maarten Bosmans <[e-mail]>

This way it can be used outside of builtin/log.c.
The next commit will make builtin/notes.c use it.

Signed-off-by: Maarten Bosmans <[e-mail]>
---
 Makefile      |  1 +
 builtin/log.c | 39 +++++----------------------------------
 log.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 log.h         | 11 +++++++++++
 4 files changed, 58 insertions(+), 34 deletions(-)
 create mode 100644 log.c
 create mode 100644 log.h

Comments

Junio C Hamano Feb. 20, 2024, 1:22 a.m. UTC | #1

Maarten Bosmans <[e-mail]> writes:

> Subject: Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c

"Move" -> "move".

> From: Maarten Bosmans <[e-mail]>
>
> From: Maarten Bosmans <[e-mail]>

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.

Jeff King Feb. 20, 2024, 1:59 a.m. UTC | #2

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

Junio C Hamano Feb. 20, 2024, 3:03 a.m. UTC | #3

Jeff King <[e-mail]> 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.

Maarten Bosmans Feb. 20, 2024, 11:40 a.m. UTC | #4

Op di 20 feb 2024 om 02:59 schreef Jeff King <[e-mail]>:
> 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

Patch

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