threads / patch / 37276

patch, 2 partsbundle: Fix exclusion of annotated tags

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

## tl;dr

7 messages between Aug 2, 2014 and Aug 4, 2014. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Lukas Fleischer· Aug 2, 2014, 08:39 UTC · lore

[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 <git@cryptocrack.de>
---
 t/t5704-bundle.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Show changes to t/t5704-bundle.sh +5 −2
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index a45c316..2f063ea 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -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

-- 
2.0.3
Lukas Fleischer· Aug 2, 2014, 08:39 UTC · re: Lukas Fleischer · lore

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 <git@cryptocrack.de>
---
 bundle.c          | 4 ++--
 t/t5704-bundle.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

bundle.c, t/t5704-bundle.sh

diff --git a/bundle.c b/bundle.c
index 71a21a6..b708906 100644
--- a/bundle.c
+++ b/bundle.c
@@ -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);
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 2f063ea..8a4d299 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -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 &&

-- 
2.0.3
Junio C Hamano· Aug 4, 2014, 20:10 UTC · re: Lukas Fleischer · lore

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

Lukas Fleischer <git@cryptocrack.de> writes:
Show 24 quoted lines
> 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 <git@cryptocrack.de>
> ---
>  bundle.c          | 4 ++--
>  t/t5704-bundle.sh | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 71a21a6..b708906 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -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.
Show 16 quoted lines
>  	if (!line++)
>  		return 1;
>  	date = strtoul(line, NULL, 10);
> diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
> index 2f063ea..8a4d299 100755
> --- a/t/t5704-bundle.sh
> +++ b/t/t5704-bundle.sh
> @@ -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· Aug 4, 2014, 20:08 UTC · re: Lukas Fleischer · lore

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

Lukas Fleischer <git@cryptocrack.de> writes:
Show 27 quoted lines
> 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 <git@cryptocrack.de>
> ---
>  t/t5704-bundle.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
> index a45c316..2f063ea 100755
> --- a/t/t5704-bundle.sh
> +++ b/t/t5704-bundle.sh
> @@ -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?

Show 15 quoted lines
>  	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· Aug 4, 2014, 20:28 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
>> diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
>> index a45c316..2f063ea 100755
>> --- a/t/t5704-bundle.sh
>> +++ b/t/t5704-bundle.sh
>> @@ -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.
Show 12 quoted lines
>> @@ -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
Lukas Fleischer· Aug 4, 2014, 20:57 UTC · re: Lukas Fleischer · lore

[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 <git@cryptocrack.de>
---
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(-)
Show changes to t/t5704-bundle.sh +4 −1
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index a45c316..2d53388 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -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

-- 
2.0.4
Lukas Fleischer· Aug 4, 2014, 20:57 UTC · re: Lukas Fleischer · lore

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 <git@cryptocrack.de>
---
 bundle.c          | 4 ++--
 t/t5704-bundle.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

bundle.c, t/t5704-bundle.sh

diff --git a/bundle.c b/bundle.c
index 71a21a6..b708906 100644
--- a/bundle.c
+++ b/bundle.c
@@ -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);
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 2d53388..a828c71 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -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 &&

-- 
2.0.4

← back to recent threads