{"thread":{"id":"10607","subject":"[PATCH] Reuse previous annotation when overwriting a tag","startedAt":"2007-11-03T09:31:13Z","lastAt":"2007-11-07T21:51:21Z","messageCount":16,"participants":["Mike Hommey","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"58084","messageId":"1194082273-19486-1-git-send-email-mh@glandium.org","threadId":"10607","inReplyTo":null,"subject":"[PATCH] Reuse previous annotation when overwriting a tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-03T09:31:13Z","receivedAt":"2007-11-03T09:31:13Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When forcing to overwrite an annotated tag, there are good chances one\nwants to keep the old annotation, or modify it, not start from scratch.\n\nThis is obviously only triggered for annotated tagging (-a or -s).\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\nThe write_annotation function could be made more generic and be used in\nvarious different coming builtins such as git-commit. Also, it could be\nused in show_reference() in builtin-tag.c.\n\n builtin-tag.c |   47 ++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 44 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 66e5a58..cfd8017 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -247,9 +247,42 @@ static int git_tag_config(const char *var, const char *value)\n \treturn git_default_config(var, value);\n }\n \n+static void write_annotation(int fd, const unsigned char *sha1)\n+{\n+\tint i;\n+\tunsigned long size;\n+\tenum object_type type;\n+\tchar *buf, *sp, *eol;\n+\tsize_t len;\n+\n+\tsp = buf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn;\n+\tif (!size || (type != OBJ_TAG)) {\n+\t\tfree(buf);\n+\t\treturn;\n+\t}\n+\t/* skip header */\n+\twhile (sp + 1 < buf + size &&\n+\t\t\t!(sp[0] == '\\n' && sp[1] == '\\n'))\n+\t\tsp++;\n+\t/* strip the signature */\n+\tfor (i = 0, sp += 2; sp < buf + size &&\n+\t\t\tprefixcmp(sp, PGP_SIGNATURE \"\\n\");\n+\t\t\ti++) {\n+\t\teol = memchr(sp, '\\n', size - (sp - buf));\n+\t\tlen = eol ? eol - sp : size - (sp - buf);\n+\t\twrite_or_die(fd, sp, len + 1);\n+\t\tif (!eol)\n+\t\t\tbreak;\n+\t\tsp = eol + 1;\n+\t}\n+\tfree(buf);\n+}\n+\n static void create_tag(const unsigned char *object, const char *tag,\n \t\t       struct strbuf *buf, int message, int sign,\n-\t\t\t   unsigned char *result)\n+\t\t\tunsigned char *prev, unsigned char *result)\n {\n \tenum object_type type;\n \tchar header_buf[1024];\n@@ -282,6 +315,10 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\tif (fd < 0)\n \t\t\tdie(\"could not create file '%s': %s\",\n \t\t\t\t\t\tpath, strerror(errno));\n+\n+\t\tif (prev)\n+\t\t\twrite_annotation(fd, prev);\n+\n \t\twrite_or_die(fd, tag_template, strlen(tag_template));\n \t\tclose(fd);\n \n@@ -308,7 +345,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf;\n \tunsigned char object[20], prev[20];\n-\tint annotate = 0, sign = 0, force = 0, lines = 0, message = 0;\n+\tint annotate = 0, sign = 0, force = 0, lines = 0,\n+\t    message = 0, existed = 0;\n \tchar ref[PATH_MAX];\n \tconst char *object_ref, *tag;\n \tint i;\n@@ -417,9 +455,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\thashclr(prev);\n \telse if (!force)\n \t\tdie(\"tag '%s' already exists\", tag);\n+\telse\n+\t\texisted = 1;\n \n \tif (annotate)\n-\t\tcreate_tag(object, tag, &buf, message, sign, object);\n+\t\tcreate_tag(object, tag, &buf, message, sign,\n+\t\t\t   existed ? prev : NULL, object);\n \n \tlock = lock_any_ref_for_update(ref, prev, 0);\n \tif (!lock)\n-- \n1.5.3.5\n"},{"id":"58105","messageId":"Pine.LNX.4.64.0711031148460.4362@racer.site","threadId":"10607","inReplyTo":"1194082273-19486-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] Reuse previous annotation when overwriting a tag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-03T11:54:38Z","receivedAt":"2007-11-03T11:54:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 3 Nov 2007, Mike Hommey wrote:\n\n> diff --git a/builtin-tag.c b/builtin-tag.c\n> index 66e5a58..cfd8017 100644\n> --- a/builtin-tag.c\n> +++ b/builtin-tag.c\n> @@ -247,9 +247,42 @@ static int git_tag_config(const char *var, const char *value)\n>  \treturn git_default_config(var, value);\n>  }\n>  \n> +static void write_annotation(int fd, const unsigned char *sha1)\n\nTechnically, it is the \"body\".\n\n> +{\n> +\tint i;\n> +\tunsigned long size;\n> +\tenum object_type type;\n> +\tchar *buf, *sp, *eol;\n> +\tsize_t len;\n> +\n> +\tsp = buf = read_sha1_file(sha1, &type, &size);\n> +\tif (!buf)\n> +\t\treturn;\n> +\tif (!size || (type != OBJ_TAG)) {\n\nPlease lose the extra parents.\n\n> +\t\tfree(buf);\n> +\t\treturn;\n> +\t}\n> +\t/* skip header */\n> +\twhile (sp + 1 < buf + size &&\n> +\t\t\t!(sp[0] == '\\n' && sp[1] == '\\n'))\n> +\t\tsp++;\n\nThis can be done much easier with 'sp = strstr(buf, \"\\n\\n\");'.  You can \neven do that before the previous if(), to free() && return if there is no \nbody.\n\n> +\t/* strip the signature */\n> +\tfor (i = 0, sp += 2; sp < buf + size &&\n> +\t\t\tprefixcmp(sp, PGP_SIGNATURE \"\\n\");\n> +\t\t\ti++) {\n> +\t\teol = memchr(sp, '\\n', size - (sp - buf));\n> +\t\tlen = eol ? eol - sp : size - (sp - buf);\n> +\t\twrite_or_die(fd, sp, len + 1);\n> +\t\tif (!eol)\n> +\t\t\tbreak;\n> +\t\tsp = eol + 1;\n> +\t}\n> +\tfree(buf);\n\nThis can be done much easier with 'eob = strstr(sp, \"\\n\" PGP_SIGNATURE \n\"\\n\");'.\n\n> +}\n> +\n>  static void create_tag(const unsigned char *object, const char *tag,\n>  \t\t       struct strbuf *buf, int message, int sign,\n> -\t\t\t   unsigned char *result)\n> +\t\t\tunsigned char *prev, unsigned char *result)\n\nThis changes indentation.\n\n> @@ -282,6 +315,10 @@ static void create_tag(const unsigned char *object, const char *tag,\n>  \t\tif (fd < 0)\n>  \t\t\tdie(\"could not create file '%s': %s\",\n>  \t\t\t\t\t\tpath, strerror(errno));\n> +\n> +\t\tif (prev)\n> +\t\t\twrite_annotation(fd, prev);\n> +\n>  \t\twrite_or_die(fd, tag_template, strlen(tag_template));\n\nIsn't an \"else\" missing before the write_or_die() here?\n\n> @@ -308,7 +345,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct strbuf buf;\n>  \tunsigned char object[20], prev[20];\n> -\tint annotate = 0, sign = 0, force = 0, lines = 0, message = 0;\n> +\tint annotate = 0, sign = 0, force = 0, lines = 0,\n> +\t    message = 0, existed = 0;\n>  \tchar ref[PATH_MAX];\n>  \tconst char *object_ref, *tag;\n>  \tint i;\n> @@ -417,9 +455,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t\thashclr(prev);\n>  \telse if (!force)\n>  \t\tdie(\"tag '%s' already exists\", tag);\n> +\telse\n> +\t\texisted = 1;\n>  \n>  \tif (annotate)\n> -\t\tcreate_tag(object, tag, &buf, message, sign, object);\n> +\t\tcreate_tag(object, tag, &buf, message, sign,\n> +\t\t\t   existed ? prev : NULL, object);\n\nWhy not teach write_annotations() (or write_tag_body() like I would prefer \nit to be called) to grok a null_sha1?  It's not like we care for \nperformance here, but rather for readability and ease of use.\n\nCiao,\nDscho\n"},{"id":"58111","messageId":"20071103121002.GA4295@glandium.org","threadId":"10607","inReplyTo":"Pine.LNX.4.64.0711031148460.4362@racer.site","subject":"Re: [PATCH] Reuse previous annotation when overwriting a tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-03T12:10:02Z","receivedAt":"2007-11-03T12:10:02Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:\n> > +{\n> > +\tint i;\n> > +\tunsigned long size;\n> > +\tenum object_type type;\n> > +\tchar *buf, *sp, *eol;\n> > +\tsize_t len;\n> > +\n> > +\tsp = buf = read_sha1_file(sha1, &type, &size);\n> > +\tif (!buf)\n> > +\t\treturn;\n> > +\tif (!size || (type != OBJ_TAG)) {\n> \n> Please lose the extra parents.\n\nWhat do you mean ?\n\n(...)\n> This can be done much easier with 'sp = strstr(buf, \"\\n\\n\");'.  You can \n> even do that before the previous if(), to free() && return if there is no \n> body.\n(...)\n> This can be done much easier with 'eob = strstr(sp, \"\\n\" PGP_SIGNATURE \n> \"\\n\");'.\n\nI must say I just stole most of it in show_reference() in the same file.\n\n> > +}\n> > +\n> >  static void create_tag(const unsigned char *object, const char *tag,\n> >  \t\t       struct strbuf *buf, int message, int sign,\n> > -\t\t\t   unsigned char *result)\n> > +\t\t\tunsigned char *prev, unsigned char *result)\n> \n> This changes indentation.\n\nI'll fix this.\n\n> > @@ -282,6 +315,10 @@ static void create_tag(const unsigned char *object, const char *tag,\n> >  \t\tif (fd < 0)\n> >  \t\t\tdie(\"could not create file '%s': %s\",\n> >  \t\t\t\t\t\tpath, strerror(errno));\n> > +\n> > +\t\tif (prev)\n> > +\t\t\twrite_annotation(fd, prev);\n> > +\n> >  \t\twrite_or_die(fd, tag_template, strlen(tag_template));\n> \n> Isn't an \"else\" missing before the write_or_die() here?\n\nYou're obviously right.\n\n(...)\n> Why not teach write_annotations() (or write_tag_body() like I would prefer \n> it to be called) to grok a null_sha1?  It's not like we care for \n> performance here, but rather for readability and ease of use.\n\nI would have if I had looked up for is_null_sha1() earlier ;)\n\nCheers,\n\nMike\n"},{"id":"58118","messageId":"Pine.LNX.4.64.0711031219530.4362@racer.site","threadId":"10607","inReplyTo":"20071103121002.GA4295@glandium.org","subject":"Re: [PATCH] Reuse previous annotation when overwriting a tag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-03T12:23:55Z","receivedAt":"2007-11-03T12:23:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 3 Nov 2007, Mike Hommey wrote:\n\n> On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:\n> > > +{\n> > > +\tint i;\n> > > +\tunsigned long size;\n> > > +\tenum object_type type;\n> > > +\tchar *buf, *sp, *eol;\n> > > +\tsize_t len;\n> > > +\n> > > +\tsp = buf = read_sha1_file(sha1, &type, &size);\n> > > +\tif (!buf)\n> > > +\t\treturn;\n> > > +\tif (!size || (type != OBJ_TAG)) {\n> > \n> > Please lose the extra parents.\n> \n> What do you mean ?\n\nTypo.  I meant the parens, and my fingers typed parents. D'oh.\n\n> (...)\n> > This can be done much easier with 'sp = strstr(buf, \"\\n\\n\");'.  You can \n> > even do that before the previous if(), to free() && return if there is no \n> > body.\n> (...)\n> > This can be done much easier with 'eob = strstr(sp, \"\\n\" PGP_SIGNATURE \n> > \"\\n\");'.\n> \n> I must say I just stole most of it in show_reference() in the same file.\n\nI agree for the \"\\n\\n\"; this was my mistake (IOW it should be fixed both \nin show_reference() as well as in your code).\n\nBut for the signature, show_reference() _has_ to go line by line, because \nthe user is allowed to specify a maximal line count.  This does not apply \nfor your function.\n\n> (...)\n> > Why not teach write_annotations() (or write_tag_body() like I would prefer \n> > it to be called) to grok a null_sha1?  It's not like we care for \n> > performance here, but rather for readability and ease of use.\n> \n> I would have if I had looked up for is_null_sha1() earlier ;)\n\nHehe.  This is what I really like about git's mailing list: it is a place \nwhere you learn something new every day.\n\nCiao,\nDscho\n"},{"id":"58120","messageId":"20071103122707.GA7227@glandium.org","threadId":"10607","inReplyTo":"Pine.LNX.4.64.0711031148460.4362@racer.site","subject":"Re: [PATCH] Reuse previous annotation when overwriting a tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-03T12:27:07Z","receivedAt":"2007-11-03T12:27:07Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:\n> Why not teach write_annotations() (or write_tag_body() like I would prefer \n> it to be called) to grok a null_sha1?  It's not like we care for \n> performance here, but rather for readability and ease of use.\n\nBy the way, I think it would be much better if this function was made\nmore generic and would not write, but return an strbuf containing the\nobject body. It could also be used by e.g. git-commit --amend.\n\nWhat would be the best suited place for such a function ?\n\nMike\n"},{"id":"58122","messageId":"Pine.LNX.4.64.0711031236160.4362@racer.site","threadId":"10607","inReplyTo":"20071103122707.GA7227@glandium.org","subject":"Re: [PATCH] Reuse previous annotation when overwriting a tag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-03T12:36:36Z","receivedAt":"2007-11-03T12:36:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 3 Nov 2007, Mike Hommey wrote:\n\n> On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:\n> > Why not teach write_annotations() (or write_tag_body() like I would prefer \n> > it to be called) to grok a null_sha1?  It's not like we care for \n> > performance here, but rather for readability and ease of use.\n> \n> By the way, I think it would be much better if this function was made\n> more generic and would not write, but return an strbuf containing the\n> object body. It could also be used by e.g. git-commit --amend.\n> \n> What would be the best suited place for such a function ?\n\neditor.c, I'd say.\n\nCiao,\nDscho\n"},{"id":"58125","messageId":"1194095285-18651-1-git-send-email-mh@glandium.org","threadId":"10607","inReplyTo":"Pine.LNX.4.64.0711031219530.4362@racer.site","subject":"[PATCH 1/2] Reuse previous annotation when overwriting a tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-03T13:08:04Z","receivedAt":"2007-11-03T13:08:04Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When forcing to overwrite an annotated tag, there are good chances one\nwants to keep the old annotation, or modify it, not start from scratch.\n\nThis is obviously only triggered for annotated tagging (-a or -s).\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n builtin-tag.c |   41 ++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 38 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 66e5a58..e57f57f 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -247,9 +247,40 @@ static int git_tag_config(const char *var, const char *value)\n \treturn git_default_config(var, value);\n }\n \n+static void write_tag_body(int fd, const unsigned char *sha1)\n+{\n+\tunsigned long size;\n+\tenum object_type type;\n+\tchar *buf, *sp, *eob;\n+\tsize_t len;\n+\n+\tif (is_null_sha1(sha1))\n+\t\treturn;\n+\n+\tsp = buf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn;\n+\t/* skip header */\n+\tsp = strstr(buf, \"\\n\\n\");\n+\n+\tif (!sp || !size || type != OBJ_TAG) {\n+\t\tfree(buf);\n+\t\treturn;\n+\t}\n+\tsp += 2; /* skip the 2 CRs */\n+\teob = strstr(sp, \"\\n\" PGP_SIGNATURE \"\\n\");\n+\tif (eob)\n+\t\tlen = eob - sp;\n+\telse\n+\t\tlen = buf + size - sp;\n+\twrite_or_die(fd, sp, len);\n+\n+\tfree(buf);\n+}\n+\n static void create_tag(const unsigned char *object, const char *tag,\n \t\t       struct strbuf *buf, int message, int sign,\n-\t\t\t   unsigned char *result)\n+\t\t\t   unsigned char *prev, unsigned char *result)\n {\n \tenum object_type type;\n \tchar header_buf[1024];\n@@ -282,7 +313,11 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\tif (fd < 0)\n \t\t\tdie(\"could not create file '%s': %s\",\n \t\t\t\t\t\tpath, strerror(errno));\n-\t\twrite_or_die(fd, tag_template, strlen(tag_template));\n+\n+\t\tif (prev)\n+\t\t\twrite_tag_body(fd, prev);\n+\t\telse\n+\t\t\twrite_or_die(fd, tag_template, strlen(tag_template));\n \t\tclose(fd);\n \n \t\tlaunch_editor(path, buf);\n@@ -419,7 +454,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(\"tag '%s' already exists\", tag);\n \n \tif (annotate)\n-\t\tcreate_tag(object, tag, &buf, message, sign, object);\n+\t\tcreate_tag(object, tag, &buf, message, sign, prev, object);\n \n \tlock = lock_any_ref_for_update(ref, prev, 0);\n \tif (!lock)\n-- \n1.5.3.5\n"},{"id":"58124","messageId":"1194095285-18651-2-git-send-email-mh@glandium.org","threadId":"10607","inReplyTo":"1194095285-18651-1-git-send-email-mh@glandium.org","subject":"[PATCH 2/2] Small code readability improvement in show_reference() in builtin-tag.c","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-03T13:08:05Z","receivedAt":"2007-11-03T13:08:05Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n builtin-tag.c |    9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex e57f57f..3ed5759 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -84,14 +84,13 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n \t\tsp = buf = read_sha1_file(sha1, &type, &size);\n \t\tif (!buf)\n \t\t\treturn 0;\n-\t\tif (!size) {\n+\t\t/* skip header */\n+\t\tsp = strstr(buf, \"\\n\\n\");\n+\n+\t\tif (!sp || !size) {\n \t\t\tfree(buf);\n \t\t\treturn 0;\n \t\t}\n-\t\t/* skip header */\n-\t\twhile (sp + 1 < buf + size &&\n-\t\t\t\t!(sp[0] == '\\n' && sp[1] == '\\n'))\n-\t\t\tsp++;\n \t\t/* only take up to \"lines\" lines, and strip the signature */\n \t\tfor (i = 0, sp += 2;\n \t\t\t\ti < filter->lines && sp < buf + size &&\n-- \n1.5.3.5\n"},{"id":"58126","messageId":"20071103131030.GA18670@glandium.org","threadId":"10607","inReplyTo":"Pine.LNX.4.64.0711031236160.4362@racer.site","subject":"Re: [PATCH] Reuse previous annotation when overwriting a tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-03T13:10:30Z","receivedAt":"2007-11-03T13:10:30Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, Nov 03, 2007 at 12:36:36PM +0000, Johannes Schindelin wrote:\n> Hi,\n> \n> On Sat, 3 Nov 2007, Mike Hommey wrote:\n> \n> > On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:\n> > > Why not teach write_annotations() (or write_tag_body() like I would prefer \n> > > it to be called) to grok a null_sha1?  It's not like we care for \n> > > performance here, but rather for readability and ease of use.\n> > \n> > By the way, I think it would be much better if this function was made\n> > more generic and would not write, but return an strbuf containing the\n> > object body. It could also be used by e.g. git-commit --amend.\n> > \n> > What would be the best suited place for such a function ?\n> \n> editor.c, I'd say.\n\nOn which topic is this ?\n\nMike\n"},{"id":"58130","messageId":"Pine.LNX.4.64.0711031321320.4362@racer.site","threadId":"10607","inReplyTo":"20071103131030.GA18670@glandium.org","subject":"Re: [PATCH] Reuse previous annotation when overwriting a tag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-03T13:22:44Z","receivedAt":"2007-11-03T13:22:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 3 Nov 2007, Mike Hommey wrote:\n\n> On Sat, Nov 03, 2007 at 12:36:36PM +0000, Johannes Schindelin wrote:\n> \n> > On Sat, 3 Nov 2007, Mike Hommey wrote:\n> > \n> > > On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:\n> > > > Why not teach write_annotations() (or write_tag_body() like I \n> > > > would prefer it to be called) to grok a null_sha1?  It's not like \n> > > > we care for performance here, but rather for readability and ease \n> > > > of use.\n> > > \n> > > By the way, I think it would be much better if this function was \n> > > made more generic and would not write, but return an strbuf \n> > > containing the object body. It could also be used by e.g. git-commit \n> > > --amend.\n> > > \n> > > What would be the best suited place for such a function ?\n> > \n> > editor.c, I'd say.\n> \n> On which topic is this ?\n\nOn none so far.  But the plan was to move some functions used by both \nbuiltin-tag and builtin-commit (such as launch_editor()) into the files \neditor.[ch].\n\nUnfortunately, that plan has not been executed by anybody.  Yet.\n\nCiao,\nDscho\n"},{"id":"58137","messageId":"20071103135954.GA26204@glandium.org","threadId":"10607","inReplyTo":"Pine.LNX.4.64.0711031321320.4362@racer.site","subject":"Re: [PATCH] Reuse previous annotation when overwriting a tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-03T13:59:54Z","receivedAt":"2007-11-03T13:59:54Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, Nov 03, 2007 at 01:22:44PM +0000, Johannes Schindelin wrote:\n> Hi,\n> \n> On Sat, 3 Nov 2007, Mike Hommey wrote:\n> \n> > On Sat, Nov 03, 2007 at 12:36:36PM +0000, Johannes Schindelin wrote:\n> > \n> > > On Sat, 3 Nov 2007, Mike Hommey wrote:\n> > > \n> > > > On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:\n> > > > > Why not teach write_annotations() (or write_tag_body() like I \n> > > > > would prefer it to be called) to grok a null_sha1?  It's not like \n> > > > > we care for performance here, but rather for readability and ease \n> > > > > of use.\n> > > > \n> > > > By the way, I think it would be much better if this function was \n> > > > made more generic and would not write, but return an strbuf \n> > > > containing the object body. It could also be used by e.g. git-commit \n> > > > --amend.\n> > > > \n> > > > What would be the best suited place for such a function ?\n> > > \n> > > editor.c, I'd say.\n> > \n> > On which topic is this ?\n> \n> On none so far.  But the plan was to move some functions used by both \n> builtin-tag and builtin-commit (such as launch_editor()) into the files \n> editor.[ch].\n> \n> Unfortunately, that plan has not been executed by anybody.  Yet.\n\nAnyways, I took a quick glance at builtin-commit.c on pu, and it doesn't\nlook like it would benefit from having a shared function to get the\ncommit body. So I'll just forget about this idea for now.\n\nMike\n"},{"id":"58162","messageId":"7vlk9fxj1r.fsf@gitster.siamese.dyndns.org","threadId":"10607","inReplyTo":"1194095285-18651-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH 1/2] Reuse previous annotation when overwriting a tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-03T18:47:44Z","receivedAt":"2007-11-03T18:47:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> +static void write_tag_body(int fd, const unsigned char *sha1)\n> +{\n> ...\n> +\tsp = buf = read_sha1_file(sha1, &type, &size);\n> +\tif (!buf)\n> +\t\treturn;\n> +\t/* skip header */\n> +\tsp = strstr(buf, \"\\n\\n\");\n\nI was relieved to see this second assignment to \"sp\" here.\n\nWhy?\n\nBecause I wanted to say something about the first assignment to\nit, that is done this way:\n\n> +\tsp = buf = read_sha1_file(sha1, &type, &size);\n\nThe original git codebase, as it came from Linus, tends to avoid\nassignment to multiple variables in a single statement like this\n(and that style is written down in the kernel coding style\ndocument).  As I do not have a strong opinion against that\ncoding style, I've tried to follow it myself.  However, I do not\npersonaly have a strong argument to support enforcing the style\nto others.\n\nBut in this case, as the variable \"sp\" is never used before it\nis reassigned, I can easily say \"drop the useless assignment to\nsp there\". ;-)\n\n> +\n> +\tif (!sp || !size || type != OBJ_TAG) {\n> +\t\tfree(buf);\n> +\t\treturn;\n> +\t}\n> +\tsp += 2; /* skip the 2 CRs */\n\nYou are not skipping carriage returns.  You are skipping line\nfeeds (i.e. s/CRs/LFs/).\n\n> @@ -282,7 +313,11 @@ static void create_tag(const unsigned char *object, const char *tag,\n>  \t\tif (fd < 0)\n>  \t\t\tdie(\"could not create file '%s': %s\",\n>  \t\t\t\t\t\tpath, strerror(errno));\n> -\t\twrite_or_die(fd, tag_template, strlen(tag_template));\n> +\n> +\t\tif (prev)\n> +\t\t\twrite_tag_body(fd, prev);\n> +\t\telse\n> +\t\t\twrite_or_die(fd, tag_template, strlen(tag_template));\n>  \t\tclose(fd);\n\nWhen prev is not NULL but points at a null_sha1 nobody writes\nanything out.  Is this intended?\n\n        In fact, the calling site always passes prev which is\n        prev[] in cmd_tag() and cannot be non-NULL.\n\nWhy is there \"else\" in the first place?  Even if you start with\nthe previous tag's message, you are launching the editor for the\nuser to further edit it, and you would want to give some\ninstructions, wouldn't you?\n        \n"},{"id":"58170","messageId":"20071103195558.GA22448@glandium.org","threadId":"10607","inReplyTo":"7vlk9fxj1r.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Reuse previous annotation when overwriting a tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-03T19:55:58Z","receivedAt":"2007-11-03T19:55:58Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, Nov 03, 2007 at 11:47:44AM -0700, Junio C Hamano wrote:\n(...)\n> But in this case, as the variable \"sp\" is never used before it\n> is reassigned, I can easily say \"drop the useless assignment to\n> sp there\". ;-)\n\nYou got me here ;)\n\n(...)\n> When prev is not NULL but points at a null_sha1 nobody writes\n> anything out.  Is this intended?\n> \n>         In fact, the calling site always passes prev which is\n>         prev[] in cmd_tag() and cannot be non-NULL.\n \nDamn, I overlooked this, and since the test suite doesn't do anything\non that, that got through. Indeed either the test can be removed, since\nwrite_tag_body does the is_null_sha1() test, or the is_null_sha1() test\ncan be moved here.\n\n> Why is there \"else\" in the first place?  Even if you start with\n> the previous tag's message, you are launching the editor for the\n> user to further edit it, and you would want to give some\n> instructions, wouldn't you?\n\nWell, it could be true if the text was more verbose than \"Write a tag\nmessage\". Anyways, as the test is now, the text is not going to appear.\n:(\n\nI'll fix this and will try to enhance the test suite to catch these\nproblems.\n\nMike\n"},{"id":"58190","messageId":"1194135075-28706-1-git-send-email-mh@glandium.org","threadId":"10607","inReplyTo":"7vlk9fxj1r.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/2] Reuse previous annotation when overwriting a tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-04T00:11:14Z","receivedAt":"2007-11-04T00:11:14Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When forcing to overwrite an annotated tag, there are good chances one\nwants to keep the old annotation, or modify it, not start from scratch.\n\nThis is obviously only triggered for annotated tagging (-a or -s).\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n\nYou are free to remove the else that makes the tag template not show up\nin the editor, but I'm not convinced it is any useful. Maybe a new\ntemplate for this annotation reuse would be sensible.\n\n builtin-tag.c |   38 +++++++++++++++++++++++++++++++++++---\n 1 files changed, 35 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 66e5a58..566d123 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -247,9 +247,37 @@ static int git_tag_config(const char *var, const char *value)\n \treturn git_default_config(var, value);\n }\n \n+static void write_tag_body(int fd, const unsigned char *sha1)\n+{\n+\tunsigned long size;\n+\tenum object_type type;\n+\tchar *buf, *sp, *eob;\n+\tsize_t len;\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn;\n+\t/* skip header */\n+\tsp = strstr(buf, \"\\n\\n\");\n+\n+\tif (!sp || !size || type != OBJ_TAG) {\n+\t\tfree(buf);\n+\t\treturn;\n+\t}\n+\tsp += 2; /* skip the 2 LFs */\n+\teob = strstr(sp, \"\\n\" PGP_SIGNATURE \"\\n\");\n+\tif (eob)\n+\t\tlen = eob - sp;\n+\telse\n+\t\tlen = buf + size - sp;\n+\twrite_or_die(fd, sp, len);\n+\n+\tfree(buf);\n+}\n+\n static void create_tag(const unsigned char *object, const char *tag,\n \t\t       struct strbuf *buf, int message, int sign,\n-\t\t\t   unsigned char *result)\n+\t\t\t   unsigned char *prev, unsigned char *result)\n {\n \tenum object_type type;\n \tchar header_buf[1024];\n@@ -282,7 +310,11 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\tif (fd < 0)\n \t\t\tdie(\"could not create file '%s': %s\",\n \t\t\t\t\t\tpath, strerror(errno));\n-\t\twrite_or_die(fd, tag_template, strlen(tag_template));\n+\n+\t\tif (!is_null_sha1(prev))\n+\t\t\twrite_tag_body(fd, prev);\n+\t\telse\n+\t\t\twrite_or_die(fd, tag_template, strlen(tag_template));\n \t\tclose(fd);\n \n \t\tlaunch_editor(path, buf);\n@@ -419,7 +451,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(\"tag '%s' already exists\", tag);\n \n \tif (annotate)\n-\t\tcreate_tag(object, tag, &buf, message, sign, object);\n+\t\tcreate_tag(object, tag, &buf, message, sign, prev, object);\n \n \tlock = lock_any_ref_for_update(ref, prev, 0);\n \tif (!lock)\n-- \n1.5.3.5\n"},{"id":"58189","messageId":"1194135075-28706-2-git-send-email-mh@glandium.org","threadId":"10607","inReplyTo":"1194135075-28706-1-git-send-email-mh@glandium.org","subject":"[PATCH 2/2] Add tests for git tag","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-04T00:11:15Z","receivedAt":"2007-11-04T00:11:15Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"These tests check whether git-tag properly sends a comment into the\neditor, and whether it reuses previous annotation when overwriting\nan existing tag.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n t/t7004-tag.sh |   16 ++++++++++++++++\n 1 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 0d07bc3..096fe33 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1004,4 +1004,20 @@ test_expect_failure \\\n \t'verify signed tag fails when public key is not present' \\\n \t'git-tag -v signed-tag'\n \n+test_expect_success \\\n+\t'message in editor has initial comment' '\n+\tGIT_EDITOR=cat git tag -a initial-comment > actual || true &&\n+\ttest $(sed -n \"/^\\(#\\|\\$\\)/p\" actual | wc -l) -gt 0\n+'\n+\n+get_tag_header reuse $commit commit $time >expect\n+echo \"An annotation to be reused\" >> expect\n+test_expect_success \\\n+\t'overwriting an annoted tag should use its previous body' '\n+\tgit tag -a -m \"An annotation to be reused\" reuse &&\n+\tGIT_EDITOR=true git tag -f -a reuse &&\n+\tget_tag_msg reuse >actual &&\n+\tgit diff expect actual\n+'\n+\n test_done\n-- \n1.5.3.5\n"},{"id":"58732","messageId":"20071107215121.GA30248@glandium.org","threadId":"10607","inReplyTo":"1194095285-18651-2-git-send-email-mh@glandium.org","subject":"Re: [PATCH 2/2] Small code readability improvement in show_reference() in builtin-tag.c","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-11-07T21:51:21Z","receivedAt":"2007-11-07T21:51:21Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"I think this patch got lost with my bad behaviour of sending another\nseries of patch not including this one.\n\nCheers,\n\nMike\n\nOn Sat, Nov 03, 2007 at 02:08:05PM +0100, Mike Hommey wrote:\n> \n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>  builtin-tag.c |    9 ++++-----\n>  1 files changed, 4 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin-tag.c b/builtin-tag.c\n> index e57f57f..3ed5759 100644\n> --- a/builtin-tag.c\n> +++ b/builtin-tag.c\n> @@ -84,14 +84,13 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n>  \t\tsp = buf = read_sha1_file(sha1, &type, &size);\n>  \t\tif (!buf)\n>  \t\t\treturn 0;\n> -\t\tif (!size) {\n> +\t\t/* skip header */\n> +\t\tsp = strstr(buf, \"\\n\\n\");\n> +\n> +\t\tif (!sp || !size) {\n>  \t\t\tfree(buf);\n>  \t\t\treturn 0;\n>  \t\t}\n> -\t\t/* skip header */\n> -\t\twhile (sp + 1 < buf + size &&\n> -\t\t\t\t!(sp[0] == '\\n' && sp[1] == '\\n'))\n> -\t\t\tsp++;\n>  \t\t/* only take up to \"lines\" lines, and strip the signature */\n>  \t\tfor (i = 0, sp += 2;\n>  \t\t\t\ti < filter->lines && sp < buf + size &&\n> -- \n> 1.5.3.5\n> \n> -\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"}]}