Re: [PATCH] peel_onion(): add support for rev^{tag}
Richard Hansen wrote:
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -677,6 +677,8 @@ static int peel_onion(const char *name, int len, unsigned
char *sha1)
sp++; /* beginning of type name, or closing brace for empty */
if (!strncmp(commit_type, sp, 6) sp[6] == '}')
expected_type = OBJ_COMMIT;
+ else if (!strncmp(tag_type, sp, 3) sp[3] == '}')
+ expected_type = OBJ_TAG;
Interesting.
gitrevisions(7) implies that rev^{tag} should work, but before now
it did not:
The wording (especially of rev^{}) special-cases tags as objects to
dereference.
$ git rev-parse --verify v1.8.3.1^{}^{object}
362de916c06521205276acb7f51c99f47db94727
$ git rev-parse --verify v1.8.3.1^{}^{tag}
error: v1.8.3.1^{}^{tag}: expected tag type, but the object dereferences
to tree type
fatal: Needed a single revision
And the points out the problem: while ^{object} means expect
OBJ_ANY, ^{} means expect OBJ_NONE. What does that even mean? See
sha1_name.c:704 where this is handled sneakily: it just calls
deref_tag(), bypassing peel_to_type() altogether. If anything, ^{} is
already a poor-man's version of your ^{tag}; the reason it's a
poor-man's version is precisely because it doesn't error out when
rev isn't a tag, while your ^{tag} does. I would argue that ^{} is
very poorly done and must be deprecated in favor of your ^{tag}. What
is the point of using ^{} if you can't even be sure that what you get
is a deref? peel_to_type() already does the right thing by not using
deref_tag(), and explicitly checking.
Your commit message needs some tweaking, but I'm happy with your patch
otherwise.
Thanks.
--
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] peel_onion(): add support for rev^{tag}
Richard Hansen rhan...@bbn.com writes:
Barfing on non-tags is the feature this adds. It's otherwise useless,
just like object^{object} is useless except to barf when object
doesn't exist.
Thanks.
I could buy that. And after re-reading the proposed log message,
you do not quite have anything to say that. Instead, you have this:
Note that rev^{tag} is not the same as rev^{object} when rev is
not a tag:
$ git rev-parse --verify v1.8.3.1^{}^{object}
362de916c06521205276acb7f51c99f47db94727
$ git rev-parse --verify v1.8.3.1^{}^{tag}
error: v1.8.3.1^{}^{tag}: expected tag type, but the object deref...
fatal: Needed a single revision
The latter peels v1.8.3.1 to a non-tag (i.e. a commit) and then asks
to peel that commit to a tag, which will of course fail, but that is
not a good example.
Perhaps something like this instead.
Note that rev^{tag} can be used to make sure rev names a tag:
$ git rev-parse --verify v1.8.3.1^{tag}
$ git rev-parse --verify master^{tag}
The former succeeds, while the latter fails.
--
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] peel_onion(): add support for rev^{tag}
Richard Hansen rhan...@bbn.com writes:
gitrevisions(7) implies that rev^{tag} should work,...
Does it? Is it possible that that should be fixed?
What does it even _mean_ to peel something to a TAG?
A commit, a tree or a blob cannot be peeled to a tag---none of them
can contain a tag.
When you have a tag that points at something else, what you have is
already a tag, so that-tag^{tag} would be that-tag itself.
Even more confusingly, when you have a tag that points at another
tag, what does that-outer-tag^{tag} mean? The outer tag itself,
or do you need to peel at least once to reveal the inner-tag? What
if that inner-tag points at yet another tag?
The patch does not touch peel_to_type(), so your answer to the above
question seems to be if T is already a tag, T^{tag} is T itself,
but then that operation does not look all that useful.
Confused...
diff --git a/sha1_name.c b/sha1_name.c
index 90419ef..68fd0e4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -677,6 +677,8 @@ static int peel_onion(const char *name, int len, unsigned
char *sha1)
sp++; /* beginning of type name, or closing brace for empty */
if (!strncmp(commit_type, sp, 6) sp[6] == '}')
expected_type = OBJ_COMMIT;
+ else if (!strncmp(tag_type, sp, 3) sp[3] == '}')
+ expected_type = OBJ_TAG;
else if (!strncmp(tree_type, sp, 4) sp[4] == '}')
expected_type = OBJ_TREE;
else if (!strncmp(blob_type, sp, 4) sp[4] == '}')
--
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] peel_onion(): add support for rev^{tag}
On 2013-06-19 14:38, Junio C Hamano wrote:
Richard Hansen rhan...@bbn.com writes:
gitrevisions(7) implies that rev^{tag} should work,...
Does it? Is it possible that that should be fixed?
Depends on whether you think ^{tag} is a useful feature or not; see below.
What does it even _mean_ to peel something to a TAG?
It's the same as peeling something to any other object type: If the
object is that type, done. Otherwise dereference and try again. If it
can't be dereferenced, barf.
A commit, a tree or a blob cannot be peeled to a tag---none of them
can contain a tag.
Right, so all of those would barf.
When you have a tag that points at something else, what you have is
already a tag, so that-tag^{tag} would be that-tag itself.
Exactly, just like object^{object} is object itself.
Even more confusingly, when you have a tag that points at another
tag, what does that-outer-tag^{tag} mean? The outer tag itself,
or do you need to peel at least once to reveal the inner-tag? What
if that inner-tag points at yet another tag?
The patch does not touch peel_to_type(), so your answer to the above
question seems to be if T is already a tag, T^{tag} is T itself,
but then that operation does not look all that useful.
Barfing on non-tags is the feature this adds. It's otherwise useless,
just like object^{object} is useless except to barf when object
doesn't exist.
It's a sometimes-convenient way to assert that an object specifier
refers to a tag object and not something else. For example, instead of:
fatal() { printf %s\\n ERROR: $* 2; exit 1; }
type=$(git cat-file -t $1) || fatal $1 is not a valid object
[ ${type} = tag ] || fatal $1 is not a tag object
use $1 here
you can do:
use $1^{tag} here
-Richard
Confused...
diff --git a/sha1_name.c b/sha1_name.c
index 90419ef..68fd0e4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -677,6 +677,8 @@ static int peel_onion(const char *name, int len,
unsigned char *sha1)
sp++; /* beginning of type name, or closing brace for empty */
if (!strncmp(commit_type, sp, 6) sp[6] == '}')
expected_type = OBJ_COMMIT;
+else if (!strncmp(tag_type, sp, 3) sp[3] == '}')
+expected_type = OBJ_TAG;
else if (!strncmp(tree_type, sp, 4) sp[4] == '}')
expected_type = OBJ_TREE;
else if (!strncmp(blob_type, sp, 4) sp[4] == '}')
--
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
