[PATCH 0/2] filter-branch: support for incremental update + fix for ancient tag format

[PATCH 0/2] filter-branch: support for incremental update + fix for ancient tag format

[Ian Campbell]

Hi,

I've long (since 2013, urk!) been carrying these two changes to git-
filter-branch in the split out devicetree source tree[0] which extracts
all the device tree sources from the Linux kernel source tree.

I think it's about time I sent them here, sorry for the rather extreme
delay! I've rebased to 2.14 and retested, I've also pushed a copy to
[1] where Travis seems happy.

Ian.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/
[1] https://github.com/ijc/git/tree/git-filter-branch

[PATCH 1/2] filter-branch: Add --state-branch to hold pickled copy of ref map

[Ian Campbell]

Allowing for incremental updates of large trees.

I have been using this as part of the device tree extraction from the Linux
kernel source since 2013, about time I sent the patch upstream!

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
---
 git-filter-branch.sh | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 3a74602ef..d07db3fee 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -86,7 +86,7 @@ USAGE="[--setup <command>] [--env-filter <command>]
 	[--parent-filter <command>] [--msg-filter <command>]
 	[--commit-filter <command>] [--tag-name-filter <command>]
 	[--subdirectory-filter <directory>] [--original <namespace>]
-	[-d <directory>] [-f | --force]
+	[-d <directory>] [-f | --force] [--state-branch <branch>]
 	[--] [<rev-list options>...]"
 
 OPTIONS_SPEC=
@@ -106,6 +106,7 @@ filter_msg=cat
 filter_commit=
 filter_tag_name=
 filter_subdir=
+state_branch=
 orig_namespace=refs/original/
 force=
 prune_empty=
@@ -181,6 +182,9 @@ do
 	--original)
 		orig_namespace=$(expr "$OPTARG/" : '\(.*[^/]\)/*$')/
 		;;
+	--state-branch)
+		state_branch="$OPTARG"
+		;;
 	*)
 		usage
 		;;
@@ -252,6 +256,20 @@ export GIT_INDEX_FILE
 # map old->new commit ids for rewriting parents
 mkdir ../map || die "Could not create map/ directory"
 
+if [ -n "$state_branch" ] ; then
+	state_commit=`git show-ref -s "$state_branch"`
+	if [ -n "$state_commit" ] ; then
+		echo "Populating map from $state_branch ($state_commit)" 1>&2
+		git show "$state_commit":filter.map |
+		    perl -n -e 'm/(.*):(.*)/ or die;
+				open F, ">../map/$1" or die;
+				print F "$2" or die;
+				close(F) or die'
+	else
+		echo "Branch $state_branch does not exist. Will create" 1>&2
+	fi
+fi
+
 # we need "--" only if there are no path arguments in $@
 nonrevs=$(git rev-parse --no-revs "$@") || exit
 if test -z "$nonrevs"
@@ -544,6 +562,25 @@ if [ "$filter_tag_name" ]; then
 	done
 fi
 
+if [ -n "$state_branch" ] ; then
+	echo "Saving rewrite state to $state_branch" 1>&2
+	STATE_BLOB=$(ls ../map |
+	    perl -n -e 'chomp();
+			open F, "<../map/$_" or die;
+			chomp($f = <F>); print "$_:$f\n";' |
+	    git hash-object -w --stdin )
+	STATE_TREE=$(/bin/echo -e "100644 blob $STATE_BLOB\tfilter.map" | git mktree)
+	STATE_PARENT=$(git show-ref -s "$state_branch")
+	unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
+	unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL GIT_COMMITTER_DATE
+	if [ -n "$STATE_PARENT" ] ; then
+	    STATE_COMMIT=$(/bin/echo "Sync" | git commit-tree "$STATE_TREE" -p "$STATE_PARENT")
+	else
+	    STATE_COMMIT=$(/bin/echo "Sync" | git commit-tree "$STATE_TREE" )
+	fi
+	git update-ref "$state_branch" "$STATE_COMMIT"
+fi
+
 cd "$orig_dir"
 rm -rf "$tempdir"
 
-- 
2.11.0

[PATCH 2/2] filter-branch: Handle rewritting (very) old style tags which lack tagger

[Ian Campbell]

Such as v2.6.12-rc2..v2.6.13-rc3 in the Linux kernel source tree.

