Re: [PATCH 0/17] removing questionable uses of git_path

[Jeff King <peff@peff.net>]

On Mon, Aug 10, 2015 at 10:31:32AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The problem is that git_path uses a static buffer that gets overwritten
> > by subsequent calls.
> 
> As the rotating static buffer pattern used in get_pathname() was
> modeled after sha1_to_hex(), we have the same issue there.  They are
> troubles waiting to happen unless the callers are careful.

Yeah, I think Michael mentioned that to me off-list. I don't _think_
sha1_to_hex is nearly such a problem, because we tend to store sha1s in
their binary form. So sha1_to_hex is almost always an argument to
fprintf() or similar.

Of course there are some exceptions. :) I notice we pass one return
value to add_pending_object, which was almost certainly horribly broken
before 31faeb2 started strdup'ing object_array names.

So certainly it would be nice to audit them all, but there are over 600
calls. Given the likelihood of finding a useful bug, I'm not sure it's
the greatest use of developer time.

> > producing a fairly tame-looking set of function calls. It's OK to pass
> > the result of git_path() to a system call, or something that is a thin
> > wrapper around one (e.g., strbuf_read_file).
> 
> That is a short and good rule to follow, but the problem is that not
> everybody has good taste to interpret the above rule and apply it with
> an eye toward maintainability X-<.

Yeah. Part of me wants to eradicate git_path entirely. My series takes
out over half of the calls, but there are still close to 100, I think.
I think it would make the code worse to convert all of them naively. We
could provide format-aware wrappers for some filesystem functions, like:

  git_stat(&st, "%s", refname);

or something. That feels horribly coupled compared to:

  stat(git_path("%s", refname), &st);

but it makes it very clear what the memory ownership/lifetime rules are.

Anyway, part of my goal with the series was not just to clean up the
existing suspicious spots, but to raise awareness of the issue. At least
for me, I know it's something I will look for more carefully when
reviewing patches. Once bitten, twice shy. :)

-Peff