Re: [PATCH] filter-branch: rewrite only refs which were not excludedbythe options

From: Johannes Sixt <j.sixt@eudaptics.com>
Date: Jul 24, 2007, 13:32 UTC
Message-ID: <46A5FF69.F5D75C9E@eudaptics.com>
In-Reply-To: <Pine.LNX.4.64.0707241229170.14781@racer.site>

Johannes Schindelin wrote:

Show 25 quoted lines

> 
> Hi,
> 
> On Tue, 24 Jul 2007, Johannes Sixt wrote:
> 
> > Johannes Schindelin wrote:
> > > So really exclude excluded refs from being rewritten.  This also allows
> > > you to safely call
> > >
> > >         git filter-branch <some-filter> --all <rev-list options>
> > >
> > > to rewrite _all_ branches and tags.
> >
> > BTW, '--all' in the argument list of filter-branch works only if it is
> > preceded by '--':
> >
> >       git filter-branch <some-filter> -- --all <rev-list options>
> 
> Hmm.  Maybe we should reconsider the logic.  I.e. instead of
> 
>         *)
>                 usage
> 
>         *)
>                 break

That is not enough: This case block comes after a test that checks that one additional argument is present, which would not be true anymore.

Show 45 quoted lines

> > > @@ -181,6 +181,7 @@ export GIT_DIR GIT_WORK_TREE=.
> > >
> > >  # These refs should be updated if their heads were rewritten
> > >
> > > +negatives="$(git rev-parse --revs-only "$@" | grep "^\^")"
> > >  git rev-parse --revs-only --symbolic "$@" |
> > >  while read ref
> > >  do
> > > @@ -196,7 +197,13 @@ do
> > >                         grep "refs/\(tags\|heads\)/$ref$")"
> > >         esac
> > >
> > > -       git check-ref-format "$ref" && echo "$ref"
> > > +       # make sure we have a valid ref
> > > +       git check-ref-format "$ref" || continue
> > > +
> > > +       # if the ref has been excluded by the other options, skip it
> > > +       test -z "$(git rev-list -1 "$ref" $negatives)" && continue
> >
> > Does this catch my use-case with --since? I think not, because:
> >
> > $ git rev-parse --revs-only --since=2007.01.01 master topic
> > --max-age=1167606000
> > 257061f3323dc0162f731d934f0870e919211fdf
> > 3405729b94a654df8afbb9a1e13a4cf49a1c351c
> >
> > There are no negatives. Does it help to filter the non-positives?
> >
> > negatives=$(git rev-parse --revs-only "$@" | egrep -v '^[0-9a-f]{40}$')
> >
> > (Except the the '{40}' part is not portable. Hmpf.)
> 
> To keep the "--since=..." we have to lose the "--revs-only"...
> Darn.  I thought that "--since=" was expanded by rev-parse.  FWIW this
> might work:
> 
> negatives="$(git rev-parse "$@" | while read line
>         do
>                 case "$line" in
>                 $_x40) ;;
>                 *) echo "$line";;
>                 esac
>         done)"
> 
> Can you please test?  I am off for lunch.

This worked:

negatives=`git rev-parse --revs-only "$@" | while read line
	do
		case "$line" in
		$_x40) ;;
		*) echo "$line";;
		esac
	done`

i.e. the closing parenthesis in the case arms together with the opening $( made for a syntax error. The --revs-only did not hurt in my tests, but you may have other reasons to remove it.

But there's another problem. Consider this history:

   ---X--o--M         <- master
             \
          ...-o-...-o <- topic

Then this (rather contrieved) command:

   $ git-filter-branch -n $n master topic --not X

If $n is small enough so that M is never rewritten, then

   git rev-list -1 "$ref" $negatives

still expands to non-empty even for 'master' (= M), which then incorrectly ends up in "$tempdir"/heads.

I think the decision whether a positive ref should be rewritten should be postponed until the rewrite has completed. Because then we know for certain which revs were treated and can pick the matching refs. We only lose the check for the error "Which ref do you want to rewrite?"

-- Hannes