Re: [PATCH v2] for-each-ref: `:short` format for `refname`

Bert Wesarg [off-list ref] writes:

Tries to shorten the refname to a non-ambiguous name.
I.e. the full and the short refname points to the same object.

The definition of "ambiguity" here is wrong (see below).

+	/* skip first rule, will always match */
+	for (i = 0; i < nr_rules - 1; i++) {
+		const char **p;
+		int short_name_len;
+
+		if (1 != sscanf(ref->refname, scanf_fmts[nr_rules - 1 - i],
+				short_name))
+			continue;
+
+		short_name_len = strlen(short_name);
+
+		/* check if full and short point to the same object

Style?

+		 * by checking all rules in forward direction
+		 */

I think this part of the code is wrong, in that it talks about what object
the ref points at.  That is not what ref ambiguity is about.

Given a tag that points at a version 1.0.0 commit, this sequence will
create:

	$ git tag foo v1.0.0^0
        $ git branch foo v1.0.0^0

ambiguous branch and tag whose names are both 'foo', even though they
point at the same thing.  The right API to use would be resolve_ref(), I
think.

Other than that, it is well done.

Although I was initially a bit surprised by the size of the patch to
implement something so (conceptually) simple, the code was easy and
straightforward to follow.

Thanks.

On Wed, Sep 3, 2008 at 01:10, Junio C Hamano [off-list ref] wrote:

Bert Wesarg [off-list ref] writes:

quoted

Tries to shorten the refname to a non-ambiguous name.
I.e. the full and the short refname points to the same object.

The definition of "ambiguity" here is wrong (see below).

quoted

+              * by checking all rules in forward direction
+              */

I think this part of the code is wrong, in that it talks about what object
the ref points at.  That is not what ref ambiguity is about.

Given a tag that points at a version 1.0.0 commit, this sequence will
create:

       $ git tag foo v1.0.0^0
       $ git branch foo v1.0.0^0

ambiguous branch and tag whose names are both 'foo', even though they
point at the same thing.  The right API to use would be resolve_ref(), I
think.

I use resolve_ref():

int read_ref(const char *ref, unsigned char *sha1)
{
	if (resolve_ref(ref, sha1, 1, NULL))
		return 0;
	return -1;
}

Ok, I think I get your point: it doesn't matter if the full and short
point to different objects, it's enough for ambiguity that the short
name resolves to more than one ref.

New patch in reply.

Other than that, it is well done.

Although I was initially a bit surprised by the size of the patch to
implement something so (conceptually) simple, the code was easy and
straightforward to follow.

Thanks.

Your welcome.

Bert

Try to shorten the refname to a non-ambiguous name.

Changes in v3:
 * don't compare sha1's, its ambiguous if the short name
   resovles to more than one ref
 * use xstrdup()
 * use indexes for the loops to clarify backward/forward
   direction


Signed-off-by: Bert Wesarg <redacted>
Cc: git@vger.kernel.org
Cc: szeder@ira.uka.de
Cc: Shawn O. Pearce <redacted>
---
 Documentation/git-for-each-ref.txt |    1 +
 builtin-for-each-ref.c             |  138 ++++++++++++++++++++++++++++++++++--
 t/t6300-for-each-ref.sh            |   44 +++++++++++
 3 files changed, 176 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index eae6c0e..4016f59 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt

@@ -74,6 +74,7 @@ For all objects, the following names can be used:
 
 refname::
 	The name of the ref (the part after $GIT_DIR/).
+	For a non-ambiguous short name of the ref append `:short`.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 21e92bb..abe0804 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c

@@ -546,6 +546,110 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
 }
 
 /*
+ * generate a format suitable for scanf from a ref_rev_parse_rules
+ * rule, that is replace the "%.*s" spec with a "%s" spec
+ */
+static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
+{
+	char *spec;
+
+	spec = strstr(rule, "%.*s");
+	if (!spec || strstr(spec + 4, "%.*s"))
+		die("invalid rule in ref_rev_parse_rules: %s", rule);
+
+	/* copy all until spec */
+	strncpy(scanf_fmt, rule, spec - rule);
+	scanf_fmt[spec - rule] = '\0';
+	/* copy new spec */
+	strcat(scanf_fmt, "%s");
+	/* copy remaining rule */
+	strcat(scanf_fmt, spec + 4);
+
+	return;
+}
+
+/*
+ * Shorten the refname to an non-ambiguous form
+ */
+static char *get_short_ref(struct refinfo *ref)
+{
+	int i;
+	static char **scanf_fmts;
+	static int nr_rules;
+	char *short_name;
+
+	/* pre generate scanf formats from ref_rev_parse_rules[] */
+	if (!nr_rules) {
+		size_t total_len = 0;
+
+		/* the rule list is NULL terminated, count them first */
+		for (; ref_rev_parse_rules[nr_rules]; nr_rules++)
+			/* no +1 because strlen("%s") < strlen("%.*s") */
+			total_len += strlen(ref_rev_parse_rules[nr_rules]);
+
+		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
+
+		total_len = 0;
+		for (i = 0; i < nr_rules; i++) {
+			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules]
+					+ total_len;
+			gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);
+			total_len += strlen(ref_rev_parse_rules[i]);
+		}
+	}
+
+	/* bail out if there are no rules */
+	if (!nr_rules)
+		return ref->refname;
+
+	/* buffer for scanf result, at most ref->refname must fit */
+	short_name = xstrdup(ref->refname);
+
+	/* skip first rule, it will always match */
+	for (i = nr_rules - 1; i > 0 ; --i) {
+		int j;
+		int short_name_len;
+
+		if (1 != sscanf(ref->refname, scanf_fmts[i], short_name))
+			continue;
+
+		short_name_len = strlen(short_name);
+
+		/*
+		 * check if the short name resolves to a valid ref,
+		 * but use only rules prior to the matched one
+		 */
+		for (j = 0; j < i; j++) {
+			const char *p = ref_rev_parse_rules[j];
+			unsigned char short_objectname[20];
+
+			/* check for valid ref */
+			if (read_ref(mkpath(p, short_name_len, short_name),
+				     short_objectname))
+				continue;
+
+			/*
+			 * short_name resolves to a valid ref,
+			 * but its a differnet path than that of ref->refname,
+			 * therefore the short name is ambiguous
+			 */
+			break;
+		}
+
+		/*
+		 * short name is non-ambiguous if all previous rules
+		 * don't resolve to a valid ref
+		 */
+		if (j == i)
+			return short_name;
+	}
+
+	free(short_name);
+	return ref->refname;
+}
+
+
+/*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct refinfo *ref)

@@ -570,13 +674,33 @@ static void populate_value(struct refinfo *ref)
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
-		if (!strcmp(name, "refname"))
-			v->s = ref->refname;
-		else if (!strcmp(name, "*refname")) {
-			int len = strlen(ref->refname);
-			char *s = xmalloc(len + 4);
-			sprintf(s, "%s^{}", ref->refname);
-			v->s = s;
+		int deref = 0;
+		if (*name == '*') {
+			deref = 1;
+			name++;
+		}
+		if (!prefixcmp(name, "refname")) {
+			const char *formatp = strchr(name, ':');
+			const char *refname = ref->refname;
+
+			/* look for "short" refname format */
+			if (formatp) {
+				formatp++;
+				if (!strcmp(formatp, "short"))
+					refname = get_short_ref(ref);
+				else
+					die("unknown refname format %s",
+					    formatp);
+			}
+
+			if (!deref)
+				v->s = refname;
+			else {
+				int len = strlen(refname);
+				char *s = xmalloc(len + 4);
+				sprintf(s, "%s^{}", refname);
+				v->s = s;
+			}
 		}
 	}
 

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8ced593..4f247dd 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh

@@ -262,6 +262,50 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
 	"
 done
 
+cat >expected <<\EOF
+master
+testtag
+EOF
+
+test_expect_success 'Check short refname format' '
+	(git for-each-ref --format="%(refname:short)" refs/heads &&
+	git for-each-ref --format="%(refname:short)" refs/tags) >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check for invalid refname format' '
+	test_must_fail git for-each-ref --format="%(refname:INVALID)"
+'
+
+cat >expected <<\EOF
+heads/master
+master
+EOF
+
+test_expect_success 'Check ambiguous head and tag refs' '
+	git checkout -b newtag &&
+	echo "Using $datestamp" > one &&
+	git add one &&
+	git commit -m "Branch" &&
+	setdate_and_increment &&
+	git tag -m "Tagging at $datestamp" master &&
+	git for-each-ref --format "%(refname:short)" refs/heads/master refs/tags/master >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+heads/ambiguous
+ambiguous
+EOF
+
+test_expect_success 'Check ambiguous head and tag refs II' '
+	git checkout master &&
+	git tag ambiguous testtag^0 &&
+	git branch ambiguous testtag^0 &&
+	git for-each-ref --format "%(refname:short)" refs/heads/ambiguous refs/tags/ambiguous >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'an unusual tag with an incomplete line' '
 
 	git tag -m "bogo" bogo &&

