Re: [PATCH] Port git-tag.sh to C.

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 9, 2007, 18:26 UTC
Message-ID: <7v7iqdf0gn.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11813427591137-git-send-email-krh@redhat.com>

Kristian Høgsberg <krh@redhat.com> writes:

Show 8 quoted lines

> Content-Type: TEXT/PLAIN; charset=ISO-8859-1
>
> From: Kristian HÃ¸gsberg <krh@redhat.com>
>
> A more or less straight-forward port of git-tag.sh to C.
>
> Signed-off-by: Kristian HÃ¸gsberg <krh@redhat.com>
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>

I think your name in your commit message is in UTF-8 but munged your mail was mismarked as iso-8859-1.

> +static int launch_editor(const char *path, const char *template,
> +			  char *buffer, size_t size)
> +{

It would have been nicer to have this in editor.c or somesuch, as other commands will be redone in C in the future.

We could do the moving later, but the problem is that later is conditional: "if we are lucky enough to remember that we already have this function in builtin-tag when doing so".

> +	fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0644);

I would understand an argument to use 0666 (honor umask) or 0600 (this is a temporary file and others have no business looking at it while an edit is in progress), but I cannot justify 0644.

> +	fd = open(path, O_RDONLY, 0644);

Open for reading with mode ;-)?

Show 19 quoted lines

> +	if (fd == -1)
> +		die("could not read %s.", path);
> +	len = read_in_full(fd, buffer, size);
> +	if (len < 0)
> +		die("failed to read '%s', %m", path);
> +	close(fd);
> +
> +	blank_lines = 1;
> +	for (i = 0, j = 0; i < len; i++) {
> ...
> +	}
> +
> +	if (buffer[j - 1] != '\n')
> +		buffer[j++] = '\n';
> +
> +	unlink(path);
> +
> +	return j;
> +}

I really think this function needs to be refactored into three.

 * A generic "spawn an editor with this initial seed template,
   return the result of editing in memory and also give exit
   status of the editor" function that does not take path
   parameter (instead perhaps mkstemp a temporary file on your
   own);

 * A function that does what git-stripspace does in core;

 * A function for builtin-tag to use, that calls the above two
   and uses the result (e.g. "did the user kill the editor?
   does the resulting buffer have any nonempty line?") to decide
   what it does.

Show 33 quoted lines

> +static void create_tag(const unsigned char *object, const char *tag,
> +		       const char *message, int sign, unsigned char *result)
> +{
> +	enum object_type type;
> +	char buffer[4096];
> +	int header, body, total;
> +
> +	type = sha1_object_info(object, NULL);
> +	if (type <= 0)
> +	    die("bad object type.");
> +
> +	header = snprintf(buffer, sizeof buffer,
> +			  "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 (message == NULL)
> +		body = launch_editor(git_path("TAGMSG"), tag_template,
> +				     buffer + header, sizeof buffer - header);
> +	else
> +		body = snprintf(buffer + header, sizeof buffer - header,
> +				"%s\n", message);
> +
> +	if (body == 0)
> +		die("no tag message?");
> +
> +	if (header + body > sizeof buffer)
> +		die("tag message too big.");

Two issues:

 * It used to be a tag had limit of 8kB which was lifted some
   time ago; now it is limited to 4kB.  Fixing this implies that
   the "launch editor and get results in core" function I
   mentioned above may need to realloc, and probably the buffer
   is better passed as (char *, ulong) pair as done everywhere
   else (although we know this is text so you can pass only a
   pointer and have the user run strlen() when needed).

 * I do not see any validation on the value of "tag".  Do we want
   to allow passing "" to it?  What about "my\ntag"?