Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Junio C Hamano]

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

[Ramkumar Ramachandra]

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

[Ramkumar Ramachandra]

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

[Jeff King]

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

[Jeff King]

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

[Junio C Hamano]

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

[Jeff King]

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

[PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Junio C Hamano]

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.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 builtin/describe.c |  1 +
 builtin/name-rev.c | 35 +--
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index b5434e4..f7adda6 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -447,6 +447,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
 
argv_array_init(args);
argv_array_push(args, name-rev);
+   argv_array_push(args, --peel-to-commit);
argv_array_push(args, --name-only);
argv_array_push(args, --no-undefined);
if (always)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 29a6f56..fa37731 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -15,6 +15,7 @@ typedef struct rev_name {
 } rev_name;
 
 static long cutoff = LONG_MAX;
+static int peel_to_commit;
 
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
@@ -33,7 +34,7 @@ static void name_rev(struct commit *commit,
if (commit-date  cutoff)
return;
 
-   if (deref) {
+   if (deref  !peel_to_commit) {
char *new_name = xmalloc(strlen(tip_name)+3);
strcpy(new_name, tip_name);
strcat(new_name, ^0);
@@ -320,6 +321,8 @@ int cmd_name_rev(int argc, const char **argv, const char 
*prefix)
OPT_BOOLEAN(0, undefined, allow_undefined, N_(allow to 
print `undefined` names)),
OPT_BOOLEAN(0, always, always,
   N_(show abbreviated commit object as fallback)),
+   OPT_BOOLEAN(0, peel-to-commit, peel_to_commit,
+   N_(peel tag object names in the input to a 
commmit)),
OPT_END(),
};
 
@@ -334,7 +337,7 @@ int cmd_name_rev(int argc, const char **argv, const char 
*prefix)
 
for (; argc; argc--, argv++) {
unsigned char sha1[20];
-   struct object *o;
+   struct object *object;
struct commit *commit;
 
if (get_sha1(*argv, sha1)) {
@@ -343,17 +346,29 @@ int cmd_name_rev(int argc, const char **argv, const char 
*prefix)
continue;
}
 
-   o = deref_tag(parse_object(sha1), *argv, 0);
-   if (!o || o-type != OBJ_COMMIT) {
+   commit = NULL;
+   object = parse_object(sha1);
+   if (object) {
+   struct object *peeled = deref_tag(object, *argv, 0);
+   if (peeled  peeled-type == OBJ_COMMIT)
+   commit = (struct commit *) peeled;
+   }
+
+   if (!object) {
+   fprintf(stderr, Could not get object for %s. 
Skipping.\n,
+   *argv);
+   continue;
+   }
+   if (peel_to_commit  !commit) {
fprintf(stderr, Could not get commit for %s. 
Skipping.\n,
-   *argv);
+   *argv);
continue;
}
-
-   commit = (struct commit *)o;
-   if (cutoff  commit-date)
-   cutoff = commit-date;
-   add_object_array((struct object *)commit, *argv, revs);
+   if (commit) {
+   if (cutoff  commit-date)
+   cutoff = commit-date;
+   }
+   add_object_array(object, *argv, revs);
}
 
if (cutoff)
-- 
1.8.3.2-853-ga8cbcc9

--
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