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.
> +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.
> + fd = open(path, O_RDONLY, 0644);
Open for reading with mode ;-)?
> + 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).
* 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
