From: Kristian Høgsberg Date: Sat, 09 Jun 2007 23:27:41 GMT Subject: Re: [PATCH] Port git-tag.sh to C. Message-ID: <466B376D.8040303@redhat.com> In-Reply-To: <7v7iqdf0gn.fsf@assigned-by-dhcp.cox.net> Junio C Hamano wrote: > Kristian Høgsberg writes: > >> Content-Type: TEXT/PLAIN; charset=ISO-8859-1 >> >> From: Kristian Høgsberg >> >> A more or less straight-forward port of git-tag.sh to C. >> >> Signed-off-by: Kristian Høgsberg >> Cc: Johannes Schindelin > > 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