Re: [PATCHv3 3/7] show: honor --textconv for blobs

Jeff King [off-list ref] writes:

On Fri, May 10, 2013 at 10:02:51AM -0700, Junio C Hamano wrote:

quoted

quoted

Make "show" on blobs behave like "diff", i.e. honor "--textconv" by
default and "--no-textconv" when given.

[...]
So "show" on blobs does show the raw contents by default, but the
user can explicitly ask to enable textconv with --[no-]textconv.  Is
the second paragraph in the log message still valid?

Yes, I had the same thought...

I'd rewrite the paragraph to something like:

    Make "show" on blobs honor "--textconv" when it is asked.  The default
    is not to apply textconv, which is in line with what "cat-file" does.

quoted

quoted

+	if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
+		die("Not a valid object name %s", obj_name);

This looks somewhat unfortunate.
[...]
I wonder if enriching rev_info->pending with the context information
might be a clean solution to avoid this redundant but unavoidable
conversion, but that is a separate and future topic, I think.

It would be, and indeed, that is similar to what the final patch does.

OK, I wasn't paying attention ;-)

The problem is that it requires an extra allocation (we do not want to
unconditionally put the object_context into the object_array because it
is too big, so we add only a pointer). So having rev_info->pending store
that information would mean that callers would have to know to free it
when freeing the pending array. We would have to either teach each
existing caller to do so, or perhaps enable the behavior only when a
certain flag is set (e.g., rev->keep_object_context or something).

One thing to notice is that those accessing rev->pending before
calling prepare_revision_walk(), as opposed to those receiving
objects in rev->commits via get_revision(), are the only ones that
care about the context and wants to act differently depending on
where these came from and how they were specified.

That suggests at least two possibilities to me:

 - Perhaps we can place the context in rev->pending and clear them
   when prepare_revision_walk() moves them to rev->commits, without
   introducing rev->keep_object_context?

 - Perhaps instead of extending object-array, we can move this kind
   of information to rev_cmdline and enrich that structure?

On Fri, May 10, 2013 at 11:04:01AM -0700, Junio C Hamano wrote:

One thing to notice is that those accessing rev->pending before
calling prepare_revision_walk(), as opposed to those receiving
objects in rev->commits via get_revision(), are the only ones that
care about the context and wants to act differently depending on
where these came from and how they were specified.

That suggests at least two possibilities to me:

 - Perhaps we can place the context in rev->pending and clear them
   when prepare_revision_walk() moves them to rev->commits, without
   introducing rev->keep_object_context?

 - Perhaps instead of extending object-array, we can move this kind
   of information to rev_cmdline and enrich that structure?

Without looking too closely to see whether it is feasible, I would think
the latter would end up being much more elegant, since I think it
already deals with some allocation issues already.

-Peff