Thank you for your review, Junio, I'm studing all of your suggestions.
2007/7/12, Junio C Hamano <gitster@xxxxxxxxx>:
Carlos Rica <jasampler@xxxxxxxxx> writes:
> Mime-Version: 1.0
> Content-Type: text/plain; charset=windows-1252
Hmmmmmm.....
> diff --git a/builtin-tag.c b/builtin-tag.c
> new file mode 100644
> index 0000000..1824379
> --- /dev/null
> +++ b/builtin-tag.c
> @@ -0,0 +1,430 @@
> +/*
> + * Builtin "git tag"
> + *
> + * Copyright (c) 2007 Kristian H??gsberg <krh@xxxxxxxxxx>,
I do not know how the above would come out, but it surely was
not Høgsberg in the copy I received...
I think it is my fault. My original builtin-tag.c file has the
name encoded in UTF-8, and the patch too, except because
I also added his name in the commit's message in Latin-1 encoding,
and then, programs cannot recode or recognize both. Sorry.
> + * Carlos Rica <jasampler@xxxxxxxxx>
> + * Based on git-tag.sh and mktag.c by Linus Torvalds.
> + */
> +
> +#include "cache.h"
> +#include "builtin.h"
> +#include "refs.h"
> +#include "tag.h"
> +#include "run-command.h"
> +
> +static const char builtin_tag_usage[] =
> + "git-tag [-n [<num>]] -l [<pattern>] | [-a | -s | -u <key-id>] [-f | -d | -v] [-m <msg>] <tagname> [<head>]";
> +
> +static char signingkey[1000];
> +static int lines;
> +
> +static void launch_editor(const char *path, char **buffer, unsigned long *len)
> +{
> + const char *editor, *terminal;
> + struct child_process child;
> + const char *args[3];
> + int fd;
> +
> + editor = getenv("VISUAL");
> + if (!editor)
> + editor = getenv("EDITOR");
> +
> + terminal = getenv("TERM");
> + if (!editor && (!terminal || !strcmp(terminal, "dumb"))) {
> + fprintf(stderr,
> + "Terminal is dumb but no VISUAL nor EDITOR defined.\n"
> + "Please supply the message using either -m or -F option.\n");
> + exit(1);
> + }
Ah, this is taken from git-commit.sh ;-) Does your "git tag"
support the -F option (builtin_tag_usage[] does not seem to
mention it)? I wonder what happens when this function migrates
to editor.c and a new program, other than git-tag and
git-commit, that is without -F nor -m options wants to call
this.
I will include that -F option in the builtin_tag_usage and I will also find
if documentation already has it.
As I said above, launch_editor is copied and modified from the builtin-commit.c
that Kristian is currently writing. Indeed, I removed every mention on
it related
specifically to commits, so it will be difficult to share the code between both
versions in the current state. Perhaps a raw function not printing out errors
(and returning error codes instead) could be the only option to reuse it. I need
to ask Kristian for this.
> +static int run_verify_tag_command(unsigned char *sha1)
> +{
> + int ret;
> + const char *argv_verify_tag[] = {"git-verify-tag",
> + "-v", "SHA1_HEX", NULL};
> + argv_verify_tag[2] = sha1_to_hex(sha1);
> +
> + ret = run_command_v_opt(argv_verify_tag, 0);
> +
> + if (ret <= -10000)
> + die("unable to run %s\n", argv_verify_tag[0]);
> + return -ret;
> +}
I wonder why you need to differentiate between ERR_RUN_COMMAND_*
and non-zero exit status... Also do you need to "return -ret",
instead of not negating? In C programs error returns are often
negative and finish_command() follows that convention.
The point here was to mimic the behaviour of git-tag.sh,
returning the exit status returned by the called script: "git-verify-tag.sh".
However, I didn't realize that git-verify tag.sh was exiting only with 1 or 0,
so "return ret;" will also work since the call to the function is:
if (run_verify_tag_command(sha1))
had_error = 1;
When die is called (because of an ERR_RUN_COMMAND_* error condition),
it will return 128 to the system, and won't do more processing to other tags.
Do yo mean that it would be better to remove this "die" call here?
> + if (!message) {
> ...
> + }
> + else {
> + buffer = message;
> + size = strlen(message);
> + }
> +
> + size = stripspace(buffer, size, 1);
> +
> + if (!message && !size)
> + die("no tag message?");
Why check 'message' here?
Because the behaviour of git-tag.sh here is not allow empty
messages when the editor is executed, but allow them when
-m or -F options are given (message then will be not NULL).
> +int cmd_tag(int argc, const char **argv, const char *prefix)
> +{
> ...
> + if (!strcmp(arg, "-F")) {
> + unsigned long len;
> + annotate = 1;
> + i++;
> + if (i == argc)
> + die("option -F needs an argument.");
> +
> + fd = open(argv[i], O_RDONLY);
> + if (fd < 0)
> + die("cannot open %s", argv[1]);
The shell script version relies on the magic "cat -" to read
from standard input upon "git tag -F -". It is understandable
that both of you and Dscho missed it, though.
It should be easy to implement, I will do it.
> + len = 1024;
> + message = xmalloc(len);
> + if (read_pipe(fd, &message, &len))
> + die("cannot read %s", argv[1]);
> + message = xrealloc(message, len + 1);
> + message[len] = '\0';
> + continue;
> + }
We might be better off if read_pipe() is renamed to read_fd()
and made internally always NUL-terminate the buffer (but not
count that NUL as part of length). I dunno.
I saw an implementation for read_pipe named read_fd from Kristian:
http://article.gmane.org/gmane.comp.version-control.git/51366
but it is only focused on allowing NULL and 0 in pointer and size
parameters. It should be harmless NUL-terminating the buffer
since the routine is already allocating more memory than needed
for efficiency purposes.
-
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
