Re: [PATCH] Silence error messages unless 'thorough_verify' is set

Johannes Schindelin [off-list ref] writes:

On Sat, 9 Jun 2007, Johan Herland wrote:
...

quoted

@@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	}
 
 	if (size < 65)
-		return error("Tag object failed preliminary size check");
+		return FAIL("Tag object failed preliminary size check");

This is ugly.

... quite a bit.  A less uglier alternative we seem to use in
other places is not much better (return NULL on failure or an
error message string on error).

...  Guess how surprised 
_I_ was, when I hit the error message which made me go mad.

To be fair, that ugly "char%d" was taken from mktag and not
Johan's invention.

To drive that point home: strict checking when creating tags is good. 
Strict checking when reading tags is bad.

I strongly encourage keeping both validations separate.

While I tend ot think that keeping two separate versions is
probably better for this particular case, the above statement
has a leap in its logic.  With your "error code" scheme, you
could implement a single, verifier/parser that defines the
concrete and complete rule of how the data should look like.
That unified verifier/parser itself should be silent.  Then, you
can have each of the callers decide how lenient it wants to be,
depending on the seriousness of the error.  You can make
producer very strict and chatty while leaving consumer liberal
and more silent.

There are pros-and-cons, however.

 - Such a scheme to return error codes and have two callers that
   have different behaviours is cumbersome to set up and use.

   A good example of this is the switch/case mess in each of the
   callers of run_command_v_opt() in builtin-push.c,
   builtin-revert.c, receive-pack.c etc.  For run_command, the
   mess is justifiable because the function has enough number of
   different callers, but in the current thread, we are only
   talking about two callers (parsing vs verifying of tag
   objects).

 - It has a risk to introduce inconsitent definition of the data
   format to have completely separate producer and consumer
   implementations; this is especially true when the data in
   question is complex.

   However, a tag is sufficiently simple that my personal
   feeling is that, combined with the cumbersomeness argument
   against the unified verifier, separate producer and consumer
   implementations would be easier to manage for this particular
   case.

Hi,

On Sun, 10 Jun 2007, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

...  Guess how surprised 
_I_ was, when I hit the error message which made me go mad.

To be fair, that ugly "char%d" was taken from mktag and not
Johan's invention.

Yes, I should have said that. I tried to hint to this by "you could just 
as well clean the code up", meaning the existing code.

Now, _that_ would be a patch I'd be really thankful for.

As for the general direction of implementing notes as tags: If you want to 
make them fetchable, you have to deal with conflicts. If you want to be 
able to amend notes, _especially_ when they should be fetchable, you want 
a history on them.

Which makes me think that tags are not the right object type for notes.

