Re: [PATCH 2/4 v4] ls-files: optionally recurse into submodules

From: Brandon Williams <bmwill@google.com>
Date: Sep 27, 2016, 20:33 UTC
Message-ID: <20160927203338.GB32565@google.com>
In-Reply-To: <xmqqlgydnqlh.fsf@gitster.mtv.corp.google.com>

On 09/27, Junio C Hamano wrote:

Show 7 quoted lines

> Brandon Williams <bmwill@google.com> writes:
> > +static const char *submodule_prefix;
> 
> I would have expected this to added to environment.c in the previous
> step, but it is OK--I'd imagine you'd grab this from the environment
> and carrying a piece of information from git.c to here by setenv()
> followed by getenv() feels somewhat roundabout, though.

If it would make sense to do the caching of prefix string in environment.c I can move it there and add a get_submodule_prefix() function which either reads the envvar or the cached value if its already been read. Would that be a better route?

Show 16 quoted lines

> 
> >  static const char *prefix;
> >  static int max_prefix_len;
> > @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
> >  static void write_name(const char *name)
> >  {
> >  	/*
> > +	 * NEEDSWORK: To make this thread-safe, full_name would have to be owned
> > +	 * by the caller.
> 
> As Peff mentioned in his review in another thread, a large number of
> functions in git are not reentrant, and I do not think we would want
> to give the impression that those missing a warning are safe to use.
> 
> Other than that, this step looks OK.  3/4 and later would be a lot
> more fun to review ;-)

Oh yes, I can remove the comment. Seemed to miss that bit while rerolling the series.

-- 
Brandon Williams