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

From: Jeff King <peff@peff.net>
Date: Feb 20, 2024, 01:59 UTC
Message-ID: <20240220015928.GB2713741@coredump.intra.peff.net>
In-Reply-To: <xmqqjzn0x72z.fsf@gitster.g>

On Mon, Feb 19, 2024 at 05:22:44PM -0800, Junio C Hamano wrote:

Show 10 quoted lines

> > 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