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

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

Jeff King venit, vidit, dixit 07.02.2013 10:26:
> On Thu, Feb 07, 2013 at 10:05:57AM +0100, Michael J Gruber wrote:
> 
>>>> @@ -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?
> 
> We cheat on those two. "item" is always a pointer to a "struct object",
> which lasts forever and never gets freed. When "name" is set by
> setup_revisions, it comes from the argv list, which is assumed to last
> forever (and when we add pending blobs for a "--objects" traversal, it
> is the empty string (literal).

I see, so they are really different.

> I'd be OK if we had an exterior object_context that could be handled
> in the same way. But how do we tell setup_revisions that we are
> interested in seeing the object_context from each parsed item, where
> does the allocation come from (is it malloc'd by setup_revisions?), and
> who is responsible for freeing it when we pop pending objects in
> get_revisions and similar?

Do we really need all of tree, path and mode in object_context (I mean
not just here, but other users), or only the path? I'd try and resurrect
the virtual path name objects then, they would be just like "item"
storage-wise.

> I don't think it's as clear cut.
> 
> I wonder, though...what we really care about here is just the pathname.
> But if it is a pending object that comes from a blob revision argument,
> won't it always be of the form "treeish:path"? Could we not even resolve
> the sha1 again, but instead just parse out the ":path" bit?

Do we have that, and in what form (e.g. magic expanded etc.)?

> That is sort of like what the repeated call to get_sha1_with_context
> does in your first patch. Except that we do not actually want to lookup
> the sha1, and it is harmful to do so (e.g., if the ref had moved on to a
> new tree that does not have that path, get_sha1 would fail, but we do
> not even care what is in the tree; we only want the parsing side effects
> of get_sha1).
> 
> Hmm.
> 
> -Peff
> 
> PS By the way, while looking at the object_array code (which I have not
>    really used much before), I noticed that add_pending_commit_list sets
>    the "name" field to the result of sha1_to_hex. Which means that it is
>    likely to be completely bogus by the time you read it. I'm not even
>    sure where it gets read or if this matters. And obviously it's
>    completely unrelated to what we were discussing; just something I
>    noticed.

Another thing I noted is that our path mangling at least for grep has
some issues:

(cd t && git grep GET_SHA1_QUIETLY HEAD:../cache.h)
../HEAD:../cache.h:#define GET_SHA1_QUIETLY        01

Taking everything right of ":" could still work.

Michael