* [PATCH 1/3] tag: fix output of "tag -n" when errors occur
2012-02-06 8:11 ` Jeff King
@ 2012-02-06 8:13 ` Jeff King
2012-02-06 8:13 ` [PATCH 2/3] tag: die when listing missing or corrupt objects Jeff King
` (3 subsequent siblings)
4 siblings, 0 replies; 53+ messages in thread
From: Jeff King @ 2012-02-06 8:13 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, Junio C Hamano, jasampler
When "git tag" is instructed to print lines from annotated
tags via "-n", it first prints the tag name, then attempts
to parse and print the lines of the tag object, and then
finally adds a trailing newline.
If an error occurs, we return early from the function and
never print the newline, screwing up the output for the next
tag. Let's factor the line-printing into its own function so
we can manage the early returns better, and make sure that
we always terminate the line.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/tag.c | 66 +++++++++++++++++++++++++++++---------------------------
1 files changed, 34 insertions(+), 32 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..2250915 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -83,18 +83,45 @@ static int contains(struct commit *candidate, const struct commit_list *want)
return contains_recurse(candidate, want);
}
+static void show_tag_lines(const unsigned char *sha1, int lines)
+{
+ int i;
+ unsigned long size;
+ enum object_type type;
+ char *buf, *sp, *eol;
+ size_t len;
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf || !size)
+ return;
+
+ /* skip header */
+ sp = strstr(buf, "\n\n");
+ if (!sp) {
+ free(buf);
+ return;
+ }
+ /* only take up to "lines" lines, and strip the signature */
+ size = parse_signature(buf, size);
+ for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
+ if (i)
+ printf("\n ");
+ eol = memchr(sp, '\n', size - (sp - buf));
+ len = eol ? eol - sp : size - (sp - buf);
+ fwrite(sp, len, 1, stdout);
+ if (!eol)
+ break;
+ sp = eol + 1;
+ }
+ free(buf);
+}
+
static int show_reference(const char *refname, const unsigned char *sha1,
int flag, void *cb_data)
{
struct tag_filter *filter = cb_data;
if (match_pattern(filter->patterns, refname)) {
- int i;
- unsigned long size;
- enum object_type type;
- char *buf, *sp, *eol;
- size_t len;
-
if (filter->with_commit) {
struct commit *commit;
@@ -110,33 +137,8 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
printf("%-15s ", refname);
-
- buf = read_sha1_file(sha1, &type, &size);
- if (!buf || !size)
- return 0;
-
- /* skip header */
- sp = strstr(buf, "\n\n");
- if (!sp) {
- free(buf);
- return 0;
- }
- /* only take up to "lines" lines, and strip the signature */
- size = parse_signature(buf, size);
- for (i = 0, sp += 2;
- i < filter->lines && sp < buf + size;
- i++) {
- if (i)
- printf("\n ");
- eol = memchr(sp, '\n', size - (sp - buf));
- len = eol ? eol - sp : size - (sp - buf);
- fwrite(sp, len, 1, stdout);
- if (!eol)
- break;
- sp = eol + 1;
- }
+ show_tag_lines(sha1, filter->lines);
putchar('\n');
- free(buf);
}
return 0;
--
1.7.9.rc1.29.g43677
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 8:11 ` Jeff King
2012-02-06 8:13 ` [PATCH 1/3] tag: fix output of "tag -n" when errors occur Jeff King
@ 2012-02-06 8:13 ` Jeff King
2012-02-06 8:32 ` Junio C Hamano
2012-02-06 8:14 ` [PATCH 3/3] tag: don't show non-tag contents with "-n" Jeff King
` (2 subsequent siblings)
4 siblings, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-06 8:13 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, Junio C Hamano, jasampler
We don't usually bother looking at tagged objects at all
when listing. However, if "-n" is specified, we open the
objects to read the annotations of the tags. If we fail to
read an object, or if the object has zero length, we simply
silently return.
The first case is an indication of a broken or corrupt repo,
and we should notify the user of the error.
The second case is OK to silently ignore; however, the
existing code leaked the buffer returned by read_sha1_file.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/tag.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 2250915..1bb42a4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -92,8 +92,12 @@ static void show_tag_lines(const unsigned char *sha1, int lines)
size_t len;
buf = read_sha1_file(sha1, &type, &size);
- if (!buf || !size)
+ if (!buf)
+ die_errno("unable to read object %s", sha1_to_hex(sha1));
+ if (!size) {
+ free(buf);
return;
+ }
/* skip header */
sp = strstr(buf, "\n\n");
--
1.7.9.rc1.29.g43677
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 8:13 ` [PATCH 2/3] tag: die when listing missing or corrupt objects Jeff King
@ 2012-02-06 8:32 ` Junio C Hamano
2012-02-06 8:34 ` Jeff King
2012-02-06 8:36 ` Junio C Hamano
0 siblings, 2 replies; 53+ messages in thread
From: Junio C Hamano @ 2012-02-06 8:32 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Grennan, git, jasampler
Jeff King <peff@peff.net> writes:
> The first case is an indication of a broken or corrupt repo,
> and we should notify the user of the error.
>
> The second case is OK to silently ignore; however, the
> existing code leaked the buffer returned by read_sha1_file.
> ...
> buf = read_sha1_file(sha1, &type, &size);
> - if (!buf || !size)
> + if (!buf)
> + die_errno("unable to read object %s", sha1_to_hex(sha1));
> + if (!size) {
> + free(buf);
> return;
> + }
>
> /* skip header */
> sp = strstr(buf, "\n\n");
Hmm, a pedant in me says a tag object cannot have zero length, so the
second case is also an indication of a corrupt repository, unless the tag
happens to be a lightweight one that refers directly to a blob object that
is empty.
For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
because the definition of "first N lines" is compatible between tag and
commit for the purpose of the -n option.
For example, in the kernel repository, what would this do, I have to
wonder:
$ git tag c2.6.12 v2.6.12^{commit}
$ git tag t2.6.12 v2.6.12^{tree}
$ git tag -l -n 12 c2.6.12 t2.6.12
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 8:32 ` Junio C Hamano
@ 2012-02-06 8:34 ` Jeff King
2012-02-06 8:36 ` Junio C Hamano
1 sibling, 0 replies; 53+ messages in thread
From: Jeff King @ 2012-02-06 8:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
On Mon, Feb 06, 2012 at 12:32:13AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The first case is an indication of a broken or corrupt repo,
> > and we should notify the user of the error.
> >
> > The second case is OK to silently ignore; however, the
> > existing code leaked the buffer returned by read_sha1_file.
> > ...
> > buf = read_sha1_file(sha1, &type, &size);
> > - if (!buf || !size)
> > + if (!buf)
> > + die_errno("unable to read object %s", sha1_to_hex(sha1));
> > + if (!size) {
> > + free(buf);
> > return;
> > + }
> >
> > /* skip header */
> > sp = strstr(buf, "\n\n");
>
> Hmm, a pedant in me says a tag object cannot have zero length, so the
> second case is also an indication of a corrupt repository, unless the tag
> happens to be a lightweight one that refers directly to a blob object that
> is empty.
Yes. Or alternatively, it should just be caught in the strstr() case
below (which would silently ignore it).
> For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> because the definition of "first N lines" is compatible between tag and
> commit for the purpose of the -n option.
Yup. See patch 3. :)
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 8:32 ` Junio C Hamano
2012-02-06 8:34 ` Jeff King
@ 2012-02-06 8:36 ` Junio C Hamano
2012-02-06 8:38 ` Jeff King
1 sibling, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2012-02-06 8:36 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Grennan, git, jasampler
Junio C Hamano <gitster@pobox.com> writes:
> Hmm, a pedant in me says a tag object cannot have zero length, so the
> second case is also an indication of a corrupt repository, unless the tag
> happens to be a lightweight one that refers directly to a blob object that
> is empty.
>
> For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> because the definition of "first N lines" is compatible between tag and
> commit for the purpose of the -n option.
Ahh, Ok, your 3/3 addresses this exact issue.
I do not object to silently return when the object is not OBJ_TAG (even
though I slightly prefer showing the first N lines of commit log contents
for OBJ_COMMIT lightweight tag), but I still think it should be warned
just like a corruption when we see (type == OBJ_TAG && !size).
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 8:36 ` Junio C Hamano
@ 2012-02-06 8:38 ` Jeff King
2012-02-06 18:04 ` Junio C Hamano
2012-02-06 18:13 ` Junio C Hamano
0 siblings, 2 replies; 53+ messages in thread
From: Jeff King @ 2012-02-06 8:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
On Mon, Feb 06, 2012 at 12:36:05AM -0800, Junio C Hamano wrote:
> > For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> > make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> > because the definition of "first N lines" is compatible between tag and
> > commit for the purpose of the -n option.
>
> Ahh, Ok, your 3/3 addresses this exact issue.
>
> I do not object to silently return when the object is not OBJ_TAG (even
> though I slightly prefer showing the first N lines of commit log contents
> for OBJ_COMMIT lightweight tag), but I still think it should be warned
> just like a corruption when we see (type == OBJ_TAG && !size).
OK, that's easy enough to do. Should we show lightweight tags to commits
for backwards compatibility (and just drop the parse_signature junk in
that case)? The showing of blobs or trees is the really bad thing, I
think.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 8:38 ` Jeff King
@ 2012-02-06 18:04 ` Junio C Hamano
2012-02-06 18:13 ` Junio C Hamano
1 sibling, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2012-02-06 18:04 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Tom Grennan, git, jasampler
Jeff King <peff@peff.net> writes:
> OK, that's easy enough to do. Should we show lightweight tags to commits
> for backwards compatibility (and just drop the parse_signature junk in
> that case)? The showing of blobs or trees is the really bad thing, I
> think.
I think that is a sensible thing to do. I see many end-user documents on
the Interweb that uses lightweight "git tag", and I do not think they are
shooting for brevity of their illustration. The authors of these pages do
primarily use lightweight tags because they do not have anything more to
add in the message more than the log message commit objects they point at.
And it is a huge regression if we stop showing them if they are used to
use "tag -n".
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 8:38 ` Jeff King
2012-02-06 18:04 ` Junio C Hamano
@ 2012-02-06 18:13 ` Junio C Hamano
2012-02-06 20:12 ` Jeff King
2012-02-08 21:01 ` Jeff King
1 sibling, 2 replies; 53+ messages in thread
From: Junio C Hamano @ 2012-02-06 18:13 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Tom Grennan, git, jasampler
Jeff King <peff@peff.net> writes:
> OK, that's easy enough to do. Should we show lightweight tags to commits
> for backwards compatibility (and just drop the parse_signature junk in
> that case)? The showing of blobs or trees is the really bad thing, I
> think.
For now, dropping 3/3 and queuing this instead...
---
Subject: tag: do not show non-tag contents with "-n"
"git tag -n" did not check the type of the object it is reading the top n
lines from. At least, avoid showing the beginning of trees and blobs when
dealing with lightweight tags that point at them.
As the payload of a tag and a commit look similar in that they both start
with a header block, which is skipped for the purpose of "-n" output,
followed by human readable text, allow the message of commit objects to be
shown just like the contents of tag objects. This avoids regression for
people who have been using "tag -n" to show the log messages of commits
that are pointed at by lightweight tags.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/tag.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 1e27f5c..6d6ae88 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -95,19 +95,20 @@ static void show_tag_lines(const unsigned char *sha1, int lines)
buf = read_sha1_file(sha1, &type, &size);
if (!buf)
die_errno("unable to read object %s", sha1_to_hex(sha1));
- if (!size) {
- free(buf);
- return;
- }
+ if (type != OBJ_COMMIT || type != OBJ_TAG)
+ goto free_return;
+ if (!size)
+ die("an empty %s object %s?",
+ typename(type), sha1_to_hex(sha1));
/* skip header */
sp = strstr(buf, "\n\n");
- if (!sp) {
- free(buf);
- return;
- }
- /* only take up to "lines" lines, and strip the signature */
- size = parse_signature(buf, size);
+ if (!sp)
+ goto free_return;
+
+ /* only take up to "lines" lines, and strip the signature from a tag */
+ if (type == OBJ_TAG)
+ size = parse_signature(buf, size);
for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
if (i)
printf("\n ");
@@ -118,6 +119,7 @@ static void show_tag_lines(const unsigned char *sha1, int lines)
break;
sp = eol + 1;
}
+free_return:
free(buf);
}
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 18:13 ` Junio C Hamano
@ 2012-02-06 20:12 ` Jeff King
2012-02-06 23:34 ` Junio C Hamano
2012-02-08 21:01 ` Jeff King
1 sibling, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-06 20:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
On Mon, Feb 06, 2012 at 10:13:27AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > OK, that's easy enough to do. Should we show lightweight tags to commits
> > for backwards compatibility (and just drop the parse_signature junk in
> > that case)? The showing of blobs or trees is the really bad thing, I
> > think.
>
> For now, dropping 3/3 and queuing this instead...
>
> ---
> Subject: tag: do not show non-tag contents with "-n"
Looks perfect. Thanks.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 20:12 ` Jeff King
@ 2012-02-06 23:34 ` Junio C Hamano
0 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2012-02-06 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Grennan, git, jasampler
Jeff King <peff@peff.net> writes:
>> Subject: tag: do not show non-tag contents with "-n"
>
> Looks perfect. Thanks.
>
> -Peff
I was an idiot and you were being too polite to point it out X-<.
+ if (type != OBJ_COMMIT || type != OBJ_TAG)
+ goto free_return;
When will I ever get any output from this crap? What kind of object
should I craft to pass through this stupid gate? ;-)
Fixed and requeued.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
2012-02-06 18:13 ` Junio C Hamano
2012-02-06 20:12 ` Jeff King
@ 2012-02-08 21:01 ` Jeff King
2012-02-09 4:33 ` Junio C Hamano
1 sibling, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-08 21:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
On Mon, Feb 06, 2012 at 10:13:27AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > OK, that's easy enough to do. Should we show lightweight tags to commits
> > for backwards compatibility (and just drop the parse_signature junk in
> > that case)? The showing of blobs or trees is the really bad thing, I
> > think.
>
> For now, dropping 3/3 and queuing this instead...
>
> ---
> Subject: tag: do not show non-tag contents with "-n"
Since jk/maint-tag-show-fixes is still in pu, perhaps we can squash in
this test from my 3/3:
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..0db0f6a 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -586,6 +586,19 @@ test_expect_success \
test_cmp expect actual
'
+test_expect_success 'annotations for blobs are empty' '
+ blob=$(git hash-object -w --stdin <<-\EOF
+ Blob paragraph 1.
+
+ Blob paragraph 2.
+ EOF
+ ) &&
+ git tag tag-blob $blob &&
+ echo "tag-blob " >expect &&
+ git tag -n1 -l tag-blob >actual &&
+ test_cmp expect actual
+'
+
# trying to verify annotated non-signed tags:
test_expect_success GPG \
If we want to be more thorough, I can write up a more complete test
battery making sure tags and commits are both shown, but blobs and trees
are not.
-Peff
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH 3/3] tag: don't show non-tag contents with "-n"
2012-02-06 8:11 ` Jeff King
2012-02-06 8:13 ` [PATCH 1/3] tag: fix output of "tag -n" when errors occur Jeff King
2012-02-06 8:13 ` [PATCH 2/3] tag: die when listing missing or corrupt objects Jeff King
@ 2012-02-06 8:14 ` Jeff King
2012-02-07 7:01 ` [PATCHv2] tag: add --points-at list option Tom Grennan
2012-02-07 7:01 ` Tom Grennan
4 siblings, 0 replies; 53+ messages in thread
From: Jeff King @ 2012-02-06 8:14 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, Junio C Hamano, jasampler
When given "-n", tag will show one or more lines of the body
of an annotated tag. However, we never actually checked to
see that what we got was a tag; we might end up showing
random lines from a lightweight-tagged blob or commit.
With this patch, we'll show lines only from tag objects (but
still include non-tag objects in the listing). It might make
more sense to omit lightweight tags from the listing
entirely when "-n" is in effect. I stuck with this behavior
because it is slightly more compatible with the original
behavior.
This might be seen as a regression for people with
lightweight tags to commit, who would previously get the
subject line of the commit. The code seems to indicate that
is not expected (since it does things like parsing off
gpg signatures), but it's possible somebody has been relying
on it.
Signed-off-by: Jeff King <peff@peff.net>
---
The regression comment above makes me a little nervous. Still, if we
want to handle commits, we should do so explicitly and not munge them
with parse_signature. So I think it's a step in the right direction, and
we should let it cook for a bit and see if anybody complains.
builtin/tag.c | 2 +-
t/t7004-tag.sh | 13 +++++++++++++
2 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 1bb42a4..0a7c174 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -94,7 +94,7 @@ static void show_tag_lines(const unsigned char *sha1, int lines)
buf = read_sha1_file(sha1, &type, &size);
if (!buf)
die_errno("unable to read object %s", sha1_to_hex(sha1));
- if (!size) {
+ if (!size || type != OBJ_TAG) {
free(buf);
return;
}
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..0db0f6a 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -586,6 +586,19 @@ test_expect_success \
test_cmp expect actual
'
+test_expect_success 'annotations for non-tags are empty' '
+ blob=$(git hash-object -w --stdin <<-\EOF
+ Blob paragraph 1.
+
+ Blob paragraph 2.
+ EOF
+ ) &&
+ git tag tag-blob $blob &&
+ echo "tag-blob " >expect &&
+ git tag -n1 -l tag-blob >actual &&
+ test_cmp expect actual
+'
+
# trying to verify annotated non-signed tags:
test_expect_success GPG \
--
1.7.9.rc1.29.g43677
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCHv2] tag: add --points-at list option
2012-02-06 8:11 ` Jeff King
` (2 preceding siblings ...)
2012-02-06 8:14 ` [PATCH 3/3] tag: don't show non-tag contents with "-n" Jeff King
@ 2012-02-07 7:01 ` Tom Grennan
2012-02-07 7:01 ` Tom Grennan
4 siblings, 0 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-07 7:01 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
The following is version 2 of the "points-at" feature. I think that I've
addressed all of the comments on this discussion other than the name objection.
I suggest "of" instead, as in: "Show me the tags of...".
But, this may be too terse, so I welcome any other suggestions.
Note that this has been rebased onto pu for integration of 'jk/maint-tag-show-fixes'.
Thanks,
Tom Grennan (1):
tag: add --points-at list option
Documentation/git-tag.txt | 5 ++-
builtin/tag.c | 87 ++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 86 insertions(+), 6 deletions(-)
--
1.7.8
^ permalink raw reply [flat|nested] 53+ messages in thread* [PATCHv2] tag: add --points-at list option
2012-02-06 8:11 ` Jeff King
` (3 preceding siblings ...)
2012-02-07 7:01 ` [PATCHv2] tag: add --points-at list option Tom Grennan
@ 2012-02-07 7:01 ` Tom Grennan
2012-02-07 8:35 ` Junio C Hamano
2012-02-07 16:05 ` Jeff King
4 siblings, 2 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-07 7:01 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
This filters the list for annotated|signed tags of the given object.
Example,
john$ git tag -s v1.0-john v1.0
john$ git tag -l --points-at v1.0
v1.0-john
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-tag.txt | 5 ++-
builtin/tag.c | 86 ++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 85 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5ead91e..97bedec 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
<tagname> [<commit> | <object>]
'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
[--column[=<options>] | --no-column] [<pattern>...]
'git tag' -v <tagname>...
@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.
--contains <commit>::
Only list tags which contain the specified commit.
+--points-at <object>::
+ Only list annotated or signed tags of the given object.
+
-m <msg>::
--message=<msg>::
Use the given tag message (instead of prompting).
diff --git a/builtin/tag.c b/builtin/tag.c
index 5fbd62c..a1d3a04 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -20,17 +20,34 @@
static const char * const git_tag_usage[] = {
"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
"git tag -d <tagname>...",
- "git tag -l [-n[<num>]] [<pattern>...]",
+ "git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\"
+ "\n\t\t[<pattern>...]",
"git tag -v <tagname>...",
NULL
};
+struct points_at {
+ struct points_at *next;
+ unsigned char *sha1;
+};
+
struct tag_filter {
const char **patterns;
int lines;
struct commit_list *with_commit;
+ struct points_at *points_at;
};
+static void free_points_at (struct points_at *points_at)
+{
+ while (points_at) {
+ struct points_at *next = points_at->next;
+ free(points_at->sha1);
+ free(points_at);
+ points_at = next;
+ }
+}
+
static unsigned int colopts;
static int match_pattern(const char **patterns, const char *ref)
@@ -44,6 +61,29 @@ static int match_pattern(const char **patterns, const char *ref)
return 0;
}
+static struct points_at *match_points_at(struct points_at *points_at,
+ const unsigned char *sha1)
+{
+ char *buf;
+ struct tag *tag;
+ unsigned long size;
+ enum object_type type;
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf)
+ return NULL;
+ if (type != OBJ_TAG
+ || (tag = lookup_tag(sha1), !tag)
+ || parse_tag_buffer(tag, buf, size) < 0) {
+ free(buf);
+ return NULL;
+ }
+ while (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))
+ points_at = points_at->next;
+ free(buf);
+ return points_at;
+}
+
static int in_commit_list(const struct commit_list *want, struct commit *c)
{
for (; want; want = want->next)
@@ -141,6 +181,10 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
+ if (filter->points_at
+ && !match_points_at(filter->points_at, sha1))
+ return 0;
+
if (!filter->lines) {
printf("%s\n", refname);
return 0;
@@ -154,16 +198,19 @@ static int show_reference(const char *refname, const unsigned char *sha1,
}
static int list_tags(const char **patterns, int lines,
- struct commit_list *with_commit)
+ struct commit_list *with_commit,
+ struct points_at *points_at)
{
struct tag_filter filter;
filter.patterns = patterns;
filter.lines = lines;
filter.with_commit = with_commit;
+ filter.points_at = points_at;
for_each_tag_ref(show_reference, (void *) &filter);
+ free_points_at(points_at);
return 0;
}
@@ -389,12 +436,33 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
return check_refname_format(sb->buf, 0);
}
+int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
+{
+ struct points_at *new, **opt_value = (struct points_at **)opt->value;
+ unsigned char *sha1;
+
+ if (!arg)
+ return error(_("missing <object>"));
+ new = xmalloc(sizeof(struct points_at));
+ sha1 = xmalloc(20);
+ if (get_sha1(arg, sha1)) {
+ free(new);
+ free(sha1);
+ return error(_("malformed object name '%s'"), arg);
+ }
+ new->sha1 = sha1;
+ new->next = *opt_value;
+ *opt_value = new;
+ return 0;
+}
+
int cmd_tag(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
+ struct points_at *points_at = NULL;
struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
@@ -432,6 +500,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
PARSE_OPT_LASTARG_DEFAULT,
parse_opt_with_commit, (intptr_t)"HEAD",
},
+ {
+ OPTION_CALLBACK, 0, "points-at", &points_at, "object",
+ "print only annotated|signed tags of the object",
+ PARSE_OPT_LASTARG_DEFAULT,
+ parse_opt_points_at, (intptr_t)NULL,
+ },
OPT_END()
};
@@ -471,15 +545,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
copts.padding = 2;
run_column_filter(colopts, &copts);
}
- ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit);
+ ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit,
+ points_at);
if (lines == -1 && colopts & COL_ENABLED)
stop_column_filter();
return ret;
}
if (lines != -1)
die(_("-n option is only allowed with -l."));
- if (with_commit)
- die(_("--contains option is only allowed with -l."));
+ if (with_commit || points_at)
+ die(_("--contains and --points-at options "
+ "are only allowed with -l."));
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
--
1.7.8
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 7:01 ` Tom Grennan
@ 2012-02-07 8:35 ` Junio C Hamano
2012-02-07 18:05 ` Tom Grennan
2012-02-07 16:05 ` Jeff King
1 sibling, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2012-02-07 8:35 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, peff, jasampler
Tom Grennan <tmgrennan@gmail.com> writes:
> +struct points_at {
> + struct points_at *next;
> + unsigned char *sha1;
> +};
struct points_at {
struct points_at *next;
unsigned char sha1[20];
};
would save you from having to allocate and free always in pairs, no?
> +static void free_points_at (struct points_at *points_at)
Please lose the SP before (.
> + if (type != OBJ_TAG
> + || (tag = lookup_tag(sha1), !tag)
> + || parse_tag_buffer(tag, buf, size) < 0) {
Even though I personally prefer to cascade a long expression like this, so
that you see a parse tree when you tilt your head 90-degrees to the left,
I think the prevalent style in Git codebase is
if (A-long-long-expression ||
B-long-long-expression ||
C-long-long-expression) {
Also we try to avoid assignment in the conditional.
> @@ -432,6 +500,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> PARSE_OPT_LASTARG_DEFAULT,
> parse_opt_with_commit, (intptr_t)"HEAD",
> },
> + {
> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
> + "print only annotated|signed tags of the object",
> + PARSE_OPT_LASTARG_DEFAULT,
> + parse_opt_points_at, (intptr_t)NULL,
> + },
If you are going to reject NULL anyway, do you still need to mark this as
lastarg-default?
Looking for example in parse-options.h, I found this:
#define OPT_STRING_LIST(s, l, v, a, h) \
{ OPTION_CALLBACK, (s), (l), (v), (a), \
(h), 0, &parse_opt_string_list }
which is used by "git clone" to mark its -c option.
Running "git clone -c" gives me
error: switch 'c' requires a value
without any extra code in the caller of parse_options().
Other than that, looks cleanly done.
Thanks. I'll take another look after I wake up in the morning.
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 8:35 ` Junio C Hamano
@ 2012-02-07 18:05 ` Tom Grennan
0 siblings, 0 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-07 18:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, jasampler
On Tue, Feb 07, 2012 at 12:35:19AM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +struct points_at {
>> + struct points_at *next;
>> + unsigned char *sha1;
>> +};
>
>struct points_at {
> struct points_at *next;
> unsigned char sha1[20];
>};
>
>would save you from having to allocate and free always in pairs, no?
Yep
>> +static void free_points_at (struct points_at *points_at)
>
>Please lose the SP before (.
Oops
>> + if (type != OBJ_TAG
>> + || (tag = lookup_tag(sha1), !tag)
>> + || parse_tag_buffer(tag, buf, size) < 0) {
>
>Even though I personally prefer to cascade a long expression like this, so
>that you see a parse tree when you tilt your head 90-degrees to the left,
>I think the prevalent style in Git codebase is
>
> if (A-long-long-expression ||
> B-long-long-expression ||
> C-long-long-expression) {
>
>Also we try to avoid assignment in the conditional.
I like to compact multiple conditions to a common exit but also appreciate
the fear and loathing of comma's.
While rearranging this I finally understand how to include lightweight tags.
struct points_at *pa;
const unsigned char *tagged_sha1 = (const unsigned char *)"";
/* First look for lightweight tags - those with matching sha's
* but different names */
for (pa = points_at; pa; pa = pa->next)
if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
return pa;
buf = read_sha1_file(sha1, &type, &size);
if (buf) {
if (type == OBJ_TAG) {
tag = lookup_tag(sha1);
if (parse_tag_buffer(tag, buf, size) >= 0)
tagged_sha1 = tag->tagged->sha1;
}
free(buf);
}
while (points_at && hashcmp(points_at->sha1, tagged_sha1))
points_at = points_at->next;
return points_at;
For example,
$ ./git-tag tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -a tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -s tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -s tomg-README HEAD:README
$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README
tomg-README
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README \*v1.7.9
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
>> @@ -432,6 +500,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> PARSE_OPT_LASTARG_DEFAULT,
>> parse_opt_with_commit, (intptr_t)"HEAD",
>> },
>> + {
>> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> + "print only annotated|signed tags of the object",
>> + PARSE_OPT_LASTARG_DEFAULT,
>> + parse_opt_points_at, (intptr_t)NULL,
>> + },
>
>If you are going to reject NULL anyway, do you still need to mark this as
>lastarg-default?
>
>Looking for example in parse-options.h, I found this:
>
> #define OPT_STRING_LIST(s, l, v, a, h) \
> { OPTION_CALLBACK, (s), (l), (v), (a), \
> (h), 0, &parse_opt_string_list }
>
>which is used by "git clone" to mark its -c option.
>
>Running "git clone -c" gives me
>
> error: switch 'c' requires a value
>
>without any extra code in the caller of parse_options().
Cool
>Other than that, looks cleanly done.
>
>Thanks. I'll take another look after I wake up in the morning.
Thanks, I'll send v3 later today.
--
TomG
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 7:01 ` Tom Grennan
2012-02-07 8:35 ` Junio C Hamano
@ 2012-02-07 16:05 ` Jeff King
2012-02-07 19:02 ` Tom Grennan
1 sibling, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-07 16:05 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
On Mon, Feb 06, 2012 at 11:01:16PM -0800, Tom Grennan wrote:
> +struct points_at {
> + struct points_at *next;
> + unsigned char *sha1;
> +};
Would using sha1_array save us from having to create our own data
structure? As a bonus, it can do O(lg n) lookups, though I seriously
doubt anyone will provide a large number of "--points-at".
> +static void free_points_at (struct points_at *points_at)
> +{
> + while (points_at) {
> + struct points_at *next = points_at->next;
> + free(points_at->sha1);
> + free(points_at);
> + points_at = next;
> + }
> +}
Then this could go away in favor of sha1_array_clear.
> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
> +{
> + struct points_at *new, **opt_value = (struct points_at **)opt->value;
> + unsigned char *sha1;
> +
> + if (!arg)
> + return error(_("missing <object>"));
> + new = xmalloc(sizeof(struct points_at));
> + sha1 = xmalloc(20);
> + if (get_sha1(arg, sha1)) {
> + free(new);
> + free(sha1);
> + return error(_("malformed object name '%s'"), arg);
> + }
> + new->sha1 = sha1;
> + new->next = *opt_value;
> + *opt_value = new;
> + return 0;
> +}
And this can drop all of the memory management bits, like:
unsigned char sha1[20];
if (!arg)
return error(_("missing <object>"));
if (get_sha1(arg, sha1))
return error(_("malformed object name '%s'"), arg);
sha1_array_append(opt->value, sha1);
return 0;
Also, should you check "unset"? When we have options that build a list,
usually doing "--no-foo" will clear the list. E.g., this:
git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
should look only for "baz".
> +static struct points_at *match_points_at(struct points_at *points_at,
> + const unsigned char *sha1)
> +{
> + char *buf;
> + struct tag *tag;
> + unsigned long size;
> + enum object_type type;
> +
> + buf = read_sha1_file(sha1, &type, &size);
> + if (!buf)
> + return NULL;
> + if (type != OBJ_TAG
> + || (tag = lookup_tag(sha1), !tag)
> + || parse_tag_buffer(tag, buf, size) < 0) {
> + free(buf);
> + return NULL;
> + }
> + while (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))
> + points_at = points_at->next;
> + free(buf);
> + return points_at;
> +}
Sorry, I threw a lot of object lookup code at you last time, so I think
my point may have been lost in the noise. But I think this is slightly
nicer as:
static int tag_points_at(struct sha1_array *sa,
const unsigned char *sha1)
{
struct object *obj = parse_object(sha1);
if (!obj)
return 0; /* or probably we should even just die() */
if (obj->type != OBJ_TAG)
return 0;
if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
return 0;
return 1;
}
I.e., using parse_object lets you avoid dealing with memory management
yourself. And as a bonus, it will reuse the cached information if you
happen to have already parsed that object (not likely in typical
repositories, but a huge win in certain pathological cases, like repos
storing shared objects and refs for a large number of forks).
> + {
> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
> + "print only annotated|signed tags of the object",
> + PARSE_OPT_LASTARG_DEFAULT,
> + parse_opt_points_at, (intptr_t)NULL,
> + },
I think you can drop the LASTARG_DEFAULT here, as it is no longer
optional, no?
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 16:05 ` Jeff King
@ 2012-02-07 19:02 ` Tom Grennan
2012-02-07 19:12 ` Jeff King
0 siblings, 1 reply; 53+ messages in thread
From: Tom Grennan @ 2012-02-07 19:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
On Tue, Feb 07, 2012 at 11:05:27AM -0500, Jeff King wrote:
>On Mon, Feb 06, 2012 at 11:01:16PM -0800, Tom Grennan wrote:
>
>> +struct points_at {
>> + struct points_at *next;
>> + unsigned char *sha1;
>> +};
>
>Would using sha1_array save us from having to create our own data
>structure? As a bonus, it can do O(lg n) lookups, though I seriously
>doubt anyone will provide a large number of "--points-at".
Thanks, but I now realize that I also need to save the pointed at
refname to detect lightweight tags that have matching sha's but
different names.
>> +static void free_points_at (struct points_at *points_at)
>> +{
>> + while (points_at) {
>> + struct points_at *next = points_at->next;
>> + free(points_at->sha1);
>> + free(points_at);
>> + points_at = next;
>> + }
>> +}
>
>Then this could go away in favor of sha1_array_clear.
>
>> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
>> +{
>> + struct points_at *new, **opt_value = (struct points_at **)opt->value;
>> + unsigned char *sha1;
>> +
>> + if (!arg)
>> + return error(_("missing <object>"));
>> + new = xmalloc(sizeof(struct points_at));
>> + sha1 = xmalloc(20);
>> + if (get_sha1(arg, sha1)) {
>> + free(new);
>> + free(sha1);
>> + return error(_("malformed object name '%s'"), arg);
>> + }
>> + new->sha1 = sha1;
>> + new->next = *opt_value;
>> + *opt_value = new;
>> + return 0;
>> +}
>
>And this can drop all of the memory management bits, like:
>
> unsigned char sha1[20];
>
> if (!arg)
> return error(_("missing <object>"));
> if (get_sha1(arg, sha1))
> return error(_("malformed object name '%s'"), arg);
> sha1_array_append(opt->value, sha1);
> return 0;
>
>Also, should you check "unset"? When we have options that build a list,
>usually doing "--no-foo" will clear the list. E.g., this:
>
> git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
>
>should look only for "baz".
Ahh, so I just need to:
if (unset) {
if (*opt_value)
free_points_at(*opt_value);
*opt_value = NULL;
return 0;
}
>> +static struct points_at *match_points_at(struct points_at *points_at,
>> + const unsigned char *sha1)
>> +{
>> + char *buf;
>> + struct tag *tag;
>> + unsigned long size;
>> + enum object_type type;
>> +
>> + buf = read_sha1_file(sha1, &type, &size);
>> + if (!buf)
>> + return NULL;
>> + if (type != OBJ_TAG
>> + || (tag = lookup_tag(sha1), !tag)
>> + || parse_tag_buffer(tag, buf, size) < 0) {
>> + free(buf);
>> + return NULL;
>> + }
>> + while (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))
>> + points_at = points_at->next;
>> + free(buf);
>> + return points_at;
>> +}
>
>Sorry, I threw a lot of object lookup code at you last time, so I think
>my point may have been lost in the noise. But I think this is slightly
>nicer as:
>
> static int tag_points_at(struct sha1_array *sa,
> const unsigned char *sha1)
> {
> struct object *obj = parse_object(sha1);
> if (!obj)
> return 0; /* or probably we should even just die() */
> if (obj->type != OBJ_TAG)
> return 0;
> if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
> return 0;
> return 1;
> }
>
>I.e., using parse_object lets you avoid dealing with memory management
>yourself. And as a bonus, it will reuse the cached information if you
>happen to have already parsed that object (not likely in typical
>repositories, but a huge win in certain pathological cases, like repos
>storing shared objects and refs for a large number of forks).
Aye
>> + {
>> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> + "print only annotated|signed tags of the object",
>> + PARSE_OPT_LASTARG_DEFAULT,
>> + parse_opt_points_at, (intptr_t)NULL,
>> + },
>
>I think you can drop the LASTARG_DEFAULT here, as it is no longer
>optional, no?
You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?
Thanks,
TomG
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 19:02 ` Tom Grennan
@ 2012-02-07 19:12 ` Jeff King
2012-02-07 19:22 ` Tom Grennan
0 siblings, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-07 19:12 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
On Tue, Feb 07, 2012 at 11:02:28AM -0800, Tom Grennan wrote:
> >Would using sha1_array save us from having to create our own data
> >structure? As a bonus, it can do O(lg n) lookups, though I seriously
> >doubt anyone will provide a large number of "--points-at".
>
> Thanks, but I now realize that I also need to save the pointed at
> refname to detect lightweight tags that have matching sha's but
> different names.
I'm not sure I understand. Wouldn't you match lightweight tags by the
sha1 they point at? Something like:
static int tag_points_at(struct sha1_array *sa,
const unsigned char *sha1)
{
struct object *obj;
/* Lightweight tag of an interesting sha1? */
if (sha1_array_lookup(sa, sha1) >= 0)
return 1;
/* Otherwise, maybe a tag object pointing to an interesting sha1 */
obj = parse_object(sha1);
if (!obj)
return 0; /* or probably we should even just die() */
if (obj->type != OBJ_TAG)
return 0;
if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
return 0;
return 1;
}
> >Also, should you check "unset"? When we have options that build a list,
> >usually doing "--no-foo" will clear the list. E.g., this:
> >
> > git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
> >
> >should look only for "baz".
>
> Ahh, so I just need to:
> if (unset) {
> if (*opt_value)
> free_points_at(*opt_value);
> *opt_value = NULL;
> return 0;
> }
Yes, exactly.
> >> + {
> >> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
> >> + "print only annotated|signed tags of the object",
> >> + PARSE_OPT_LASTARG_DEFAULT,
> >> + parse_opt_points_at, (intptr_t)NULL,
> >> + },
> >
> >I think you can drop the LASTARG_DEFAULT here, as it is no longer
> >optional, no?
>
> You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?
Right. Though without flags, you can probably just use the OPT_CALLBACK
wrapper, like:
OPT_CALLBACK(0, "points-at", &points_at, "object",
"print only annotated|signed tags of the object",
parse_opt_points_at)
Note that if you are going to handle lightweight tags, that description
should probably be updated.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 19:12 ` Jeff King
@ 2012-02-07 19:22 ` Tom Grennan
2012-02-07 19:36 ` Jeff King
0 siblings, 1 reply; 53+ messages in thread
From: Tom Grennan @ 2012-02-07 19:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
On Tue, Feb 07, 2012 at 02:12:02PM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 11:02:28AM -0800, Tom Grennan wrote:
>
>> >Would using sha1_array save us from having to create our own data
>> >structure? As a bonus, it can do O(lg n) lookups, though I seriously
>> >doubt anyone will provide a large number of "--points-at".
>>
>> Thanks, but I now realize that I also need to save the pointed at
>> refname to detect lightweight tags that have matching sha's but
>> different names.
>
>I'm not sure I understand. Wouldn't you match lightweight tags by the
>sha1 they point at? Something like:
I think the following would show the pointed at tag too.
$ git tag my-v1.7.9 v1.7.9
$ ./git-tag -l --points-at v1.7.9
my-v1.7.9
v1.7.9
vs.
$ ./git-tag -l --points-at v1.7.9
my-v1.7.9
I found that I had to filter matching refnames.
> static int tag_points_at(struct sha1_array *sa,
> const unsigned char *sha1)
> {
> struct object *obj;
>
> /* Lightweight tag of an interesting sha1? */
> if (sha1_array_lookup(sa, sha1) >= 0)
> return 1;
>
> /* Otherwise, maybe a tag object pointing to an interesting sha1 */
> obj = parse_object(sha1);
> if (!obj)
> return 0; /* or probably we should even just die() */
> if (obj->type != OBJ_TAG)
> return 0;
> if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
> return 0;
> return 1;
> }
>
>> >Also, should you check "unset"? When we have options that build a list,
>> >usually doing "--no-foo" will clear the list. E.g., this:
>> >
>> > git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
>> >
>> >should look only for "baz".
>>
>> Ahh, so I just need to:
>> if (unset) {
>> if (*opt_value)
>> free_points_at(*opt_value);
>> *opt_value = NULL;
>> return 0;
>> }
>
>Yes, exactly.
>
>> >> + {
>> >> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> >> + "print only annotated|signed tags of the object",
>> >> + PARSE_OPT_LASTARG_DEFAULT,
>> >> + parse_opt_points_at, (intptr_t)NULL,
>> >> + },
>> >
>> >I think you can drop the LASTARG_DEFAULT here, as it is no longer
>> >optional, no?
>>
>> You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?
>
>Right. Though without flags, you can probably just use the OPT_CALLBACK
>wrapper, like:
>
> OPT_CALLBACK(0, "points-at", &points_at, "object",
> "print only annotated|signed tags of the object",
> parse_opt_points_at)
>
>Note that if you are going to handle lightweight tags, that description
>should probably be updated.
>
>-Peff
--
TomG
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 19:22 ` Tom Grennan
@ 2012-02-07 19:36 ` Jeff King
2012-02-07 20:20 ` Junio C Hamano
0 siblings, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-07 19:36 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
On Tue, Feb 07, 2012 at 11:22:50AM -0800, Tom Grennan wrote:
> >> Thanks, but I now realize that I also need to save the pointed at
> >> refname to detect lightweight tags that have matching sha's but
> >> different names.
> >
> >I'm not sure I understand. Wouldn't you match lightweight tags by the
> >sha1 they point at? Something like:
>
> I think the following would show the pointed at tag too.
> $ git tag my-v1.7.9 v1.7.9
> $ ./git-tag -l --points-at v1.7.9
> my-v1.7.9
> v1.7.9
>
> vs.
>
> $ ./git-tag -l --points-at v1.7.9
> my-v1.7.9
>
> I found that I had to filter matching refnames.
Ah, so you are trying _not_ to show lightweight tags (I thought you
meant you also wanted to show them)? But I still don't see why the code
I posted before wouldn't work in that case. The "object" field of v1.7.9
is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
would not match.
Maybe I don't understand what you mean. Can you show a test case that
is buggy with the v2 version of the patch that you sent? I'm not sure in
the example above what is different between the two "git-tag"
invocations.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 19:36 ` Jeff King
@ 2012-02-07 20:20 ` Junio C Hamano
2012-02-07 21:30 ` Jeff King
0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2012-02-07 20:20 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Grennan, git, gitster, jasampler
Jeff King <peff@peff.net> writes:
>> I think the following would show the pointed at tag too.
>> $ git tag my-v1.7.9 v1.7.9
>> $ ./git-tag -l --points-at v1.7.9
>> my-v1.7.9
>> v1.7.9
>>
>> vs.
>>
>> $ ./git-tag -l --points-at v1.7.9
>> my-v1.7.9
>>
>> I found that I had to filter matching refnames.
>
> Ah, so you are trying _not_ to show lightweight tags (I thought you
> meant you also wanted to show them)? But I still don't see why the code
> I posted before wouldn't work in that case. The "object" field of v1.7.9
> is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
> would not match.
I think he is trying to avoid saying "v1.7.9 points at itself", and wants
to know not just the value of $(rev-parse v1.7.9) but the refname.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 20:20 ` Junio C Hamano
@ 2012-02-07 21:30 ` Jeff King
2012-02-07 22:08 ` Tom Grennan
0 siblings, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-07 21:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
On Tue, Feb 07, 2012 at 12:20:44PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> I think the following would show the pointed at tag too.
> >> $ git tag my-v1.7.9 v1.7.9
> >> $ ./git-tag -l --points-at v1.7.9
> >> my-v1.7.9
> >> v1.7.9
> >>
> >> vs.
> >>
> >> $ ./git-tag -l --points-at v1.7.9
> >> my-v1.7.9
> >>
> >> I found that I had to filter matching refnames.
> >
> > Ah, so you are trying _not_ to show lightweight tags (I thought you
> > meant you also wanted to show them)? But I still don't see why the code
> > I posted before wouldn't work in that case. The "object" field of v1.7.9
> > is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
> > would not match.
>
> I think he is trying to avoid saying "v1.7.9 points at itself", and wants
> to know not just the value of $(rev-parse v1.7.9) but the refname.
Hmm. I read his example again, and now I'm even more confused.
If I give an object name to --points-at, should or should not a
lightweight tag pointing to that object be found?
If not, then I don't see how "git tag --points-at v1.7.9" would find
v1.7.9. Because we would use get_sha1 to parse "v1.7.9", returning the
sha1 of the tag object. And then when trying to match, we would look at
each tag object, find its "object" line, and compare that. In the case
of considering whether to show the v1.7.9 tag, we would be comparing the
sha1 of the commit that it points to to the actual tag sha1 itself, and
not match.
But in that case, nor would we match "my-v1.7.9" above, as it is a
lightweight tag that also points to v1.7.9's tag object.
If we _do_ want to match lightweight tags, then in the matching phase we
look for both the sha1 contained in the tag ref, as well as the sha1 of
the thing the tag points to (_if_ it is a tag object). In that case, we
would find both v1.7.9 and my-v1.7.9.
So I am not sure which is preferable. But I don't see how you could or
would want to distinguish the two tags above. They are functionally
identical, in that they are both refs pointing to the exact same tag
object. If the example had started with "git tag -s my-v1.7.9 v1.7.9"
then it would make more sense to me.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 21:30 ` Jeff King
@ 2012-02-07 22:08 ` Tom Grennan
2012-02-08 0:25 ` Jeff King
0 siblings, 1 reply; 53+ messages in thread
From: Tom Grennan @ 2012-02-07 22:08 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, jasampler
On Tue, Feb 07, 2012 at 04:30:12PM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 12:20:44PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> >> I think the following would show the pointed at tag too.
>> >> $ git tag my-v1.7.9 v1.7.9
>> >> $ ./git-tag -l --points-at v1.7.9
>> >> my-v1.7.9
>> >> v1.7.9
>> >>
>> >> vs.
>> >>
>> >> $ ./git-tag -l --points-at v1.7.9
>> >> my-v1.7.9
>> >>
>> >> I found that I had to filter matching refnames.
>> >
>> > Ah, so you are trying _not_ to show lightweight tags (I thought you
>> > meant you also wanted to show them)? But I still don't see why the code
>> > I posted before wouldn't work in that case. The "object" field of v1.7.9
>> > is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
>> > would not match.
>>
>> I think he is trying to avoid saying "v1.7.9 points at itself", and wants
>> to know not just the value of $(rev-parse v1.7.9) but the refname.
>
>Hmm. I read his example again, and now I'm even more confused.
>
>If I give an object name to --points-at, should or should not a
>lightweight tag pointing to that object be found?
>
>If not, then I don't see how "git tag --points-at v1.7.9" would find
>v1.7.9. Because we would use get_sha1 to parse "v1.7.9", returning the
>sha1 of the tag object. And then when trying to match, we would look at
>each tag object, find its "object" line, and compare that. In the case
>of considering whether to show the v1.7.9 tag, we would be comparing the
>sha1 of the commit that it points to to the actual tag sha1 itself, and
>not match.
>
>But in that case, nor would we match "my-v1.7.9" above, as it is a
>lightweight tag that also points to v1.7.9's tag object.
>
>If we _do_ want to match lightweight tags, then in the matching phase we
>look for both the sha1 contained in the tag ref, as well as the sha1 of
>the thing the tag points to (_if_ it is a tag object). In that case, we
>would find both v1.7.9 and my-v1.7.9.
>
>So I am not sure which is preferable. But I don't see how you could or
>would want to distinguish the two tags above. They are functionally
>identical, in that they are both refs pointing to the exact same tag
>object. If the example had started with "git tag -s my-v1.7.9 v1.7.9"
>then it would make more sense to me.
v1 and v2 wouldn't list lightweight tags of the points-at objects.
Both versions behave like this:
$ git tag my-lw-v1.7.9 v1.7.9
$ git tag my-a-v1.7.9 v1.7.9
$ git tag my-s-v1.7.9 v1.7.9
$ git tag -l --points-at v1.7.9
my-a-v1.7.9
my-s-v1.7.9
While addressing Junio's comments I realized that by first matching the
sha's and not refnames like the following will show LW tags too.
So, v3 will act like this:
$ git tag my-lw-v1.7.9 v1.7.9
$ git tag my-a-v1.7.9 v1.7.9
$ git tag my-s-v1.7.9 v1.7.9
$ git tag -l --points-at v1.7.9
my-lw-v1.7.9
my-a-v1.7.9
my-s-v1.7.9
Note, w/o strcmp(pa->refname, refname), this shows the points-at too:
$ git tag my-lw-v1.7.9 v1.7.9
$ git tag my-a-v1.7.9 v1.7.9
$ git tag my-s-v1.7.9 v1.7.9
$ git tag -l --points-at v1.7.9
my-lw-v1.7.9
my-a-v1.7.9
my-s-v1.7.9
v1.7.9
Which I don't think we'd want.
static struct points_at *match_points_at(struct points_at *points_at,
const char *refname,
const unsigned char *sha1)
{
struct object *obj;
struct points_at *pa;
const unsigned char *tagged_sha1;
/* First look for lightweight tags - those with matching sha's
* but different names */
for (pa = points_at; pa; pa = pa->next)
if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
return pa;
obj = parse_object(sha1);
if (!obj || obj->type != OBJ_TAG)
return 0;
tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
while (points_at && hashcmp(points_at->sha1, tagged_sha1))
points_at = points_at->next;
return points_at;
}
--
TomG
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-07 22:08 ` Tom Grennan
@ 2012-02-08 0:25 ` Jeff King
2012-02-08 1:45 ` Tom Grennan
` (2 more replies)
0 siblings, 3 replies; 53+ messages in thread
From: Jeff King @ 2012-02-08 0:25 UTC (permalink / raw)
To: Tom Grennan; +Cc: Junio C Hamano, git, jasampler
On Tue, Feb 07, 2012 at 02:08:06PM -0800, Tom Grennan wrote:
> v1 and v2 wouldn't list lightweight tags of the points-at objects.
> Both versions behave like this:
> $ git tag my-lw-v1.7.9 v1.7.9
> $ git tag my-a-v1.7.9 v1.7.9
> $ git tag my-s-v1.7.9 v1.7.9
> $ git tag -l --points-at v1.7.9
> my-a-v1.7.9
> my-s-v1.7.9
I assume the 2nd and 3rd line should be:
$ git tag -a my-a-v1.7.9 v1.7.9
$ git tag -s my-s-v1.7.9 v1.7.9
> static struct points_at *match_points_at(struct points_at *points_at,
> const char *refname,
> const unsigned char *sha1)
> {
> struct object *obj;
> struct points_at *pa;
> const unsigned char *tagged_sha1;
>
> /* First look for lightweight tags - those with matching sha's
> * but different names */
> for (pa = points_at; pa; pa = pa->next)
> if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
> return pa;
OK, I see what you are trying to accomplish here. But I really don't
like it. Two complaints:
1. Why is the name of the tag relevant? That is, if you are interested
in lightweight tags, and you have two tag refs, "refs/tags/a" and
"refs/tags/b", both pointing to the same tag object, then in what
situation is it useful to show "a" but not "b"?
It seems to me you would either want lightweight tags or not. And I
thought not, because the point of this was to reveal signatures or
annotations about a tag. Your my-lw-v1.7.9 says neither. Why do we
want to show it?
Also, it's not symmetric. What if I say "git tag
--points-at=my-lw-v1.7.9"? Then I would get your signed and
annotated tags (even though they're _not_ saying anything about
ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying
anything about it either; in fact, it's the opposite!).
2. I thought --points-at was about providing an object name. But it's
not. It's about providing a particular string. So with this code,
"git tag --points-at=v1.7.9" and "git tag --points-at=$(git
rev-parse v1.7.9)" are two different things. Which seems odd and
un-git-like to me.
Your documentation says "Only list annotated or signed tags of the
given object", which implies to me that --points-at is an arbitrary
object specifier, not a specific tagname.
It seems like your rationale is just avoiding a mention of v1.7.9
because, hey, it was obviously on the command line and the user isn't
interested in it. But I don't think that's true. The user asked for
every tag pointing to v1.7.9's object, and v1.7.9 is such a tag. It is
no more or less true for v1.7.9 than it is for my-lw-v1.7.9.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-08 0:25 ` Jeff King
@ 2012-02-08 1:45 ` Tom Grennan
2012-02-08 15:31 ` Jeff King
2012-02-08 6:21 ` [PATCHv3] " Tom Grennan
2012-02-08 6:21 ` Tom Grennan
2 siblings, 1 reply; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 1:45 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, jasampler
On Tue, Feb 07, 2012 at 07:25:54PM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 02:08:06PM -0800, Tom Grennan wrote:
>
>> v1 and v2 wouldn't list lightweight tags of the points-at objects.
>> Both versions behave like this:
>> $ git tag my-lw-v1.7.9 v1.7.9
>> $ git tag my-a-v1.7.9 v1.7.9
>> $ git tag my-s-v1.7.9 v1.7.9
>> $ git tag -l --points-at v1.7.9
>> my-a-v1.7.9
>> my-s-v1.7.9
>
>I assume the 2nd and 3rd line should be:
>
> $ git tag -a my-a-v1.7.9 v1.7.9
> $ git tag -s my-s-v1.7.9 v1.7.9
Yes
>> static struct points_at *match_points_at(struct points_at *points_at,
>> const char *refname,
>> const unsigned char *sha1)
>> {
>> struct object *obj;
>> struct points_at *pa;
>> const unsigned char *tagged_sha1;
>>
>> /* First look for lightweight tags - those with matching sha's
>> * but different names */
>> for (pa = points_at; pa; pa = pa->next)
>> if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
>> return pa;
>
>OK, I see what you are trying to accomplish here. But I really don't
>like it. Two complaints:
>
> 1. Why is the name of the tag relevant? That is, if you are interested
> in lightweight tags, and you have two tag refs, "refs/tags/a" and
> "refs/tags/b", both pointing to the same tag object, then in what
> situation is it useful to show "a" but not "b"?
Yes, I suppose this is more "tags or aliases of <object>" rather than
"tags that point at <object>".
> It seems to me you would either want lightweight tags or not. And I
> thought not, because the point of this was to reveal signatures or
> annotations about a tag. Your my-lw-v1.7.9 says neither. Why do we
> want to show it?
Initially I didn't care about listing these lightweight tags (aliases)
but now I see that this could be useful to find turds in refs/tags.
$ git tag my-v.1.7.9 v1.7.9
...
$ git tag -l --points-at v1.7.9
my-v.1.7.9
Oops
> Also, it's not symmetric. What if I say "git tag
> --points-at=my-lw-v1.7.9"? Then I would get your signed and
> annotated tags (even though they're _not_ saying anything about
> ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying
> anything about it either; in fact, it's the opposite!).
Huh? As you noted, the lightweight tag is just an alternate reference,
so why wouldn't want to see the annotated and signed tags of that common
object?
$ ./git-tag -l --points-at tomg-lw-v1.7.9
tomg-annotate-v1.7.9
tomg-signed-v1.7.9
v1.7.9
$ ./git-tag -l --points-at v1.7.9
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
> 2. I thought --points-at was about providing an object name. But it's
> not. It's about providing a particular string. So with this code,
> "git tag --points-at=v1.7.9" and "git tag --points-at=$(git
> rev-parse v1.7.9)" are two different things. Which seems odd and
> un-git-like to me.
Yep,
$ ./git-tag -l --points-at $(git rev-parse v1.7.9)
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
v1.7.9
> Your documentation says "Only list annotated or signed tags of the
> given object", which implies to me that --points-at is an arbitrary
> object specifier, not a specific tagname.
Yes, I changed that in the patch that I've prepared but will revert this
if you'd rather not list these lightweight tags.
>It seems like your rationale is just avoiding a mention of v1.7.9
>because, hey, it was obviously on the command line and the user isn't
>interested in it.
Yes, exactly.
>But I don't think that's true. The user asked for every tag pointing to
>v1.7.9's object, and v1.7.9 is such a tag. It is no more or less true
>for v1.7.9 than it is for my-lw-v1.7.9.
My reaction when I tested this was, "don't tell me what I already know."
But consistency with $(git rev-parse ...) seems more important.
And as you noted, a sha1_array would save code and to me, less code is
always better.
Thanks,
TomG
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv2] tag: add --points-at list option
2012-02-08 1:45 ` Tom Grennan
@ 2012-02-08 15:31 ` Jeff King
0 siblings, 0 replies; 53+ messages in thread
From: Jeff King @ 2012-02-08 15:31 UTC (permalink / raw)
To: Tom Grennan; +Cc: Junio C Hamano, git, jasampler
On Tue, Feb 07, 2012 at 05:45:15PM -0800, Tom Grennan wrote:
> > Also, it's not symmetric. What if I say "git tag
> > --points-at=my-lw-v1.7.9"? Then I would get your signed and
> > annotated tags (even though they're _not_ saying anything about
> > ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying
> > anything about it either; in fact, it's the opposite!).
>
> Huh? As you noted, the lightweight tag is just an alternate reference,
> so why wouldn't want to see the annotated and signed tags of that common
> object?
>
> $ ./git-tag -l --points-at tomg-lw-v1.7.9
> tomg-annotate-v1.7.9
> tomg-signed-v1.7.9
> v1.7.9
> $ ./git-tag -l --points-at v1.7.9
> tomg-annotate-v1.7.9
> tomg-lw-v1.7.9
> tomg-signed-v1.7.9
Sorry, I should have been more clear here (the word symmetric isn't
right; it _is_ symmetric). My understanding of the point of your
original feature was to mention things that talk about a tag (because
you wanted to know what signatures were made around it).
With tag objects this is easy, because they contain a pointer. But when
it comes to lightweight tags, you cannot tell in which direction the
"talking about" occurred[1]. That is, a lightweight tag of another tag
is just creating a new ref, which looks the same as the old ref. So
something like --points-at cannot say "X talks about Y", because it
might as well have been "Y talks about X".
So I think you are better off to mention both X and Y (or to mention
neither).
> > Your documentation says "Only list annotated or signed tags of the
> > given object", which implies to me that --points-at is an arbitrary
> > object specifier, not a specific tagname.
>
> Yes, I changed that in the patch that I've prepared but will revert this
> if you'd rather not list these lightweight tags.
I'm OK with not mentioning lightweight tags. I just feel it should be
all-or-nothing. It was specifically the "given object" that I took issue
with, since in your examples v1.7.9 was treated differently from its
sha1.
> My reaction when I tested this was, "don't tell me what I already know."
> But consistency with $(git rev-parse ...) seems more important.
> And as you noted, a sha1_array would save code and to me, less code is
> always better.
Thanks. Either I've convinced you, or I've made you so sick of the
discussion that you're agreeing. The system works. :)
-Peff
[1] Actually, a tag object embeds the name of the ref under which it was
originally created (so the refs/tags/v1.7.9 tag has a "tag v1.7.9"
header in it). So in some cases, you _can_ determine the "original" ref
of a lightweight tag versus other refs made about it later. I'm still
not sure --points-at is a good place to try to make that distinction,
though.
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCHv3] tag: add --points-at list option
2012-02-08 0:25 ` Jeff King
2012-02-08 1:45 ` Tom Grennan
@ 2012-02-08 6:21 ` Tom Grennan
2012-02-08 6:21 ` Tom Grennan
2 siblings, 0 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 6:21 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
Please see version 3 of the "points-at" feature. In addition to addressing
the comments on v2, this now lists lightweight tags to the given object.
Tom Grennan (1):
tag: add --points-at list option
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 51 insertions(+), 4 deletions(-)
--
1.7.8
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCHv3] tag: add --points-at list option
2012-02-08 0:25 ` Jeff King
2012-02-08 1:45 ` Tom Grennan
2012-02-08 6:21 ` [PATCHv3] " Tom Grennan
@ 2012-02-08 6:21 ` Tom Grennan
2012-02-08 15:44 ` Jeff King
2 siblings, 1 reply; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 6:21 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
This filters the list for tags of the given object.
Example,
john$ git tag v1.0-john v1.0
john$ git tag -l --points-at v1.0
v1.0-john
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5ead91e..124ed36 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
<tagname> [<commit> | <object>]
'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
[--column[=<options>] | --no-column] [<pattern>...]
'git tag' -v <tagname>...
@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.
--contains <commit>::
Only list tags which contain the specified commit.
+--points-at <object>::
+ Only list tags of the given object.
+
-m <msg>::
--message=<msg>::
Use the given tag message (instead of prompting).
diff --git a/builtin/tag.c b/builtin/tag.c
index 5fbd62c..c5da622 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -16,11 +16,13 @@
#include "revision.h"
#include "gpg-interface.h"
#include "column.h"
+#include "sha1-array.h"
static const char * const git_tag_usage[] = {
"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
"git tag -d <tagname>...",
- "git tag -l [-n[<num>]] [<pattern>...]",
+ "git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\"
+ "\n\t\t[<pattern>...]",
"git tag -v <tagname>...",
NULL
};
@@ -31,6 +33,7 @@ struct tag_filter {
struct commit_list *with_commit;
};
+static struct sha1_array points_at;
static unsigned int colopts;
static int match_pattern(const char **patterns, const char *ref)
@@ -44,6 +47,22 @@ static int match_pattern(const char **patterns, const char *ref)
return 0;
}
+static const unsigned char *match_points_at(const unsigned char *sha1)
+{
+ int i;
+ const unsigned char *tagged_sha1 = (unsigned char*)"";
+ struct object *obj = parse_object(sha1);
+
+ if (obj && obj->type == OBJ_TAG)
+ tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+ for (i = 0; i < points_at.nr; i++)
+ if (!hashcmp(points_at.sha1[i], sha1))
+ return sha1;
+ else if (!hashcmp(points_at.sha1[i], tagged_sha1))
+ return tagged_sha1;
+ return NULL;
+}
+
static int in_commit_list(const struct commit_list *want, struct commit *c)
{
for (; want; want = want->next)
@@ -141,6 +160,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
+ if (points_at.nr && !match_points_at(sha1))
+ return 0;
+
if (!filter->lines) {
printf("%s\n", refname);
return 0;
@@ -389,6 +411,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
return check_refname_format(sb->buf, 0);
}
+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+ const char *arg, int unset)
+{
+ unsigned char sha1[20];
+
+ if (unset) {
+ sha1_array_clear(&points_at);
+ return 0;
+ }
+ if (!arg)
+ return error(_("switch 'points-at' requires an object"));
+ if (get_sha1(arg, sha1))
+ return error(_("malformed object name '%s'"), arg);
+ sha1_array_append(&points_at, sha1);
+ return 0;
+}
+
int cmd_tag(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
@@ -432,6 +471,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
PARSE_OPT_LASTARG_DEFAULT,
parse_opt_with_commit, (intptr_t)"HEAD",
},
+ {
+ OPTION_CALLBACK, 0, "points-at", NULL, "object",
+ "print only tags of the object", 0, parse_opt_points_at
+ },
OPT_END()
};
@@ -478,8 +521,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
}
if (lines != -1)
die(_("-n option is only allowed with -l."));
- if (with_commit)
- die(_("--contains option is only allowed with -l."));
+ if (with_commit || points_at.nr)
+ die(_("--contains and --points-at options "
+ "are only allowed with -l."));
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
--
1.7.8
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCHv3] tag: add --points-at list option
2012-02-08 6:21 ` Tom Grennan
@ 2012-02-08 15:44 ` Jeff King
2012-02-08 18:43 ` Tom Grennan
0 siblings, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-08 15:44 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
> +static const unsigned char *match_points_at(const unsigned char *sha1)
> +{
> + int i;
> + const unsigned char *tagged_sha1 = (unsigned char*)"";
> + struct object *obj = parse_object(sha1);
> +
> + if (obj && obj->type == OBJ_TAG)
> + tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
This is not safe. A sha1 is not NUL-terminated, but is rather _always_
20 bytes. So when the object is not a tag, you do the hashcmp against
your single-byte string literal above, and we end up comparing whatever
garbage is in the data segment after the string literal.
What you want instead is the all-zeros sha1, like:
const unsigned char null_sha1[20] = { 0 };
Though we provide a null_sha1 global already. So doing:
const unsigned char *tagged_sha1 = null_sha1;
would be sufficient.
That being said, I don't know why you want to do both lookups in the
same loop of the points_at. If it's a lightweight tag and the tag
matches, you can get away with not parsing the object at all (although
to be fair, that is the minority case, so it is unlikely to matter).
Also, should we be producing an error if !obj? It would indicate a tag
that points to a bogus object.
> + for (i = 0; i < points_at.nr; i++)
> + if (!hashcmp(points_at.sha1[i], sha1))
> + return sha1;
> + else if (!hashcmp(points_at.sha1[i], tagged_sha1))
> + return tagged_sha1;
> + return NULL;
Why write your own linear search? sha1_array_lookup will do a binary
search for you.
Other than that, the patch looks OK to me.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv3] tag: add --points-at list option
2012-02-08 15:44 ` Jeff King
@ 2012-02-08 18:43 ` Tom Grennan
2012-02-08 18:57 ` Jeff King
2012-02-08 18:58 ` [PATCHv3] " Tom Grennan
0 siblings, 2 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 18:43 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
>
>> +static const unsigned char *match_points_at(const unsigned char *sha1)
>> +{
>> + int i;
>> + const unsigned char *tagged_sha1 = (unsigned char*)"";
>> + struct object *obj = parse_object(sha1);
>> +
>> + if (obj && obj->type == OBJ_TAG)
>> + tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
>
>This is not safe. A sha1 is not NUL-terminated, but is rather _always_
>20 bytes. So when the object is not a tag, you do the hashcmp against
>your single-byte string literal above, and we end up comparing whatever
>garbage is in the data segment after the string literal.
Yikes! That was dumb.
>What you want instead is the all-zeros sha1, like:
>
> const unsigned char null_sha1[20] = { 0 };
>
>Though we provide a null_sha1 global already. So doing:
>
> const unsigned char *tagged_sha1 = null_sha1;
>
>would be sufficient.
Or just initialize at test tagged_sha1 with NULL.
static const unsigned char *match_points_at(const unsigned char *sha1)
{
int i;
const unsigned char *tagged_sha1 = NULL;
struct object *obj = parse_object(sha1);
if (obj && obj->type == OBJ_TAG)
tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
for (i = 0; i < points_at.nr; i++)
if (!hashcmp(points_at.sha1[i], sha1))
return sha1;
else if (tagged_sha1 &&
!hashcmp(points_at.sha1[i], tagged_sha1))
return tagged_sha1;
return NULL;
}
>That being said, I don't know why you want to do both lookups in the
>same loop of the points_at. If it's a lightweight tag and the tag
>matches, you can get away with not parsing the object at all (although
>to be fair, that is the minority case, so it is unlikely to matter).
Yes, I think your saying that the lightweight search could go before the
tag object search like this.
static const unsigned char *match_points_at(const unsigned char *sha1)
{
const unsigned char *tagged_sha1 = NULL;
struct object *obj = parse_object(sha1);
if (sha1_array_lookup(&points_at, sha1) >= 0)
return sha1;
if (obj && obj->type == OBJ_TAG)
tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
return tagged_sha1;
return NULL;
}
>Also, should we be producing an error if !obj? It would indicate a tag
>that points to a bogus object.
I think the test of (obj) is redundant as this should be caught
by get_sha1() in parse_opt_points_at()
int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
const char *arg, int unset)
{
unsigned char sha1[20];
if (unset) {
sha1_array_clear(&points_at);
return 0;
}
if (!arg)
return error(_("switch 'points-at' requires an object"));
if (get_sha1(arg, sha1))
return error(_("malformed object name '%s'"), arg);
sha1_array_append(&points_at, sha1);
return 0;
}
>> + for (i = 0; i < points_at.nr; i++)
>> + if (!hashcmp(points_at.sha1[i], sha1))
>> + return sha1;
>> + else if (!hashcmp(points_at.sha1[i], tagged_sha1))
>> + return tagged_sha1;
>> + return NULL;
>
>Why write your own linear search? sha1_array_lookup will do a binary
>search for you.
Well, it's only a linear search of the points_at command arguments.
But by that reasoning, might as well do two sha1_array_lookups like
above and save some code b/c "less code is always better"(TM).
>Other than that, the patch looks OK to me.
Thanks, I'll send what I hope to be the final version later today.
--
TomG
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCHv3] tag: add --points-at list option
2012-02-08 18:43 ` Tom Grennan
@ 2012-02-08 18:57 ` Jeff King
2012-02-08 20:12 ` [PATCHv4] " Tom Grennan
2012-02-08 20:12 ` Tom Grennan
2012-02-08 18:58 ` [PATCHv3] " Tom Grennan
1 sibling, 2 replies; 53+ messages in thread
From: Jeff King @ 2012-02-08 18:57 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:
> >Though we provide a null_sha1 global already. So doing:
> >
> > const unsigned char *tagged_sha1 = null_sha1;
> >
> >would be sufficient.
>
> Or just initialize at test tagged_sha1 with NULL.
Oh yeah, that is even better.
> >That being said, I don't know why you want to do both lookups in the
> >same loop of the points_at. If it's a lightweight tag and the tag
> >matches, you can get away with not parsing the object at all (although
> >to be fair, that is the minority case, so it is unlikely to matter).
>
> Yes, I think your saying that the lightweight search could go before the
> tag object search like this.
Exactly, though:
> static const unsigned char *match_points_at(const unsigned char *sha1)
> {
> const unsigned char *tagged_sha1 = NULL;
> struct object *obj = parse_object(sha1);
>
> if (sha1_array_lookup(&points_at, sha1) >= 0)
> return sha1;
> if (obj && obj->type == OBJ_TAG)
> tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
You can delay the relatively expensive parse_object until you find the
results of the first lookup (though like I said earlier, it is unlikely to
matter, as it only helps in the positive-match case. Out of N tags, you
will likely end up parsing N-1 of them anyway).
> >Also, should we be producing an error if !obj? It would indicate a tag
> >that points to a bogus object.
>
> I think the test of (obj) is redundant as this should be caught
> by get_sha1() in parse_opt_points_at()
No, it's not redundant. get_sha1 is purely about looking up the name and
finding a sha1. parse_object is about looking up the object represented
by that sha1 in the object db. get_sha1 can sometimes involve parsing
objects (e.g., looking for "foo^1" will need to parse the commit object
at "foo"), but does not have to.
Besides which, you are not calling parse_object on the sha1 from
--points-at, but rather the sha1 for each tag ref given to us by
for_each_tag_ref.
> >Why write your own linear search? sha1_array_lookup will do a binary
> >search for you.
>
> Well, it's only a linear search of the points_at command arguments.
> But by that reasoning, might as well do two sha1_array_lookups like
> above and save some code b/c "less code is always better"(TM).
Right. I expect the N to be small in this case, so I doubt it matters.
But two sha1_array_lookups is still asymptotically smaller, because the
expensive operation is hashcmp(). So two binary searches is O(2*lg n),
whereas a linear walk with 2 hashcmps per item is O(2*n).
> >Other than that, the patch looks OK to me.
>
> Thanks, I'll send what I hope to be the final version later today.
Thanks for working on this and being so responsive to review.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread* [PATCHv4] tag: add --points-at list option
2012-02-08 18:57 ` Jeff King
@ 2012-02-08 20:12 ` Tom Grennan
2012-02-08 20:12 ` Tom Grennan
1 sibling, 0 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 20:12 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
Please see the following patch that I hoped is the last version of the
"points-at" feature. Thank you for your patience.
Tom Grennan (1):
tag: add --points-at list option
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 52 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 53 insertions(+), 4 deletions(-)
--
1.7.8
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCHv4] tag: add --points-at list option
2012-02-08 18:57 ` Jeff King
2012-02-08 20:12 ` [PATCHv4] " Tom Grennan
@ 2012-02-08 20:12 ` Tom Grennan
2012-02-08 20:58 ` Jeff King
1 sibling, 1 reply; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 20:12 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
This filters the list for tags of the given object.
Example,
john$ git tag v1.0-john v1.0
john$ git tag -l --points-at v1.0
v1.0-john
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 52 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5ead91e..124ed36 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
<tagname> [<commit> | <object>]
'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
[--column[=<options>] | --no-column] [<pattern>...]
'git tag' -v <tagname>...
@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.
--contains <commit>::
Only list tags which contain the specified commit.
+--points-at <object>::
+ Only list tags of the given object.
+
-m <msg>::
--message=<msg>::
Use the given tag message (instead of prompting).
diff --git a/builtin/tag.c b/builtin/tag.c
index 5fbd62c..f3051c7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -16,11 +16,13 @@
#include "revision.h"
#include "gpg-interface.h"
#include "column.h"
+#include "sha1-array.h"
static const char * const git_tag_usage[] = {
"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
"git tag -d <tagname>...",
- "git tag -l [-n[<num>]] [<pattern>...]",
+ "git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\"
+ "\n\t\t[<pattern>...]",
"git tag -v <tagname>...",
NULL
};
@@ -31,6 +33,7 @@ struct tag_filter {
struct commit_list *with_commit;
};
+static struct sha1_array points_at;
static unsigned int colopts;
static int match_pattern(const char **patterns, const char *ref)
@@ -44,6 +47,24 @@ static int match_pattern(const char **patterns, const char *ref)
return 0;
}
+static const unsigned char *match_points_at(const char *refname,
+ const unsigned char *sha1)
+{
+ const unsigned char *tagged_sha1 = NULL;
+ struct object *obj;
+
+ if (sha1_array_lookup(&points_at, sha1) >= 0)
+ return sha1;
+ obj = parse_object(sha1);
+ if (!obj)
+ die(_("malformed object at '%s'"), refname);
+ if (obj->type == OBJ_TAG)
+ tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+ if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+ return tagged_sha1;
+ return NULL;
+}
+
static int in_commit_list(const struct commit_list *want, struct commit *c)
{
for (; want; want = want->next)
@@ -141,6 +162,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
+ if (points_at.nr && !match_points_at(refname, sha1))
+ return 0;
+
if (!filter->lines) {
printf("%s\n", refname);
return 0;
@@ -389,6 +413,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
return check_refname_format(sb->buf, 0);
}
+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+ const char *arg, int unset)
+{
+ unsigned char sha1[20];
+
+ if (unset) {
+ sha1_array_clear(&points_at);
+ return 0;
+ }
+ if (!arg)
+ return error(_("switch 'points-at' requires an object"));
+ if (get_sha1(arg, sha1))
+ return error(_("malformed object name '%s'"), arg);
+ sha1_array_append(&points_at, sha1);
+ return 0;
+}
+
int cmd_tag(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
@@ -432,6 +473,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
PARSE_OPT_LASTARG_DEFAULT,
parse_opt_with_commit, (intptr_t)"HEAD",
},
+ {
+ OPTION_CALLBACK, 0, "points-at", NULL, "object",
+ "print only tags of the object", 0, parse_opt_points_at
+ },
OPT_END()
};
@@ -478,8 +523,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
}
if (lines != -1)
die(_("-n option is only allowed with -l."));
- if (with_commit)
- die(_("--contains option is only allowed with -l."));
+ if (with_commit || points_at.nr)
+ die(_("--contains and --points-at options "
+ "are only allowed with -l."));
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
--
1.7.8
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCHv4] tag: add --points-at list option
2012-02-08 20:12 ` Tom Grennan
@ 2012-02-08 20:58 ` Jeff King
2012-02-08 22:15 ` Tom Grennan
` (2 more replies)
0 siblings, 3 replies; 53+ messages in thread
From: Jeff King @ 2012-02-08 20:58 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:
> This filters the list for tags of the given object.
> Example,
>
> john$ git tag v1.0-john v1.0
> john$ git tag -l --points-at v1.0
> v1.0-john
And probably "v1.0", as well, in this iteration. :)
The patch content itself looks good to me, except:
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -12,7 +12,7 @@ SYNOPSIS
> 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
> <tagname> [<commit> | <object>]
> 'git tag' -d <tagname>...
> -'git tag' [-n[<num>]] -l [--contains <commit>]
> +'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
> [--column[=<options>] | --no-column] [<pattern>...]
What's this "column" stuff doing here? The nd/columns topic is still in
"next", isn't it? Did you base this on "next" or "pu"?
Usually topics should be based on master, so they can graduate
independently of each other. In this case, it might make sense to build
on top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is
even necessary here (my fixes ended up not being too closely related, I
think).
Other than that, I think the patch is fine. There are no tests, so
perhaps these should be squashed in:
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..f61e398 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
test_must_fail git tag -v -s
'
+# check points-at
+
+test_expect_success '--points-at cannot be used in non-list mode' '
+ test_must_fail git tag --points-at=v4.0 foo
+'
+
+test_expect_success '--points-at finds lightweight tags' '
+ echo v4.0 >expect &&
+ git tag --points-at v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of commits' '
+ git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
+ echo annotated-v4.0 >expect &&
+ git tag -l --points-at v4.0 "annotated*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of tags' '
+ git tag -m "describing the v4.0 tag object" \
+ annotated-again-v4.0 annotated-v4.0 &&
+ cat >expect <<-\EOF &&
+ annotated-again-v4.0
+ annotated-v4.0
+ EOF
+ git tag --points-at=annotated-v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple --points-at are OR-ed together' '
+ cat >expect <<-\EOF &&
+ v2.0
+ v3.0
+ EOF
+ git tag --points-at=v2.0 --points-at=v3.0 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.9.rc2.14.g3da2b
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCHv4] tag: add --points-at list option
2012-02-08 20:58 ` Jeff King
@ 2012-02-08 22:15 ` Tom Grennan
2012-02-08 23:03 ` [PATCH-master] " Tom Grennan
2012-02-08 23:03 ` Tom Grennan
2 siblings, 0 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 22:15 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
On Wed, Feb 08, 2012 at 03:58:57PM -0500, Jeff King wrote:
>On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:
>
>> This filters the list for tags of the given object.
>> Example,
>>
>> john$ git tag v1.0-john v1.0
>> john$ git tag -l --points-at v1.0
>> v1.0-john
>
>And probably "v1.0", as well, in this iteration. :)
Yep.
>The patch content itself looks good to me, except:
>
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -12,7 +12,7 @@ SYNOPSIS
>> 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
>> <tagname> [<commit> | <object>]
>> 'git tag' -d <tagname>...
>> -'git tag' [-n[<num>]] -l [--contains <commit>]
>> +'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>> [--column[=<options>] | --no-column] [<pattern>...]
>
>What's this "column" stuff doing here? The nd/columns topic is still in
>"next", isn't it? Did you base this on "next" or "pu"?
>
>Usually topics should be based on master, so they can graduate
>independently of each other. In this case, it might make sense to build
>on top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is
>even necessary here (my fixes ended up not being too closely related, I
>think).
Yes, it's no longer related to jk/maint-tag-show-fixes.
I've prepared a rebase patch to master and will add these tests.
Thanks,
TomG
>Other than that, I think the patch is fine. There are no tests, so
>perhaps these should be squashed in:
>
>diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>index e93ac73..f61e398 100755
>--- a/t/t7004-tag.sh
>+++ b/t/t7004-tag.sh
>@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
> test_must_fail git tag -v -s
> '
>
>+# check points-at
>+
>+test_expect_success '--points-at cannot be used in non-list mode' '
>+ test_must_fail git tag --points-at=v4.0 foo
>+'
>+
>+test_expect_success '--points-at finds lightweight tags' '
>+ echo v4.0 >expect &&
>+ git tag --points-at v4.0 >actual &&
>+ test_cmp expect actual
>+'
>+
>+test_expect_success '--points-at finds annotated tags of commits' '
>+ git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
>+ echo annotated-v4.0 >expect &&
>+ git tag -l --points-at v4.0 "annotated*" >actual &&
>+ test_cmp expect actual
>+'
>+
>+test_expect_success '--points-at finds annotated tags of tags' '
>+ git tag -m "describing the v4.0 tag object" \
>+ annotated-again-v4.0 annotated-v4.0 &&
>+ cat >expect <<-\EOF &&
>+ annotated-again-v4.0
>+ annotated-v4.0
>+ EOF
>+ git tag --points-at=annotated-v4.0 >actual &&
>+ test_cmp expect actual
>+'
>+
>+test_expect_success 'multiple --points-at are OR-ed together' '
>+ cat >expect <<-\EOF &&
>+ v2.0
>+ v3.0
>+ EOF
>+ git tag --points-at=v2.0 --points-at=v3.0 >actual &&
>+ test_cmp expect actual
>+'
>+
> test_done
>--
>1.7.9.rc2.14.g3da2b
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH-master] tag: add --points-at list option
2012-02-08 20:58 ` Jeff King
2012-02-08 22:15 ` Tom Grennan
@ 2012-02-08 23:03 ` Tom Grennan
2012-02-08 23:03 ` Tom Grennan
2 siblings, 0 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 23:03 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
The following applies the "points-at" feature to master and includes unit tests
from Jeff King <peff@peff.net>
Tom Grennan (1):
tag: add --points-at list option
Documentation/git-tag.txt | 6 ++++-
builtin/tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
t/t7004-tag.sh | 39 +++++++++++++++++++++++++++++++++++
3 files changed, 93 insertions(+), 2 deletions(-)
--
1.7.8
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH-master] tag: add --points-at list option
2012-02-08 20:58 ` Jeff King
2012-02-08 22:15 ` Tom Grennan
2012-02-08 23:03 ` [PATCH-master] " Tom Grennan
@ 2012-02-08 23:03 ` Tom Grennan
2012-02-09 1:44 ` Jeff King
2 siblings, 1 reply; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 23:03 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
This filters the list for tags of the given object.
Example,
john$ git tag v1.0-john v1.0
john$ git tag -l --points-at v1.0
v1.0-john
v1.0
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-tag.txt | 6 ++++-
builtin/tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
t/t7004-tag.sh | 39 +++++++++++++++++++++++++++++++++++
3 files changed, 93 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 53ff5f6..8d32b9a 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,8 @@ SYNOPSIS
'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
<tagname> [<commit> | <object>]
'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>...]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
+ [<pattern>...]
'git tag' -v <tagname>...
DESCRIPTION
@@ -86,6 +87,9 @@ OPTIONS
--contains <commit>::
Only list tags which contain the specified commit.
+--points-at <object>::
+ Only list tags of the given object.
+
-m <msg>::
--message=<msg>::
Use the given tag message (instead of prompting).
diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..27c3557 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -15,11 +15,13 @@
#include "diff.h"
#include "revision.h"
#include "gpg-interface.h"
+#include "sha1-array.h"
static const char * const git_tag_usage[] = {
"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
"git tag -d <tagname>...",
- "git tag -l [-n[<num>]] [<pattern>...]",
+ "git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] "
+ "\n\t\t[<pattern>...]",
"git tag -v <tagname>...",
NULL
};
@@ -30,6 +32,8 @@ struct tag_filter {
struct commit_list *with_commit;
};
+static struct sha1_array points_at;
+
static int match_pattern(const char **patterns, const char *ref)
{
/* no pattern means match everything */
@@ -41,6 +45,24 @@ static int match_pattern(const char **patterns, const char *ref)
return 0;
}
+static const unsigned char *match_points_at(const char *refname,
+ const unsigned char *sha1)
+{
+ const unsigned char *tagged_sha1 = NULL;
+ struct object *obj;
+
+ if (sha1_array_lookup(&points_at, sha1) >= 0)
+ return sha1;
+ obj = parse_object(sha1);
+ if (!obj)
+ die(_("malformed object at '%s'"), refname);
+ if (obj->type == OBJ_TAG)
+ tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+ if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+ return tagged_sha1;
+ return NULL;
+}
+
static int in_commit_list(const struct commit_list *want, struct commit *c)
{
for (; want; want = want->next)
@@ -105,6 +127,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
+ if (points_at.nr && !match_points_at(refname, sha1))
+ return 0;
+
if (!filter->lines) {
printf("%s\n", refname);
return 0;
@@ -375,6 +400,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
return check_refname_format(sb->buf, 0);
}
+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+ const char *arg, int unset)
+{
+ unsigned char sha1[20];
+
+ if (unset) {
+ sha1_array_clear(&points_at);
+ return 0;
+ }
+ if (!arg)
+ return error(_("switch 'points-at' requires an object"));
+ if (get_sha1(arg, sha1))
+ return error(_("malformed object name '%s'"), arg);
+ sha1_array_append(&points_at, sha1);
+ return 0;
+}
+
int cmd_tag(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
@@ -417,6 +459,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
PARSE_OPT_LASTARG_DEFAULT,
parse_opt_with_commit, (intptr_t)"HEAD",
},
+ {
+ OPTION_CALLBACK, 0, "points-at", NULL, "object",
+ "print only tags of the object", 0, parse_opt_points_at
+ },
OPT_END()
};
@@ -448,6 +494,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("-n option is only allowed with -l."));
if (with_commit)
die(_("--contains option is only allowed with -l."));
+ if (points_at.nr)
+ die(_("--points-at option is only allowed with -l."));
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..f61e398 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
test_must_fail git tag -v -s
'
+# check points-at
+
+test_expect_success '--points-at cannot be used in non-list mode' '
+ test_must_fail git tag --points-at=v4.0 foo
+'
+
+test_expect_success '--points-at finds lightweight tags' '
+ echo v4.0 >expect &&
+ git tag --points-at v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of commits' '
+ git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
+ echo annotated-v4.0 >expect &&
+ git tag -l --points-at v4.0 "annotated*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of tags' '
+ git tag -m "describing the v4.0 tag object" \
+ annotated-again-v4.0 annotated-v4.0 &&
+ cat >expect <<-\EOF &&
+ annotated-again-v4.0
+ annotated-v4.0
+ EOF
+ git tag --points-at=annotated-v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple --points-at are OR-ed together' '
+ cat >expect <<-\EOF &&
+ v2.0
+ v3.0
+ EOF
+ git tag --points-at=v2.0 --points-at=v3.0 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.8
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH-master] tag: add --points-at list option
2012-02-08 23:03 ` Tom Grennan
@ 2012-02-09 1:44 ` Jeff King
2012-02-09 4:29 ` Junio C Hamano
0 siblings, 1 reply; 53+ messages in thread
From: Jeff King @ 2012-02-09 1:44 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
On Wed, Feb 08, 2012 at 03:03:43PM -0800, Tom Grennan wrote:
> This filters the list for tags of the given object.
> Example,
>
> john$ git tag v1.0-john v1.0
> john$ git tag -l --points-at v1.0
> v1.0-john
> v1.0
>
> Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
This version looks fine to me. Thanks.
-Peff
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCHv3] tag: add --points-at list option
2012-02-08 18:43 ` Tom Grennan
2012-02-08 18:57 ` Jeff King
@ 2012-02-08 18:58 ` Tom Grennan
1 sibling, 0 replies; 53+ messages in thread
From: Tom Grennan @ 2012-02-08 18:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:
>On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:
>>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
>>
>>Also, should we be producing an error if !obj? It would indicate a tag
>>that points to a bogus object.
>
>I think the test of (obj) is redundant as this should be caught
>by get_sha1() in parse_opt_points_at()
I'm wrong. That tests the sha of the point-at argument, not the
sha/objects of the refs/tags entry. I'll add...
if (!obj)
die(_("invalid tag, 'refs/tags/%s'"), refname);
--
TomG
^ permalink raw reply [flat|nested] 53+ messages in thread