Re: [PATCH 2/2] peel_onion(): teach $foo^{object} peeler
Michael Haggerty writes:
> On 04/02/2013 05:45 PM, Junio C Hamano wrote:
>
>> Also, v1.8.2^{tag} would be give the tag itself, while master^{tag}
>> would not report the commit "master" but would error out, which
>> would be useless. You are better off doing `git cat-file -t foo`
>> and seeing if it is a tag object at that point.
>
> All correct, of course. But the user would never use "master^{tag}"
> unless he wants a tag and nothing else, so erroring out would be exactly
> the thing he wants in that case. This is no different than the
> "^{commit}" part of "master^{tree}^{commit}", which correctly errors out
> because a commit cannot be inferred from a tree.
Correct; I was only saying adding it is not something that solves a
problem that cannot be solved with the current system (i.e. no added
value from feature point-of-view). I would not object to a patch to
allow "git rev-parse v1.8.2^{tag}" for completeness.
We may want to rethink if we can lose the hardcoded lengths like 6,
3, 4, 4 from here, but I didn't bother ;-).
sha1_name.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 85b6e75..47f39a8 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -639,16 +639,18 @@ static int peel_onion(const char *name, int len, unsigned
char *sha1)
return -1;
sp++; /* beginning of type name, or closing brace for empty */
- if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
+ if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
+ expected_type = OBJ_TAG;
+ else if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
expected_type = OBJ_COMMIT;
else if (!strncmp(tree_type, sp, 4) && sp[4] == '}')
expected_type = OBJ_TREE;
else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
expected_type = OBJ_BLOB;
else if (!prefixcmp(sp, "object}"))
- expected_type = OBJ_ANY;
+ expected_type = OBJ_ANY; /* ok as long as it exists */
else if (sp[0] == '}')
- expected_type = OBJ_NONE;
+ expected_type = OBJ_NONE; /* unwrap until we get a non-tag */
else if (sp[0] == '/')
expected_type = OBJ_COMMIT;
else
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] peel_onion(): teach $foo^{object} peeler
On 04/02/2013 05:45 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> Off topic: Your patch reminds me of something else that surprised me:
>> there is no "$userstring^{tag}". I suppose it would be a bit ambiguous,
>> given that tags can point at tags, and it would also be less useful than
>> the other suffixes. But its absence irked the completionist in me :-)
>
> Yes, unfortunately, foo^{type} means "start from foo, and until what
> you are looking at it is of type, repeatedly peel to see if you can
> get to an object of that type, or stop and report an error". If a
> tag A points at another tag B, which in turn points at an object C,
> you will never see B by applying usual peeling operator.
>
> Also, v1.8.2^{tag} would be give the tag itself, while master^{tag}
> would not report the commit "master" but would error out, which
> would be useless. You are better off doing `git cat-file -t foo`
> and seeing if it is a tag object at that point.
All correct, of course. But the user would never use "master^{tag}"
unless he wants a tag and nothing else, so erroring out would be exactly
the thing he wants in that case. This is no different than the
"^{commit}" part of "master^{tree}^{commit}", which correctly errors out
because a commit cannot be inferred from a tree.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] peel_onion(): teach $foo^{object} peeler
Michael Haggerty writes:
> Off topic: Your patch reminds me of something else that surprised me:
> there is no "$userstring^{tag}". I suppose it would be a bit ambiguous,
> given that tags can point at tags, and it would also be less useful than
> the other suffixes. But its absence irked the completionist in me :-)
Yes, unfortunately, foo^{type} means "start from foo, and until what
you are looking at it is of type, repeatedly peel to see if you can
get to an object of that type, or stop and report an error". If a
tag A points at another tag B, which in turn points at an object C,
you will never see B by applying usual peeling operator.
Also, v1.8.2^{tag} would be give the tag itself, while master^{tag}
would not report the commit "master" but would error out, which
would be useless. You are better off doing `git cat-file -t foo`
and seeing if it is a tag object at that point.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] peel_onion(): teach $foo^{object} peeler
While I was in the middle of suggesting documentation for this new
syntax, I discovered that you already added documentation to your repo
but didn't mention the new version on the mailing list (or maybe I
overlooked it). It would be helpful if you would submit your own
changes to the mailing list to make it harder for the rest of us to
overlook them--and easier to look over them :-)
The documentation looks fine to me.
Off topic: Your patch reminds me of something else that surprised me:
there is no "$userstring^{tag}". I suppose it would be a bit ambiguous,
given that tags can point at tags, and it would also be less useful than
the other suffixes. But its absence irked the completionist in me :-)
Michael
On 04/01/2013 12:38 AM, Junio C Hamano wrote:
> A string that names an object can be suffixed with ^{type} peeler to
> say "I have this object name; peel it until you get this type. If
> you cannot do so, it is an error". v1.8.2^{commit} asks for a commit
> that is pointed at an annotated tag v1.8.2; v1.8.2^{tree} unwraps it
> further to the top-level tree object. A special suffix ^{} (i.e. no
> type specified) means "I do not care what it unwraps to; just peel
> annotated tag until you get something that is not a tag".
>
> When you have a random user-supplied string, you can turn it to a
> bare 40-hex object name, and cause it to error out if such an object
> does not exist, with:
>
> git rev-parse --verify "$userstring^{}"
>
> for most objects, but this does not yield the tag object name when
> $userstring refers to an annotated tag.
>
> Introduce a new suffix, ^{object}, that only makes sure the given
> name refers to an existing object. Then
>
> git rev-parse --verify "$userstring^{object}"
>
> becomes a way to make sure $userstring refers to an existing object.
>
> This is necessary because the plumbing "rev-parse --verify" is only
> about "make sure the argument is something we can feed to get_sha1()
> and turn it into a raw 20-byte object name SHA-1" and is not about
> "make sure that 20-byte object name SHA-1 refers to an object that
> exists in our object store". When the given $userstring is already
> a 40-hex, by definition "rev-parse --verify $userstring" can turn it
> into a raw 20-byte object name. With "$userstring^{object}", we can
> make sure that the 40-hex string names an object that exists in our
> object store before "--verify" kicks in.
>
> Signed-off-by: Junio C Hamano
> ---
> sha1_name.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 45788df..85b6e75 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -594,7 +594,7 @@ struct object *peel_to_type(const char *name, int namelen,
> while (1) {
> if (!o || (!o->parsed && !parse_object(o->sha1)))
> return NULL;
> - if (o->type == expected_type)
> + if (expected_type == OBJ_ANY || o->type == expected_type)
> return o;
> if (o->type == OBJ_TAG)
> o = ((struct tag*) o)->tagged;
> @@ -645,6 +645,8 @@ static int peel_onion(const char *name, int len, unsigned
> char *sha1)
> expected_type = OBJ_TREE;
> else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
> expected_type = OBJ_BLOB;
> + else if (!prefixcmp(sp, "object}"))
> + expected_type = OBJ_ANY;
> else if (sp[0] == '}')
> expected_type = OBJ_NONE;
> else if (sp[0] == '/')
>
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
