Re: [PATCH v2] format-patch --signature-file <file>

From: Jeff King <peff@peff.net>
Date: May 17, 2014, 07:42 UTC
Message-ID: <20140517074224.GA16697@sigill.intra.peff.net>
In-Reply-To: <20140517072548.GA18239@hudson.localdomain>

On Sat, May 17, 2014 at 12:25:48AM -0700, Jeremiah Mahler wrote:

Show 12 quoted lines

> > We have routines for reading directly into a strbuf, which eliminates
> > the need for this 1024-byte limit. We even have a wrapper that can make
> > this much shorter:
> > 
> >   struct strbuf buf = STRBUF_INIT;
> > 
> >   strbuf_read_file(&buf, arg, 128);
> >   *signature = strbuf_detach(&buf, NULL);
> > 
> 
> Yes, that is much cleaner.
> The memory returned by strbuf_detach() will have to be freed as well.

In cases like this, we often let the memory leak. It's in a global that stays valid through the whole program, so we just let the program's exit clean it up.

> Having --signature-file override --signature seems simpler to implement.
> The signature variable has a default value which complicates
> determining whether it was set or not.

Yeah, the default value complicates it. I think you can handle that just by moving the default to the main logic, like:

  static const char *signature;
  static const char *signature_file;

...

  if (signature) {
	if (signature_file)
		die("you cannot specify both a signature and a signature-file");
	/* otherwise, we already have the value */
  } else if (signature_file) {
	struct strbuf buf = STRBUF_INIT;
	strbuf_read(&buf, signature_file, 128);
	signature = strbuf_detach(&buf);
  } else
	signature = git_version_string;

and as a bonus, that keeps all of the logic together in one (fairly readable) chain.

-Peff