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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html