From: Clément Poulain Date: Mon, 07 Jun 2010 08:34:01 GMT Subject: Re: [RFC/PATCH 1/4] sha1_name: creating context cache Message-ID: In-Reply-To: <20100606221557.GF6993@coredump.intra.peff.net> 2010/6/7 Jeff King : > On Sun, Jun 06, 2010 at 05:56:41PM +0200, Matthieu Moy wrote: > >> > +struct object_resolve_context { >> > +   unsigned char tree[20]; >> > +   char path[PATH_MAX]; >> > +   unsigned mode; >> > +}; >> > +extern struct object_resolve_context object_context; >> >> Is it really a good idea to make this a global variable? As I >> understand it, the semantics of this variable is that it contains >> information on the last sha1 name parsed (BTW, you probably want to >> add a comment here explaining that). Wouldn't it be more robust to >> have this value returned by the function doing the parsing? > > This is straight from my earlier patch, but I think I said in that > thread that it could use some cleaning up. You added the 'mode' field, > but it probably makes sense to then get rid of the distinction between > get_sha1 and get_sha1_with_mode, and just have: > >  get_sha1_with_context(const char *name, unsigned char *sha1, struct >                        object_resolve_context *orc) { >     /* basically get_sha1_with_mode, but set mode in orc */ >  } >  get_sha1(const char *name, unsigned char *sha1) { >         struct object_resolve_context orc; /* to be thrown away */ >         return get_sha1_with_context(name, sha1, &orc); >  } > > -Peff > Yes, we've already thought about it (you've said it, Jeff ;-) ), and we have actually changed it with something very close to what you say. We'll soon propose a v2 for this part of the patch. Thank you