Re: [PATCH 6/6] grep: obey --textconv for the case rev:path

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

Jeff King venit, vidit, dixit 20.04.2013 06:24:
> On Fri, Apr 19, 2013 at 06:44:49PM +0200, Michael J Gruber wrote:
> 
>> @@ -820,12 +820,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>  	for (i = 0; i < argc; i++) {
>>  		const char *arg = argv[i];
>>  		unsigned char sha1[20];
>> +		struct object_context oc;
>>  		/* Is it a rev? */
>> -		if (!get_sha1(arg, sha1)) {
>> +		if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
>>  			struct object *object = parse_object_or_die(sha1, arg);
>>  			if (!seen_dashdash)
>>  				verify_non_filename(prefix, arg);
>> -			add_object_array(object, arg, &list);
>> +			add_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));
> 
> Hrm. I'm not excited about the extra allocation here. Who frees it?
> 
>> +void add_object_array(struct object *obj, const char *name, struct object_array *array)
>> +{
>> +	add_object_array_with_mode(obj, name, array, S_IFINVALID);
>> +}
>> +
>> +void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)
>> +{
>> +	add_object_array_with_mode_context(obj, name, array, mode, NULL);
>> +}
>> +
>> +void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context)
>> +{
>> +	if (context)
>> +		add_object_array_with_mode_context(obj, name, array, context->mode, context);
>> +	else
>> +		add_object_array_with_mode_context(obj, name, array, S_IFINVALID, context);
>> +}
> 
> And this mass of almost-the-same functions is gross, too, especially
> given that the object_context contains a mode itself.

Well, it's just providing different ways to call into the one and only
function, in order to satisfy different callers' needs. It's not unheard
of (or rather: unseen) in our code, is it?

> Unfortunately, I'm not sure if I have a more pleasant suggestion. I seem
> to recall wrestling with this issue during the last round, too.

Yes, I think that's what we ended up with. At least it's just one
context struct per argument to grep, so it's not that bad after all.

I vaguely seem to recall we had some more general framework cooking but
I may be wrong (I was offline due to sickness for a while). It was about
attaching some additional info to something. Yes, I said "vaguely" ...

Michael