Make git tag a builtin.
Subsystems (2)
PATCH 1/1 Make git tag a builtin.
This replaces the script "git-tag.sh" with "builtin-tag.c".
The existing test suite for "git tag" guarantees the compatibility with the features provided by the script version.
There are some minor changes in the behaviour of "git tag" here:
"git tag -v" now can get more than one tag to verify, like "git tag -d" does, "git tag" with no arguments prints all tags, more like "git branch" does, and "git tag -n" also prints all tags with annotations (without needing -l). Tests and documentation were also updated to reflect these changes.
The program is currently calling the script "git verify-tag" for verify. This can be changed porting it to C and calling its functions directly from builtin-tag.c.
---
This patch is a resend including the suggestions and corrections commented by Junio in the previous proposal for builtin-tag.c:
http://article.gmane.org/gmane.comp.version-control.git/52246
In this version, a bit of refactoring was also done grouping the common code between the previous functions delete_tags() and verify_tags().
The function launch_editor will be moved to another file (editor.c is the suggested name), along with other tools for editing and reading text files, in order to share its code wiht other builtins like the upcoming builtin-commit.c from Kristian.
Documentation/git-tag.txt | 8 +- Makefile | 3 +- builtin-tag.c | 450 +++++++++++++++++++++++++++++ builtin.h | 1 + git-tag.sh => contrib/examples/git-tag.sh | 0 git.c | 1 + t/t7004-tag.sh | 88 +++++- 7 files changed, 538 insertions(+), 13 deletions(-) create mode 100644 builtin-tag.c rename git-tag.sh => contrib/examples/git-tag.sh (100%)
Documentation/git-tag.txt +5 -3
@@ -12,7 +12,7 @@ SYNOPSIS 'git-tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <name> [<head>] 'git-tag' -d <name>... 'git-tag' [-n [<num>]] -l [<pattern>] -'git-tag' -v <name> +'git-tag' -v <name>... DESCRIPTION -----------
@@ -23,7 +23,7 @@ Unless `-f` is given, the tag must not yet exist in If one of `-a`, `-s`, or `-u <key-id>` is passed, the command creates a 'tag' object, and requires the tag message. Unless -`-m <msg>` is given, an editor is started for the user to type +`-m <msg>` or `-F <file>` is given, an editor is started for the user to type in the tag message. Otherwise just the SHA1 object name of the commit object is
@@ -59,15 +59,17 @@ OPTIONS Delete existing tags with the given names. -v:: - Verify the gpg signature of given the tag + Verify the gpg signature of the given tag names. -n <num>:: <num> specifies how many lines from the annotation, if any, are printed when using -l. The default is not to print any annotation lines. + If no number is given to `-n`, only the first line is printed. -l <pattern>:: List tags with names that match the given pattern (or all if no pattern is given). + Typing "git tag" without arguments, also lists all tags. -m <msg>:: Use the given tag message (instead of prompting)
Makefile +2 -1
@@ -206,7 +206,7 @@ SCRIPT_SH = \ git-pull.sh git-rebase.sh git-rebase--interactive.sh \ git-repack.sh git-request-pull.sh git-reset.sh \ git-sh-setup.sh \ - git-tag.sh git-verify-tag.sh \ + git-verify-tag.sh \ git-am.sh \ git-merge.sh git-merge-stupid.sh git-merge-octopus.sh \ git-merge-resolve.sh git-merge-ours.sh \
@@ -361,6 +361,7 @@ BUILTIN_OBJS = \ builtin-show-branch.o \ builtin-stripspace.o \ builtin-symbolic-ref.o \ + builtin-tag.o \ builtin-tar-tree.o \ builtin-unpack-objects.o \ builtin-update-index.o \
builtin-tag.c new file +450 -0
450 lines not on this page · raw message
builtin.h +1 -0
@@ -70,6 +70,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix);extern int cmd_show_branch (int argc,const char **argv,const char *prefix);extern int cmd_stripspace (int argc,const char **argv,const char *prefix);extern int cmd_symbolic_ref (int argc,const char **argv,const char *prefix); +extern int cmd_tag (int argc,const char **argv,const char *prefix);extern int cmd_tar_tree (int argc,const char **argv,const char *prefix);extern int cmd_unpack_objects (int argc,const char **argv,const char *prefix);extern int cmd_update_index (int argc,const char **argv,const char *prefix);
contrib/examples/git-tag.sh renamed from git-tag.sh +0 -0
git.c +1 -0
@@ -363,6 +363,7 @@ static void handle_internal_command(int argc, const char **argv) { "show", cmd_show, RUN_SETUP | USE_PAGER }, { "stripspace", cmd_stripspace }, { "symbolic-ref", cmd_symbolic_ref, RUN_SETUP }, + { "tag", cmd_tag, RUN_SETUP }, { "tar-tree", cmd_tar_tree }, { "unpack-objects", cmd_unpack_objects, RUN_SETUP }, { "update-index", cmd_update_index, RUN_SETUP },
t/t7004-tag.sh +79 -9
141 lines not on this page · raw message
Signature · 1 line
1.5.0
Quoted 1 line · Carlos Rica
This replaces the script "
git-tag.sh" with "builtin-tag.c".
Thanks. Will queue in 'next', and perhaps with a few nit fixups to merge as the first thing after 1.5.3.
builtin-tag.c line 4
Quoted 10 lines
builtin-tag.c new file +4
-0@@ -0,0 +1,450 @@ +/* +* Builtin "git tag" +* +* Copyright (c) 2007 Kristian H??gsberg <[email protected]>,
I've fixed up the name here in my copy.
Quoted 11 lines
builtin-tag.c new file +11
-0+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" ); +
builtin-tag.c line 40
Quoted 10 lines
builtin-tag.c new file +10
-0+ 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 ); + } + +if (!editor) + editor ="vi" ;
launch-editor() would need adjustment for GIT_EDITOR and core.editor which should be straightforward.
Quoted 18 lines
builtin-tag.c new file +18
-0+struct tag_filter { +const char *pattern; +int lines; +}; + +#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----" + +static int show_reference (const char *refname,const unsigned char *sha1, +int flag,void *cb_data) +{ +struct tag_filter *filter = cb_data; + +if (!fnmatch (filter->pattern, refname,0 )) { +int i; +unsigned long size; +enum object_type type; +char *buf, *sp, *eol; +size_t len;
builtin-tag.c line 89
Quoted 10 lines
builtin-tag.c new file +10
-0+ +if (!filter->lines) { +printf ("%s\n" , refname); +return 0 ; + } +printf ("%-15s " , refname); + + sp = buf =read_sha1_file (sha1, &type, &size); +if (!buf || !size) +return 0 ;
Theoretically, I can create millions of lightweight tags, all of them pointing at a zero-length blob object (or an empty tree object) and kill you with memory leak here ;-).
builtin-tag.c line 97
Quoted 8 lines
builtin-tag.c new file +8
-0+/* skip header */ +while (sp +1 < buf + size && + !(sp[0 ] =='\n' && sp[1 ] =='\n' )) + sp++; +/* only take up to "lines" lines, and strip the signature */ +for (i =0 , sp +=2 ; i < filter->lines && sp < buf + size && +prefixcmp (sp,PGP_SIGNATURE "\n" ); + i++) {
Minor nit; I would have split this to four physical lines, like:
for (i = 0, sp += 2;
i < filter->lines && sp < buf + size &&
prefixcmp(sp, PGP_SIGNATURE "\n");
i++) {The places that semicolons appear are more significant gaps when reading the code.
Quoted 12 lines
builtin-tag.c new file +11
-0+int cmd_tag (int argc,const char **argv,const char *prefix) +{ +if (!strcmp (arg,"-F" )) { +unsigned long len; +int fd; + + annotate =1 ; + i++; +if (i == argc) +die ("option -F needs an argument." ); +
builtin-tag.c line 386
Quoted 10 lines
builtin-tag.c new file +10
-0+if (!strcmp (argv[i],"-" )) + fd =0 ; +else { + fd =open (argv[i],O_RDONLY ); +if (fd <0 ) +die ("could not open '%s': %s" , + argv[i],strerror (errno)); + } + len =1024 ; + message =xmalloc (len);
You cannot anticipate how many bytes the user will type (or pipe-in), but when you opened the file you could fstat() to see how many bytes you would need to hold the contents of that file. Even in stdin case fstat(fd) could tell you the size, but I am not sure how to tell if the st_size is reliable. But for the purposes of "git tag", 1k buffer that grows on demand is probably cheaper than a fstat() syscall.
builtin-tag.c line 391
Quoted 5 lines
builtin-tag.c new file +5
-0+if (read_fd (fd, &message, &len)) { +free (message); +die ("cannot read %s" , argv[i]); + } +continue ;
We seem to leak fd here, but the user is doing something insane if he gives more than one -F anyway, so it probably is Ok, as long as we have some sanity checks. Should we barf on "git tag -m foo -F bar"? What should we do with "git tag -m one -m two"?
Does the test suite check any of these conditions?
builtin-tag.c line 431
Quoted 2 lines
builtin-tag.c new file +2
-0+if (snprintf (ref,sizeof (ref),"refs/tags/%s" , tag) >=sizeof (ref)) +die ("tag name too long: %.*s..." ,50 , tag);
Cute and considerate ;-).
contrib/examples/git-tag.sh
Quoted 4 lines
contrib/examples/git-tag.sh renamed from git-tag.sh +0
-0
This actually caught my attention for completely different reason. If this were removal of git-tag.sh, this would have conflicted when applied on top of Adam's GIT_EDITOR/core.editor patch. With a rename, however, there is no preimage/context shown, so it just cleanly applied.
Also Johannes's final minor nits.
Hi,
Quoted 6 lines · Junio C Hamano
Quoted 1 line · Carlos Rica
This replaces the script "
git-tag.sh" with "builtin-tag.c".Thanks. Will queue in 'next', and perhaps with a few nit fixups to merge as the first thing after 1.5.3.
Nice!
Quoted 2 lines
launch-editor() would need adjustment for
GIT_EDITORand core.editor which should be straightforward.
Yes. As a separate commit? After a couple of revisions, I lost track of what I looked at, and what not... So it would make it easier for me to have these changes after the big builtin-tag commit.
Quoted 7 lines
builtin-tag.c line 89
Quoted 3 lines
builtin-tag.c new file +3
-0+ sp = buf =read_sha1_file (sha1, &type, &size); +if (!buf || !size) +return 0 ;Theoretically, I can create millions of lightweight tags, all of them pointing at a zero-length blob object (or an empty tree object) and kill you with memory leak here ;-).
Yes ;-) Would we really want to say
if (!buf)
return 0;
if (!size) {
free(buf);
return 0;
}here? IMHO that is overkill.
Quoted 11 lines
builtin-tag.c line 97
Quoted 8 lines
builtin-tag.c new file +8
-0+/* skip header */ +while (sp +1 < buf + size && + !(sp[0 ] =='\n' && sp[1 ] =='\n' )) + sp++; +/* only take up to "lines" lines, and strip the signature */ +for (i =0 , sp +=2 ; i < filter->lines && sp < buf + size && +prefixcmp (sp,PGP_SIGNATURE "\n" ); + i++) {Minor nit; I would have split this to four physical lines, like:
Quoted 7 lines
for (i = 0, sp += 2; i < filter->lines && sp < buf + size && prefixcmp(sp, PGP_SIGNATURE "\n"); i++) {The places that semicolons appear are more significant gaps when reading the code.
I am to blame for that. That code was outlined by me.
Quoted 23 lines
Quoted 12 lines
builtin-tag.c new file +11
-0+int cmd_tag (int argc,const char **argv,const char *prefix) +{ +if (!strcmp (arg,"-F" )) { +unsigned long len; +int fd; + + annotate =1 ; + i++; +if (i == argc) +die ("option -F needs an argument." ); +builtin-tag.c line 386
Quoted 10 lines
builtin-tag.c new file +10
-0+if (!strcmp (argv[i],"-" )) + fd =0 ; +else { + fd =open (argv[i],O_RDONLY ); +if (fd <0 ) +die ("could not open '%s': %s" , + argv[i],strerror (errno)); + } + len =1024 ; + message =xmalloc (len);
Quoted 7 lines
You cannot anticipate how many bytes the user will type (or pipe-in), but when you opened the file you could
fstat()to see how many bytes you would need to hold the contents of that file. Even in stdin case fstat(fd) could tell you the size, but I am not sure how to tell if thest_sizeis reliable. But for the purposes of "git tag", 1k buffer that grows on demand is probably cheaper than afstat()syscall.
For a one-shot program as git-tag, I agree, the current patch is sufficient.
Ciao,
Dscho
Hi,
builtin-tag.c line 4
Quoted 10 lines · Carlos Rica
builtin-tag.c new file +4
-0@@ -0,0 +1,450 @@ +/* +* Builtin "git tag" +* +* Copyright (c) 2007 Kristian Hv/gsberg <[email protected]>,
In case of encoding problems, I suggest uploading it to repo.or.cz, so it can be pulled. Uploading has more advantages, such as visibility and a free backup, too...
builtin-tag.c line 139
Quoted 4 lines
builtin-tag.c new file +4
-0+typedef int (*func_tag )(const char *name,const char *ref, +const unsigned char *sha1); + +static int do_tag_names (const char **argv,func_tag fn)
Neat! Just a minor style nit here, though: in refs.h, we have something similar, and I would have expected to read "each_tag_name_fn" and "for_each_tag_name" instead of "func_tag" and "do_tag_names", respectively. Hmm?
builtin-tag.c line 194
Quoted 10 lines
builtin-tag.c new file +10
-0+static ssize_t do_sign (char *buffer,size_t size,size_t max) +{ +struct child_process gpg; +const char *args[4 ]; +char *bracket; +int len; + +if (!*signingkey) { +if (strlcpy (signingkey,git_committer_info (1 ), +sizeof (signingkey)) >=sizeof (signingkey))
sizeof(signingkey) - 1?
builtin-tag.c line 221
Quoted 6 lines
builtin-tag.c new file +6
-0+ len =read_in_full (gpg.out, buffer + size, max - size); + +finish_command (&gpg); + +if (len == max - size) +return error ("could not read the entire signature from gpg." );
Maybe at a later stage, and if this turns out to be not sufficient to begin with, we might consider not using a buffer, but writing incrementally into an object right away. Think "git hash-object -w --stdin".
But at the moment, this is not a problem, so let's keep it as-is.
builtin-tag.c line 238
Quoted 7 lines
builtin-tag.c new file +7
-0+static int git_tag_config (const char *var,const char *value) +{ +if (!strcmp (var,"user.signingkey" )) { +if (!value) +die ("user.signingkey without value" ); +if (strlcpy (signingkey, value,sizeof (signingkey)) + >=sizeof (signingkey))
sizeof(signingkey) - 1?
builtin-tag.c line 257
Quoted 10 lines
builtin-tag.c new file +10
-0+static void create_tag (const unsigned char *object,const char *tag, +char *message,int sign,unsigned char *result) +{ +enum object_type type; +char header_buf[1024 ], *buffer; +int header_len, max_size; +unsigned long size; + + type =sha1_object_info (object,NULL ); +if (type <=0 )
Maybe use OBJ_NONE instead of 0? Dunno.
builtin-tag.c line 270
Quoted 13 lines
builtin-tag.c new file +13
-0+die ("bad object type." ); + + header_len =snprintf (header_buf,sizeof (header_buf), +"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 (header_len >=sizeof (header_buf))
sizeof(header_buf) - 1?
builtin-tag.c line 306
Quoted 1 line
builtin-tag.c new file +1
-0+memmove (buffer + header_len, buffer, size);
Just two ideas for the future (it is not really important now, so we should not do it now):
- Instead of memmove()ing, we could teach read_fd() to take an offset, too.
- launch_editor() could learn to write the buffer itself (IMHO it is more common to write a buffer to the file before editing it, than editing an existing file).
But as I said, this is just something to keep in mind, not to change now.
Quoted 10 lines
builtin-tag.c new file +10
-0+if (!strcmp (arg,"-F" )) { +unsigned long len; +int fd; + + annotate =1 ; + i++; +if (i == argc) +die ("option -F needs an argument." ); + +if (!strcmp (argv[i],"-" ))
builtin-tag.c line 387
Quoted 10 lines
builtin-tag.c new file +10
-0+ fd =0 ; + else { + fd =open (argv[i],O_RDONLY ); +if (fd <0 ) +die ("could not open '%s': %s" , + argv[i],strerror (errno)); + } + len =1024 ; + message =xmalloc (len); +if (read_fd (fd, &message, &len)) {
With your recent sanification of read_fd(), you can initialise len and message to 0 and NULL, respectively.
builtin-tag.c line 400
Quoted 8 lines
builtin-tag.c new file +8
-0+if (!strcmp (arg,"-u" )) { + annotate =1 ; + sign =1 ; + i++; +if (i == argc) +die ("option -u needs an argument." ); +if (strlcpy (signingkey, argv[i],sizeof (signingkey)) + >=sizeof (signingkey))
sizeof(signingkey) - 1?
builtin-tag.c line 412
Quoted 12 lines
builtin-tag.c new file +12
-0+die ("argument to option -u too long" ); +continue ; + } +if (!strcmp (arg,"-l" )) { +return list_tags (argv[i +1 ], lines); + } +if (!strcmp (arg,"-d" )) { +return do_tag_names (argv + i +1 , delete_tag); + } +if (!strcmp (arg,"-v" )) { +return do_tag_names (argv + i +1 , verify_tag); + }
Please lose the "{..}" around single lines.
builtin-tag.c line 430
Quoted 4 lines
builtin-tag.c new file +4
-0+if (get_sha1 (object_ref, object)) +die ("Failed to resolve '%s' as a valid ref." , object_ref); + +if (snprintf (ref,sizeof (ref),"refs/tags/%s" , tag) >=sizeof (ref))
sizeof(ref) - 1?
contrib/examples/git-tag.sh
Quoted 4 lines
contrib/examples/git-tag.sh renamed from git-tag.sh +0
-0
That should make Nico and Junio happy.
These are my last minor nits. If nobody else has objections, let's put this ship to sea.
Ciao,
Dscho