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

quoted text
> 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.

quoted text
> +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".

quoted text
> +	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.

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

Open for reading with mode ;-)?

quoted text
> +	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.

quoted text
> +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"?


-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html