Hi,
On Fri, 20 Jul 2007, Carlos Rica wrote:
> diff --git a/builtin-tag.c b/builtin-tag.c
> new file mode 100644
> index 0000000..507f510
> --- /dev/null
> +++ b/builtin-tag.c
> @@ -0,0 +1,450 @@
> +/*
> + * Builtin "git tag"
> + *
> + * Copyright (c) 2007 Kristian Hv/gsberg <krh@xxxxxxxxxx>,
In case of encoding problems, I suggest uploading it to repo.or.cz, so it
can be pulled. Uploading has more advantages, such as visibility and
a free backup, too...
> +typedef int (*func_tag)(const char *name, const char *ref,
> + const unsigned char *sha1);
> +
> +static int do_tag_names(const char **argv, func_tag fn)
Neat! Just a minor style nit here, though: in refs.h, we have something
similar, and I would have expected to read "each_tag_name_fn" and
"for_each_tag_name" instead of "func_tag" and "do_tag_names",
respectively. Hmm?
> +static ssize_t do_sign(char *buffer, size_t size, size_t max)
> +{
> + struct child_process gpg;
> + const char *args[4];
> + char *bracket;
> + int len;
> +
> + if (!*signingkey) {
> + if (strlcpy(signingkey, git_committer_info(1),
> + sizeof(signingkey)) >= sizeof(signingkey))
sizeof(signingkey) - 1?
> + len = read_in_full(gpg.out, buffer + size, max - size);
> +
> + finish_command(&gpg);
> +
> + if (len == max - size)
> + return error("could not read the entire signature from gpg.");
Maybe at a later stage, and if this turns out to be not sufficient to
begin with, we might consider not using a buffer, but writing
incrementally into an object right away. Think "git hash-object -w
--stdin".
But at the moment, this is not a problem, so let's keep it as-is.
> +static int git_tag_config(const char *var, const char *value)
> +{
> + if (!strcmp(var, "user.signingkey")) {
> + if (!value)
> + die("user.signingkey without value");
> + if (strlcpy(signingkey, value, sizeof(signingkey))
> + >= sizeof(signingkey))
sizeof(signingkey) - 1?
> +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)
Maybe use OBJ_NONE instead of 0? Dunno.
> + 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))
sizeof(header_buf) - 1?
> + memmove(buffer + header_len, buffer, size);
Just two ideas for the future (it is not really important now, so we
should not do it now):
- Instead of memmove()ing, we could teach read_fd() to take an offset,
too.
- launch_editor() could learn to write the buffer itself (IMHO it is more
common to write a buffer to the file before editing it, than editing an
existing file).
But as I said, this is just something to keep in mind, not to change now.
> + if (!strcmp(arg, "-F")) {
> + unsigned long len;
> + int fd;
> +
> + annotate = 1;
> + i++;
> + if (i == argc)
> + die("option -F needs an argument.");
> +
> + if (!strcmp(argv[i], "-"))
> + fd = 0;
> + else {
> + fd = open(argv[i], O_RDONLY);
> + if (fd < 0)
> + die("could not open '%s': %s",
> + argv[i], strerror(errno));
> + }
> + len = 1024;
> + message = xmalloc(len);
> + if (read_fd(fd, &message, &len)) {
With your recent sanification of read_fd(), you can initialise len and
message to 0 and NULL, respectively.
> + if (!strcmp(arg, "-u")) {
> + annotate = 1;
> + sign = 1;
> + i++;
> + if (i == argc)
> + die("option -u needs an argument.");
> + if (strlcpy(signingkey, argv[i], sizeof(signingkey))
> + >= sizeof(signingkey))
sizeof(signingkey) - 1?
> + die("argument to option -u too long");
> + continue;
> + }
> + if (!strcmp(arg, "-l")) {
> + return list_tags(argv[i + 1], lines);
> + }
> + if (!strcmp(arg, "-d")) {
> + return do_tag_names(argv + i + 1, delete_tag);
> + }
> + if (!strcmp(arg, "-v")) {
> + return do_tag_names(argv + i + 1, verify_tag);
> + }
Please lose the "{..}" around single lines.
> + if (get_sha1(object_ref, object))
> + die("Failed to resolve '%s' as a valid ref.", object_ref);
> +
> + if (snprintf(ref, sizeof(ref), "refs/tags/%s", tag) >= sizeof(ref))
sizeof(ref) - 1?
> diff --git a/git-tag.sh b/contrib/examples/git-tag.sh
> similarity index 100%
> rename from git-tag.sh
> rename to contrib/examples/git-tag.sh
That should make Nico and Junio happy.
These are my last minor nits. If nobody else has objections, let's put
this ship to sea.
Ciao,
Dscho
-
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
