Re: [PATCH] Make git tag a builtin.
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jul 20, 2007, 08:53 UTC
- Message-ID
- <Pine.LNX.4.64.0707200928050.14781@racer.site>
- In-Reply-To
- <469FF6E4.9010501@gmail.com>
Hi,
On Fri, 20 Jul 2007, Carlos Rica wrote:
Show 10 quoted lines
> 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@redhat.com>,
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?
Show 10 quoted lines
> +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?
Show 6 quoted lines
> + 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.
Show 7 quoted lines
> +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?
Show 10 quoted lines
> +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.
Show 13 quoted lines
> + 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.
Show 20 quoted lines
> + 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.
Show 8 quoted lines
> + 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?
Show 12 quoted lines
> + 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