Re: [PATCH] sha1_name: use strlcpy() to copy strings

From: Jeff King <peff@peff.net>
Date: Feb 23, 2015, 18:36 UTC
Message-ID: <20150223183609.GC19904@peff.net>
In-Reply-To: <54EA592C.6040701@web.de>

On Sun, Feb 22, 2015 at 11:33:16PM +0100, René Scharfe wrote:

Show 16 quoted lines

> Am 22.02.2015 um 21:00 schrieb Junio C Hamano:
> >René Scharfe <l.s.r@web.de> writes:
> >
> >>Use strlcpy() instead of calling strncpy() and then setting the last
> >>byte of the target buffer to NUL explicitly.  This shortens and
> >>simplifies the code a bit.
> >
> >Thanks.  It makes me wonder if the longer term direction should be
> >not to use a bound buffer for oc->path, though.
> 
> That's a good idea in general, but a bit more involved since we'd need to
> introduce a cleanup function that releases the memory allocated by the new
> version of get_sha1_with_context() first and call it from the appropriate
> places.
> 
> Would that be a good micro-project for GSoC or is it too simple?

Yeah, avoiding resource ownership questions was one of the reasons I went with the static buffer in the first place. But I would love to see it go away. Not only does it potentially truncate paths, but I recall there was some complication with the size of "struct object_context" (I couldn't find the details in a cursory search, but basically it was not reasonable to have a big array of them).

Could we perhaps make this more like sha1_object_info_extended, where the caller "asks" for fields by filling in pointers, and the object_context itself can be discarded without leaking resources?

Like:

  struct strbuf path = STRBUF_INIT;
  struct object_context oc = OBJECT_CONTEXT_INIT;

  oc.path = &path;
  get_sha1_with_context(sha1, &oc);

  ... use path directly ...
  strbuf_release(&path);

Then callers who do not care about the path do not have to even know the feature exists (and it opens us up to adding new string-like context fields in the future if we need to).

-Peff