[PATCH] git-filter-branch could be confused by similar names

[PATCH] git-filter-branch could be confused by similar names

[Dmitry Potapov]

'git-filter-branch branch' could fail producing the error:
"Which ref do you want to rewrite?" if existed another branch
or tag, which name was 'branch-something' or 'something/branch'.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
 git-filter-branch.sh     |    2 +-
 t/t7003-filter-branch.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index dbab1a9..b89a720 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -219,7 +219,7 @@ do
 	;;
 	*)
 		ref="$(git for-each-ref --format='%(refname)' |
-			grep /"$ref")"
+			grep '^refs/[^/]\+/'"$ref"'$')"
 	esac
 
 	git check-ref-format "$ref" && echo "$ref"
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 5f60b22..c3e5207 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -36,6 +36,16 @@ test_expect_success 'result is really identical' '
 	test $H = $(git rev-parse HEAD)
 '
 
+test_expect_success 'rewrite branch with similar names' '
+	git branch my &&
+	git tag my/orig &&
+	git tag my-orig &&
+	git tag orig/my &&
+	git tag orig-my &&
+	git-filter-branch my &&
+	test $H = $(git rev-parse HEAD)
+'
+
 test_expect_success 'rewrite, renaming a specific file' '
 	git-filter-branch -f --tree-filter "mv d doh || :" HEAD
 '
-- 
1.5.3.5

Re: [PATCH] git-filter-branch could be confused by similar names

[Johannes Schindelin]

Hi,

On Tue, 25 Dec 2007, Dmitry Potapov wrote:

> 'git-filter-branch branch' could fail producing the error:
> "Which ref do you want to rewrite?" if existed another branch
> or tag, which name was 'branch-something' or 'something/branch'.
> 
> Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
> ---
>  git-filter-branch.sh     |    2 +-
>  t/t7003-filter-branch.sh |   10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index dbab1a9..b89a720 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -219,7 +219,7 @@ do
>  	;;
>  	*)
>  		ref="$(git for-each-ref --format='%(refname)' |
> -			grep /"$ref")"
> +			grep '^refs/[^/]\+/'"$ref"'$')"

Hmm.  I wonder if this is a proper solution.  It still does not error out 
when you have a tag and a branch of the same name.

I kinda hoped that by 1.5.4, rewrite-commits would be finished, but it 
seems that nothing happened in that area after 1.5.3-rcX.

It would be so much easier to have checks like this -- returning the real 
refname for short but unique short refnames -- in C.

Ciao,
Dscho

Re: [PATCH] git-filter-branch could be confused by similar names

[Dmitry Potapov]

Hi,

On Sat, Dec 29, 2007 at 11:36:51PM +0100, Johannes Schindelin wrote:
> 
> On Tue, 25 Dec 2007, Dmitry Potapov wrote:
> 
> > 'git-filter-branch branch' could fail producing the error:
> > "Which ref do you want to rewrite?" if existed another branch
> > or tag, which name was 'branch-something' or 'something/branch'.
> > 
> > Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
> > ---
> >  git-filter-branch.sh     |    2 +-
> >  t/t7003-filter-branch.sh |   10 ++++++++++
> >  2 files changed, 11 insertions(+), 1 deletions(-)
> > 
> > diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> > index dbab1a9..b89a720 100755
> > --- a/git-filter-branch.sh
> > +++ b/git-filter-branch.sh
> > @@ -219,7 +219,7 @@ do
> >  	;;
> >  	*)
> >  		ref="$(git for-each-ref --format='%(refname)' |
> > -			grep /"$ref")"
> > +			grep '^refs/[^/]\+/'"$ref"'$')"
> 
> Hmm.  I wonder if this is a proper solution.  It still does not error out 
> when you have a tag and a branch of the same name.
> 

Are you sure? I had created a tag and a branch with the same name, and
then tried git filter-branch on it, and it did error out:
===
warning: refname 'test1' is ambiguous.
Which ref do you want to rewrite?
===

Maybe, my fix is not a perfect solution, but it works correctly in all
known to me situations, while the original code is clearly broken in
most common cases, like when you have created a tag with a name that
consists of the name of a branch plus some arbitrary suffix. When you
run git-filter-branch on that branch, you only get: "Which ref do you
want to rewrite?", which is very confusing, because you have only one
reference with the given name.


Dmitry

Re: [PATCH] git-filter-branch could be confused by similar names

[Johannes Schindelin]

