Junio C Hamano wrote:
Kristian Høgsberg <krh@xxxxxxxxxx> writes:
Content-Type: TEXT/PLAIN; charset=ISO-8859-1
From: Kristian Høgsberg <krh@xxxxxxxxxx>
A more or less straight-forward port of git-tag.sh to C.
Signed-off-by: Kristian Høgsberg <krh@xxxxxxxxxx>
Cc: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