Insert a fake tag header, since newer `git mktag` wont accept the input
otherwise:

    $ git cat-file tag v2.6.12-rc2
    object 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
    type commit
    tag v2.6.12-rc2

    Linux v2.6.12-rc2 release
    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.2.4 (GNU/Linux)

    iD8DBQBCbW8ZF3YsRnbiHLsRAgFRAKCq/TkuDaEombFABkPqYgGCgWN2lQCcC0qc
    wznDbFU45A54dZC8RZ5JxyE=
    =ESRP
    -----END PGP SIGNATURE-----

    $ git cat-file tag v2.6.12-rc2 | git mktag
    error: char76: could not find "tagger "
    fatal: invalid tag signature file
    $ git cat-file tag v2.6.13-rc4 | git mktag
    7eab951de91d95875ba34ec4c599f37e1208db93

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
---
 git-filter-branch.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index d07db3fee..6927aa2da 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -540,6 +540,9 @@ if [ "$filter_tag_name" ]; then
 			new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
 						"$new_sha1" "$new_ref"
 				git cat-file tag "$ref" |
+				awk '/^tagger/	{ tagged=1 }
+				     /^$/	{ if (!tagged && !done) { print "tagger Unknown <unknown@example.com> 0 +0000" } ; done=1 }
+				     //		{ print }' |
 				sed -n \
 				    -e '1,/^$/{
 					  /^object /d
-- 
2.11.0

Re: [PATCH 1/2] filter-branch: Add --state-branch to hold pickled copy of ref map

[Junio C Hamano]

Ian Campbell <ijc@hellion.org.uk> writes:

> Allowing for incremental updates of large trees.

"by doing what" is missing.  And ...

>
> I have been using this as part of the device tree extraction from the Linux
> kernel source since 2013, about time I sent the patch upstream!

... this does not help understanding what is going on.  It belongs
to the space after three dashes.

Perhaps

	Subject: filter-branch: stash away ref map in a branch

	With "--state-branch=<branchname>" option, the mapping from
	old object names and filtered ones in ./map/ directory is
	stashed away in the object database, and the one from the
	previous run is read to populate the ./map/ directory,
	allowing for incremental updates of large trees.

or something?

>
> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> ---
>  git-filter-branch.sh | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 3a74602ef..d07db3fee 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -86,7 +86,7 @@ USAGE="[--setup <command>] [--env-filter <command>]
>  	[--parent-filter <command>] [--msg-filter <command>]
>  	[--commit-filter <command>] [--tag-name-filter <command>]
>  	[--subdirectory-filter <directory>] [--original <namespace>]
> -	[-d <directory>] [-f | --force]
> +	[-d <directory>] [-f | --force] [--state-branch <branch>]
>  	[--] [<rev-list options>...]"
>  
>  OPTIONS_SPEC=
> @@ -106,6 +106,7 @@ filter_msg=cat
>  filter_commit=
>  filter_tag_name=
>  filter_subdir=
> +state_branch=
>  orig_namespace=refs/original/
>  force=
>  prune_empty=
> @@ -181,6 +182,9 @@ do
>  	--original)
>  		orig_namespace=$(expr "$OPTARG/" : '\(.*[^/]\)/*$')/
>  		;;
> +	--state-branch)
> +		state_branch="$OPTARG"
> +		;;
>  	*)
>  		usage
>  		;;
> @@ -252,6 +256,20 @@ export GIT_INDEX_FILE
>  # map old->new commit ids for rewriting parents
>  mkdir ../map || die "Could not create map/ directory"
>  
> +if [ -n "$state_branch" ] ; then
> +	state_commit=`git show-ref -s "$state_branch"`

I hate to nitpick styles, especially on this script that already has
existing violations, but for completeness:

Style: we prefer to write $(command substitution) instead.
Style: we prefer to write "if test", not "if [".
Style: we prefer to avoid ';' and write "if test condtion" and
       "then" on different lines.

It is a bit curious use of "show-ref".  It is not wrong per-se, but
"git rev-parse" may be more common.  I do not care too deeply either
way, though.

Don't we want to make sure the value given to --state-branch is a
full refname, not just a branch name?  What happens when you say 

	filter-branch --state-branch master

by mistake?  "show-ref -s" is likely to show your refs/heads/master,
and other master branches that appear as remote-tracking branches for
the remotes you interact with.

> +	if [ -n "$state_commit" ] ; then
> +		echo "Populating map from $state_branch ($state_commit)" 1>&2
> +		git show "$state_commit":filter.map |
> +		    perl -n -e 'm/(.*):(.*)/ or die;
> +				open F, ">../map/$1" or die;
> +				print F "$2" or die;
> +				close(F) or die'

The process calling this perl script, which carefully diagnoses
malformed input and dies, does not seem to do anything when it sees
errors.  Intended?

