t5704: Fix the test that checks for excluded tags

PATCH In review Lukas Fleischer 2 participants 7 messages Jun 15, 2016

Share & tools

Lukas Fleischer

author

2016-06-15 23:02 UTC #0 lore raw

PATCH 1/2 t5704: Fix the test that checks for excluded tags

In c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), we added a test to check whether annotated tags, which fall outside the specified date range, are excluded from bundles. However, when initializing the repository, a command to create a lightweight tag was used. Fix this by replacing git tag by git tag -a. Furthermore, explicitly mention in the test message that an annotated tag is created and also test whether tags within the specified date range are included properly.

Note that this fix reveals that the annotated tag exclusion actually does not work. Therefore, the test is marked expect-failure for now.

Signed-off-by Lukas Fleischer <[email protected]>

---
 t/t5704-bundle.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

t/t5704-bundle.sh +5 -2

index a45c316..2f063ea 100755
@@ -6,7 +6,7 @@ test_description='some bundle related tests'
 test_expect_success 'setup' '
 	test_commit initial &&
 	test_tick &&
-	git tag -m tag tag &&
+	git tag -am tag tag &&
 	test_commit second &&
 	test_commit third &&
 	git tag -d initial &&

@@ -14,7 +14,10 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 
-test_expect_success 'tags can be excluded by rev-list options' '
+test_expect_failure 'annotated tags can be excluded by rev-list options' '
+	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
+	git ls-remote bundle > output &&
+	grep tag output &&
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
 	git ls-remote bundle > output &&
 	! grep tag output

Signature · 1 line

2.0.3

Lukas Fleischer

author

2016-06-15 23:02 UTC #1 lore raw

PATCH 1/2 t5704: Complement annotated tag exclusion test

In c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), we added a test to check whether annotated tags, which fall outside the specified date range, are excluded from bundles. Complement this test by also checking whether tags inside the date range are included. Since this addition reveals that the annotated tag exclusion is flawed, mark the test expect-failure for now.

Signed-off-by Lukas Fleischer <[email protected]>

---
I decided that it is still worthwhile to have this in a separate patch. Feel free to squash 1/2 and 2/2 or let me know that they should be merged if you prefer that.

 t/t5704-bundle.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

t/t5704-bundle.sh +4 -1

index a45c316..2d53388 100755
@@ -14,7 +14,10 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 
-test_expect_success 'tags can be excluded by rev-list options' '
+test_expect_failure 'tags can be excluded by rev-list options' '
+	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
+	git ls-remote bundle > output &&
+	grep tag output &&
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
 	git ls-remote bundle > output &&
 	! grep tag output

Signature · 1 line

2.0.4

Lukas Fleischer

author

2016-06-15 23:02 UTC #2 lore raw

PATCH 2/2 bundle: Fix exclusion of annotated tags

In commit c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), support for excluding annotated tags outside the specified date range was added. However, the wrong order of parameters was chosen when calling memchr(). Fix this by swapping the character to search for with the maximum length parameter.

Signed-off-by Lukas Fleischer <[email protected]>

---
 bundle.c          | 4 ++--
 t/t5704-bundle.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

bundle.c +2 -2

index 71a21a6..b708906 100644
@@ -221,8 +221,8 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 	line = memmem(buf, size, "\ntagger ", 8);
 	if (!line++)
 		return 1;
-	lineend = memchr(line, buf + size - line, '\n');
-	line = memchr(line, lineend ? lineend - line : buf + size - line, '>');
+	lineend = memchr(line, '\n', buf + size - line);
+	line = memchr(line, '>', lineend ? lineend - line : buf + size - line);
 	if (!line++)
 		return 1;
 	date = strtoul(line, NULL, 10);

t/t5704-bundle.sh +1 -1

index 2d53388..a828c71 100755
@@ -14,7 +14,7 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 
-test_expect_failure 'tags can be excluded by rev-list options' '
+test_expect_success 'tags can be excluded by rev-list options' '
 	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
 	git ls-remote bundle > output &&
 	grep tag output &&

Signature · 1 line

2.0.4

Lukas Fleischer

author

2016-06-15 23:02 UTC #3 lore raw

PATCH 2/2 bundle: Fix exclusion of annotated tags

In reply to Lukas Fleischer

Signed-off-by Lukas Fleischer <[email protected]>

---
 bundle.c          | 4 ++--
 t/t5704-bundle.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

bundle.c +2 -2

index 71a21a6..b708906 100644
@@ -221,8 +221,8 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 	line = memmem(buf, size, "\ntagger ", 8);
 	if (!line++)
 		return 1;
-	lineend = memchr(line, buf + size - line, '\n');
-	line = memchr(line, lineend ? lineend - line : buf + size - line, '>');
+	lineend = memchr(line, '\n', buf + size - line);
+	line = memchr(line, '>', lineend ? lineend - line : buf + size - line);
 	if (!line++)
 		return 1;
 	date = strtoul(line, NULL, 10);

t/t5704-bundle.sh +1 -1

index 2f063ea..8a4d299 100755
@@ -14,7 +14,7 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 
-test_expect_failure 'annotated tags can be excluded by rev-list options' '
+test_expect_success 'annotated tags can be excluded by rev-list options' '
 	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
 	git ls-remote bundle > output &&
 	grep tag output &&

Signature · 1 line

2.0.3

Junio C Hamano

2016-06-15 23:02 UTC #4 lore raw

Re: [PATCH 2/2] bundle: Fix exclusion of annotated tags