Hi,

On Sun, 30 Dec 2007, Dmitry Potapov wrote:

> On Sat, Dec 29, 2007 at 11:36:51PM +0100, Johannes Schindelin wrote:
> > 
> > On Tue, 25 Dec 2007, Dmitry Potapov wrote:
> > 
> > > 'git-filter-branch branch' could fail producing the error: "Which 
> > > ref do you want to rewrite?" if existed another branch or tag, which 
> > > name was 'branch-something' or 'something/branch'.
> > > 
> > > Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
> > > ---
> > >  git-filter-branch.sh     |    2 +-
> > >  t/t7003-filter-branch.sh |   10 ++++++++++
> > >  2 files changed, 11 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> > > index dbab1a9..b89a720 100755
> > > --- a/git-filter-branch.sh
> > > +++ b/git-filter-branch.sh
> > > @@ -219,7 +219,7 @@ do
> > >  	;;
> > >  	*)
> > >  		ref="$(git for-each-ref --format='%(refname)' |
> > > -			grep /"$ref")"
> > > +			grep '^refs/[^/]\+/'"$ref"'$')"
> > 
> > Hmm.  I wonder if this is a proper solution.  It still does not error 
> > out when you have a tag and a branch of the same name.
> 
> Are you sure? I had created a tag and a branch with the same name, and
> then tried git filter-branch on it, and it did error out:
> ===
> warning: refname 'test1' is ambiguous.
> Which ref do you want to rewrite?
> ===

Okay, bad example.  But try "heads/master".  Or "origin" in a repository 
which has "refs/remotes/origin/HEAD".

Ciao,
Dscho

Re: [PATCH] git-filter-branch could be confused by similar names

[Dmitry Potapov]

On Sun, Dec 30, 2007 at 11:46:59AM +0100, Johannes Schindelin wrote:
> 
> On Sun, 30 Dec 2007, Dmitry Potapov wrote:
> 
> > On Sat, Dec 29, 2007 at 11:36:51PM +0100, Johannes Schindelin wrote:
> > > 
> > > On Tue, 25 Dec 2007, Dmitry Potapov wrote:
> > > 
> > > > 'git-filter-branch branch' could fail producing the error: "Which 
> > > > ref do you want to rewrite?" if existed another branch or tag, which 
> > > > name was 'branch-something' or 'something/branch'.
> > > > 
> > > > Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
> > > > ---
> > > >  git-filter-branch.sh     |    2 +-
> > > >  t/t7003-filter-branch.sh |   10 ++++++++++
> > > >  2 files changed, 11 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> > > > index dbab1a9..b89a720 100755
> > > > --- a/git-filter-branch.sh
> > > > +++ b/git-filter-branch.sh
> > > > @@ -219,7 +219,7 @@ do
> > > >  	;;
> > > >  	*)
> > > >  		ref="$(git for-each-ref --format='%(refname)' |
> > > > -			grep /"$ref")"
> > > > +			grep '^refs/[^/]\+/'"$ref"'$')"
> > > 
> > > Hmm.  I wonder if this is a proper solution.  It still does not error 
> > > out when you have a tag and a branch of the same name.
> > 
> > Are you sure? I had created a tag and a branch with the same name, and
> > then tried git filter-branch on it, and it did error out:
> > ===
> > warning: refname 'test1' is ambiguous.
> > Which ref do you want to rewrite?
> > ===
> 
> Okay, bad example.  But try "heads/master". 

You are right. Somehow, I forgot about this possibility. How about this:

+			grep '^refs/\([^/]\+/\)\?'"$ref"'$')"

> Or "origin" in a repository 
> which has "refs/remotes/origin/HEAD".

Well, it does not work, but it would not work before either, because you
are very likely to have something else in origin. Actually, I doubt that
anyone will want to filter "origin", but if you insist, here is another
grep expression, which should accommodate that case too:

+			grep '^refs/\([^/]\+/\)\?'"$ref"'\(/HEAD\)\?$')"

In any case, I believe it would be better to have a more strict grep
expression than one that is used by git-filter-branch now, because now
you either have a very confusing error message, or accidentally you
could filter a wrong branch. And as you said before, the proper C
solution is not feasible for 1.5.4, so I believe a better grep
expression is the right thing to do for now.

If you have no other objection, I will resent the patch with the
corrected version of the grep expression.

Dmitry

Re: [PATCH] git-filter-branch could be confused by similar names

