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

From: Jeff King <peff@peff.net>
Date: May 16, 2014, 08:14 UTC
Message-ID: <20140516081445.GA21468@sigill.intra.peff.net>
In-Reply-To: <1400203881-2794-2-git-send-email-jmmahler@gmail.com>

On Thu, May 15, 2014 at 06:31:21PM -0700, Jeremiah Mahler wrote:

Show 6 quoted lines

> Added feature that allows a signature file to be used with format-patch.
> 
>   $ git format-patch --signature-file ~/.signature -1
> 
> Now signatures with newlines and other special characters can be
> easily included.

I think this version looks nicer than the original.

A few questions/comments:

Show 20 quoted lines

> +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);

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.

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:

  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