But I guess I'll just wait if somebody actually comments on my RFC for 
lightweight commit annotations (that's what I put into that discussion). 
BTW I just realized that I marked it [PATCH], while it should have been 
[RFC]. Sorry.

Ciao,
Dscho

Ok, I'm pulling the 21-part patch series from hell. It's just not worth all
the flak. Here's a 4-part patch series that tries to do the changes needed
without all the crap^Wrefactoring.

Obviously this patch series does none of the much needed cleanup in this
part of the code (e.g. better error messages, specifying encodings of header
fields, possibly unifying the common parts between the parser and the
verifier). I'll leave that cleanup to someone who writes less crappy code.

Here's the shortlog for the series:

Johan Herland (4):
      Make tag names (i.e. the tag object's "tag" line) optional
      Introduce optional "keywords" on tag objects
      Documentation/git-mktag: Document the changes in tag object structure
      git-mktag tests: Expand on mktag selftests according to the new tag object structure

 Documentation/git-mktag.txt |   38 +++++++---
 mktag.c                     |   65 +++++++++++-----
 t/t3800-mktag.sh            |  172 ++++++++++++++++++++++++++++++++++++++++---
 tag.c                       |   44 +++++++++--
 tag.h                       |    3 +-
 5 files changed, 270 insertions(+), 52 deletions(-)


...Johan

The tag line is now optional. If not given in the tag object data, it
defaults to the empty string ("") in the parsed tag object.

Also includes selftest tweaks to make them work with optional tag names.

Signed-off-by: Johan Herland <redacted>
---
 mktag.c          |   37 ++++++++++++++++++++-----------------
 t/t3800-mktag.sh |   16 +++++++++-------
 tag.c            |   18 ++++++++++++------
 tag.h            |    2 +-
 4 files changed, 42 insertions(+), 31 deletions(-)

diff --git a/mktag.c b/mktag.c
index 070bc96..5e80d3d 100644
--- a/mktag.c
+++ b/mktag.c

@@ -2,16 +2,15 @@
 #include "tag.h"
 
 /*
- * A signature file has a very simple fixed format: four lines
- * of "object <sha1>" + "type <typename>" + "tag <tagname>" +
+ * A signature file has a very simple format: 3-4 lines
+ * of "object <sha1>" + "type <typename>" + "tag <tagname>" (optional) +
  * "tagger <committer>", followed by a blank line, a free-form tag
  * message and a signature block that git itself doesn't care about,
  * but that can be verified with gpg or similar.
  *
- * The first three lines are guaranteed to be at least 63 bytes:
- * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
- * shortest possible type-line, and "tag .\n" at 6 bytes is the
- * shortest single-character-tag line.
+ * The first two lines are guaranteed to be at least 57 bytes:
+ * "object <sha1>\n" is 48 bytes and "type tag\n" at 9 bytes is the
+ * shortest possible type-line.
  *
  * We also artificially limit the size of the full object to 8kB.
  * Just because I'm a lazy bastard, and if you can't fit a signature

@@ -52,7 +51,7 @@ static int verify_tag(char *buffer, unsigned long size)
 	unsigned char sha1[20];
 	const char *object, *type_line, *tag_line, *tagger_line;
 
-	if (size < 64)
+	if (size < 58)
 		return error("wanna fool me ? you obviously got the size wrong !");
 
 	buffer[size] = 0;

@@ -75,8 +74,6 @@ static int verify_tag(char *buffer, unsigned long size)
 	if (!tag_line)
 		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - buffer);
 	tag_line++;
-	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
-		return error("char" PD_FMT ": no \"tag \" found", tag_line - buffer);
 
 	/* Get the actual type */
 	typelen = tag_line - type_line - strlen("type \n");

@@ -91,14 +88,20 @@ static int verify_tag(char *buffer, unsigned long size)
 		return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
 
 	/* Verify the tag-name: we don't allow control characters or spaces in it */
-	tag_line += 4;
-	for (;;) {
-		unsigned char c = *tag_line++;
-		if (c == '\n')
-			break;
-		if (c > ' ')
-			continue;
-		return error("char" PD_FMT ": could not verify tag name", tag_line - buffer);
+	if (!memcmp(tag_line, "tag ", 4)) {
+		if (tag_line[4] == '\n')
+			return error("char" PD_FMT ": no \"tag \" found",
+					tag_line - buffer);
+		tag_line += 4;
+		for (;;) {
+			unsigned char c = *tag_line++;
+			if (c == '\n')
+				break;
+			if (c > ' ')
+				continue;
+			return error("char" PD_FMT ": could not verify tag name",
+					tag_line - buffer);
+		}
 	}
 
 	/* Verify the tagger line */

diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index 7c7e433..ca90662 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh

@@ -100,13 +100,14 @@ check_verify_failure '"type" line eol check'
 #  6. tag line label check #1
 
 cat >tag.sig <<EOF
-object 779e9b33986b1c2670fff52c5067603117b3e895
-type tag
+object $head
+type commit
 xxx mytag
+tagger a
 EOF
 
 cat >expect.pat <<EOF
-^error: char57: no "tag " found$
+^error: char60: could not find "tagger"$
 EOF
 
 check_verify_failure '"tag" line label check #1'

@@ -115,13 +116,14 @@ check_verify_failure '"tag" line label check #1'
 #  7. tag line label check #2
 
 cat >tag.sig <<EOF
-object 779e9b33986b1c2670fff52c5067603117b3e895
-type taggggggggggggggggggggggggggggggg
+object $head
+type commit
 tag
+tagger a
 EOF
 
 cat >expect.pat <<EOF
-^error: char87: no "tag " found$
+^error: char60: could not find "tagger"$
 EOF
 
 check_verify_failure '"tag" line label check #2'

@@ -130,7 +132,7 @@ check_verify_failure '"tag" line label check #2'
 #  8. type line type-name length check
 
 cat >tag.sig <<EOF
-object 779e9b33986b1c2670fff52c5067603117b3e895
+object $head
 type taggggggggggggggggggggggggggggggg
 tag mytag
 EOF

diff --git a/tag.c b/tag.c
index bbacd59..a7a3454 100644
--- a/tag.c
+++ b/tag.c

@@ -44,7 +44,7 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
                 return 0;
         item->object.parsed = 1;
 
-	if (size < 64)
+	if (size < 58)
 		return -1;
 	if (memcmp("object ", data, 7) || get_sha1_hex((char *) data + 7, sha1))
 		return -1;

@@ -54,13 +54,17 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 		return -1;
 
 	tag_line = strchr(type_line, '\n');
-	if (!tag_line || memcmp("tag ", ++tag_line, 4))
+	if (!tag_line)
 		return -1;
 
-	sig_line = strchr(tag_line, '\n');
-	if (!sig_line)
-		return -1;
-	sig_line++;
+	if (!memcmp("tag ", ++tag_line, 4)) {
+		sig_line = strchr(tag_line, '\n');
+		if (!sig_line)
+			return -1;
+		sig_line++;
+	}
+	else
+		sig_line = tag_line;
 
 	typelen = tag_line - type_line - strlen("type \n");
 	if (typelen >= 20)

@@ -68,6 +72,8 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 	memcpy(type, type_line + 5, typelen);
 	type[typelen] = '\0';
 	taglen = sig_line - tag_line - strlen("tag \n");
+	if (taglen < 0) /* missing tag name */
+		taglen = 0;
 	item->tag = xmalloc(taglen + 1);
 	memcpy(item->tag, tag_line + 4, taglen);
 	item->tag[taglen] = '\0';

diff --git a/tag.h b/tag.h
index 7a0cb00..7e0abbe 100644
--- a/tag.h
+++ b/tag.h

@@ -8,7 +8,7 @@ extern const char *tag_type;
 struct tag {
 	struct object object;
 	struct object *tagged;
-	char *tag;
+	char *tag;       /* optional, may be empty ("") */
 	char *signature; /* not actually implemented */
 };
 

-- 
1.5.2.1.144.gabc40

This patch introduces a new optional header line to the tag object, called
"keywords". The "keywords" line may contain a comma-separated list of
custom keywords associated with the tag object. There are two "special"
keywords, however: "tag" and "note": When the "keywords" header is
missing, its default value is set to "tag" if a "tag" header is
present; else the default "keywords" value is set to "note". The
"keywords" header is meant to be used by porcelains for classifying
different types of tag objects. This classification may then be used to
filter tag objects in the presentation layer (e.g. by implementing
extra filter options to --decorate, etc.).

The encoding rules for keywords are identical to those of tag names.

Signed-off-by: Johan Herland <redacted>
---
 mktag.c |   30 ++++++++++++++++++++++++++----
 tag.c   |   30 +++++++++++++++++++++++++-----
 tag.h   |    1 +
 3 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/mktag.c b/mktag.c
index 5e80d3d..37e10c6 100644
--- a/mktag.c
+++ b/mktag.c

@@ -2,9 +2,10 @@
 #include "tag.h"
 
 /*
- * A signature file has a very simple format: 3-4 lines
+ * A signature file has a very simple format: 3-5 lines
  * of "object <sha1>" + "type <typename>" + "tag <tagname>" (optional) +
- * "tagger <committer>", followed by a blank line, a free-form tag
+ * "keywords <keywords>" (optional) + "tagger <committer>",
+ * followed by a blank line, a free-form tag
  * message and a signature block that git itself doesn't care about,
  * but that can be verified with gpg or similar.
  *

@@ -49,7 +50,7 @@ static int verify_tag(char *buffer, unsigned long size)
 	int typelen;
 	char type[20];
 	unsigned char sha1[20];
-	const char *object, *type_line, *tag_line, *tagger_line;
+	const char *object, *type_line, *tag_line, *keywords_line, *tagger_line;
 
 	if (size < 58)
 		return error("wanna fool me ? you obviously got the size wrong !");

@@ -104,8 +105,29 @@ static int verify_tag(char *buffer, unsigned long size)
 		}
 	}
 
+	/* Verify the keywords: disallow ctrl chars, spaces and double commas */
+	keywords_line = tag_line;
+
+	if (!memcmp(tag_line, "keywords ", 9)) {
+		if (tag_line[9] == '\n')
+			return error("char" PD_FMT ": no \"keywords \" found",
+					keywords_line - buffer);
+		keywords_line += 9;
+		for (;;) {
+			unsigned char c = *keywords_line++;
+			if (c == '\n')
+				break;
+			if (c == ',' && *keywords_line == ',')
+				/* double commas. fall through to error() */;
+			else if (c > ' ')
+				continue;
+			return error("char" PD_FMT ": could not verify keywords",
+					keywords_line - buffer);
+		}
+	}
+
 	/* Verify the tagger line */
-	tagger_line = tag_line;
+	tagger_line = keywords_line;
 
 	if (memcmp(tagger_line, "tagger", 6) || (tagger_line[6] == '\n'))
 		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - buffer);

diff --git a/tag.c b/tag.c
index a7a3454..b74f09f 100644
--- a/tag.c
+++ b/tag.c

@@ -35,9 +35,9 @@ struct tag *lookup_tag(const unsigned char *sha1)
 
 int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 {
-	int typelen, taglen;
+	int typelen, taglen, keywordslen;
 	unsigned char sha1[20];
-	const char *type_line, *tag_line, *sig_line;
+	const char *type_line, *tag_line, *keywords_line, *sig_line;
 	char type[20];
 
         if (item->object.parsed)

@@ -58,25 +58,45 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 		return -1;
 
 	if (!memcmp("tag ", ++tag_line, 4)) {
-		sig_line = strchr(tag_line, '\n');
+		keywords_line = strchr(tag_line, '\n');
+		if (!keywords_line)
+			return -1;
+		keywords_line++;
+	}
+	else
+		keywords_line = tag_line;
+
+	if (!memcmp("keywords ", keywords_line, 9)) {
+		sig_line = strchr(keywords_line, '\n');
 		if (!sig_line)
 			return -1;
 		sig_line++;
 	}
 	else
-		sig_line = tag_line;
+		sig_line = keywords_line;
 
 	typelen = tag_line - type_line - strlen("type \n");
 	if (typelen >= 20)
 		return -1;
 	memcpy(type, type_line + 5, typelen);
 	type[typelen] = '\0';
-	taglen = sig_line - tag_line - strlen("tag \n");
+	taglen = keywords_line - tag_line - strlen("tag \n");
 	if (taglen < 0) /* missing tag name */
 		taglen = 0;
 	item->tag = xmalloc(taglen + 1);
 	memcpy(item->tag, tag_line + 4, taglen);
 	item->tag[taglen] = '\0';
+	keywordslen = sig_line - keywords_line - strlen("keywords \n");
+	if (keywordslen > 0)
+		keywords_line += strlen("keywords ");
+	else { /* missing keywords */
+		if (taglen) /* tag name given */
+			keywords_line = "tag";
+		else
+			keywords_line = "note";
+		keywordslen = strlen(keywords_line);
+	}
+	item->keywords = xstrndup(keywords_line, keywordslen);
 
 	if (!strcmp(type, blob_type)) {
 		item->tagged = &lookup_blob(sha1)->object;

diff --git a/tag.h b/tag.h
index 7e0abbe..6e687b2 100644
--- a/tag.h
+++ b/tag.h

@@ -9,6 +9,7 @@ struct tag {
 	struct object object;
 	struct object *tagged;
 	char *tag;       /* optional, may be empty ("") */
+	char *keywords;  /* optional, defaults to (tag ? "tag" : "note") */
 	char *signature; /* not actually implemented */
 };
 

-- 
1.5.2.1.144.gabc40

The new structure of tag objects is documented.

Also some much-needed cleanup is done. E.g. remove the paragraph on the
8kB limit, since this limit was removed ages ago.

Signed-off-by: Johan Herland <redacted>
---
 Documentation/git-mktag.txt |   38 +++++++++++++++++++++++++++-----------
 1 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-mktag.txt b/Documentation/git-mktag.txt
index 0ac3be1..e6dfed6 100644
--- a/Documentation/git-mktag.txt
+++ b/Documentation/git-mktag.txt

@@ -8,29 +8,44 @@ git-mktag - Creates a tag object
 
 SYNOPSIS
 --------
-'git-mktag' < signature_file
+[verse]
+'git-mktag' < tag_data_file
 
 DESCRIPTION
 -----------
-Reads a tag contents on standard input and creates a tag object
+Reads tag object data on standard input and creates a tag object
 that can also be used to sign other objects.
 
 The output is the new tag's <object> identifier.
 
-Tag Format
+DISCUSSION
 ----------
-A tag signature file has a very simple fixed format: three lines of
+Tag object data has the following format
 
+[verse]
   object <sha1>
   type <typename>
-  tag <tagname>
+  tag <tagname>               (optional)
+  keywords <keywords>         (optional)
+  tagger <committer>
 
-followed by some 'optional' free-form signature that git itself
-doesn't care about, but that can be verified with gpg or similar.
+followed by a blank line and a free-form message and an optional
+signature that git itself doesn't care about, but that may be
+verified with gpg or similar.
 
-The size of the full object is artificially limited to 8kB.  (Just
-because I'm a lazy bastard, and if you can't fit a signature in that
-size, you're doing something wrong)
+In the above listing, `<sha1>` represents the object pointed to
+by this tag, `<typename>` is the type of the object pointed to
+("tag", "blob", "tree" or "commit"), `<tagname>` is the name of
+this tag object (and must correspond to the name of the corresponding
+ref (if any) in `.git/refs/`). `<keywords>` is a comma-separated
+list of keywords associated with this tag object, and `<committer>`
+holds the "`name <email>`" of the tag creator and timestamp of when
+the tag object was created (analogous to "committer" in commit
+objects).
+
+If "`tag <tagname>`" is omitted, <tagname> defaults to the empty
+string. If "`keywords <keywords>`" is omitted, <keywords> defaults
+to "`tag`" if a <tagname> was given, "`note`" otherwise.
 
 
 Author

@@ -39,7 +54,8 @@ Written by Linus Torvalds <torvalds@osdl.org>
 
 Documentation
 --------------
-Documentation by David Greaves, Junio C Hamano and the git-list <git@vger.kernel.org>.
+Documentation by Johan Herland, David Greaves, Junio C Hamano
+and the git-list <git@vger.kernel.org>.
 
 GIT
 ---

-- 
1.5.2.1.144.gabc40

Some more tests are added to test the new "keywords" header.

Signed-off-by: Johan Herland <redacted>
---
 t/t3800-mktag.sh |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 152 insertions(+), 4 deletions(-)

diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index ca90662..cc2f246 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh

@@ -174,7 +174,95 @@ EOF
 check_verify_failure 'verify tag-name check'
 
 ############################################################
-# 11. tagger line label check #1
+# 11. keywords line label check #1
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+xxxxxxxx foo
+EOF
+
+cat >expect.pat <<EOF
+^error: char70: could not find "tagger"$
+EOF
+
+check_verify_failure '"keywords" line label check #1'
+
+############################################################
+# 12. keywords line label check #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: char70: could not find "tagger"$
+EOF
+
+check_verify_failure '"keywords" line label check #2'
+
+############################################################
+# 13. keywords line check #1
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo bar	baz
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: char83: could not verify keywords$
+EOF
+
+check_verify_failure '"keywords" line check #1'
+
+############################################################
+# 14. keywords line check #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo,bar	baz
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: char87: could not verify keywords$
+EOF
+
+check_verify_failure '"keywords" line check #2'
+
+############################################################
+# 15. keywords line check #3
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo,,bar
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: char83: could not verify keywords$
+EOF
+
+check_verify_failure '"keywords" line check #3'
+
+############################################################
+# 16. tagger line label check #1
 
 cat >tag.sig <<EOF
 object $head

@@ -189,7 +277,7 @@ EOF
 check_verify_failure '"tagger" line label check #1'
 
 ############################################################
-# 12. tagger line label check #2
+# 17. tagger line label check #2
 
 cat >tag.sig <<EOF
 object $head

@@ -205,7 +293,7 @@ EOF
 check_verify_failure '"tagger" line label check #2'
 
 ############################################################
-# 13. create valid tag
+# 18. create valid tag #1
 
 cat >tag.sig <<EOF
 object $head

@@ -219,11 +307,71 @@ test_expect_success \
     'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
 
 ############################################################
-# 14. check mytag
+# 19. check mytag
 
 test_expect_success \
     'check mytag' \
     'git-tag -l | grep mytag'
 
+############################################################
+# 20. create valid tag #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #2' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 21. create valid tag #3
+
+cat >tag.sig <<EOF
+object $head
+type commit
+keywords foo,bar,baz,spam,spam,spam,spam,spam,spam,spam,spam
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #3' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 22. create valid tag #4
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords note
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #4' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 23. create valid tag #5 (with empty message)
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords note
+tagger a
+EOF
+
+test_expect_success \
+    'create valid tag #4' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
 
 test_done

-- 
1.5.2.1.144.gabc40

On Sunday 10 June 2007, Johannes Schindelin wrote:

As for the general direction of implementing notes as tags: If you want to 
make them fetchable, you have to deal with conflicts. If you want to be 
able to amend notes, _especially_ when they should be fetchable, you want 
a history on them.

I'm not sure what kind of notes you're talking about here. If you're talking 
about my git-note concept, I designed notes to be immutable (thus not 
amendable) and there is therefore _no_ merging or potential for conflicts 
between notes. The only resolution needed is to figure out which order the 
notes for a given object should be presented. The default here is 
chronological sorting.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Hi,

On Sun, 10 Jun 2007, Johan Herland wrote:

Johan Herland (4):
      Make tag names (i.e. the tag object's "tag" line) optional
      Introduce optional "keywords" on tag objects
      Documentation/git-mktag: Document the changes in tag object structure
      git-mktag tests: Expand on mktag selftests according to the new tag object structure

Much nicer, thank you.

Ciao,
Dscho

Hi,

On Sun, 10 Jun 2007, Johan Herland wrote:

+	/* Verify the keywords: disallow ctrl chars, spaces and double commas */

What about Junio's suggestion, making it really strict at first, and only 
loosening it if we need to? IIRC it was alnum + '_', maybe even '-'.

Other than that, looks good to me. I trust that the test cases are 
exhaustive enough to support the patch from the practical side.

BTW this patch is exactly what I meant by conceptually closed. Thank you.

And please accept my apologies for my language. Reading some of it, I have 
to admit that it sounded as harsh as Junio suggested it to be. My only 
excuse is that I had an unplanned stay at the Paris airport for more than 
9 hours (after a night in the plane where I could hardly sleep), so I 
should really have stayed away from writing emails. But since you 
addressed your emails to me, I wanted to reply to you as soon as I had the 
chance to.

Ciao,
Dscho

Hi,

On Sun, 10 Jun 2007, Johan Herland wrote:

On Sunday 10 June 2007, Johannes Schindelin wrote:

quoted

As for the general direction of implementing notes as tags: If you 
want to make them fetchable, you have to deal with conflicts. If you 
want to be able to amend notes, _especially_ when they should be 
fetchable, you want a history on them.

I'm not sure what kind of notes you're talking about here. If you're 
talking about my git-note concept, I designed notes to be immutable 
(thus not amendable) and there is therefore _no_ merging or potential 
for conflicts between notes.

Okay, that is one way you can go about implementing notes.

The only resolution needed is to figure out which order the notes for a 
given object should be presented. The default here is chronological 
sorting.

There are several problems with that approach I'd like to point out:

- In distributed environments, you can not rely on timestamps. Ever.

- If a note is deleted, you will fetch it again as long as the other side 
  did not delete it.

- You cannot undo a typo (since the notes are immutable, you would see 
  both versions), once the typoed note was fetched.

Basically, everything I see as a problem here suggests that note writing 
is very much like working on a branch. That's why I suggest to treat it 
exactly like a branch to begin with.

Ciao,
Dscho

On Sunday 10 June 2007, Johannes Schindelin wrote:

Hi,

On Sun, 10 Jun 2007, Johan Herland wrote:

quoted

+	/* Verify the keywords: disallow ctrl chars, spaces and double commas */

What about Junio's suggestion, making it really strict at first, and only 
loosening it if we need to? IIRC it was alnum + '_', maybe even '-'.

For now, I couldn't find a good reason why the set of allowed characters
for keywords should be smaller than for the tag name.
Feel free to tighten the set of characters before this makes it into a
release. However, if you do, the same tightening should be considered
for the tag name as well, I guess. Can't see any good reasons for why
one should be tighter than the other.

And please accept my apologies for my language. Reading some of it, I have 
to admit that it sounded as harsh as Junio suggested it to be. My only 
excuse is that I had an unplanned stay at the Paris airport for more than 
9 hours (after a night in the plane where I could hardly sleep), so I 
should really have stayed away from writing emails. But since you 
addressed your emails to me, I wanted to reply to you as soon as I had the 
chance to.

Apology accepted. I'm sorry my patch-series-from-hell came at such an
inconvenient time for you. 


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Sunday 10 June 2007, Johannes Schindelin wrote:

On Sun, 10 Jun 2007, Johan Herland wrote:

quoted

On Sunday 10 June 2007, Johannes Schindelin wrote:

quoted

As for the general direction of implementing notes as tags: If you 
want to make them fetchable, you have to deal with conflicts. If you 
want to be able to amend notes, _especially_ when they should be 
fetchable, you want a history on them.

I'm not sure what kind of notes you're talking about here. If you're 
talking about my git-note concept, I designed notes to be immutable 
(thus not amendable) and there is therefore _no_ merging or potential 
for conflicts between notes.

Okay, that is one way you can go about implementing notes.

quoted

The only resolution needed is to figure out which order the notes for a 
given object should be presented. The default here is chronological 
sorting.

There are several problems with that approach I'd like to point out:

- In distributed environments, you can not rely on timestamps. Ever.

Not really, but that doesn't stop many programs from trying anyway...
(e.g. email clients). And still, it's not like the date (or sorting)
is crucial to the 'notes' concept or implementation.

- If a note is deleted, you will fetch it again as long as the other side 
  did not delete it.

Yep. This was considered an acceptable tradeoff in the design. But I
understand that some people won't like it.

- You cannot undo a typo (since the notes are immutable, you would see 
  both versions), once the typoed note was fetched.

Yep. Also a tradeoff in the design. Also going to piss off some
people, I guess.

Basically, everything I see as a problem here suggests that note writing 
is very much like working on a branch. That's why I suggest to treat it 
exactly like a branch to begin with.

I see you point.


BTW, I have some patches implementing the 'notes' concept on top
of the softrefs patches. They're just lying around now waiting
to be cleaned up and sent to the list, but I'm not sure it's worth
it, since they don't add anything that's not in your lightweight
annotation patch...


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Johan Herland [off-list ref] writes:

For now, I couldn't find a good reason why the set of allowed characters
for keywords should be smaller than for the tag name.

The set of allowed tag names excludes shell metacharacters,
primarily to help scripting.  I think keywords can share the
same reasoning to exclude them.

It also excludes '^', '~' and ':', because tag names can be used
in revision range expressions (i.e. prefix '^' is the "exclude
from the resulting set" operation, postfix "~<number>" is the
"Nth generation ancestor" operation) and general SHA-1
expression (i.e. infix ':' is the "find in the tree-ish the
object at path" operation).  These reasons would not apply to 
keywords.

Having said all of that, I suspect it is premature to talk about
keywords, as it is unclear what their intended use is.  What
kind of operations are useful on them?  

It does not count that "git cat-file tag" would show "keywords
blah" on the header instead of in body.  It is not a compelling
enough reason to introduce a new header type. grep would work
just fine for such a use.

On the other hand, for example, if (the syntax is totally made
up) we make '::keywords=foo::' expand to set of all tags that
have the specified keyword 'foo', and it turns out to be useful
to be able to say "git show ::keywords=foo::" instead of listing
individual tags, that kind of use case may make it a good reason
to add such a new header type.

On Sunday 10 June 2007, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

For now, I couldn't find a good reason why the set of allowed characters
for keywords should be smaller than for the tag name.

The set of allowed tag names excludes shell metacharacters,
primarily to help scripting.  

It already does? Or are you proposing this? Right now the code doesn't 
enforce anything like this, AFAICS...

I think keywords can share the same reasoning to exclude them.

It also excludes '^', '~' and ':', because tag names can be used 
in revision range expressions (i.e. prefix '^' is the "exclude
from the resulting set" operation, postfix "~<number>" is the
"Nth generation ancestor" operation) and general SHA-1
expression (i.e. infix ':' is the "find in the tree-ish the
object at path" operation).  These reasons would not apply to 
keywords.

I have nothing against limiting keywords to fairly small set, say
alphanumerics plus a couple of "safe" symbols. It just didn't make
sense to do this when I made the patch without doing it to the
tag name at the same time, and I'm not sure what that restricted
set should be, so I held off on it. Feel free to fix.

Having said all of that, I suspect it is premature to talk about
keywords, as it is unclear what their intended use is.  What
kind of operations are useful on them?  

It does not count that "git cat-file tag" would show "keywords
blah" on the header instead of in body.  It is not a compelling
enough reason to introduce a new header type. grep would work
just fine for such a use.

On the other hand, for example, if (the syntax is totally made
up) we make '::keywords=foo::' expand to set of all tags that
have the specified keyword 'foo', and it turns out to be useful
to be able to say "git show ::keywords=foo::" instead of listing
individual tags, that kind of use case may make it a good reason
to add such a new header type.

Yes, this is what I'm thinking; using keywords to filter tag objects in 
various settings. Haven't thought much about the syntax yet, but as it 
would have to work on the command-line (possibly together with the other 
weird characters git uses for specifying revisions), I imagine a character 
set similar to the one for tag names should be good.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net