[RFC/PATCH 1/2] for-each-ref: add new format 'refbasename'

[RFC/PATCH 1/2] for-each-ref: add new format 'refbasename'

[SZEDER Gábor]

fot-each-ref's refname format outputs each ref in its full format, e.g.
'refs/heads/foo' and 'refs/tags/bar'.  However, there are tools that
need only the last part of the refname, e.g. only 'foo' and 'bar'.  Such
a tool is git's bash completion script, which spends considerable amount
of time removing the unneeded parts from for-each-ref's output.

Therefore, we introduce a new for-each-ref format called 'refbasename',
which strips everything before and including the second '/' in the ref's
name from the output.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

  I assumed that refs always look like 'refs/{heads,tags,whatever}/foo',
  hence this patch breaks if a ref might look like 'refs/foo' or just
  'foo'.  But can I really rely on that?

 builtin-for-each-ref.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 21e92bb..1993ff4 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -66,6 +66,7 @@ static struct {
 	{ "subject" },
 	{ "body" },
 	{ "contents" },
+	{ "refbasename" },
 };
 
 /*
@@ -577,6 +578,10 @@ static void populate_value(struct refinfo *ref)
 			char *s = xmalloc(len + 4);
 			sprintf(s, "%s^{}", ref->refname);
 			v->s = s;
+		} else if (!strcmp(name, "refbasename")) {
+			char * p = strchr(ref->refname, '/');
+			p = strchr(p+1, '/');
+			v->s = p+1;
 		}
 	}
 
-- 
1.6.0.1.133.g10dd.dirty

[RFC/PATCH 2/2] bash: use for-each-ref format 'refbasename'

[SZEDER Gábor]

Using this format simplifies the code for completing refs and (in some
cases) improves performance significantly.

For repositories like the current git.git (with more than 200 refs)
there are no real performance difference, but for a repository with 2000
refs the total time needed to complete the refs is around 40% less (from
around 480ms to around 295ms).

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |   26 ++++++--------------------
 1 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4f64f8a..8269c42 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -154,11 +154,8 @@ __git_heads ()
 {
 	local cmd i is_hash=y dir="$(__gitdir "$1")"
 	if [ -d "$dir" ]; then
-		for i in $(git --git-dir="$dir" \
-			for-each-ref --format='%(refname)' \
-			refs/heads ); do
-			echo "${i#refs/heads/}"
-		done
+		git --git-dir="$dir" for-each-ref --format='%(refbasename)' \
+			refs/heads
 		return
 	fi
 	for i in $(git ls-remote "$1" 2>/dev/null); do
@@ -175,11 +172,8 @@ __git_tags ()
 {
 	local cmd i is_hash=y dir="$(__gitdir "$1")"
 	if [ -d "$dir" ]; then
-		for i in $(git --git-dir="$dir" \
-			for-each-ref --format='%(refname)' \
-			refs/tags ); do
-			echo "${i#refs/tags/}"
-		done
+		git --git-dir="$dir" for-each-ref --format='%(refbasename)' \
+			refs/tags
 		return
 	fi
 	for i in $(git ls-remote "$1" 2>/dev/null); do
@@ -197,16 +191,8 @@ __git_refs ()
 	local cmd i is_hash=y dir="$(__gitdir "$1")"
 	if [ -d "$dir" ]; then
 		if [ -e "$dir/HEAD" ]; then echo HEAD; fi
-		for i in $(git --git-dir="$dir" \
-			for-each-ref --format='%(refname)' \
-			refs/tags refs/heads refs/remotes); do
-			case "$i" in
-				refs/tags/*)    echo "${i#refs/tags/}" ;;
-				refs/heads/*)   echo "${i#refs/heads/}" ;;
-				refs/remotes/*) echo "${i#refs/remotes/}" ;;
-				*)              echo "$i" ;;
-			esac
-		done
+		git --git-dir="$dir" for-each-ref --format='%(refbasename)' \
+			refs/tags refs/heads refs/remotes
 		return
 	fi
 	for i in $(git ls-remote "$dir" 2>/dev/null); do
-- 
1.6.0.1.133.g10dd.dirty

Re: [RFC/PATCH 1/2] for-each-ref: add new format 'refbasename'

[Shawn O. Pearce]

SZEDER GGGbor <szeder@ira.uka.de> wrote:
> Therefore, we introduce a new for-each-ref format called 'refbasename',
> which strips everything before and including the second '/' in the ref's
> name from the output.
> 
>   I assumed that refs always look like 'refs/{heads,tags,whatever}/foo',
>   hence this patch breaks if a ref might look like 'refs/foo' or just
>   'foo'.  But can I really rely on that?
> 
> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index 21e92bb..1993ff4 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -577,6 +578,10 @@ static void populate_value(struct refinfo *ref)
>  			char *s = xmalloc(len + 4);
>  			sprintf(s, "%s^{}", ref->refname);
>  			v->s = s;
> +		} else if (!strcmp(name, "refbasename")) {
> +			char * p = strchr(ref->refname, '/');
> +			p = strchr(p+1, '/');
> +			v->s = p+1;

Please be careful here and check for !p.  A refname may be missing
one or two '/' in which case you will cause the process to segfault.

I don't think its a good idea to assume you'll always have to '/'
in the name.  "refs/foo" can be created by git-update-ref.  Or if
we ever started to report on HEAD this output tag would crash.

Also, as a style nit, its "p + 1" not "p+1".

Test cases?

On the other hand, I like where this is going.  Given that it
has such a nice effect on the bash completion for larger repos
I'd like to see it in Git.

-- 
Shawn.

[RFC/PATCH 1/2 v2] for-each-ref: add new format 'refbasename'

[SZEDER Gábor]

for-each-ref's refname format outputs each ref in its full format, e.g.
'refs/heads/foo' and 'refs/tags/bar'.  However, there are tools that
need only the last part of the refname, e.g. only 'foo' and 'bar'.  Such
a tool is git's bash completion script, which spends considerable amount
of time removing the unneeded parts from for-each-ref's output.

Therefore, we introduce a new for-each-ref format called 'refbasename',
which strips the leading parts of the refname:

  * If the refname doesn't contain any '/', then it is printed as is.

  * If the refname contains one '/', then the string following the '/'
    is printed (e.g. 'refs/foo' becomes 'foo').

  * If the refname contains two (or more) '/', then the string following
    the second '/' is printed (e.g. 'refs/heads/foo' becomes 'foo').
    
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

On Fri, Aug 29, 2008 at 07:34:48AM -0700, Shawn O. Pearce wrote:
> SZEDER GGGbor <szeder@ira.uka.de> wrote:
> > +		} else if (!strcmp(name, "refbasename")) {
> > +			char * p = strchr(ref->refname, '/');
> > +			p = strchr(p+1, '/');
> > +			v->s = p+1;
> 
> Please be careful here and check for !p.  A refname may be missing
> one or two '/' in which case you will cause the process to segfault.
> 
> I don't think its a good idea to assume you'll always have to '/'
> in the name.  "refs/foo" can be created by git-update-ref.  Or if
> we ever started to report on HEAD this output tag would crash.
Ah, I feared as much.

This raises the question what should the refbasename format print in
those cases.  The first case (no '/' at all) is trivial, but the other
two are a bit problematic:

  * The refname 'foo/bar' seems to be valid, or at least the command
    sequence 'mkdir .git/foo; cp .git/refs/heads/master .git/foo/bar;
    git checkout foo/bar' works as expected.  OTOH, 'git for-each-ref
    --format="%(refname)"' doesn't print anything.

  * The refname 'refs/head/foo/bar' should become 'foo/bar':  it's in 
    sync with how 'git branch' lists it, and this behaviour is needed
    by the bash completion changes.  However, unfortunately it
    contradicts to the format's name, because basename removes all
    directory components.  Maybe someone has a better idea on how to
    name this format...


 builtin-for-each-ref.c  |   12 ++++++++++++
 t/t6300-for-each-ref.sh |    2 ++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 21e92bb..23d064b 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -66,6 +66,7 @@ static struct {
 	{ "subject" },
 	{ "body" },
 	{ "contents" },
+	{ "refbasename" },
 };
 
 /*
@@ -577,6 +578,17 @@ static void populate_value(struct refinfo *ref)
 			char *s = xmalloc(len + 4);
 			sprintf(s, "%s^{}", ref->refname);
 			v->s = s;
+		} else if (!strcmp(name, "refbasename")) {
+			char * slash1 = strchr(ref->refname, '/');
+			if (!slash1)
+				v->s = ref->refname;
+			else {
+				char * slash2 = strchr(slash1 + 1, '/');
+				if (!slash2)
+					v->s = slash1 + 1;
+				else
+					v->s = slash2 + 1;
+			}
 		}
 	}
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8ced593..b317a01 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -66,6 +66,7 @@ test_atom head subject 'Initial'
 test_atom head body ''
 test_atom head contents 'Initial
 '
+test_atom head refbasename 'master'
 
 test_atom tag refname refs/tags/testtag
 test_atom tag objecttype tag
@@ -95,6 +96,7 @@ test_atom tag subject 'Tagging at 1151939927'
 test_atom tag body ''
 test_atom tag contents 'Tagging at 1151939927
 '
+test_atom tag refbasename 'testtag'
 
 test_expect_success 'Check invalid atoms names are errors' '
 	test_must_fail git-for-each-ref --format="%(INVALID)" refs/heads
-- 
1.6.0.1.149.g903b0

Re: [RFC/PATCH 1/2] for-each-ref: add new format 'refbasename'

[Bert Wesarg]

On Fri, Aug 29, 2008 at 12:37, SZEDER Gábor <szeder@ira.uka.de> wrote:
> fot-each-ref's refname format outputs each ref in its full format, e.g.
> 'refs/heads/foo' and 'refs/tags/bar'.  However, there are tools that
> need only the last part of the refname, e.g. only 'foo' and 'bar'.  Such
> a tool is git's bash completion script, which spends considerable amount
> of time removing the unneeded parts from for-each-ref's output.
>
> Therefore, we introduce a new for-each-ref format called 'refbasename',
> which strips everything before and including the second '/' in the ref's
> name from the output.

Why not strip the pattern (given on the command line) from the
refname? I.e. with a new format 'refshort' a 'git for-each-ref
--format=%(refshort) refs/heads/' would strip off the refs/heads/ from
each ref.

This should also work for multiple patterns (refs/heads/ refs/tags/)
and for patterns which needs a fnmatch call (which has the
FNM_PATHNAME option).

The bash completion script calls for-each-ref always with patterns.

Bert

[PATCH] for-each-ref: new 'refshort' format

[Bert Wesarg]

Here is a first sketch for this idea.

This strips from the refname the common prefix with the matched pattern.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

---

I think we should use a ':short' modifier to 'reflog' and '*reflog'.

 builtin-for-each-ref.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 21e92bb..b80d753 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -31,6 +31,7 @@ struct ref_sort {
 
 struct refinfo {
 	char *refname;
+	const char *pattern; /* the pattern which matched this ref */
 	unsigned char objectname[20];
 	struct atom_value *value;
 };
