Re: [PATCH] Make git tag a builtin.

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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@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?

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