> +	else
> +		echo "Branch $state_branch does not exist. Will create" 1>&2
> +	fi
> +fi
> +
>  # we need "--" only if there are no path arguments in $@
>  nonrevs=$(git rev-parse --no-revs "$@") || exit
>  if test -z "$nonrevs"
> @@ -544,6 +562,25 @@ if [ "$filter_tag_name" ]; then
>  	done
>  fi
>  
> +if [ -n "$state_branch" ] ; then
> +	echo "Saving rewrite state to $state_branch" 1>&2
> +	STATE_BLOB=$(ls ../map |
> +	    perl -n -e 'chomp();
> +			open F, "<../map/$_" or die;
> +			chomp($f = <F>); print "$_:$f\n";' |

I see it somewhat gross to pipe the output of "/bin/ls" to a Perl
script, instead of iterating over "while (<../map/*>)" inside the
script itself.

> +	    git hash-object -w --stdin )
> +	STATE_TREE=$(/bin/echo -e "100644 blob $STATE_BLOB\tfilter.map" | git mktree)
> +	STATE_PARENT=$(git show-ref -s "$state_branch")

Don't you already have this in $state_commit?

One advantage of reading $state_branch again at this point is to
detect mistakes of running more than one filter-branch (which may
cause you to read $STATE_PARENT that is different from $state_commit
you read earlier), but I do not think that is being done here, so...

> +	unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
> +	unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL GIT_COMMITTER_DATE

Hmph.  I can see that you are trying not to be affected by the
committers and authors of the commits on the branch being filtered
(which are set by finish_ident shell function), but I wonder if we
could (and more importantly "want to") do better to preserve the
real committer the user who runs the script may have in the
environment before running it.  I guess it does not matter that
much, as long as the user has properly user.{name,email} configured
elsewhere without relying on the environment variable.

> +	if [ -n "$STATE_PARENT" ] ; then
> +	    STATE_COMMIT=$(/bin/echo "Sync" | git commit-tree "$STATE_TREE" -p "$STATE_PARENT")
> +	else
> +	    STATE_COMMIT=$(/bin/echo "Sync" | git commit-tree "$STATE_TREE" )
> +	fi
> +	git update-ref "$state_branch" "$STATE_COMMIT"
> +fi
> +
>  cd "$orig_dir"
>  rm -rf "$tempdir"

Despite all the above comments, I like what you are trying to
achieve here.  Thanks for sharing.

Re: [PATCH 2/2] filter-branch: Handle rewritting (very) old style tags which lack tagger

[Junio C Hamano]

Ian Campbell <ijc@hellion.org.uk> writes:

> Such as v2.6.12-rc2..v2.6.13-rc3 in the Linux kernel source tree.
>
> Insert a fake tag header, since newer `git mktag` wont accept the input
> otherwise:
>
>     $ git cat-file tag v2.6.12-rc2
>     object 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
>     type commit
>     tag v2.6.12-rc2
>
>     Linux v2.6.12-rc2 release
>     -----BEGIN PGP SIGNATURE-----
>     Version: GnuPG v1.2.4 (GNU/Linux)
>
>     iD8DBQBCbW8ZF3YsRnbiHLsRAgFRAKCq/TkuDaEombFABkPqYgGCgWN2lQCcC0qc
>     wznDbFU45A54dZC8RZ5JxyE=
>     =ESRP
>     -----END PGP SIGNATURE-----
>
>     $ git cat-file tag v2.6.12-rc2 | git mktag
>     error: char76: could not find "tagger "
>     fatal: invalid tag signature file
>     $ git cat-file tag v2.6.13-rc4 | git mktag
>     7eab951de91d95875ba34ec4c599f37e1208db93
>
> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> ---
>  git-filter-branch.sh | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index d07db3fee..6927aa2da 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -540,6 +540,9 @@ if [ "$filter_tag_name" ]; then
>  			new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
>  						"$new_sha1" "$new_ref"
>  				git cat-file tag "$ref" |
> +				awk '/^tagger/	{ tagged=1 }
> +				     /^$/	{ if (!tagged && !done) { print "tagger Unknown <unknown@example.com> 0 +0000" } ; done=1 }
> +				     //		{ print }' |
>  				sed -n \
>  				    -e '1,/^$/{
>  					  /^object /d

What the change wants to do makes perfect sense, but piping output
from awk into sed looks somewhat gross.  Perhaps we'd want to roll
what the existing sed script is trying to do into this new awk
script?

Re: [PATCH 2/2] filter-branch: Handle rewritting (very) old style tags which lack tagger

[Ian Campbell]