@@ -66,6 +67,7 @@ static struct {
 	{ "subject" },
 	{ "body" },
 	{ "contents" },
+	{ "refshort" },
 };
 
 /*
@@ -546,6 +548,36 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
 }
 
 /*
+ * Use the matched pattern from ref to shorten the refname
+ */
+static char *get_short_ref(struct refinfo *ref)
+{
+	int rlen, plen, len = 0;
+
+	if (!ref->pattern)
+		return ref->refname;
+
+	rlen = strlen(ref->refname);
+	plen = strlen(ref->pattern);
+
+	if ((plen <= rlen) &&
+	    !strncmp(ref->refname, ref->pattern, plen) &&
+	    (ref->refname[plen] == '\0' ||
+	     ref->refname[plen] == '/' ||
+	     ref->pattern[plen - 1] == '/')) {
+		len = plen + (ref->refname[plen] == '/');
+	} else {
+		len = strcspn(ref->pattern, "*?[");
+		while (len >= 0 && ref->pattern[len] != '/')
+			--len;
+		len++;
+	}
+
+	return ref->refname + len;
+}
+
+
+/*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct refinfo *ref)
@@ -577,6 +609,8 @@ static void populate_value(struct refinfo *ref)
 			char *s = xmalloc(len + 4);
 			sprintf(s, "%s^{}", ref->refname);
 			v->s = s;
+		} else if (!strcmp(name, "refshort")) {
+			v->s = get_short_ref(ref);
 		}
 	}
 
@@ -641,9 +675,9 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
 	struct grab_ref_cbdata *cb = cb_data;
 	struct refinfo *ref;
 	int cnt;
+	const char **pattern = cb->grab_pattern;
 
-	if (*cb->grab_pattern) {
-		const char **pattern;
+	if (*pattern) {
 		int namelen = strlen(refname);
 		for (pattern = cb->grab_pattern; *pattern; pattern++) {
 			const char *p = *pattern;
@@ -668,6 +702,7 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
 	 */
 	ref = xcalloc(1, sizeof(*ref));
 	ref->refname = xstrdup(refname);
+	ref->pattern = *pattern;
 	hashcpy(ref->objectname, sha1);
 
 	cnt = cb->grab_cnt;
-- 
1.6.0

Re: [PATCH] for-each-ref: new 'refshort' format

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> Here is a first sketch for this idea.
>
> This strips from the refname the common prefix with the matched pattern.

Looks quite straightforward and to the point.

Re: [PATCH] for-each-ref: new 'refshort' format

[Bert Wesarg]

On Sun, Aug 31, 2008 at 06:31, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> Here is a first sketch for this idea.
>>
>> This strips from the refname the common prefix with the matched pattern.
>
> Looks quite straightforward and to the point.
>
What do you think about the ':short' modifier for 'refname' or
'*refname' instead 'refshort'? i.e. %(refname:short)

Bert