[Johannes Schindelin]

Hi,

On Sun, 30 Dec 2007, Dmitry Potapov wrote:

> How about this:
> 
> +			grep '^refs/\([^/]\+/\)\?'"$ref"'$')"

Maybe.  I wonder whether just adding a "$" (which I obviously forgot) 
would not be enough...

Ciao,
Dscho

Re: [PATCH] git-filter-branch could be confused by similar names

[Dmitry Potapov]

On Sun, Dec 30, 2007 at 05:03:32PM +0100, Johannes Schindelin wrote:
> 
> On Sun, 30 Dec 2007, Dmitry Potapov wrote:
> 
> > How about this:
> > 
> > +			grep '^refs/\([^/]\+/\)\?'"$ref"'$')"
> 
> Maybe.  I wonder whether just adding a "$" (which I obviously forgot) 
> would not be enough...

Adding '$' will certainly make things much better, but you will still
have the same problem if you want to filter "master", but you have
"origin/master" in your repo.

Dmitry

[PATCH] git-filter-branch could be confused by similar names

[Dmitry Potapov]

'git-filter-branch branch' could fail producing the error:
"Which ref do you want to rewrite?" if existed another branch
or tag, which name was 'branch-something' or 'something/branch'.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---

I have corrected my previous patch to allow "heads" or "tags"
in the name of a branch or tag, i.e. to write it like this:
   git filter-branch heads/master

 git-filter-branch.sh     |    2 +-
 t/t7003-filter-branch.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index dbab1a9..5de8b12 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -219,7 +219,7 @@ do
 	;;
 	*)
 		ref="$(git for-each-ref --format='%(refname)' |
-			grep /"$ref")"
+			grep '^refs/\([^/]\+/\)\?'"$ref"'$')"
 	esac
 
 	git check-ref-format "$ref" && echo "$ref"
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 5f60b22..c3e5207 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -36,6 +36,16 @@ test_expect_success 'result is really identical' '
 	test $H = $(git rev-parse HEAD)
 '
 
+test_expect_success 'rewrite branch with similar names' '
+	git branch my &&
+	git tag my/orig &&
+	git tag my-orig &&
+	git tag orig/my &&
+	git tag orig-my &&
+	git-filter-branch my &&
+	test $H = $(git rev-parse HEAD)
+'
+
 test_expect_success 'rewrite, renaming a specific file' '
 	git-filter-branch -f --tree-filter "mv d doh || :" HEAD
 '
-- 
1.5.3.5

Re: [PATCH] git-filter-branch could be confused by similar names

[Junio C Hamano]

Dmitry Potapov <dpotapov@gmail.com> writes:

> 'git-filter-branch branch' could fail producing the error:
> "Which ref do you want to rewrite?" if existed another branch
> or tag, which name was 'branch-something' or 'something/branch'.
>
> Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
> ---
>
> I have corrected my previous patch to allow "heads" or "tags"
> in the name of a branch or tag, i.e. to write it like this:
>    git filter-branch heads/master
>
>  git-filter-branch.sh     |    2 +-
>  t/t7003-filter-branch.sh |   10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index dbab1a9..5de8b12 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -219,7 +219,7 @@ do
>  	;;
>  	*)
>  		ref="$(git for-each-ref --format='%(refname)' |
> -			grep /"$ref")"
> +			grep '^refs/\([^/]\+/\)\?'"$ref"'$')"
>  	esac

Do we assume everybody's grep groks ERE these days?  I had an
impression that we try to stick to a subset of BRE (namely, no
\{m,n\}, [::], [==], nor [..]).

Also as a general rule when dealing with refname, we use
fileglob not regex.

What's the goal here?  Is it to make sure given refname is
unambiguous by being a unique suffix of tags or heads, as in

	test $(git show-ref "$ref" | wc -l) = 1

or is there anything more going on?

Ah, it also wants the full name of the ref.  How about...

	ref=$(git show-ref "$ref" | sed -e 's/^.* //')

and have the "git check-ref-format" that comes later to issue an
error message?        

A better error message would be obtained with perhaps doing

	LF='
        '
        
at the beginning and then doing:

	candidate=$(git show-ref "$ref" | sed -e 's/^.* //')
	case "$candidate" in
        '')
        	die "should not happen -- $ref did not match?"
                ;;
        ?*"$LF"?*)
		die "$ref is ambiguous, which one of: $canidate?"
		;;
	esac
        ref=$candidate

