Re: [RFC/PATCH 1/4] sha1_name: creating context cache

From: Jeff King <peff@peff.net>
Date: Jun 6, 2010, 22:15 UTC
Message-ID: <20100606221557.GF6993@coredump.intra.peff.net>
In-Reply-To: <vpqpr045f46.fsf@bauges.imag.fr>

On Sun, Jun 06, 2010 at 05:56:41PM +0200, Matthieu Moy wrote:

Show 12 quoted lines

> > +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