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

> Kristian Høgsberg <krh@redhat.com> writes:
> 
>> 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.

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

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