Re: [PATCH] git-filter-branch could be confused by similar names

[Dmitry Potapov]

On Thu, Jan 03, 2008 at 01:27:27PM -0800, Junio C Hamano wrote:
> Dmitry Potapov <dpotapov@gmail.com> writes:
> >  		ref="$(git for-each-ref --format='%(refname)' |
> > -			grep /"$ref")"
> > +			grep '^refs/\([^/]\+/\)\?'"$ref"'$')"
> >  	esac
> 
> Do we assume everybody's grep groks ERE these days?  I had an
> impression that we try to stick to a subset of BRE (namely, no
> \{m,n\}, [::], [==], nor [..]).

I was not aware about this policy, and I am not aware about
existing any grep that does not grok the expressions I used
above. So, I thought they are commonly accepted, but I might
be wrong.

> 
> Also as a general rule when dealing with refname, we use
> fileglob not regex.

Actually, refname was not meant to be used as regex here, and
it was written in the hope that there will be no special regex
symbols in the refname, but, yes, this use looks like hack.

BTW, accordingly to man, git filter-branch has <rev-list options>,
and git rev-list described as <commit(s)>, so fileglob may not
be used here. I look also at git for-each-ref and git show-ref,
and though they could have <pattern> as arguments, they meant
completely different by that. git for-each-ref requires the
full specification starting with refs but allows fileglob, while
git-show-ref does not allow fileglob, but it goes deeper in
refs, so it will match with those refs that are inside origin,
which git ref-list does not do. Here are a few examples:

===
$ git rev-list -1 master
257f3020f69f3222cdefc1d84b148fb35b2c4f5b
$ git rev-list -1 heads/master
257f3020f69f3222cdefc1d84b148fb35b2c4f5b
$ git rev-list -1 refs/heads/master
257f3020f69f3222cdefc1d84b148fb35b2c4f5b
$ git rev-list -1 'refs/heads/maste?'
fatal: ambiguous argument 'refs/heads/maste?': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
$ git rev-list -1 maint
fatal: ambiguous argument 'maint': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
===
$ git for-each-ref refs/heads/master
257f3020f69f3222cdefc1d84b148fb35b2c4f5b commit	refs/heads/master
$ git for-each-ref refs/heads/maste?
257f3020f69f3222cdefc1d84b148fb35b2c4f5b commit	refs/heads/master
$ git for-each-ref heads/master
$ git for-each-ref master
$ git for-each-ref maint
===
$ git show-ref refs/heads/master
257f3020f69f3222cdefc1d84b148fb35b2c4f5b refs/heads/master
$ git show-ref refs/heads/maste?
$ git show-ref heads/master
257f3020f69f3222cdefc1d84b148fb35b2c4f5b refs/heads/master
$ git show-ref master
257f3020f69f3222cdefc1d84b148fb35b2c4f5b refs/heads/master
257f3020f69f3222cdefc1d84b148fb35b2c4f5b refs/remotes/origin/master
$ git show-ref maint
4f3d37035a7c735a3b69f962656819f4ff7e4927 refs/remotes/origin/maint
===

> 
> What's the goal here?  Is it to make sure given refname is
> unambiguous by being a unique suffix of tags or heads, as in
> 
> 	test $(git show-ref "$ref" | wc -l) = 1

Because, I am not the author of this script, I can't be sure,
but it seems to me, the goal is to select among all parameters
only those that represents tops of branches, for example,
being given: A..B ^C D, we should choose only B and D and
convert them into the full refname in the same way as rev-list
does that.

> 
> or is there anything more going on?
> 
> Ah, it also wants the full name of the ref.  How about...
> 
> 	ref=$(git show-ref "$ref" | sed -e 's/^.* //')

It works only if the name "unambiguous" for git show-ref, which
interprets refname differently than rev-list as I wrote above.
Nevertheless, I believe we can use 'git show-ref' if we try
something like this:

for prefix in refs refs/tags refs/heads refs/remote; do
  fullref=$(git show-ref "$prefix/$ref" | sed -e 's/^.* //')
  test -n "$fullref" && break
done
ref="$fullref"

If this idea does not raise any objection, I will test it a bit
and then send the patch.

Dmitry

Re: [PATCH] git-filter-branch could be confused by similar names

[Junio C Hamano]

Dmitry Potapov <dpotapov@gmail.com> writes:

> It works only if the name "unambiguous" for git show-ref, which
> interprets refname differently than rev-list as I wrote above.
> Nevertheless, I believe we can use 'git show-ref' if we try
> something like this:

Ahh.

But at that point I would say that exposing the refname dwimming
logic to the scripts could be a much cleaner solution.  After
all, get_sha1_basic() knows what ref it used to come up with the
answer, but we are discarding that information.

How about making "rev-parse --symbolic-full-name" give what it
eventually dwimmed the given ref to, perhaps like the attached
patch?  Then you can do:

  git rev-parse --revs-only --symbolic-full-name "$@" >"$tempdir"/heads

without any need for the loop there in filter-branch.

---

 builtin-rev-parse.c |   37 ++++++++++++++++++++++++++++++++++---
 1 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 20d1789..b9af1a5 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -21,6 +21,9 @@ static const char *def;
 #define NORMAL 0
 #define REVERSED 1
 static int show_type = NORMAL;
+
+#define SHOW_SYMBOLIC_ASIS 1
+#define SHOW_SYMBOLIC_FULL 2
 static int symbolic;
 static int abbrev;
 static int output_sq;
@@ -103,8 +106,32 @@ static void show_rev(int type, const unsigned char *sha1, const char *name)
 
 	if (type != show_type)
 		putchar('^');
-	if (symbolic && name)
-		show(name);
+	if (symbolic && name) {
+		if (symbolic == SHOW_SYMBOLIC_FULL) {
+			unsigned char discard[20];
+			char *full;
+
+			switch (dwim_ref(name, strlen(name), discard, &full)) {
+			case 0:
+				/*
+				 * Not found -- not a ref.  We could
+				 * emit "name" here, but symbolic-full
+				 * users are interested in finding the
+				 * refs spelled in full, and they would
+				 * need to filter non-refs if we did so.
+				 */
+				break;
+			case 1: /* happy */
+				show(full);
+				break;
+			default: /* ambiguous */
+				error("refname '%s' is ambiguous", name);
+				break;
+			}
+		} else {
+			show(name);
+		}
+	}
 	else if (abbrev)
 		show(find_unique_abbrev(sha1, abbrev));
 	else
@@ -421,7 +448,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--symbolic")) {
-				symbolic = 1;
+				symbolic = SHOW_SYMBOLIC_ASIS;
+				continue;
+			}
+			if (!strcmp(arg, "--symbolic-full-name")) {
+				symbolic = SHOW_SYMBOLIC_FULL;
 				continue;
 			}
 			if (!strcmp(arg, "--all")) {

Re: [PATCH] git-filter-branch could be confused by similar names

[Johannes Schindelin]

Hi,

On Fri, 4 Jan 2008, Junio C Hamano wrote:

> Dmitry Potapov <dpotapov@gmail.com> writes:
> 
> > It works only if the name "unambiguous" for git show-ref, which 
> > interprets refname differently than rev-list as I wrote above. 
> > Nevertheless, I believe we can use 'git show-ref' if we try something 
> > like this:
> 
> Ahh.
> 
> But at that point I would say that exposing the refname dwimming
> logic to the scripts could be a much cleaner solution.

I considered that when ripping the script from cogito, but it seemed to me 
at that time that not requiring an up-to-date git for testing the script 
would be better.

Now is a different situation, however, so I agree.

Ciao,
Dscho

[PATCH 1/2] git-rev-parse --symbolic-full-name

[Junio C Hamano]

The plumbing level can understand that the user meant
"refs/heads/master" when the user says "master" or
"heads/master", but there is no easy way for the scripts to
figure it out without duplicating the dwim_ref() logic.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is the same patch as I showed yesterday but with a doc
   update.

 Documentation/git-rev-parse.txt |    7 +++++++
 builtin-rev-parse.c             |   37 ++++++++++++++++++++++++++++++++++---
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 329fce0..0cedc13 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -70,6 +70,13 @@ OPTIONS
 	possible '{caret}' prefix); this option makes them output in a
 	form as close to the original input as possible.
 
+--symbolic-full-name::
+	This is similar to \--symbolic, but it omits input that
+	are not refs (i.e. branch or tag names; or more
+	explicitly disambiguating "heads/master" form, when you
+	want to name the "master" branch when there is an
+	unfortunately named tag "master"), and show them as full
+	refnames (e.g. "refs/heads/master").
 
 --all::
 	Show all refs found in `$GIT_DIR/refs`.
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 20d1789..b9af1a5 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -21,6 +21,9 @@ static const char *def;
 #define NORMAL 0
 #define REVERSED 1
 static int show_type = NORMAL;
