* [PATCH 0/4] Make "git name-rev $(git rev-parse v1.8.3)" work
@ 2013-07-07 22:33 Junio C Hamano
2013-07-07 22:33 ` [PATCH 1/4] name-ref: factor out name shortening logic from name_ref() Junio C Hamano
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Junio C Hamano @ 2013-07-07 22:33 UTC (permalink / raw)
To: git
So here is a set of small preparatory steps to help the other topic
to allow "git describe -contains v1.8.3" omit trailing "^0" from its
output. We do not want to prevent people from allowing "name-rev"
to convert object names other than commit-ishes.
The series should apply on 96ffd4ca (Merge branch
'nk/name-rev-abbreviated-refs', 2013-06-30).
Junio C Hamano (4):
name-ref: factor out name shortening logic from name_ref()
name-rev: allow converting the exact object name at the tip of a ref
describe: use argv-array
describe/name-rev: tell name-rev to peel the incoming object to commit first
builtin/describe.c | 32 ++++++++-------
builtin/name-rev.c | 113 ++++++++++++++++++++++++++++++++++++++++++++---------
2 files changed, 112 insertions(+), 33 deletions(-)
--
1.8.3.2-853-ga8cbcc9
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/4] name-ref: factor out name shortening logic from name_ref()
2013-07-07 22:33 [PATCH 0/4] Make "git name-rev $(git rev-parse v1.8.3)" work Junio C Hamano
@ 2013-07-07 22:33 ` Junio C Hamano
2013-07-08 8:52 ` Michael Haggerty
2013-07-07 22:33 ` [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref Junio C Hamano
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2013-07-07 22:33 UTC (permalink / raw)
To: git
The logic will be used in a new codepath for showing exact matches.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/name-rev.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 87d4854..1234ebb 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)
return -1;
}
+static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
+{
+ if (shorten_unambiguous)
+ refname = shorten_unambiguous_ref(refname, 0);
+ else if (!prefixcmp(refname, "refs/heads/"))
+ refname = refname + 11;
+ else if (!prefixcmp(refname, "refs/"))
+ refname = refname + 5;
+ return refname;
+}
+
struct name_ref_data {
int tags_only;
int name_only;
@@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
if (o && o->type == OBJ_COMMIT) {
struct commit *commit = (struct commit *)o;
- if (can_abbreviate_output)
- path = shorten_unambiguous_ref(path, 0);
- else if (!prefixcmp(path, "refs/heads/"))
- path = path + 11;
- else if (!prefixcmp(path, "refs/"))
- path = path + 5;
-
+ path = name_ref_abbrev(path, can_abbreviate_output);
name_rev(commit, xstrdup(path), 0, 0, deref);
}
return 0;
--
1.8.3.2-853-ga8cbcc9
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref
2013-07-07 22:33 [PATCH 0/4] Make "git name-rev $(git rev-parse v1.8.3)" work Junio C Hamano
2013-07-07 22:33 ` [PATCH 1/4] name-ref: factor out name shortening logic from name_ref() Junio C Hamano
@ 2013-07-07 22:33 ` Junio C Hamano
2013-07-08 12:20 ` Ramkumar Ramachandra
2013-07-07 22:33 ` [PATCH 3/4] describe: use argv-array Junio C Hamano
2013-07-07 22:33 ` [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first Junio C Hamano
3 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2013-07-07 22:33 UTC (permalink / raw)
To: git
"git name-rev" is supposed to convert 40-hex object names into
strings that name the same objects based on refs, that can be fed to
"git rev-parse" to get the same object names back, so
$ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
8af06057d0c31a24e8737ae846ac2e116e8bafb9
edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)
has to have "^0" at the end, as "edca41" is a commit, not the tag
that references it.
The command however did not bother to see if the object is at the
tip of some ref, and failed to convert a tag object.
Teach it to show this instead:
$ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
8af06057d0c31a24e8737ae846ac2e116e8bafb9 (tags/v1.8.3)
edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/name-rev.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 58 insertions(+), 1 deletion(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 1234ebb..29a6f56 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -4,6 +4,7 @@
#include "tag.h"
#include "refs.h"
#include "parse-options.h"
+#include "sha1-lookup.h"
#define CUTOFF_DATE_SLOP 86400 /* one day */
@@ -113,6 +114,34 @@ struct name_ref_data {
const char *ref_filter;
};
+static struct tip_table {
+ struct tip_table_entry {
+ unsigned char sha1[20];
+ const char *refname;
+ } *table;
+ int nr;
+ int alloc;
+ int sorted;
+} tip_table;
+
+static void add_to_tip_table(const unsigned char *sha1, const char *refname,
+ int shorten_unambiguous)
+{
+ refname = name_ref_abbrev(refname, shorten_unambiguous);
+
+ ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc);
+ hashcpy(tip_table.table[tip_table.nr].sha1, sha1);
+ tip_table.table[tip_table.nr].refname = xstrdup(refname);
+ tip_table.nr++;
+ tip_table.sorted = 0;
+}
+
+static int tipcmp(const void *a_, const void *b_)
+{
+ const struct tip_table_entry *a = a_, *b = b_;
+ return hashcmp(a->sha1, b->sha1);
+}
+
static int name_ref(const char *path, const unsigned char *sha1, int flags, void *cb_data)
{
struct object *o = parse_object(sha1);
@@ -135,6 +164,8 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
}
}
+ add_to_tip_table(sha1, path, can_abbreviate_output);
+
while (o && o->type == OBJ_TAG) {
struct tag *t = (struct tag *) o;
if (!t->tagged)
@@ -151,6 +182,32 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
return 0;
}
+static const unsigned char *nth_tip_table_ent(size_t ix, void *table_)
+{
+ struct tip_table_entry *table = table_;
+ return table[ix].sha1;
+}
+
+static const char *get_exact_ref_match(const struct object *o)
+{
+ int found;
+
+ if (!tip_table.table || !tip_table.nr)
+ return NULL;
+
+ if (!tip_table.sorted) {
+ qsort(tip_table.table, tip_table.nr, sizeof(*tip_table.table),
+ tipcmp);
+ tip_table.sorted = 1;
+ }
+
+ found = sha1_pos(o->sha1, tip_table.table, tip_table.nr,
+ nth_tip_table_ent);
+ if (0 <= found)
+ return tip_table.table[found].refname;
+ return NULL;
+}
+
/* returns a static buffer */
static const char *get_rev_name(const struct object *o)
{
@@ -159,7 +216,7 @@ static const char *get_rev_name(const struct object *o)
struct commit *c;
if (o->type != OBJ_COMMIT)
- return NULL;
+ return get_exact_ref_match(o);
c = (struct commit *) o;
n = c->util;
if (!n)
--
1.8.3.2-853-ga8cbcc9
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] describe: use argv-array
2013-07-07 22:33 [PATCH 0/4] Make "git name-rev $(git rev-parse v1.8.3)" work Junio C Hamano
2013-07-07 22:33 ` [PATCH 1/4] name-ref: factor out name shortening logic from name_ref() Junio C Hamano
2013-07-07 22:33 ` [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref Junio C Hamano
@ 2013-07-07 22:33 ` Junio C Hamano
2013-07-09 4:51 ` Jeff King
2013-07-07 22:33 ` [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first Junio C Hamano
3 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2013-07-07 22:33 UTC (permalink / raw)
To: git
Instead of using a hand allocated args[] array, use argv-array API
to manage the dynamically created list of arguments when invoking
name-rev.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/describe.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 4e675c3..b5434e4 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -7,6 +7,7 @@
#include "parse-options.h"
#include "diff.h"
#include "hash.h"
+#include "argv-array.h"
#define SEEN (1u<<0)
#define MAX_TAGS (FLAG_BITS - 1)
@@ -442,24 +443,24 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
die(_("--long is incompatible with --abbrev=0"));
if (contains) {
- const char **args = xmalloc((7 + argc) * sizeof(char *));
- int i = 0;
- args[i++] = "name-rev";
- args[i++] = "--name-only";
- args[i++] = "--no-undefined";
+ struct argv_array args;
+
+ argv_array_init(&args);
+ argv_array_push(&args, "name-rev");
+ argv_array_push(&args, "--name-only");
+ argv_array_push(&args, "--no-undefined");
if (always)
- args[i++] = "--always";
+ argv_array_push(&args, "--always");
if (!all) {
- args[i++] = "--tags";
- if (pattern) {
- char *s = xmalloc(strlen("--refs=refs/tags/") + strlen(pattern) + 1);
- sprintf(s, "--refs=refs/tags/%s", pattern);
- args[i++] = s;
- }
+ argv_array_push(&args, "--tags");
+ if (pattern)
+ argv_array_pushf(&args, "--refs=refs/tags/%s", pattern);
+ }
+ while (*argv) {
+ argv_array_push(&args, *argv);
+ argv++;
}
- memcpy(args + i, argv, argc * sizeof(char *));
- args[i + argc] = NULL;
- return cmd_name_rev(i + argc, args, prefix);
+ return cmd_name_rev(args.argc, args.argv, prefix);
}
init_hash(&names);
--
1.8.3.2-853-ga8cbcc9
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
2013-07-07 22:33 [PATCH 0/4] Make "git name-rev $(git rev-parse v1.8.3)" work Junio C Hamano
` (2 preceding siblings ...)
2013-07-07 22:33 ` [PATCH 3/4] describe: use argv-array Junio C Hamano
@ 2013-07-07 22:33 ` Junio C Hamano
2013-07-08 13:08 ` Ramkumar Ramachandra
2013-07-09 5:06 ` Jeff King
3 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2013-07-07 22:33 UTC (permalink / raw)
To: git
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 <gitster@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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] name-ref: factor out name shortening logic from name_ref()
2013-07-07 22:33 ` [PATCH 1/4] name-ref: factor out name shortening logic from name_ref() Junio C Hamano
@ 2013-07-08 8:52 ` Michael Haggerty
2013-07-08 15:04 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Michael Haggerty @ 2013-07-08 8:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On 07/08/2013 12:33 AM, Junio C Hamano wrote:
> The logic will be used in a new codepath for showing exact matches.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/name-rev.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 87d4854..1234ebb 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)
> return -1;
> }
>
> +static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
> +{
> + if (shorten_unambiguous)
> + refname = shorten_unambiguous_ref(refname, 0);
> + else if (!prefixcmp(refname, "refs/heads/"))
> + refname = refname + 11;
> + else if (!prefixcmp(refname, "refs/"))
> + refname = refname + 5;
> + return refname;
> +}
> +
In my opinion this would be a tad clearer if each of the branches of the
"if" returned the value directly rather than setting refname and relying
on the "return" statement that follows. But it's probably a matter of
taste.
> struct name_ref_data {
> int tags_only;
> int name_only;
> @@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
> if (o && o->type == OBJ_COMMIT) {
> struct commit *commit = (struct commit *)o;
>
> - if (can_abbreviate_output)
> - path = shorten_unambiguous_ref(path, 0);
> - else if (!prefixcmp(path, "refs/heads/"))
> - path = path + 11;
> - else if (!prefixcmp(path, "refs/"))
> - path = path + 5;
> -
> + path = name_ref_abbrev(path, can_abbreviate_output);
> name_rev(commit, xstrdup(path), 0, 0, deref);
> }
> return 0;
>
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref
2013-07-07 22:33 ` [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref Junio C Hamano
@ 2013-07-08 12:20 ` Ramkumar Ramachandra
2013-07-08 15:12 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Ramkumar Ramachandra @ 2013-07-08 12:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano wrote:
> "git name-rev" is supposed to convert 40-hex object names into
> strings that name the same objects based on refs, that can be fed to
> "git rev-parse" to get the same object names back, so
>
> $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
> 8af06057d0c31a24e8737ae846ac2e116e8bafb9
> edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)
Wait, what?
$ git name-rev 8af060
8af060 tags/v1.8.3^0
Isn't this a failure specific to --stdin?
> Teach it to show this instead:
>
> $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
> 8af06057d0c31a24e8737ae846ac2e116e8bafb9 (tags/v1.8.3)
> edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)
Wait, what is name-rev?
Finds symbolic names suitable for human digestion for revisions
given in any format parsable by git rev-parse.
It is meant to name _revisions_ (aka. commits): in that context, what
sense does it make to distinguish between tags/v1.8.3 and
tags/v1.8.3^0?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
2013-07-07 22:33 ` [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first Junio C Hamano
@ 2013-07-08 13:08 ` Ramkumar Ramachandra
2013-07-09 5:12 ` Jeff King
2013-07-09 5:06 ` Jeff King
1 sibling, 1 reply; 20+ messages in thread
From: Ramkumar Ramachandra @ 2013-07-08 13:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] name-ref: factor out name shortening logic from name_ref()
2013-07-08 8:52 ` Michael Haggerty
@ 2013-07-08 15:04 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2013-07-08 15:04 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 07/08/2013 12:33 AM, Junio C Hamano wrote:
>> The logic will be used in a new codepath for showing exact matches.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> builtin/name-rev.c | 19 ++++++++++++-------
>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index 87d4854..1234ebb 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)
>> return -1;
>> }
>>
>> +static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
>> +{
>> + if (shorten_unambiguous)
>> + refname = shorten_unambiguous_ref(refname, 0);
>> + else if (!prefixcmp(refname, "refs/heads/"))
>> + refname = refname + 11;
>> + else if (!prefixcmp(refname, "refs/"))
>> + refname = refname + 5;
>> + return refname;
>> +}
>> +
>
> In my opinion this would be a tad clearer if each of the branches of the
> "if" returned the value directly rather than setting refname and relying
> on the "return" statement that follows. But it's probably a matter of
> taste.
I tend to agree; this is a straight-forward code movement (with the
variable name changed to conform to your recent refs.c update to
call these things "refname"), and that was the primary reason I kept
them like so.
>
>> struct name_ref_data {
>> int tags_only;
>> int name_only;
>> @@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
>> if (o && o->type == OBJ_COMMIT) {
>> struct commit *commit = (struct commit *)o;
>>
>> - if (can_abbreviate_output)
>> - path = shorten_unambiguous_ref(path, 0);
>> - else if (!prefixcmp(path, "refs/heads/"))
>> - path = path + 11;
>> - else if (!prefixcmp(path, "refs/"))
>> - path = path + 5;
>> -
>> + path = name_ref_abbrev(path, can_abbreviate_output);
>> name_rev(commit, xstrdup(path), 0, 0, deref);
>> }
>> return 0;
>>
>
> Michael
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref
2013-07-08 12:20 ` Ramkumar Ramachandra
@ 2013-07-08 15:12 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2013-07-08 15:12 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: git
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Finds symbolic names suitable for human digestion for revisions
> given in any format parsable by git rev-parse.
>
> It is meant to name _revisions_ (aka. commits):
That is a mistaken documentation, written based on a half-baked
implementation that conflated "the current code only can handle
commits" with "we need to only handle commit and nothing, ever".
We had to fix a similar misguided/short-sighted implementation in
the "git fetch" codepath when adding "git pull/merge $tag" (the code
peeled a tag object too early when writing FETCH_HEAD out, losing
information if what we were given was a tag or a commit).
I do not want to see us making the mistake worse unnecessarily.
When we see a commit object that is pointed by tag v1.8.3, it is the
right thing to do to return v1.8.3^0 as its string representation so
that "git rev-parse v1.8.3^0" give us the same thing back. name-rev
that is fed a tag object v1.8.3 should give v1.8.3 (not v1.8.3^0)
back, otherwise feeding its output to "git rev-parse" will not give
us the same thing we fed as the input to name-rev.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] describe: use argv-array
2013-07-07 22:33 ` [PATCH 3/4] describe: use argv-array Junio C Hamano
@ 2013-07-09 4:51 ` Jeff King
2013-07-09 14:55 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Jeff King @ 2013-07-09 4:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:
> + argv_array_init(&args);
> + argv_array_push(&args, "name-rev");
> + argv_array_push(&args, "--name-only");
> + argv_array_push(&args, "--no-undefined");
> [...]
> - memcpy(args + i, argv, argc * sizeof(char *));
> - args[i + argc] = NULL;
> - return cmd_name_rev(i + argc, args, prefix);
> + return cmd_name_rev(args.argc, args.argv, prefix);
This leaks the memory allocated by "args". The original did, too, and it
is probably not that big a deal (we exit right after anyway). The fix
would be something like:
rc = cmd_name_rev(args.argc, args.argv, prefix);
argv_array_clear(&args);
return rc;
-Peff
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
2013-07-07 22:33 ` [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first Junio C Hamano
2013-07-08 13:08 ` Ramkumar Ramachandra
@ 2013-07-09 5:06 ` Jeff King
2013-07-09 5:33 ` Junio C Hamano
1 sibling, 1 reply; 20+ messages in thread
From: Jeff King @ 2013-07-09 5:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
2013-07-08 13:08 ` Ramkumar Ramachandra
@ 2013-07-09 5:12 ` Jeff King
0 siblings, 0 replies; 20+ messages in thread
From: Jeff King @ 2013-07-09 5:12 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
2013-07-09 5:06 ` Jeff King
@ 2013-07-09 5:33 ` Junio C Hamano
2013-07-09 5:35 ` Jeff King
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2013-07-09 5:33 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@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).
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
2013-07-09 5:33 ` Junio C Hamano
@ 2013-07-09 5:35 ` Jeff King
2013-07-09 11:45 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Jeff King @ 2013-07-09 5:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Jul 08, 2013 at 10:33:26PM -0700, Junio C Hamano wrote:
> Jeff King <peff@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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
2013-07-09 5:35 ` Jeff King
@ 2013-07-09 11:45 ` Junio C Hamano
2013-07-09 12:42 ` Ramkumar Ramachandra
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2013-07-09 11:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first
2013-07-09 11:45 ` Junio C Hamano
@ 2013-07-09 12:42 ` Ramkumar Ramachandra
0 siblings, 0 replies; 20+ messages in thread
From: Ramkumar Ramachandra @ 2013-07-09 12:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
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).
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] describe: use argv-array
2013-07-09 4:51 ` Jeff King
@ 2013-07-09 14:55 ` Junio C Hamano
2013-07-09 16:00 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2013-07-09 14:55 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:
>
>> + argv_array_init(&args);
>> + argv_array_push(&args, "name-rev");
>> + argv_array_push(&args, "--name-only");
>> + argv_array_push(&args, "--no-undefined");
>> [...]
>> - memcpy(args + i, argv, argc * sizeof(char *));
>> - args[i + argc] = NULL;
>> - return cmd_name_rev(i + argc, args, prefix);
>> + return cmd_name_rev(args.argc, args.argv, prefix);
>
> This leaks the memory allocated by "args". The original did, too, and it
> is probably not that big a deal (we exit right after anyway). The fix
> would be something like:
>
> rc = cmd_name_rev(args.argc, args.argv, prefix);
> argv_array_clear(&args);
> return rc;
Yes; this was meant as a straight rewrite and I did not bother, but
I should have cleaned it up as I meant to build on top.
Will amend, even though I do not think we need to build anything on
top.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] describe: use argv-array
2013-07-09 14:55 ` Junio C Hamano
@ 2013-07-09 16:00 ` Junio C Hamano
2013-07-09 18:53 ` Jeff King
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2013-07-09 16:00 UTC (permalink / raw)
To: Jeff King; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:
>>
>>> + argv_array_init(&args);
>>> + argv_array_push(&args, "name-rev");
>>> + argv_array_push(&args, "--name-only");
>>> + argv_array_push(&args, "--no-undefined");
>>> [...]
>>> - memcpy(args + i, argv, argc * sizeof(char *));
>>> - args[i + argc] = NULL;
>>> - return cmd_name_rev(i + argc, args, prefix);
>>> + return cmd_name_rev(args.argc, args.argv, prefix);
>>
>> This leaks the memory allocated by "args". The original did, too, and it
>> is probably not that big a deal (we exit right after anyway). The fix
>> would be something like:
>>
>> rc = cmd_name_rev(args.argc, args.argv, prefix);
>> argv_array_clear(&args);
>> return rc;
>
> Yes; this was meant as a straight rewrite and I did not bother, but
> I should have cleaned it up as I meant to build on top.
>
> Will amend, even though I do not think we need to build anything on
> top.
Heh, you fooled me. cmd_name_rev() uses the usual parse-options
machinery that updates args.argv[]. Dashed options that were
consumed will not remain in args.argv[] and argv_array_clear() will
not have a chance to free them, and besides, args.argc and args.argv
will be out of sync and wreaks havoc in argv_array_clear().
We could expose argv_array_push_nodup() and use it in this caller
and then free the args.argv[] but not its contents, but I do not
think it is worth it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] describe: use argv-array
2013-07-09 16:00 ` Junio C Hamano
@ 2013-07-09 18:53 ` Jeff King
0 siblings, 0 replies; 20+ messages in thread
From: Jeff King @ 2013-07-09 18:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Tue, Jul 09, 2013 at 09:00:20AM -0700, Junio C Hamano wrote:
> >>> + return cmd_name_rev(args.argc, args.argv, prefix);
> >>
> >> This leaks the memory allocated by "args". The original did, too, and it
> >> is probably not that big a deal (we exit right after anyway). The fix
> >> would be something like:
> >>
> >> rc = cmd_name_rev(args.argc, args.argv, prefix);
> >> argv_array_clear(&args);
> >> return rc;
> >
> > Yes; this was meant as a straight rewrite and I did not bother, but
> > I should have cleaned it up as I meant to build on top.
> >
> > Will amend, even though I do not think we need to build anything on
> > top.
>
> Heh, you fooled me. cmd_name_rev() uses the usual parse-options
> machinery that updates args.argv[]. Dashed options that were
> consumed will not remain in args.argv[] and argv_array_clear() will
> not have a chance to free them, and besides, args.argc and args.argv
> will be out of sync and wreaks havoc in argv_array_clear().
Ick, yeah, I forgot about that. Let's just leave it as a leak, then. We
are exiting immediately afterwards, anyway.
-Peff
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2013-07-09 18:53 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-07 22:33 [PATCH 0/4] Make "git name-rev $(git rev-parse v1.8.3)" work Junio C Hamano
2013-07-07 22:33 ` [PATCH 1/4] name-ref: factor out name shortening logic from name_ref() Junio C Hamano
2013-07-08 8:52 ` Michael Haggerty
2013-07-08 15:04 ` Junio C Hamano
2013-07-07 22:33 ` [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref Junio C Hamano
2013-07-08 12:20 ` Ramkumar Ramachandra
2013-07-08 15:12 ` Junio C Hamano
2013-07-07 22:33 ` [PATCH 3/4] describe: use argv-array Junio C Hamano
2013-07-09 4:51 ` Jeff King
2013-07-09 14:55 ` Junio C Hamano
2013-07-09 16:00 ` Junio C Hamano
2013-07-09 18:53 ` Jeff King
2013-07-07 22:33 ` [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first Junio C Hamano
2013-07-08 13:08 ` Ramkumar Ramachandra
2013-07-09 5:12 ` Jeff King
2013-07-09 5:06 ` Jeff King
2013-07-09 5:33 ` Junio C Hamano
2013-07-09 5:35 ` Jeff King
2013-07-09 11:45 ` Junio C Hamano
2013-07-09 12:42 ` Ramkumar Ramachandra
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).