On Tue, 2017-08-08 at 14:00 -0700, Junio C Hamano wrote:
> > @@ -540,6 +540,9 @@ if [ "$filter_tag_name" ]; then
> > >  			new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
> > >  						"$new_sha1" "$new_ref"
> > >  				git cat-file tag "$ref" |
> > > > +				awk '/^tagger/	{ tagged=1 }
> > > > > +				     /^$/	{ if (!tagged && !done) { print "tagger Unknown <unknown@example.com> 0 +0000" } ; done=1 }
> > > > +				     //		{ print }' |
> > >  				sed -n \
> > >  				    -e '1,/^$/{
> > >  					  /^object /d
> 
> What the change wants to do makes perfect sense, but piping output
> from awk into sed looks somewhat gross.  Perhaps we'd want to roll
> what the existing sed script is trying to do into this new awk
> script?

I'm far from an awk guru but I think (unit tested in isolation only)
that such script would look something like (I also inverted/renamed
done into header since it seemed clearer):

    BEGIN    	    	    	    	    	    { header=1 }
    /^tagger /    	    	    	    	    { tagged=1 }
    /^$/    	    	    	    	    	    { if (!tagged && header) { print "tagger Unknown <    unknown@example.com    > 0 +0000" } ; header=0 }

    /^-----BEGIN PGP SIGNATURE-----/    	    { exit(0) }

    //    	    	    	    	    	    { if (!header || $0 !~ /^(object|type|tag )/) { print } }

    Ian.

Re: [PATCH 1/2] filter-branch: Add --state-branch to hold pickled copy of ref map

[Ian Campbell]

On Tue, 2017-08-08 at 13:56 -0700, Junio C Hamano wrote:
> Ian Campbell <ijc@hellion.org.uk> writes:
> 
> > Allowing for incremental updates of large trees.
> 
> "by doing what" is missing.  And ...
> 
> >
> > I have been using this as part of the device tree extraction from
> the Linux
> > kernel source since 2013, about time I sent the patch upstream!
> 
> ... this does not help understanding what is going on.  It belongs
> to the space after three dashes.
> 
> Perhaps
> 
> 	Subject: filter-branch: stash away ref map in a branch
> 
> 	With "--state-branch=<branchname>" option, the mapping from
> 	old object names and filtered ones in ./map/ directory is
> 	stashed away in the object database, and the one from the
> 	previous run is read to populate the ./map/ directory,
> 	allowing for incremental updates of large trees.
> 
> or something?

Yes, thanks that is a lot better.

I'll address the feedback (style nits and all) in the coming weeks,
heads up that I might be a bit slow, got a busy week this week followed
by 3 weeks of travel (which might mean no time for hacking or lots,
hard to say ;-))

> Don't we want to make sure the value given to --state-branch is a
> full refname, not just a branch name?  What happens when you say 
> 
> 	filter-branch --state-branch master
> 
> by mistake?  "show-ref -s" is likely to show your refs/heads/master,
> and other master branches that appear as remote-tracking branches for
> the remotes you interact with.

I've been using this as `--state-branch refs/heads/filter-state` which
creates a local/visible filter-state branch which I also push to a
remote, so I also have a `refs/remotes/state/filter-state` too.

What is the correct way to check for a full ref name? Is it as simple
as checking for a refs/heads/ prefix or is there a better way?

> > +	if [ -n "$state_commit" ] ; then
> > +		echo "Populating map from $state_branch
> ($state_commit)" 1>&2
> > +		git show "$state_commit":filter.map |
> > +		    perl -n -e 'm/(.*):(.*)/ or die;
> > +				open F, ">../map/$1" or die;
> > +				print F "$2" or die;
> > +				close(F) or die'
> 
> The process calling this perl script, which carefully diagnoses
> malformed input and dies, does not seem to do anything when it sees
> errors.  Intended?

I hadn't realised the script wasn't using `set -e`. I'll sort this with
some local error handling.