+
+#define SHOW_SYMBOLIC_ASIS 1
+#define SHOW_SYMBOLIC_FULL 2
 static int symbolic;
 static int abbrev;
 static int output_sq;
@@ -103,8 +106,32 @@ static void show_rev(int type, const unsigned char *sha1, const char *name)
 
 	if (type != show_type)
 		putchar('^');
-	if (symbolic && name)
-		show(name);
+	if (symbolic && name) {
+		if (symbolic == SHOW_SYMBOLIC_FULL) {
+			unsigned char discard[20];
+			char *full;
+
+			switch (dwim_ref(name, strlen(name), discard, &full)) {
+			case 0:
+				/*
+				 * Not found -- not a ref.  We could
+				 * emit "name" here, but symbolic-full
+				 * users are interested in finding the
+				 * refs spelled in full, and they would
+				 * need to filter non-refs if we did so.
+				 */
+				break;
+			case 1: /* happy */
+				show(full);
+				break;
+			default: /* ambiguous */
+				error("refname '%s' is ambiguous", name);
+				break;
+			}
+		} else {
+			show(name);
+		}
+	}
 	else if (abbrev)
 		show(find_unique_abbrev(sha1, abbrev));
 	else
@@ -421,7 +448,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--symbolic")) {
-				symbolic = 1;
+				symbolic = SHOW_SYMBOLIC_ASIS;
+				continue;
+			}
+			if (!strcmp(arg, "--symbolic-full-name")) {
+				symbolic = SHOW_SYMBOLIC_FULL;
 				continue;
 			}
 			if (!strcmp(arg, "--all")) {
-- 
1.5.4.rc2.38.gd6da3

[PATCH 2/2] filter-branch: work correctly with ambiguous refnames

[Junio C Hamano]

'git-filter-branch branch' could fail producing the error:
"Which ref do you want to rewrite?" if existed another branch
or tag, which name was 'branch-something' or 'something/branch'.

[jc: original report and fix were done between Dmitry Potapov
and Dscho; I rewrote it using "rev-parse --symbolic-full-name"]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

 >> But at that point I would say that exposing the refname dwimming
 >> logic to the scripts could be a much cleaner solution.
 >
 > I considered that when ripping the script from cogito, but it seemed to me 
 > at that time that not requiring an up-to-date git for testing the script 
 > would be better.
 >
 > Now is a different situation, however, so I agree.

 It was already tied to the specific git version when
 git-filter-branch became part of git.git ;-)  

 I do not use filter-branch myself very often, but I think this
 is worth fixing.  The additional --no-flags and sed are to deal
 with something like:

	--topo-order master..next

 although I do not offhand know if filter-branch would work with
 things like --topo-order and --first-parent.

 git-filter-branch.sh |   22 +++-------------------
 1 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index ae29f47..ebf05ca 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -209,25 +209,9 @@ ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
 GIT_WORK_TREE=.
 export GIT_DIR GIT_WORK_TREE
 
