Re: [PATCH v3] builtin/pack-objects.c: introduce `pack.extraCruftTips`

From: Jeff King <peff@peff.net>
Date: May 5, 2023, 22:19 UTC
Message-ID: <20230505221921.GE3321533@coredump.intra.peff.net>
In-Reply-To: <27a7f16aab35b5cac391d9831aadb0f2e2146313.1683151485.git.me@ttaylorr.com>

On Wed, May 03, 2023 at 06:05:45PM -0400, Taylor Blau wrote:

Show 11 quoted lines

> +	out = xfdopen(cmd.out, "r");
> +	while (strbuf_getline(&buf, out) != EOF) {
> +		struct object_id oid;
> +		struct object *obj;
> +		int type;
> +		const char *rest;
> +
> +		if (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {
> +			ret = error(_("invalid extra cruft tip: '%s'"), buf.buf);
> +			goto done;
> +		}

While adapting this for my "how about this" patch elsewhere in the thread, I noticed the error handling here is a little funny.

The other side sent us unexpected output, so we stopped parsing. But we never reap the child process. We just jump to "done", which clears the memory used by the process struct, but never call finish_command().

I think you want to break out of this loop and run finish_command. But you have to make sure to fclose(out) first, so that it knows to exit (otherwise it may fill up the pipe buffer and block waiting for us to read, creating a deadlock).

And then

So something like:

  while (strbuf_getline(...)) {
	if (some_error) {
		ret = error(...);
		break;
  }

  fclose(out); /* no need for "if (out)" here; it's always open */
  ret |= finish_command(&cmd);

  /* no need to child_process_clear(); already done by finish_command() */
  strbuf_release(&buf);
  return ret;

-Peff