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

[Michael J Gruber <git@drmicha.warpmail.net>]

Junio C Hamano venit, vidit, dixit 10.05.2013 19:02:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Currently, "diff" and "cat-file" for blobs honor "--textconv" options
>> (with the former defaulting to "--textconv" and the latter to
>> "--no-textconv") whereas "show" does not honor this option, even though
>> it takes diff options.
>>
>> Make "show" on blobs behave like "diff", i.e. honor "--textconv" by
>> default and "--no-textconv" when given.
> 
> Hmm...

Sorry, I overlooked that ;)

>> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)
>>  {
>> +	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_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
>> +	    !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
>> +		return stream_blob_to_fd(1, sha1, NULL, 0);
> 
> It is surprising that the necessary change is only this, but I think
> it is correct ;-).  We ignore textconv when the command line did not
> mention --[no-]textconv, or the command line said --no-textconv
> explicitly.
> 
> This (especially the first condition) may deserve an in-code comment
> for anybody who wonders where this default behaviour is implemented.

It's not as if we would document behavior by in-code comments in
general, do we? The usual answer is "git log -S" or "git blame".

> 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?
> 
>> +	if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
>> +		die("Not a valid object name %s", obj_name);
> 
> This looks somewhat unfortunate.
> 
> We already have sha1[]; actually we not just know sha1[] but have
> the struct object for it.  How did we obtain it before we got here?
> 
> Will we always have a valid name in rev.pending.objects->name?  Will
> that name convert back to the same sha1 we got in sha1[]?
> 
> I think the answers are "Yes (it is a command line argument), Yes
> (that is what setup_revisions() got by feeding the name to give us
> sha1[])".
> 
> 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.

Yes, I think both Jeff and I have thought it and came to the same
conclusion - "later" ;)

>> +	if (!obj_context.path[0] ||
>> +	    !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))
>> +		return stream_blob_to_fd(1, sha1, NULL, 0);
>> +
>> +	if (!buf)
>> +		die("git show %s: bad file", obj_name);
>> +
>> +	write_or_die(1, buf, size);
>> +	return 0;
>>  }
>>  
>>  static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
>> @@ -526,7 +545,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>>  		const char *name = objects[i].name;
>>  		switch (o->type) {
>>  		case OBJ_BLOB:
>> -			ret = show_blob_object(o->sha1, NULL);
>> +			ret = show_blob_object(o->sha1, &rev, name);
>>  			break;
>>  		case OBJ_TAG: {
>>  			struct tag *t = (struct tag *)o;
>> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
>> index 3950fc9..0ebb028 100755
>> --- a/t/t4030-diff-textconv.sh
>> +++ b/t/t4030-diff-textconv.sh
>> @@ -96,14 +96,14 @@ test_expect_success 'show blob produces binary' '
>>  	test_cmp expect actual
>>  '
>>  
>> -test_expect_failure 'show --textconv blob produces text' '
>> +test_expect_success 'show --textconv blob produces text' '
>>  	git show --textconv HEAD:file >actual &&
>>  	printf "0\\n1\\n" >expect &&
>>  	test_cmp expect actual
>>  '
>>  
>> -test_success 'show --no-textconv blob produces binary' '
>> -	git show --textconv HEAD:file >actual &&
>> +test_expect_success 'show --no-textconv blob produces binary' '
>> +	git show --no-textconv HEAD:file >actual &&
>>  	printf "\\0\\n\\01\\n" >expect &&
>>  	test_cmp expect actual
>>  '