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

On Sat, May 17, 2014 at 03:42:24AM -0400, Jeff King wrote:

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

quoted

quoted

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.

It bugs me but I see your point.
It works just fine in this situation.

quoted

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;

Before, --no-signature would clear the &signature.
With this code it sees it as not being set and assigns
the default version string.

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

-Peff

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler