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

[Jeremiah Mahler <jmmahler@gmail.com>]

On Fri, May 16, 2014 at 04:14:45AM -0400, Jeff King wrote:
> On Thu, May 15, 2014 at 06:31:21PM -0700, Jeremiah Mahler wrote:
...
> 
> A few questions/comments:
> 
> > +static int signature_file_callback(const struct option *opt, const char *arg,
> > +							int unset)
> > +{
> > +	const char **signature = opt->value;
> > +	static char buf[1024];
> > +	size_t sz;
> > +	FILE *fd;
> > +
> > +	fd = fopen(arg, "r");
> > +	if (fd) {
> > +		sz = sizeof(buf);
> > +		sz = fread(buf, 1, sz - 1, fd);
> > +		if (sz) {
> > +			buf[sz] = '\0';
> > +			*signature = buf;
> > +		}
> > +		fclose(fd);
> > +	}
> > +	return 0;
> > +}
> 
> 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.

> I notice that you ignore any errors. Is that intentional (so that we
> silently ignore missing --signature files)? If so, should we actually
> treat it as an empty file (e.g., in my code above, we always set
> *signature, even if the file was missing)?
> 
> Finally, I suspect that:
> 
>   cd path/in/repo &&
>   git format-patch --signature-file=foo
> 
> will not work, as we chdir() to the toplevel before evaluating the
> arguments. You can fix that either by using parse-option's OPT_FILENAME
> to save the filename, followed by opening the file after all arguments
> are processed; or by manually fixing up the filename.
> 

Yes, it wouldn't have worked.
Using OPT_FILENAME is a much better solution.

> Since parse-options already knows how to do this fixup (it does it for
> OPT_FILENAME), it would be nice if it were a flag rather than a full
> type, so you could specify at as an option to your callback here:
> 
> > +		{ OPTION_CALLBACK, 0, "signature-file", &signature, N_("signature-file"),
> > +				N_("add a signature from contents of a file"),
> > +			    PARSE_OPT_NONEG, signature_file_callback },
> 
> Noticing your OPT_NONEG, though, I wonder if you should simply use an
> OPT_FILENAME. I would expect --no-signature-file to countermand any
> earlier --signature-file on the command-line (or if we eventually grow a
> config option, which seems sensible, it would tell git to ignore the
> option). The usual ordering for that is:
> 

Another case is when both --signature="foo" and --no-signature-file are used.
Currently this would only negate the file option which would allow
the --signature option to be used.

>   1. Read config and store format.signatureFile as a string
>      "signature_file".
> 
>   2. Parse arguments. --signature-file=... sets signature_file, and
>      --no-signature-file sets it to NULL.
> 
>   3. If signature_file is non-NULL, load it.
> 
> And I believe OPT_FILENAME will implement (2) for you.
> 
> One downside of doing it this way is that you need to specify what will
> happen when both "--signature" (or format.signature) and
> "--signature-file" are set. With your current code, I think
> "--signature=foo --signature-file=bar" will take the second one. I think
> it would be fine to prefer one to the other, or to just notice that both
> are set and complain.
> 
> -Peff

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.

Thanks for the great suggestions.

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