On Thu, May 15, 2014 at 06:31:21PM -0700, Jeremiah Mahler wrote:
> 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:
> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html