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

[Jeremiah Mahler <jmmahler@gmail.com>]

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:
> 
> > > 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.

> > 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