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

From: Jeff King <peff@peff.net>
Date: Feb 7, 2013, 09:26 UTC
Message-ID: <20130207092640.GC15727@sigill.intra.peff.net>
In-Reply-To: <51136E75.5060002@drmicha.warpmail.net>

On Thu, Feb 07, 2013 at 10:05:57AM +0100, Michael J Gruber wrote:

Show 14 quoted lines

> >> @@ -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'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?

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?

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.