Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
Jeff King p...@peff.net writes: Although I am still not clear on why it would not be up to the caller of git-describe in the first place to decide which they wanted. Thanks for a dose of sanity. Even though the part of the miniseries that makes sure that X (Y) output from name-rev always satisfies that rev-parse on X and Y give the same thing is an improvement, the whole thing about describe is misguided and wrong, I think. It started from the observation that these do not match: $ git describe $(git rev-parse v1.8.3) v1.8.3 $ git describe --contains $(git rev-parse v1.8.3) v1.8.3^0 and the miniseries veered in a wrong direction of fixing the latter to match the former. But the thing is, what is incosistent from the rest of the world is the describe output without --contains for a commit that is exactly at a tag (i.e. the former), and there is no need to fix this inconsistency, as we see below. The form without --contains in general reads like this: $ git describe --long $(git rev-parse v1.8.3) a717d9e v1.8.3-0-gedca415 v1.8.3-2-ga717d9e They both name a commit object, but that is sort of an afterthought; the support for describe name came late at 7dd45e15 (sha1_name.c: understand describe output as a valid object name, 2006-09-20). The primary purpose of git describe without --contains is to give a string that is suitable for a version number to be embedded in an executable. For that purpose, v1.8.3 is more convenient than v1.8.3-0-gedca415. But this convenient format breaks the consistency. While any other describe name for a commmit names a commit, the output for a commit that is exactly at a tag does not (in ancient times, describe output were not even extended SHA-1 expressions, so this inconsistency did not matter, but the afterthought brought the consistency to the foreground). The user chooses the convenience over the consistency by not using --long. And the short form cannot be v1.8.3^0 or v1.8.3~0 for the sake of consistency, as these are no more suitable as a version number than a short and sweet v1.8.3. The --contains form does not even aim to come up with a pleasant looking version string without using funny line noise characters, so it is perfectly fine for it to say: $ git describe --contains $(git rev-parse v1.8.3) a717d9e v1.8.3^0 v1.8.3.1~9 and these are internally consistent (they both roundtrip via rev-parse). Stripping ^0 from the former will break the consistency, even though it may make the output look prettier, but the --contains output is not even meant to be pretty in the first place. So let's drop 4/4; it is breaking the system by trying to solve a problem that does not exist. -- 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 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
Junio C Hamano wrote: $ git describe $(git rev-parse v1.8.3) v1.8.3 $ git describe --contains $(git rev-parse v1.8.3) v1.8.3^0 This is a correct observation, and I've already submitted the correct fix: name-rev: strip trailing ^0 in when --name-only. $ git describe --contains $(git rev-parse v1.8.3) a717d9e v1.8.3^0 v1.8.3.1~9 and these are internally consistent (they both roundtrip via rev-parse). Stripping ^0 from the former will break the consistency, even though it may make the output look prettier, but the --contains output is not even meant to be pretty in the first place. Incorrect. The --contains output _is_ meant to be pretty. That's the whole reason name-rev --name-only was invented. [2/4] is correct in that it fixes --stdin for annotated tags (although the implementation could be simpler, and the commit message is completely misleading). -- 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 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
Junio C Hamano wrote: With this on top of the other patches in this series, you would get: $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0) v1.8.3 v1.8.3 while you can still differentiate tags and the commits they point at with: $ git name-rev --refs=tags/\* --name-only $(git rev-parse v1.8.3 v1.8.3^0) v1.8.3 v1.8.3^0 The difference in these two behaviours is achieved by adding --peel-to-commit option to name-rev and using it when describe internally calls it. Essentially a revert of [2/4] for describe-purposes, achieved by adding an ugly command-line option to name-rev. Before we argue any further, let me ask: who uses name-rev (and depends strongly on its output)?! Our very own testsuite does not exercise it. There are exactly two users of describe/name-rev: 1. prompt, obviously. 2. DAG-tests, for simplification. I really can't imagine it being useful elsewhere. -- 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 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
On Sun, Jul 07, 2013 at 03:33:44PM -0700, Junio C Hamano wrote: With this on top of the other patches in this series, you would get: $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0) v1.8.3 v1.8.3 while you can still differentiate tags and the commits they point at with: $ git name-rev --refs=tags/\* --name-only $(git rev-parse v1.8.3 v1.8.3^0) v1.8.3 v1.8.3^0 The difference in these two behaviours is achieved by adding --peel-to-commit option to name-rev and using it when describe internally calls it. I am somewhat mixed on this. You are changing the default behavior of name-rev and adding a new option to restore it, so I wonder who (if anyone) might be broken. The documentation is now also out of date; not only does it not mention peel-to-commit, but it claims the argument to name-rev is a committish, which is not really true without that option. On the other hand, the new default behavior seems way more sane to me. In general, I would expect name-rev to: 1. Behave more or less the same between git name-rev $sha1 and echo $sha1 | git name-rev --stdin. Your patch improves that. Though I note that --peel-to-commit does not affect --stdin at all. Should it? And of course the two differ in that the command line will take any rev-parse expression, and --stdin only looks for full sha1s. 2. If name-rev prints $X $Y, I would expect git rev-parse $X to equal git rev-parse $Y. With peeling, that is not the case, and you get the misleading example that Ram showed: $ git name-rev 8af0605 8af0605 tags/v1.8.3^0 or more obviously weird: $ git name-rev v1.8.3 v1.8.3 tags/v1.8.3^0 So I think your series moves in a good direction, but I would just worry that it is breaking backwards compatibility (but like I said, I am not clear on who is affected and what it means for them). -Peff -- 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 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
On Mon, Jul 08, 2013 at 06:38:32PM +0530, Ramkumar Ramachandra wrote: Junio C Hamano wrote: With this on top of the other patches in this series, you would get: $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0) v1.8.3 v1.8.3 while you can still differentiate tags and the commits they point at with: $ git name-rev --refs=tags/\* --name-only $(git rev-parse v1.8.3 v1.8.3^0) v1.8.3 v1.8.3^0 The difference in these two behaviours is achieved by adding --peel-to-commit option to name-rev and using it when describe internally calls it. Essentially a revert of [2/4] for describe-purposes, achieved by adding an ugly command-line option to name-rev. I don't think it is a revert. The two patches complement each other. 2/4 is basically if we have a non-commit object which is pointed at directly by a tip, make sure we name it by that tip. But you can only get such an object by name-rev --stdin, since name-rev peels its command-line arguments. 4/4 is stop peeling command line objects, so we can find their exact tips. IOW, it lets the command line do the same thing that --stdin was able to do in 2/4. Before we argue any further, let me ask: who uses name-rev (and depends strongly on its output)?! Our very own testsuite does not exercise it. There are exactly two users of describe/name-rev: 1. prompt, obviously. 2. DAG-tests, for simplification. Yeah, I'm not clear on who we are breaking with the change in default peeling behavior, nor why the describe --contains wrapper wants to keep it. -Peff -- 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 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
Jeff King p...@peff.net writes: 1. Behave more or less the same between git name-rev $sha1 and echo $sha1 | git name-rev --stdin. Your patch improves that. Though I note that --peel-to-commit does not affect --stdin at all. Should it? And of course the two differ in that the command line will take any rev-parse expression, and --stdin only looks for full sha1s. To Should it?, I do not deeply care. --peel-to-commit is an exception that only is needed to support describe. I could instead have tucked ^0 at the end of each argument when describe calls out to name-rev without adding this new option, which is much much closer to what is really going on. And that will alleviate your #2 below. 2. If name-rev prints $X $Y, I would expect git rev-parse $X to equal git rev-parse $Y. With peeling, that is not the case, and you get the misleading example that Ram showed: $ git name-rev 8af0605 8af0605 tags/v1.8.3^0 or more obviously weird: $ git name-rev v1.8.3 v1.8.3 tags/v1.8.3^0 So I think your series moves in a good direction, but I would just worry that it is breaking backwards compatibility (but like I said, I am not clear on who is affected and what it means for them). -- 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 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
On Mon, Jul 08, 2013 at 10:33:26PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: 1. Behave more or less the same between git name-rev $sha1 and echo $sha1 | git name-rev --stdin. Your patch improves that. Though I note that --peel-to-commit does not affect --stdin at all. Should it? And of course the two differ in that the command line will take any rev-parse expression, and --stdin only looks for full sha1s. To Should it?, I do not deeply care. --peel-to-commit is an exception that only is needed to support describe. I could instead have tucked ^0 at the end of each argument when describe calls out to name-rev without adding this new option, which is much much closer to what is really going on. Yeah, I tend to think that is a more sane interface, even though it is a little more work in git-describe. Although I am still not clear on why it would not be up to the caller of git-describe in the first place to decide which they wanted. -Peff -- 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