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

["Kristian Høgsberg" <krh@redhat.com>]

Junio C Hamano wrote:
> 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.

That's odd both the email I cc'ed to my redhat.com address and the one I got 
on gmail.com through the list have

   Content-Type: text/plain; charset=utf-8

and saving the raw message and asking /usr/bin/file, it tells me its

   /home/krh/Desktop/hep: UTF-8 Unicode mail 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".

Yeah, true.  I did write it as a generally usable "launch editor" functions, 
but I didn't want to move it until there was a second user.  Is there anything 
else that git-commit that will use this, btw?

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

Oh hehe, yeah, I didn't think much about the permission bits... 0666 sounds fine.

>> +	fd = open(path, O_RDONLY, 0644);
> 
> Open for reading with mode ;-)?

Even less thinking here :)

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

and that last function should be useful for git-commit too, no?  The reason I 
didn't split it up was that I don't see any use for the two parts on their 
own, and the functions is a total of 70 lines.  And wouldn't returning 0 bytes 
for either an empty buffer or editor quit be sufficient?  Do we need to handle 
the two cases differently?

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

Oh... read_pipe() reallocs... OK.  I took the limit from mktag.c and didn't 
realize that the buffer could get realloced.  I did wonder about the xmalloc() 
for a fixed buffer though...

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

I do

         if (check_ref_format(ref))
                 die("'%s' is not a valid tag name.", tag);

on the ref, which is what git-tag.sh ends up doing, and it catches the two 
examples you mention:

[krh@dinky git]$ ./git tag "hello
 > world"
fatal: 'hello
world' is not a valid tag name.
[krh@dinky git]$ ./git tag ""
fatal: '' is not a valid tag name.

so I think it's fine.

But I think I'll leave it to Carlos to finish his builtin-tag work, and he can 
cherry pick the bits from my patch that work for him.  This was more of a 
friday afternoon hacking sprint, for my part.

cheers,
Kristian