From: "Carlos Rica" <jasampler@gmail.com>
To: "Junio C Hamano" <gitster@pobox.com>
Cc: git@vger.kernel.org,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"Kristian Høgsberg" <krh@redhat.com>
Subject: Re: [PATCH 2/2] Make git-tag a builtin.
Date: Thu, 12 Jul 2007 23:01:57 +0200 [thread overview]
Message-ID: <1b46aba20707121401n12796827n77b7e1d1ccde330c@mail.gmail.com> (raw)
In-Reply-To: <7vps2yxjgq.fsf@assigned-by-dhcp.cox.net>
Thank you for your review, Junio, I'm studing all of your suggestions.
2007/7/12, Junio C Hamano <gitster@pobox.com>:
> Carlos Rica <jasampler@gmail.com> 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@redhat.com>,
>
> 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@gmail.com>
> > + * 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.
prev parent reply other threads:[~2007-07-12 21:02 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-07-11 18:54 [PATCH 2/2] Make git-tag a builtin Carlos Rica
2007-07-12 5:46 ` Junio C Hamano
2007-07-12 21:01 ` Carlos Rica [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1b46aba20707121401n12796827n77b7e1d1ccde330c@mail.gmail.com \
--to=jasampler@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=krh@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).