Quoted 17 lines · Lukas Fleischer

In commit c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), support for excluding annotated tags outside the specified date range was added. However, the wrong order of parameters was chosen when calling memchr(). Fix this by swapping the character to search for with the maximum length parameter.
Signed-off-by Lukas Fleischer <[email protected]>
---
 bundle.c          | 4 ++--
 t/t5704-bundle.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
bundle.c +0 -0
index 71a21a6..b708906 100644

bundle.c static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) · line 225

Quoted 7 lines

bundle.c +2 -2

@@ -221,8 +221,8 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 	line = memmem(buf, size, "\ntagger ", 8);
 	if (!line++)
 		return 1;
-	lineend = memchr(line, buf + size - line, '\n');
-	line = memchr(line, lineend ? lineend - line : buf + size - line, '>');
+	lineend = memchr(line, '\n', buf + size - line);
+	line = memchr(line, '>', lineend ? lineend - line : buf + size - line);

Good spotting; thanks.

Quoted 16 lines

bundle.c +0 -0

 	if (!line++)
 		return 1;
 	date = strtoul(line, NULL, 10);

t/t5704-bundle.sh +1 -1

index 2f063ea..8a4d299 100755
@@ -14,7 +14,7 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 
-test_expect_failure 'annotated tags can be excluded by rev-list options' '
+test_expect_success 'annotated tags can be excluded by rev-list options' '
 	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
 	git ls-remote bundle > output &&
 	grep tag output &&

Junio C Hamano

2016-06-15 23:02 UTC #5 lore raw

In reply to Lukas Fleischer

Quoted 22 lines · Lukas Fleischer

In c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), we added a test to check whether annotated tags, which fall outside the specified date range, are excluded from bundles. However, when initializing the repository, a command to create a lightweight tag was used. Fix this by replacing git tag by git tag -a. Furthermore, explicitly mention in the test message that an annotated tag is created and also test whether tags within the specified date range are included properly.
Note that this fix reveals that the annotated tag exclusion actually does not work. Therefore, the test is marked expect-failure for now.
Signed-off-by Lukas Fleischer <[email protected]>
---
 t/t5704-bundle.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
t/t5704-bundle.sh +0 -0
index a45c316..2f063ea 100755

t/t5704-bundle.sh test_description='some bundle related tests' · line 9

Quoted 5 lines

t/t5704-bundle.sh +1 -1

@@ -6,7 +6,7 @@ test_description='some bundle related tests'
 test_expect_success 'setup' '
 	test_commit initial &&
 	test_tick &&
-	git tag -m tag tag &&
+	git tag -am tag tag &&

I'd prefer to see this spelled as "-a -m tag", but anyway, this suggests to me that a request to create a light-weight tag should be made to error out when -m is given, or automatically promote itself to create an annotated tag, perhaps? That is in line with what happens when you do "git tag -F <file> tagname".

Oh, wait.

	$ git tag -d foo
        $ git rev-parse refs/tags/foo --
        fatal: bad revision 'refs/tags/foo'
        $ git tag -m msg foo
        $ git cat-file -t refs/tags/foo
        tag
        $ git cat-file tag refs/tags/foo
        object d84843c...
        type commit
        tag foo
        tagger Junio ....

	msg
	$ git version
        git version 2.1.0-rc0-247-g66c8a75

The output from "git blame -L'/^int cmd_tag/,/^}/' builtin/tag.c" seems to indicate that we automatically turned annotate on when a message is given via -m or -F since the very first version of "git tag" that was re-implemented in C, i.e. 62e09ce9 (Make git tag a builtin., 2007-07-20).

Your analysis starts to sound fishy. What version of Git are you talking about?

Quoted 15 lines

t/t5704-bundle.sh +4 -1

 	test_commit second &&
 	test_commit third &&
 	git tag -d initial &&

@@ -14,7 +14,10 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 
-test_expect_success 'tags can be excluded by rev-list options' '
+test_expect_failure 'annotated tags can be excluded by rev-list options' '
+	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
+	git ls-remote bundle > output &&
+	grep tag output &&
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
 	git ls-remote bundle > output &&
 	! grep tag output

Junio C Hamano

2016-06-15 23:02 UTC #6 lore raw

Quoted 4 lines · Junio C Hamano

Quoted 4 lines
t/t5704-bundle.sh +0 -0
index a45c316..2f063ea 100755

Quoted 8 lines

t/t5704-bundle.sh test_description='some bundle related tests' · line 9
Quoted 6 lines
t/t5704-bundle.sh +1 -1
@@ -6,7 +6,7 @@ test_description='some bundle related tests'
 test_expect_success 'setup' '
 	test_commit initial &&
 	test_tick &&
-	git tag -m tag tag &&
+	git tag -am tag tag &&
...
Oh, wait.

In any case, the fix in 2/2 is real, and applying both and then reverting the above hunk passes the test. Also, applying both, reverting the above hunk *and* reverting the fix to bundle.c of course makes the rest fail.

So I would be tempted to squash these two patches into one using the log message from the latter one, while excluding the change in the above hunk.

Thanks.

Quoted 12 lines

t/t5704-bundle.sh +4 -1

@@ -14,7 +14,10 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 
-test_expect_success 'tags can be excluded by rev-list options' '
+test_expect_failure 'annotated tags can be excluded by rev-list options' '
+	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
+	git ls-remote bundle > output &&
+	grep tag output &&
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
 	git ls-remote bundle > output &&
 	! grep tag output