Re: [RFC/PATCH 1/4] show: obey --textconv for blobs

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Feb 7, 2013, 09:05 UTC
Message-ID: <51136E56.7060703@drmicha.warpmail.net>
In-Reply-To: <20130206220644.GB27507@sigill.intra.peff.net>

Jeff King venit, vidit, dixit 06.02.2013 23:06:

Show 14 quoted lines

> On Wed, Feb 06, 2013 at 04:08:50PM +0100, Michael J Gruber wrote:
> 
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 8f0b2e8..f83870d 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
>>  	strbuf_release(&out);
>>  }
>>  
>> -static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)
>> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)
> 
> Should this maybe just take the whole object_array_entry as a cleanup?

It's just a question of one or two/three pointers (I can't count), but yes, that would be possible.

Show 17 quoted lines

>>  {
>> +	unsigned char sha1c[20];
>> +	struct object_context obj_context;
>> +	char *buf;
>> +	unsigned long size;
>> +
>>  	fflush(stdout);
>> -	return stream_blob_to_fd(1, sha1, NULL, 0);
>> +	if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
>> +		return stream_blob_to_fd(1, sha1, NULL, 0);
>> +
>> +	if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
>> +		die("Not a valid object name %s", obj_name);
> 
> It seems a little hacky that we have to look up the sha1 again. What
> should happen in the off chance that "hashcmp(sha1, sha1c) != 0" due to
> a race with a simultaneous update of a ref?

I thought about a check here but didn't bother to because I knew the refactoring would come up again...

> Would it be better if object_array_entry replaced its "mode" member with
> an object_context?

Do all callers/users want to deal with object_context?

I'm wondering why o_c has a mode at all, since it is mostly used in conjunction with an object, isn't it?

> The only downside I see is that we might waste a
> significant amount of memory (each context has a PATH_MAX buffer in it).

That's why I used a reference to the struct, see my other reply.

Michael