> 
> > +	else
> > +		echo "Branch $state_branch does not exist. Will
> create" 1>&2
> > +	fi
> > +fi
> > +
> >  # we need "--" only if there are no path arguments in $@
> >  nonrevs=$(git rev-parse --no-revs "$@") || exit
> >  if test -z "$nonrevs"
> > @@ -544,6 +562,25 @@ if [ "$filter_tag_name" ]; then
> >  	done
> >  fi
> >  
> > +if [ -n "$state_branch" ] ; then
> > +	echo "Saving rewrite state to $state_branch" 1>&2
> > +	STATE_BLOB=$(ls ../map |
> > +	    perl -n -e 'chomp();
> > +			open F, "<../map/$_" or die;
> > +			chomp($f = <F>); print "$_:$f\n";' |
> 
> I see it somewhat gross to pipe the output of "/bin/ls" to a Perl
> script, instead of iterating over "while (<../map/*>)" inside the
> script itself.

I considered cleaning this up too as I was forward porting, but weirdly
it appeared to microbenchmark slower that way, I don't remember the
magnitude of the difference (and the test script is on another machine
right now). I'll revisit that and if it isn't too much slower I'll
switch to the saner looking all in Perl method.

> > +	unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
> > +	unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL
> GIT_COMMITTER_DATE
> 
> Hmph.  I can see that you are trying not to be affected by the
> committers and authors of the commits on the branch being filtered
> (which are set by finish_ident shell function), but I wonder if we
> could (and more importantly "want to") do better to preserve the
> real committer the user who runs the script may have in the
> environment before running it.  I guess it does not matter that
> much, as long as the user has properly user.{name,email} configured
> elsewhere without relying on the environment variable.

I'm glad you spotted this because I couldn't remember ;-)

I'll stash these in a bunch of ORIG_FOO near the top and then reset
them at an appropriate point (I'll use ORIG_GIT_DIR as the pattern).

> Despite all the above comments, I like what you are trying to
> achieve here.  Thanks for sharing.

Thanks for the review and feedback.


Ian.

Re: [PATCH 2/2] filter-branch: Handle rewritting (very) old style tags which lack tagger

[Jeff King]

On Tue, Aug 08, 2017 at 09:06:20AM +0100, Ian Campbell wrote:

> Such as v2.6.12-rc2..v2.6.13-rc3 in the Linux kernel source tree.
> 
> Insert a fake tag header, since newer `git mktag` wont accept the input
> otherwise:

Hmm. Now your resulting tag will have this crufty "unknown@example.com"
header baked into it, won't it?

Should we instead make git-mktag more lenient (possibly with a
command-line option to reduce accidental omissions)?

-Peff

Re: [PATCH 2/2] filter-branch: Handle rewritting (very) old style tags which lack tagger

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Aug 08, 2017 at 09:06:20AM +0100, Ian Campbell wrote:
>
>> Such as v2.6.12-rc2..v2.6.13-rc3 in the Linux kernel source tree.
>> 
>> Insert a fake tag header, since newer `git mktag` wont accept the input
>> otherwise:
>
> Hmm. Now your resulting tag will have this crufty "unknown@example.com"
> header baked into it, won't it?
>
> Should we instead make git-mktag more lenient (possibly with a
> command-line option to reduce accidental omissions)?

That sounds sensible. Thanks for injecting a dose of sanity.

Re: [PATCH 2/2] filter-branch: Handle rewritting (very) old style tags which lack tagger

[Ian Campbell]

On Wed, 2017-08-09 at 08:50 -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Aug 08, 2017 at 09:06:20AM +0100, Ian Campbell wrote:
> >
> >> Such as v2.6.12-rc2..v2.6.13-rc3 in the Linux kernel source tree.
> >> 
> >> Insert a fake tag header, since newer `git mktag` wont accept the
> input
> >> otherwise:
> >
> > Hmm. Now your resulting tag will have this crufty "unknown@example.
> com"
> > header baked into it, won't it?
> >
> > Should we instead make git-mktag more lenient (possibly with a
> > command-line option to reduce accidental omissions)?
> 
> That sounds sensible. Thanks for injecting a dose of sanity.

Indeed. I'll add a --allow-missing-tagger option (suggestions for a
snappier name accepted!) and pass it unconditionally from the filter-
branch script.

Ian.

Re: [PATCH 2/2] filter-branch: Handle rewritting (very) old style tags which lack tagger

[Junio C Hamano]

Ian Campbell <ijc@hellion.org.uk> writes:

> Indeed. I'll add a --allow-missing-tagger option (suggestions for a
> snappier name accepted!) and pass it unconditionally from the filter-
> branch script.

Thanks.  That's much better.

Re: [PATCH 2/2] filter-branch: Handle rewritting (very) old style tags which lack tagger

[Jeff King]

On Wed, Aug 09, 2017 at 08:02:33PM +0100, Ian Campbell wrote:

> > > Should we instead make git-mktag more lenient (possibly with a
> > > command-line option to reduce accidental omissions)?
> > 
> > That sounds sensible. Thanks for injecting a dose of sanity.
> 
> Indeed. I'll add a --allow-missing-tagger option (suggestions for a
> snappier name accepted!) and pass it unconditionally from the filter-
> branch script.

I think that name is the right amount of snappy. It's not meant to be
used very often. :)

-Peff