-- 
1.6.0

Bert Wesarg [off-list ref] wrote:

Try to shorten the refname to a non-ambiguous name.

Changes in v3:
 * don't compare sha1's, its ambiguous if the short name
   resovles to more than one ref
 * use xstrdup()
 * use indexes for the loops to clarify backward/forward
   direction


Signed-off-by: Bert Wesarg <redacted>
Cc: git@vger.kernel.org
Cc: szeder@ira.uka.de
Cc: Shawn O. Pearce <redacted>
---

Looks good.  But the commit messages shouldn't have the "Changes
in v3" section or probably the "Cc:" lines.  The changes in v3
part usually goes below the ---.

I don't usually work on for-each-ref, but I'll toss an ack in anyway:

Acked-by: Shawn O. Pearce <redacted>

-- 
Shawn.

On Wed, Sep 3, 2008 at 17:18, Shawn O. Pearce [off-list ref] wrote:

Bert Wesarg [off-list ref] wrote:

quoted

Try to shorten the refname to a non-ambiguous name.

Changes in v3:
 * don't compare sha1's, its ambiguous if the short name
   resovles to more than one ref
 * use xstrdup()
 * use indexes for the loops to clarify backward/forward
   direction


Signed-off-by: Bert Wesarg <redacted>
Cc: git@vger.kernel.org
Cc: szeder@ira.uka.de
Cc: Shawn O. Pearce <redacted>
---

Looks good.  But the commit messages shouldn't have the "Changes
in v3" section or probably the "Cc:" lines.  The changes in v3
part usually goes below the ---.

Ok, but shouldn't git format-patch detect that there is already a ---
in the commit message, or is it illegal to have more than one ---?

And while we are at it, shouldn't git format-patch detect, that there
is already a [*PATCH*]-prefix in the first line of the commit?

I always need to edit these two things after format-patch.

Bert

--
Shawn.

On Wed, Sep 3, 2008 at 18:33, Bert Wesarg [off-list ref] wrote:

On Wed, Sep 3, 2008 at 17:18, Shawn O. Pearce [off-list ref] wrote:

quoted

Bert Wesarg [off-list ref] wrote:

quoted

Try to shorten the refname to a non-ambiguous name.

Changes in v3:
 * don't compare sha1's, its ambiguous if the short name
   resovles to more than one ref
 * use xstrdup()
 * use indexes for the loops to clarify backward/forward
   direction


Signed-off-by: Bert Wesarg <redacted>
Cc: git@vger.kernel.org
Cc: szeder@ira.uka.de
Cc: Shawn O. Pearce <redacted>
---

Looks good.  But the commit messages shouldn't have the "Changes
in v3" section or probably the "Cc:" lines.  The changes in v3
part usually goes below the ---.

Ok, but shouldn't git format-patch detect that there is already a ---
in the commit message, or is it illegal to have more than one ---?

And while we are at it, shouldn't git format-patch detect, that there
is already a [*PATCH*]-prefix in the first line of the commit?

Ok, I was too fast, -k is obviously the right option for this.

I always need to edit these two things after format-patch.

Bert

quoted

--
Shawn.