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

From: Dmitry Potapov <dpotapov@gmail.com>
Date: Jan 4, 2008, 15:51 UTC
Message-ID: <20080104155114.GS3373@dpotapov.dyndns.org>
In-Reply-To: <7v7iiqppkw.fsf@gitster.siamese.dyndns.org>

On Thu, Jan 03, 2008 at 01:27:27PM -0800, Junio C Hamano wrote:

Show 9 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

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