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

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

On Wed, Feb 06, 2013 at 04:08:53PM +0100, Michael J Gruber wrote:

quoted

-			add_object_array(object, arg, &list);
+			add_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));

If we go this route, this new _with_context variant should be used in
patch 1, too.

quoted

@@ -265,9 +260,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj
 	objects[nr].item = obj;
 	objects[nr].name = name;
 	objects[nr].mode = mode;
+	objects[nr].context = context;
 	array->nr = ++nr;
 }

This seems a little gross. Who is responsible for allocating the
context? Who frees it? It looks like we duplicate it in cmd_grep. Which

Well, who is responsible for allocating and freeing name and item? I
didn't want to introduce a new member which is a struct when all other
complex members are pointers. Wouldn't that be confusing?

I think is OK, but it means all of this context infrastructure in
object.[ch] is just bolted-on junk waiting for somebody to use it wrong
or get confused.  It does not get set, for example, by the regular
setup_revisions code path.

Sure, it's NULL when there is no context info, just like in many other
cases.

It would be nice if we could just always have the context available,
then setup_revisions could set it up by default (and replace the "mode"
parameter entirely). But we'd need to do something to avoid the
PATH_MAX-sized buffer for each entry, as some code paths may have a
large number of pending objects.

If the information is always available even if we don't need it then it
always takes space. The only way out would be pointing into a pool of
path names rather having a copy in each entry. It's not like I hadn't
talked about providing virtual (blob) objects for path names keyed by
their sha1 before... It's just that I want my grep --textconv now ;)

Michael