Carlos Rica <jasampler@xxxxxxxxx> writes:
> Mime-Version: 1.0
> Content-Type: text/plain; charset=windows-1252
Hmmmmmm.....
> This replaces the script "git-tag.sh" with "builtin-tag.c".
> It is based in a previous work on it from Kristian Høgsberg.
>
> There are some minor changes in the behaviour of "git tag" here:
> "git tag -v" now can get more than one tag to verify, like "git tag -d" did,
> "git tag" with no arguments prints all tags, more like "git branch" does, and
> the template for the edited message adds an empty line, like in "git commit".
These probably are good changes (except that "no argument" case
might be a bit annoying, but I guess it's Ok). Is there any
need for documentation updates with these changes?
> diff --git a/builtin-tag.c b/builtin-tag.c
> new file mode 100644
> index 0000000..1824379
> --- /dev/null
> +++ b/builtin-tag.c
> @@ -0,0 +1,430 @@
> +/*
> + * Builtin "git tag"
> + *
> + * Copyright (c) 2007 Kristian H??gsberg <krh@xxxxxxxxxx>,
I do not know how the above would come out, but it surely was
not Høgsberg in the copy I received...
We probably should make sure our sources are in UTF-8; I suspect
builtin-branch.c is not.
> + * Carlos Rica <jasampler@xxxxxxxxx>
> + * Based on git-tag.sh and mktag.c by Linus Torvalds.
> + */
> +
> +#include "cache.h"
> +#include "builtin.h"
> +#include "refs.h"
> +#include "tag.h"
> +#include "run-command.h"
> +
> +static const char builtin_tag_usage[] =
> + "git-tag [-n [<num>]] -l [<pattern>] | [-a | -s | -u <key-id>] [-f | -d | -v] [-m <msg>] <tagname> [<head>]";
> +
> +static char signingkey[1000];
> +static int lines;
> +
> +static void launch_editor(const char *path, char **buffer, unsigned long *len)
> +{
> + const char *editor, *terminal;
> + struct child_process child;
> + const char *args[3];
> + int fd;
> +
> + editor = getenv("VISUAL");
> + if (!editor)
> + editor = getenv("EDITOR");
> +
> + terminal = getenv("TERM");
> + if (!editor && (!terminal || !strcmp(terminal, "dumb"))) {
> + fprintf(stderr,
> + "Terminal is dumb but no VISUAL nor EDITOR defined.\n"
> + "Please supply the message using either -m or -F option.\n");
> + exit(1);
> + }
Ah, this is taken from git-commit.sh ;-) Does your "git tag"
support the -F option (builtin_tag_usage[] does not seem to
mention it)? I wonder what happens when this function migrates
to editor.c and a new program, other than git-tag and
git-commit, that is without -F nor -m options wants to call
this.
> + if (!editor)
> + editor = "vi";
> +
> + memset(&child, 0, sizeof(child));
> + child.argv = args;
> + args[0] = editor;
> + args[1] = path;
> + args[2] = NULL;
> +
> + if (run_command(&child))
> + die("could not launch editor %s.", editor);
This message is not quite true, isn't it? The editor might have
been launched but exited with non-zero status.
> + fd = open(path, O_RDONLY);
> + if (fd == -1)
> + die("could not read %s.", path);
And this is "could not open", with probably strerror(errno) to
be helpful.
> + if (read_pipe(fd, buffer, len))
> + die("could not read message file '%s': %s",
> + path, strerror(errno));
> + close(fd);
> +}
> +
> +#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
> +
> +static int show_reference(const char *refname, const unsigned char *sha1,
> + int flag, void *cb_data)
> +{
> ...
> +}
Ok.
> +static int list_tags(const char *pattern)
> +{
> ...
> +}
Ok.
> +static int delete_tags(const char **argv)
> +{
> + const char **p;
> + char ref[PATH_MAX];
> + int had_error = 0;
> + unsigned char sha1[20];
> +
> + for (p = argv; *p; p++) {
> + if (snprintf(ref, sizeof ref, "refs/tags/%s", *p) > sizeof ref)
> + die("tag name '%s' too long.", *p);
"sizeof foo" may be proper ANSI C, but somehow many people seem
to find "sizeof(foo)" much easier to read, including me.
I think the overflow check is wrong. snprintf() does not write
more than sizeof(ref) including the training NUL, but when its
output is truncated, it returns the number of bytes that would
have been written to the buffer, excluding the NUL termination.
What this means is that if it returns sizeof(ref), it also is a
case of overflow. Further, if C library is not C99, e.g. older
glibc (say 2.0.6), it could return negative status indicating an
error condition upon truncated output.
I suspect you inherited these problems from builtin-branch.c,
but we'd better fix them there as well.
> + if (!resolve_ref(ref, sha1, 1, NULL)) {
> + fprintf(stderr, "tag '%s' not found.\n", *p);
> + had_error = 1;
> + continue;
> + }
We might want to call error("tag '%s' not found.") instead for
later libification...
> ...
> +}
> +
> +static int run_verify_tag_command(unsigned char *sha1)
> +{
> + int ret;
> + const char *argv_verify_tag[] = {"git-verify-tag",
> + "-v", "SHA1_HEX", NULL};
> + argv_verify_tag[2] = sha1_to_hex(sha1);
> +
> + ret = run_command_v_opt(argv_verify_tag, 0);
> +
> + if (ret <= -10000)
> + die("unable to run %s\n", argv_verify_tag[0]);
> + return -ret;
> +}
I wonder why you need to differentiate between ERR_RUN_COMMAND_*
and non-zero exit status... Also do you need to "return -ret",
instead of not negating? In C programs error returns are often
negative and finish_command() follows that convention.
> +static int verify_tags(const char **argv)
> +{
> ...
> +}
Ok.
> +static int do_sign(char *buffer, size_t size, size_t max)
> +{
> + struct child_process gpg;
> + const char *args[4];
> + char *bracket;
> + int len;
> +
> + if (signingkey[0] == '\0') {
> + strlcpy(signingkey, git_committer_info(1), sizeof signingkey);
> + bracket = strchr(signingkey, '>');
> + if (bracket)
> + bracket[1] = '\0';
> + }
Hmph. Silently truncate with strlcpy instead of erroring out on
insanely long committer info?
> + memset(&gpg, 0, sizeof(gpg));
> + gpg.argv = args;
> + gpg.in = -1;
> + gpg.out = -1;
> + args[0] = "gpg";
> + args[1] = "-bsau";
> + args[2] = signingkey;
> + args[3] = NULL;
> +
> + if (start_command(&gpg))
> + die("could not run gpg.");
> +
> + write_or_die(gpg.in, buffer, size);
> + close(gpg.in);
> + gpg.close_in = 0;
> + len = read_in_full(gpg.out, buffer + size, max - size);
Bi-di pipes makes me nervous, but this case should be Ok. It
could try writing "---BEGIN PGP SIGNATURE---" before reading
from you and get stuck because you are not reading from it but
that is only in theory.
I wonder why our read_in_full and write_in_full do not return
ssize_t although that's how they count things internally. At
least you could make do_sign() to return ssize_t, and change the
die() to "return error()" to have the caller deal with it, which
would be easier for people other than "git tag -s" to call this
if they later want to.
What happens when gpg gave more than you expect? We get
truncated signature and I do not see any code to notice the
breakage...
> +
> + finish_command(&gpg);
> +
> + return size + len;
> +}
> +
> +static const char tag_template[] =
> + "\n"
> + "#\n"
> + "# Write a tag message\n"
> + "#\n";
> +
> +static int git_tag_config(const char *var, const char *value)
> +{
> ...
> +}
Ok, except for the same strlcpy comment above.
> +#define MAX_SIGNATURE_LENGTH 1024
> +/* message must be NULL or allocated, it will be reallocated and freed */
> +static void create_tag(const unsigned char *object, const char *tag,
> + char *message, int sign, unsigned char *result)
> +{
> + enum object_type type;
> + char header_buf[1024], *buffer;
> + int header_len, max_size;
> + unsigned long size;
> +
> + type = sha1_object_info(object, NULL);
> + if (type <= 0)
> + die("bad object type.");
> +
> + header_len = snprintf(header_buf, sizeof header_buf,
> + "object %s\n"
> + "type %s\n"
> + "tag %s\n"
> + "tagger %s\n\n",
> + sha1_to_hex(object),
> + typename(type),
> + tag,
> + git_committer_info(1));
> +
> + if (header_len >= sizeof header_buf)
> + die("tag header too big.");
This comparison '>=' is correct (i.e. not '>'), as opposed to
the one I commented way above.
> + if (!message) {
> ...
> + }
> + else {
> + buffer = message;
> + size = strlen(message);
> + }
> +
> + size = stripspace(buffer, size, 1);
> +
> + if (!message && !size)
> + die("no tag message?");
Why check 'message' here?
> ...
> + free(buffer);
> +}
The rest is Ok.
> +int cmd_tag(int argc, const char **argv, const char *prefix)
> +{
> ...
> + if (!strcmp(arg, "-F")) {
> + unsigned long len;
> + annotate = 1;
> + i++;
> + if (i == argc)
> + die("option -F needs an argument.");
> +
> + fd = open(argv[i], O_RDONLY);
> + if (fd < 0)
> + die("cannot open %s", argv[1]);
The shell script version relies on the magic "cat -" to read
from standard input upon "git tag -F -". It is understandable
that both of you and Dscho missed it, though.
> + len = 1024;
> + message = xmalloc(len);
> + if (read_pipe(fd, &message, &len))
> + die("cannot read %s", argv[1]);
> + message = xrealloc(message, len + 1);
> + message[len] = '\0';
> + continue;
> + }
We might be better off if read_pipe() is renamed to read_fd()
and made internally always NUL-terminate the buffer (but not
count that NUL as part of length). I dunno.
> + if (!strcmp(arg, "-u")) {
> + annotate = 1;
> + sign = 1;
> + i++;
> + if (i == argc)
> + die("option -u needs an argument.");
> + strlcpy(signingkey, argv[i], sizeof signingkey);
> + continue;
Extra space "if (expr". Return from strlcpy() not checked?
> + if (snprintf(ref, sizeof ref, "refs/tags/%s", tag) > sizeof ref)
> + die("tag '%s' too long.", tag);
This use of snprintf() and checking the overflow as error is
much nicer, don't you think, instead of silently ignoring
truncation?
> ...
> +}
The remainder of cmd_tag() looks Ok.
-
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