-# These refs should be updated if their heads were rewritten
-
-git rev-parse --revs-only --symbolic "$@" |
-while read ref
-do
-	# normalize ref
-	case "$ref" in
-	HEAD)
-		ref="$(git symbolic-ref "$ref")"
-	;;
-	refs/*)
-	;;
-	*)
-		ref="$(git for-each-ref --format='%(refname)' |
-			grep /"$ref")"
-	esac
-
-	git check-ref-format "$ref" && echo "$ref"
-done > "$tempdir"/heads
+# The refs should be updated if their heads were rewritten
+git rev-parse --no-flags --revs-only --symbolic-full-name "$@" |
+sed -e '/^^/d' >"$tempdir"/heads
 
 test -s "$tempdir"/heads ||
 	die "Which ref do you want to rewrite?"
-- 
1.5.4.rc2.38.gd6da3

Re: [PATCH 2/2] filter-branch: work correctly with ambiguous refnames

[Johannes Schindelin]

Hi,

On Sat, 5 Jan 2008, Junio C Hamano wrote:

>  Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>  > Junio wrote:
>  >
>  >> But at that point I would say that exposing the refname dwimming
>  >> logic to the scripts could be a much cleaner solution.
>  >
>  > I considered that when ripping the script from cogito, but it seemed 
>  > to me at that time that not requiring an up-to-date git for testing 
>  > the script would be better.
>  >
>  > Now is a different situation, however, so I agree.
> 
>  It was already tied to the specific git version when
>  git-filter-branch became part of git.git ;-)

Heh.  But that was not my intention (at least _before_ it was in git.git's 
"master"), so that people could test it.

>  I do not use filter-branch myself very often, but I think this
>  is worth fixing.  The additional --no-flags and sed are to deal
>  with something like:
> 
> 	--topo-order master..next
> 
>  although I do not offhand know if filter-branch would work with
>  things like --topo-order and --first-parent.

Frankly, I have no idea, but --topo-order _should_ not matter, whereas 
--first-parent _should_ rewrite only commits in the first-parent chain of 
the given refs.

In any case, from a cursory look I like the 2 patches (except for the 
curly brackets around the single-line "else" clause, but I know your 
opinion about this, so I will not object).

Ciao,
Dscho

Re: [PATCH 2/2] filter-branch: work correctly with ambiguous refnames

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> In any case, from a cursory look I like the 2 patches (except for the 
> curly brackets around the single-line "else" clause, but I know your 
> opinion about this, so I will not object).

I care more about consistency across codebase than my own
preference [*1*].  I just picked the style the kernel folks seem
to use (see their Documentation/CodingStyle), only because (1)
there seem to be people familiar with it, and (2) I am not
particularly interested myself in wasting time arguing over
which style is superiour.  I just had to pick one and that was
the one I happened to have at hand.

And obviously I care more about correctness, so I'd appreciate a
review with non cursory look if you have time.

[Footnote]

*1* I favoring shorter code over consistency between when-true
and when-false clauses.  IOW, I do not like having to have {}
around a single statement in else clause when if clause needs {}
around it.

Re: [PATCH 2/2] filter-branch: work correctly with ambiguous refnames

[Johannes Schindelin]

Hi Junio,

On Sat, 5 Jan 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > In any case, from a cursory look I like the 2 patches (except for the 
> > curly brackets around the single-line "else" clause, but I know your 
> > opinion about this, so I will not object).
> 
> And obviously I care more about correctness, so I'd appreciate a review 
> with non cursory look if you have time.

I will be in the train for 5.5 hours tomorrow, and hope to do a less 
cursory review then.

Ciao,
Dscho

Re: [PATCH] git-filter-branch could be confused by similar names

[Junio C Hamano]

Dmitry Potapov <dpotapov@gmail.com> writes:

> On Thu, Jan 03, 2008 at 01:27:27PM -0800, Junio C Hamano wrote:
>> ... I had an
>> impression that we try to stick to a subset of BRE (namely, no
>> \{m,n\}, [::], [==], nor [..]).
>
> I was not aware about this policy, and I am not aware about
> existing any grep that does not grok the expressions I used
> above. So, I thought they are commonly accepted, but I might
> be wrong.

Well I might be wrong too, as I did not vet all the existing use
of grep in our code.  That's why I said I had "an impression".

Now I have ("git grep 'grep ' -- 'git-*.sh'"), and it seems to
be that we do stick to a narrow subset of BRE.

 * We do not use \{m,n\};

 * We do not use -E;

 * We do not use ? nor + (which are \{0,1\} and \{1,\}
   respectively in BRE) but that goes without saying as these
   are ERE elements not BRE (note that \? and \+ you wrote are
   not even part of BRE -- making them accessible from BRE is a
   GNU extension).

IOW, our scripts' use of grep is very 80'sh ;-)

I do not mind using things that are available in POSIX BRE, but
let's not rely on GNU extension that may cause issues to other
people.

Other things I noticed while looking at "t/*.sh":

 * t/t3600-rm.sh and t/5401-update-hooks.sh have unnecessary
   uses of egrep that can instead be grep;

 * t/t3800-mktag.sh uses egrep but I think it can be grep; also,
   I think the use of temporary file expect.pat is unnecessary.

 * t/t5510-fetch.sh does use '^[0-9a-f]\{40\} '.

 * t/t7001-mv.sh uses -E only to use '.+' when it can just as
   easily say '..*' instead.

 * t/t7600-merge.sh has two occurrences of (GNU extended) " \+"
   that should be "  *" for portability.  An alternative is to
   use grep -E and say " +" instead, but then the other plus
   sign on the same line needs to be quoted.

Other than the one in 5510 I consider them log hanging fruits
for janitors.